Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493534 > unrolled thread

Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2 James Chapman <jchapman@katalix.com> - 2016-09-29 17:30 +0200
    Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay  L2 "R. Parameswaran" <parameswaran.r7@gmail.com> - 2016-09-30 04:50 +0200
      Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2 James Chapman <jchapman@katalix.com> - 2016-10-01 19:00 +0200
        Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay  L2 "R. Parameswaran" <parameswaran.r7@gmail.com> - 2016-10-04 05:20 +0200
          Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2 James Chapman <jchapman@katalix.com> - 2016-10-04 10:00 +0200
            Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2 James Chapman <jchapman@katalix.com> - 2016-10-11 09:50 +0200

#1493534 — Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2

On 22/09/16 21:52, R. Parameswaran wrote:
> From ed585bdd6d3d2b3dec58d414f514cd764d89159d Mon Sep 17 00:00:00 2001
> From: "R. Parameswaran" <rparames@brocade.com>
> Date: Thu, 22 Sep 2016 13:19:25 -0700
> Subject: [PATCH] L2TP:Adjust intf MTU,factor underlay L3,overlay L2
>
> Take into account all of the tunnel encapsulation headers when setting
> up the MTU on the L2TP logical interface device. Otherwise, packets
> created by the applications on top of the L2TP layer are larger
> than they ought to be, relative to the underlay MTU, leading to
> needless fragmentation once the outer IP encap is added.
>
> Specifically, take into account the (outer, underlay) IP header
> imposed on the encapsulated L2TP packet, and the Layer 2 header
> imposed on the inner IP packet prior to L2TP encapsulation.
>
> Do not assume an Ethernet (non-jumbo) underlay. Use the PMTU mechanism
> and the dst entry in the L2TP tunnel socket to directly pull up
> the underlay MTU (as the baseline number on top of which the
> encapsulation headers are factored in).  Fall back to Ethernet MTU
> if this fails.
>
> Signed-off-by: R. Parameswaran <rparames@brocade.com>
>
> Reviewed-by: "N. Prachanda" <nprachan@brocade.com>,
> Reviewed-by: "R. Shearman" <rshearma@brocade.com>,
> Reviewed-by: "D. Fawcus" <dfawcus@brocade.com>
> ---
>  net/l2tp/l2tp_eth.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 57fc5a4..dbcd6bd 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -30,6 +30,9 @@
>  #include <net/xfrm.h>
>  #include <net/net_namespace.h>
>  #include <net/netns/generic.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <linux/udp.h>
>  
>  #include "l2tp_core.h"
>  
> @@ -206,6 +209,46 @@ static void l2tp_eth_show(struct seq_file *m, void *arg)
>  }
>  #endif
>  
> +static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
> +				struct l2tp_session *session,
> +				struct net_device *dev)
> +{
> +	unsigned int overhead = 0;
> +	struct dst_entry *dst;
> +
> +	if (session->mtu != 0) {
> +		dev->mtu = session->mtu;
> +		dev->needed_headroom += session->hdr_len;
> +		if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
> +			dev->needed_headroom += sizeof(struct udphdr);
> +		return;
> +	}
> +	overhead = session->hdr_len;
> +	/* Adjust MTU, factor overhead - underlay L3 hdr, overlay L2 hdr*/
> +	if (tunnel->sock->sk_family == AF_INET)
> +		overhead += (ETH_HLEN + sizeof(struct iphdr));
> +	else if (tunnel->sock->sk_family == AF_INET6)
> +		overhead += (ETH_HLEN + sizeof(struct ipv6hdr));
What about options in the IP header? If certain options are set on the
socket, the IP header may be larger.

> +	/* Additionally, if the encap is UDP, account for UDP header size */
> +	if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
> +		overhead += sizeof(struct udphdr);
> +	/* If PMTU discovery was enabled, use discovered MTU on L2TP device */
> +	dst = sk_dst_get(tunnel->sock);
> +	if (dst) {
> +		u32 pmtu = dst_mtu(dst);
> +
> +		if (pmtu != 0)
> +			dev->mtu = pmtu;
> +		dst_release(dst);
> +	}
> +	/* else (no PMTUD) L2TP dev MTU defaulted to Ethernet MTU in caller */
> +	session->mtu = dev->mtu - overhead;
> +	dev->mtu = session->mtu;
> +	dev->needed_headroom += session->hdr_len;
> +	if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
> +		dev->needed_headroom += sizeof(struct udphdr);
> +}
> +
>  static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
>  {
>  	struct net_device *dev;
> @@ -255,11 +298,8 @@ static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 p
>  	}
>  
>  	dev_net_set(dev, net);
> -	if (session->mtu == 0)
> -		session->mtu = dev->mtu - session->hdr_len;
> -	dev->mtu = session->mtu;
> -	dev->needed_headroom += session->hdr_len;
>  
> +	l2tp_eth_adjust_mtu(tunnel, session, dev);
>  	priv = netdev_priv(dev);
>  	priv->dev = dev;
>  	priv->session = session;

[toc] | [next] | [standalone]

#1493857 — Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2

Hi James,

On Thu, 29 Sep 2016, James Chapman wrote:

> On 22/09/16 21:52, R. Parameswaran wrote:
> > From ed585bdd6d3d2b3dec58d414f514cd764d89159d Mon Sep 17 00:00:00 2001
> > From: "R. Parameswaran" <rparames@brocade.com>
> > Date: Thu, 22 Sep 2016 13:19:25 -0700
> > Subject: [PATCH] L2TP:Adjust intf MTU,factor underlay L3,overlay L2
> >
> > Take into account all of the tunnel encapsulation headers when setting
> > up the MTU on the L2TP logical interface device. Otherwise, packets
> > created by the applications on top of the L2TP layer are larger
> > than they ought to be, relative to the underlay MTU, leading to
> > needless fragmentation once the outer IP encap is added.
> >
> > Specifically, take into account the (outer, underlay) IP header
> > imposed on the encapsulated L2TP packet, and the Layer 2 header
> > imposed on the inner IP packet prior to L2TP encapsulation.
> >
> > Do not assume an Ethernet (non-jumbo) underlay. Use the PMTU mechanism
> > and the dst entry in the L2TP tunnel socket to directly pull up
> > the underlay MTU (as the baseline number on top of which the
> > encapsulation headers are factored in).  Fall back to Ethernet MTU
> > if this fails.
> >
> > Signed-off-by: R. Parameswaran <rparames@brocade.com>
> >
> > Reviewed-by: "N. Prachanda" <nprachan@brocade.com>,
> > Reviewed-by: "R. Shearman" <rshearma@brocade.com>,
> > Reviewed-by: "D. Fawcus" <dfawcus@brocade.com>
> > ---
> >  net/l2tp/l2tp_eth.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> > index 57fc5a4..dbcd6bd 100644
> > --- a/net/l2tp/l2tp_eth.c
> > +++ b/net/l2tp/l2tp_eth.c
> > @@ -30,6 +30,9 @@
> >  #include <net/xfrm.h>
> >  #include <net/net_namespace.h>
> >  #include <net/netns/generic.h>
> > +#include <linux/ip.h>
> > +#include <linux/ipv6.h>
> > +#include <linux/udp.h>
> >  
> >  #include "l2tp_core.h"
> >  
> > @@ -206,6 +209,46 @@ static void l2tp_eth_show(struct seq_file *m, void *arg)
> >  }
> >  #endif
> >  
> > +static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
> > +				struct l2tp_session *session,
> > +				struct net_device *dev)
> > +{
> > +	unsigned int overhead = 0;
> > +	struct dst_entry *dst;
> > +
> > +	if (session->mtu != 0) {
> > +		dev->mtu = session->mtu;
> > +		dev->needed_headroom += session->hdr_len;
> > +		if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
> > +			dev->needed_headroom += sizeof(struct udphdr);
> > +		return;
> > +	}
> > +	overhead = session->hdr_len;
> > +	/* Adjust MTU, factor overhead - underlay L3 hdr, overlay L2 hdr*/
> > +	if (tunnel->sock->sk_family == AF_INET)
> > +		overhead += (ETH_HLEN + sizeof(struct iphdr));
> > +	else if (tunnel->sock->sk_family == AF_INET6)
> > +		overhead += (ETH_HLEN + sizeof(struct ipv6hdr));
> What about options in the IP header? If certain options are set on the
> socket, the IP header may be larger.
> 

Thanks for the reply - It looks like IP options can only be 
enabled through setsockopt on an application's socket (if there's any 
other way to turn on IP options, please let me know - didn't see any 
sysctl setting for transmit). This scenario would come 
into picture when an application opens a raw IP or UDP socket such that it 
routes into the L2TP logical interface.

If you take the case of a plain IP (ethernet) interface, even if an
application opened a socket turning on IP options, it would not change
the MTU of the underlying interface, and it would not affect other 
applications transacting packets on the same interface. I know its not an 
exact parallel to this case, but since the IP option control is per 
application, we probably should not factor it into the L2TP logical interface?
We cannot affect other applications/processes running on the same L2TP 
tunnel. Also, since the application  using IP options knows that it has turned 
on IP options, maybe we can count on it to factor the size of the options 
into the size of the payload it sends into the socket, or set the mtu on the 
L2TP interface through config? 

Other than this, I don't see keepalives or anything else in which the 
kernel will source its own packet into the L2TP interface, outside of 
an application injected packet - if there is something like that, please
let me know. The user space L2TP daemon would probably fall in the 
category of applications.

thanks,

Ramkumar 


> > +	/* Additionally, if the encap is UDP, account for UDP header size */
> > +	if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
> > +		overhead += sizeof(struct udphdr);
> > +	/* If PMTU discovery was enabled, use discovered MTU on L2TP device */
> > +	dst = sk_dst_get(tunnel->sock);
> > +	if (dst) {
> > +		u32 pmtu = dst_mtu(dst);
> > +
> > +		if (pmtu != 0)
> > +			dev->mtu = pmtu;
> > +		dst_release(dst);
> > +	}
> > +	/* else (no PMTUD) L2TP dev MTU defaulted to Ethernet MTU in caller */
> > +	session->mtu = dev->mtu - overhead;
> > +	dev->mtu = session->mtu;
> > +	dev->needed_headroom += session->hdr_len;
> > +	if (tunnel->encap == L2TP_ENCAPTYPE_UDP)
> > +		dev->needed_headroom += sizeof(struct udphdr);
> > +}
> > +
> >  static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
> >  {
> >  	struct net_device *dev;
> > @@ -255,11 +298,8 @@ static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 p
> >  	}
> >  
> >  	dev_net_set(dev, net);
> > -	if (session->mtu == 0)
> > -		session->mtu = dev->mtu - session->hdr_len;
> > -	dev->mtu = session->mtu;
> > -	dev->needed_headroom += session->hdr_len;
> >  
> > +	l2tp_eth_adjust_mtu(tunnel, session, dev);
> >  	priv = netdev_priv(dev);
> >  	priv->dev = dev;
> >  	priv->session = session;
> 
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1494440

On 30/09/16 03:39, R. Parameswaran wrote:
>
>>> +	/* Adjust MTU, factor overhead - underlay L3 hdr, overlay L2 hdr*/
>>> +	if (tunnel->sock->sk_family == AF_INET)
>>> +		overhead += (ETH_HLEN + sizeof(struct iphdr));
>>> +	else if (tunnel->sock->sk_family == AF_INET6)
>>> +		overhead += (ETH_HLEN + sizeof(struct ipv6hdr));
>> What about options in the IP header? If certain options are set on the
>> socket, the IP header may be larger.
>>
> Thanks for the reply - It looks like IP options can only be 
> enabled through setsockopt on an application's socket (if there's any 
> other way to turn on IP options, please let me know - didn't see any 
> sysctl setting for transmit). This scenario would come 
> into picture when an application opens a raw IP or UDP socket such that it 
> routes into the L2TP logical interface.

No. An L2TP daemon (userspace) will open a socket for each tunnel that
it creates. Control and data packets use the same socket, which is the
socket used by this code. It may set any options on its sockets. L2TP
tunnel sockets can be created either by an L2TP daemon (managed tunnels)
or by ip l2tp commands (unmanaged tunnels).

> If you take the case of a plain IP (ethernet) interface, even if an
> application opened a socket turning on IP options, it would not change
> the MTU of the underlying interface, and it would not affect other 
> applications transacting packets on the same interface. I know its not an 
> exact parallel to this case, but since the IP option control is per 
> application, we probably should not factor it into the L2TP logical interface?
> We cannot affect other applications/processes running on the same L2TP 
> tunnel. Also, since the application  using IP options knows that it has turned 
> on IP options, maybe we can count on it to factor the size of the options 
> into the size of the payload it sends into the socket, or set the mtu on the 
> L2TP interface through config? 

No. See above.

>
> Other than this, I don't see keepalives or anything else in which the 
> kernel will source its own packet into the L2TP interface, outside of 
> an application injected packet - if there is something like that, please
> let me know. The user space L2TP daemon would probably fall in the 
> category of applications.
>
> thanks,
>
> Ramkumar 
>
>

[toc] | [prev] | [next] | [standalone]

#1495183 — Re: [PATCH net v2] L2TP:Adjust intf MTU,factor underlay L3,overlay L2

Hi James, 

Please see inline, thanks for the reply:

On Sat, 1 Oct 2016, James Chapman wrote:

> On 30/09/16 03:39, R. Parameswaran wrote:
> >
> >>> +	/* Adjust MTU, factor overhead - underlay L3 hdr, overlay L2 hdr*/
> >>> +	if (tunnel->sock->sk_family == AF_INET)
> >>> +		overhead += (ETH_HLEN + sizeof(struct iphdr));
> >>> +	else if (tunnel->sock->sk_family == AF_INET6)
> >>> +		overhead += (ETH_HLEN + sizeof(struct ipv6hdr));
> >> What about options in the IP header? If certain options are set on the
> >> socket, the IP header may be larger.
> >>
> > Thanks for the reply - It looks like IP options can only be 
> > enabled through setsockopt on an application's socket (if there's any 
> > other way to turn on IP options, please let me know - didn't see any 
> > sysctl setting for transmit). This scenario would come 
> > into picture when an application opens a raw IP or UDP socket such that it 
> > routes into the L2TP logical interface.
> 
> No. An L2TP daemon (userspace) will open a socket for each tunnel that
> it creates. Control and data packets use the same socket, which is the
> socket used by this code. It may set any options on its sockets. L2TP
> tunnel sockets can be created either by an L2TP daemon (managed tunnels)
> or by ip l2tp commands (unmanaged tunnels).
> 

One Q I have is whether it would be sufficient to solve this for the
common case (i.e no IP options) and have an expectation that the 
administrator will explicitly provision the mtu using the 'ip link ... 
mtu'  command when dealing with infrequent occurences like IP options? 

But looking at the code, it looks to be possible to pick up whether 
options are enabled and how long the options are, from the ip_options struct 
embedded in the tunnel socket. If you want me to, I can repost the patch
with this change (will need a few days) - please let me know if this is 
what you had in mind.

thanks,

Ramkumar



> > If you take the case of a plain IP (ethernet) interface, even if an
> > application opened a socket turning on IP options, it would not change
> > the MTU of the underlying interface, and it would not affect other 
> > applications transacting packets on the same interface. I know its not an 
> > exact parallel to this case, but since the IP option control is per 
> > application, we probably should not factor it into the L2TP logical interface?
> > We cannot affect other applications/processes running on the same L2TP 
> > tunnel. Also, since the application  using IP options knows that it has turned 
> > on IP options, maybe we can count on it to factor the size of the options 
> > into the size of the payload it sends into the socket, or set the mtu on the 
> > L2TP interface through config? 
> 
> No. See above.
> 
> >
> > Other than this, I don't see keepalives or anything else in which the 
> > kernel will source its own packet into the L2TP interface, outside of 
> > an application injected packet - if there is something like that, please
> > let me know. The user space L2TP daemon would probably fall in the 
> > category of applications.
> >
> > thanks,
> >
> > Ramkumar 
> >
> >
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1495247

On 04/10/16 04:12, R. Parameswaran wrote:
>
> Hi James, 
>
> Please see inline, thanks for the reply:
>
> On Sat, 1 Oct 2016, James Chapman wrote:
>
>> On 30/09/16 03:39, R. Parameswaran wrote:
>>>>> +	/* Adjust MTU, factor overhead - underlay L3 hdr, overlay L2 hdr*/
>>>>> +	if (tunnel->sock->sk_family == AF_INET)
>>>>> +		overhead += (ETH_HLEN + sizeof(struct iphdr));
>>>>> +	else if (tunnel->sock->sk_family == AF_INET6)
>>>>> +		overhead += (ETH_HLEN + sizeof(struct ipv6hdr));
>>>> What about options in the IP header? If certain options are set on the
>>>> socket, the IP header may be larger.
>>>>
>>> Thanks for the reply - It looks like IP options can only be 
>>> enabled through setsockopt on an application's socket (if there's any 
>>> other way to turn on IP options, please let me know - didn't see any 
>>> sysctl setting for transmit). This scenario would come 
>>> into picture when an application opens a raw IP or UDP socket such that it 
>>> routes into the L2TP logical interface.
>> No. An L2TP daemon (userspace) will open a socket for each tunnel that
>> it creates. Control and data packets use the same socket, which is the
>> socket used by this code. It may set any options on its sockets. L2TP
>> tunnel sockets can be created either by an L2TP daemon (managed tunnels)
>> or by ip l2tp commands (unmanaged tunnels).
>>
> One Q I have is whether it would be sufficient to solve this for the
> common case (i.e no IP options) and have an expectation that the 
> administrator will explicitly provision the mtu using the 'ip link ... 
> mtu'  command when dealing with infrequent occurences like IP options? 
>
> But looking at the code, it looks to be possible to pick up whether 
> options are enabled and how long the options are, from the ip_options struct 
> embedded in the tunnel socket. If you want me to, I can repost the patch
> with this change (will need a few days) - please let me know if this is 
> what you had in mind.
>
>
Yes, that's what I had in mind. But my preference would be that this
would be a new function in the ip core, for use by any encap protocol,
where appropriate.

[toc] | [prev] | [next] | [standalone]

#1498643

On 11/10/16 02:54, R Parameswaran wrote:
>
>
> Hi James,
>
> Please see inline:
>
> On Tue, Oct 4, 2016 at 12:53 AM, James Chapman <jchapman@katalix.com
> <mailto:jchapman@katalix.com>> wrote:
>
>     On 04/10/16 04:12, R. Parameswaran wrote:
>     >
>     > Hi James,
>     >
>     > Please see inline, thanks for the reply:
>     >
>     > On Sat, 1 Oct 2016, James Chapman wrote:
>     >
>     >> On 30/09/16 03:39, R. Parameswaran wrote:
>     >>>>> + /* Adjust MTU, factor overhead - underlay L3 hdr, overlay
>     L2 hdr*/
>     >>>>> + if (tunnel->sock->sk_family == AF_INET)
>     >>>>> +         overhead += (ETH_HLEN + sizeof(struct iphdr));
>     >>>>> + else if (tunnel->sock->sk_family == AF_INET6)
>     >>>>> +         overhead += (ETH_HLEN + sizeof(struct ipv6hdr));
>     >>>> What about options in the IP header? If certain options are
>     set on the
>     >>>> socket, the IP header may be larger.
>     >>>>
>     >>> Thanks for the reply - It looks like IP options can only be
>     >>> enabled through setsockopt on an application's socket (if
>     there's any
>     >>> other way to turn on IP options, please let me know - didn't
>     see any
>     >>> sysctl setting for transmit). This scenario would come
>     >>> into picture when an application opens a raw IP or UDP socket
>     such that it
>     >>> routes into the L2TP logical interface.
>     >> No. An L2TP daemon (userspace) will open a socket for each
>     tunnel that
>     >> it creates. Control and data packets use the same socket, which
>     is the
>     >> socket used by this code. It may set any options on its
>     sockets. L2TP
>     >> tunnel sockets can be created either by an L2TP daemon (managed
>     tunnels)
>     >> or by ip l2tp commands (unmanaged tunnels).
>     >>
>     > One Q I have is whether it would be sufficient to solve this for the
>     > common case (i.e no IP options) and have an expectation that the
>     > administrator will explicitly provision the mtu using the 'ip
>     link ...
>     > mtu'  command when dealing with infrequent occurences like IP
>     options?
>     >
>     > But looking at the code, it looks to be possible to pick up whether
>     > options are enabled and how long the options are, from the
>     ip_options struct
>     > embedded in the tunnel socket. If you want me to, I can repost
>     the patch
>     > with this change (will need a few days) - please let me know if
>     this is
>     > what you had in mind.
>     >
>     >
>     Yes, that's what I had in mind. But my preference would be that this
>     would be a new function in the ip core, for use by any encap protocol,
>     where appropriate.
>
> Discussed this with Nachi (nprachan), we were thinking of a new
> function in ip_sockglue.c which would take the tunnel socket as
> parameter, derive the underlay device MTU and compute the underlay L3
> overhead (IPv4/IPv6 header, UDP header if it is a UDP socket, and IP
> option length if the ip_options struct exists in the socket). The
> function would be agnostic to the tunnel type (although we could
> provision tunnel-type and encap type as parameters). Callers would
> call it to figure out the cumulative underlay L3 overhead and the
> underlay MTU, and then use these numbers in the MTU calculation for
> their specific tunnel type. Let me know if that is different from what
> you had in mind, and/or if you have any suggestions on which file to
> place this in. I'll try and have this re-posted  by the end of this
> week or by early next week.
>

I think keep it simple. A function to return the size of the IP header
associated with any IP socket, not necessarily a tunnel socket. Don't
mix in any MTU derivation logic or UDP header size etc.

Post code early as an RFC. You're more likely to get review feedback
from others.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web