[PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

Given:
 - tap0, vxlan0 enslaved under a bridge
 - eth0 is the tunnel underlay having small mtu (e.g. 1400)

Assume GSO skbs arriving from tap0 having a gso_size as determined by
user-provided virtio_net_hdr (e.g. 1460 corresponding to VM mtu of 1500).

After encapsulation these skbs have skb_gso_network_seglen that exceed
underlay ip_skb_dst_mtu.

These skbs are accidentally passed to ip_finish_output2 AS IS; however
each final segment (either segmented by validate_xmit_skb of eth0, or
by eth0 hardware UFO) would be larger than eth0 mtu.
As a result, those above-mtu segments get dropped on certain underlay
networks.

The expected behavior in such a setup would be segmenting the skb first,
and then fragmenting each segment according to dst mtu, and finally
passing the resulting fragments to ip_finish_output2.

'ip_finish_output_gso' already supports this "Slowpath" behavior,
but it is only considered if IPSKB_FORWARDED is set.

However in the bridged case, IPSKB_FORWARDED is off, and the "Slowpath"
behavior is not considered.

Fix, by performing ip_finish_output_gso "Slowpath" even for non
IPSKB_FORWARDED skbs.

This is also OK for locally created skbs, as they likely to have
skb_gso_network_seglen that equals dst mtu, and thus will go directly to
'ip_finish_output2' as done prior this fix.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
---
 net/ipv4/ip_output.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index cbac493..8ae65b3 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -223,9 +223,8 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk,
 	struct sk_buff *segs;
 	int ret = 0;
 
-	/* common case: locally created skb or seglen is <= mtu */
-	if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||
-	      skb_gso_validate_mtu(skb, mtu))
+	/* common case: seglen is <= mtu */
+	if (skb_gso_validate_mtu(skb, mtu))
 		return ip_finish_output2(net, sk, skb);
 
 	/* Slowpath -  GSO segment length is exceeding the dst MTU.
-- 
1.9.1

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Florian Westphal]

Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> Given:
>  - tap0, vxlan0 enslaved under a bridge
>  - eth0 is the tunnel underlay having small mtu (e.g. 1400)
> 
> Assume GSO skbs arriving from tap0 having a gso_size as determined by
> user-provided virtio_net_hdr (e.g. 1460 corresponding to VM mtu of 1500).
> 
> After encapsulation these skbs have skb_gso_network_seglen that exceed
> underlay ip_skb_dst_mtu.
> 
> These skbs are accidentally passed to ip_finish_output2 AS IS; however
> each final segment (either segmented by validate_xmit_skb of eth0, or
> by eth0 hardware UFO) would be larger than eth0 mtu.
> As a result, those above-mtu segments get dropped on certain underlay
> networks.
> 
> The expected behavior in such a setup would be segmenting the skb first,
> and then fragmenting each segment according to dst mtu, and finally
> passing the resulting fragments to ip_finish_output2.
> 
> 'ip_finish_output_gso' already supports this "Slowpath" behavior,
> but it is only considered if IPSKB_FORWARDED is set.
> 
> However in the bridged case, IPSKB_FORWARDED is off, and the "Slowpath"
> behavior is not considered.

I placed this test there under the assumption that L2 bridges have
the same MTU on all bridge ports, so we'd only need to consider routing
case.

How does work if e.g. 1460-sized udp packet arrives on tap0?
Do we fragment (possibly ignoring DF?)

How does it work for non-ip protocols?

(Or did I misunderstand this setup...?)

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

On Tue, 5 Jul 2016 15:03:27 +0200, fw@strlen.de wrote:
> > The expected behavior in such a setup would be segmenting the skb first,
> > and then fragmenting each segment according to dst mtu, and finally
> > passing the resulting fragments to ip_finish_output2.
> > 
> > 'ip_finish_output_gso' already supports this "Slowpath" behavior,
> > but it is only considered if IPSKB_FORWARDED is set.
> > 
> > However in the bridged case, IPSKB_FORWARDED is off, and the "Slowpath"
> > behavior is not considered.
> 
> I placed this test there under the assumption that L2 bridges have
> the same MTU on all bridge ports, so we'd only need to consider routing
> case.

In our setups we have no control of VM mtu (which affects gso_size of
packets arriving from tap), and no control of vxlan's underlay mtu.

> How does work if e.g. 1460-sized udp packet arrives on tap0?
> Do we fragment (possibly ignoring DF?)

A *non* gso udp packet arriving on tap0 is bridged to vxlan0 (assuming
vxlan mtu is sufficient), and the original DF of the inner packet is
preserved.

The skb gets vxlan-udp encapsulated, with outer IP header having DF=0
(unless tun_flags & TUNNEL_DONT_FRAGMENT), and then, if skb->len > mtu,
fragmented normally at the ip_finish_output --> ip_fragment code path.

So on wire we have 2 frags of the vxlan datagram; they are reassembled
at recepient ip stack of vxlan termination. Inner packet preserved.
Not ideal, but works.

The issue is with GSO skbs arriving from tap, which eventually generates
segments larger then the mtu, which are not transmitted on eth0:

  tap0 rx:  super packet, gso_size from user's virtio_net_hdr
    ...
    vxlan0 tx:  encaps the super packet
      ...
      ip_finish_output
        ip_finish_output_gso
          *NO* skb_gso_validate_mtu()     <--- problem here
            ip_finish_output2:  tx the encapsulated super packet on eth0
              ...
              validate_xmit_skb
                netif_needs_gso
                  skb_gso_segment: segments inner payload according to
                                   original gso_size,
                                   leads to vxlan datagrams larger than mtu

> How does it work for non-ip protocols?

The specific problem is with vxlan (or any other udp based tunnel)
encapsulated GSOed packets.

> (Or did I misunderstand this setup...?)

tap0 bridged with vxlan0.
route to vxlan0's remote peer is via eth0, configured with small mtu.

Regards,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[David Miller]

From: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
Date: Tue, 5 Jul 2016 17:05:41 +0300

> On Tue, 5 Jul 2016 15:03:27 +0200, fw@strlen.de wrote:
>> (Or did I misunderstand this setup...?)
> 
> tap0 bridged with vxlan0.
> route to vxlan0's remote peer is via eth0, configured with small mtu.

Florian, any more comments?

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Florian Westphal]

David Miller <davem@davemloft.net> wrote:
> From: Shmulik Ladkani <shmulik.ladkani@ravellosystems.com>
> Date: Tue, 5 Jul 2016 17:05:41 +0300
> 
> > On Tue, 5 Jul 2016 15:03:27 +0200, fw@strlen.de wrote:
> >> (Or did I misunderstand this setup...?)
> > 
> > tap0 bridged with vxlan0.
> > route to vxlan0's remote peer is via eth0, configured with small mtu.
> 
> Florian, any more comments?

Sorry, I commented now.  But I'd really like to hear what vxlan experts
have to say about this, seems in RFC (7348) universe endpoint fragmention is
not supposed to happen :-/

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Florian Westphal]

Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:

[ CC 
> On Tue, 5 Jul 2016 15:03:27 +0200, fw@strlen.de wrote:
> > > The expected behavior in such a setup would be segmenting the skb first,
> > > and then fragmenting each segment according to dst mtu, and finally
> > > passing the resulting fragments to ip_finish_output2.
> > > 
> > > 'ip_finish_output_gso' already supports this "Slowpath" behavior,
> > > but it is only considered if IPSKB_FORWARDED is set.
> > > 
> > > However in the bridged case, IPSKB_FORWARDED is off, and the "Slowpath"
> > > behavior is not considered.
> > 
> > I placed this test there under the assumption that L2 bridges have
> > the same MTU on all bridge ports, so we'd only need to consider routing
> > case.
> 
> In our setups we have no control of VM mtu (which affects gso_size of
> packets arriving from tap), and no control of vxlan's underlay mtu.

:-(

> > How does work if e.g. 1460-sized udp packet arrives on tap0?
> > Do we fragment (possibly ignoring DF?)
> 
> A *non* gso udp packet arriving on tap0 is bridged to vxlan0 (assuming
> vxlan mtu is sufficient), and the original DF of the inner packet is
> preserved.
> 
> The skb gets vxlan-udp encapsulated, with outer IP header having DF=0
> (unless tun_flags & TUNNEL_DONT_FRAGMENT), and then, if skb->len > mtu,
> fragmented normally at the ip_finish_output --> ip_fragment code path.

I see.

> So on wire we have 2 frags of the vxlan datagram; they are reassembled
> at recepient ip stack of vxlan termination. Inner packet preserved.
> Not ideal, but works.
> 
> The issue is with GSO skbs arriving from tap, which eventually generates
> segments larger then the mtu, which are not transmitted on eth0:
> 
>   tap0 rx:  super packet, gso_size from user's virtio_net_hdr
>     ...
>     vxlan0 tx:  encaps the super packet
>       ...
>       ip_finish_output
>         ip_finish_output_gso
>           *NO* skb_gso_validate_mtu()     <--- problem here

We don't do this for ipv6 either since we're expected to send PKTOOBIG
error.

>             ip_finish_output2:  tx the encapsulated super packet on eth0
>               ...
>               validate_xmit_skb
>                 netif_needs_gso
>                   skb_gso_segment: segments inner payload according to
>                                    original gso_size,
>                                    leads to vxlan datagrams larger than mtu
>
> > How does it work for non-ip protocols?
> 
> The specific problem is with vxlan (or any other udp based tunnel)
> encapsulated GSOed packets.

If I understand correctly you have vxlan stacked on top of eth0, and tap
and vxlan in a bridge.

... and eth mtu smaller than bridge mtu.

I think that this is "working" by pure accident, and that better fix is
to set mtu values correctly so that when vxlan header is added we don't
exceed what can be handled by the real device (yes, I know you have
no control over this).

I am worried about this patch, skb_gso_validate_mtu is more costly than
the ->flags & FORWARD check; everyone pays this extra cost.

What about setting IPCB FORWARD flag in iptunnel_xmit if
skb->skb_iif != 0... instead?

Yet another possibility would be to reduce gso_size but that violates
gro/gso symmetry...

[ I tried to check rfc but seems rfc7348 simply declares that
  endpoints are not allowed to fragment so problem solved :-/ ]

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

On Sat, 9 Jul 2016 11:00:20 +0200 Florian Westphal <fw@strlen.de> wrote:
> Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> > > How does work if e.g. 1460-sized udp packet arrives on tap0?
> > > Do we fragment (possibly ignoring DF?)  
> > 
> > A *non* gso udp packet arriving on tap0 is bridged to vxlan0 (assuming
> > vxlan mtu is sufficient), and the original DF of the inner packet is
> > preserved.
> > 
> > The skb gets vxlan-udp encapsulated, with outer IP header having DF=0
> > (unless tun_flags & TUNNEL_DONT_FRAGMENT), and then, if skb->len > mtu,
> > fragmented normally at the ip_finish_output --> ip_fragment code path.  
> 
> I see.
> 
> If I understand correctly you have vxlan stacked on top of eth0, and tap
> and vxlan in a bridge.
> 
> ... and eth mtu smaller than bridge mtu.
> 
> I think that this is "working" by pure accident, and that better fix is
> to set mtu values correctly so that when vxlan header is added we don't
> exceed what can be handled by the real device (yes, I know you have
> no control over this).

Let me elaborate a bit regarding the usecase.

Consider nested virtualization. The "host" is a VM which may run in
various different cloud deployments, thus the mtu of this virtual host's
eth0 varies (I've no control over it, nor should I have).

The "host" provides a virtual network for its Nested Guest VMs - the
users of the system.
They are provided with a virtual L2 network with an MTU of their choice
(usually 1500).

Forcing the users runtime varying restrictions on whatever MTUs they
can use in their virtual network (which depend on the current choice of
where the virtual "host" is deployed), means they are forced to alter
their application's setting, per deployment. This is a non solution.

This is why guests' MTUs nor host's eth0 MTU can be set.

We have the option of using user-space based UDP tunnel (instead of
kernel's vxlan or geneve). Aside the downsides not utilizing existing
protocols and implementations, this works well as the encapsulated guest
packets are sent over a standard UDP socket via sendmsg.
As such, these datagrams may get fragmented (in deployments where host's
eth0 mtu is too small), and reassembled at the remote tunnel
termination's ip stack.

Regradless the use-case, there's currently an incosistency in kernel's
behavior:

Consider:

   VM
  eth0  # 1500 mtu; any virtual/emulated NIC that supports TSO
   .
   .
+---------------HOST-+
|  .   __br0__       |
|  .  /       \      |
| tap0       vxlan0  |
|(1500)      (1500)  |
|              .     |
|              .     |
|             eth0   |
|            (1200)  |
+--------------------+

1. If VM disables TSO, it sends 1500-sized IP packets down eth0,
   the frames arrive on tap0, bridged, get encapsulated by vxlan0, and
   the vxlan datagram is then fragmented (as any local datagram would
   have) by ip_finish_output on eth0.

2. If VM enables TSO, we have have a superpacket arriving at tap0
   with total length of say 10000 bytes, with gso_size 1460.
   The superpacket gets encapsulated by vxlan0.
   Finally upon eth0's validate_xmit_skb, the packet is udp-tunnel-segmented
   according to original gso_size of 1460, creating encapsution
   datagrams bigger than eth0's mtu - which are eventually dropped on
   the wire.

This is simply inconsistent: The GSO path should align to the non-GSO
case.
Thus my suggestion: in this specific case, within ip_finish_output_gso,
segment the GSO skb first, then fragment each segment according to dst
mtu. This aligns the GSO vs non-GSO behavior.

> I am worried about this patch, skb_gso_validate_mtu is more costly than
> the ->flags & FORWARD check; everyone pays this extra cost.

I can get back with numbers regarding the impact on local traffic.

I'd appreciate any suggestion how to determine traffic is local OTHER
THAN testing IPSKB_FORWARDED; If we have such a way, there wouldn't be an
impact on local traffic.

> What about setting IPCB FORWARD flag in iptunnel_xmit if
> skb->skb_iif != 0... instead?

Can you please elaborate?

> Yet another possibility would be to reduce gso_size but that violates
> gro/gso symmetry...

We're experimenting this path as well. But as said, fixing the
incosistency above would still be valid.

> [ I tried to check rfc but seems rfc7348 simply declares that
>   endpoints are not allowed to fragment so problem solved :-/ ]

Funny, in Geneve it's only a "best practice" recommendadtion:
  https://tools.ietf.org/html/draft-ietf-nvo3-geneve-02
  section 4.1.1

I'm not keen on vxlan; any UDP based tunnel would do ;-)

Regards,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Florian Westphal]

Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> I'd appreciate any suggestion how to determine traffic is local OTHER
> THAN testing IPSKB_FORWARDED; If we have such a way, there wouldn't be an
> impact on local traffic.
> 
> > What about setting IPCB FORWARD flag in iptunnel_xmit if
> > skb->skb_iif != 0... instead?
> 
> Can you please elaborate?

[ not even compile tested ]

diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -65,6 +65,7 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
 	struct net_device *dev = skb->dev;
 	struct iphdr *iph;
 	int err;
+	bool fwd = skb->skb_iif > 0;
 
 	skb_scrub_packet(skb, xnet);
 
@@ -72,6 +73,9 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
 	skb_dst_set(skb, &rt->dst);
 	memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
 
+	if (fwd)
+		IPCB(skb)->flags = IPSKB_FORWARDED;
+
 	/* Push down and install the IP header. */
 	skb_push(skb, sizeof(struct iphdr));
 	skb_reset_network_header(skb);

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

On Sat, 9 Jul 2016 15:22:30 +0200, fw@strlen.de wrote:
> Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> > I'd appreciate any suggestion how to determine traffic is local OTHER
> > THAN testing IPSKB_FORWARDED; If we have such a way, there wouldn't be an
> > impact on local traffic.
> > 
> > > What about setting IPCB FORWARD flag in iptunnel_xmit if
> > > skb->skb_iif != 0... instead?
> > 
> > Can you please elaborate?
> 
> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -65,6 +65,7 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
>  	struct net_device *dev = skb->dev;
>  	struct iphdr *iph;
>  	int err;
> +	bool fwd = skb->skb_iif > 0;
>  
>  	skb_scrub_packet(skb, xnet);
>  
> @@ -72,6 +73,9 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
>  	skb_dst_set(skb, &rt->dst);
>  	memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
>  
> +	if (fwd)
> +		IPCB(skb)->flags = IPSKB_FORWARDED;
> +

Thanks.

OK with the general idea; However I don't want to abuse IPSKB_FORWARDED
as the skb is not really "ip forwarded", and it might have undesirable
affects, such as in 'ip_skb_dst_mtu' which follows.

How about a new IPSKB bit flag, say IPSKB_TUNNEL_FORWARDED, or a
different way of marking?

Regards,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Florian Westphal]

Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> OK with the general idea; However I don't want to abuse IPSKB_FORWARDED
> as the skb is not really "ip forwarded", and it might have undesirable
> affects, such as in 'ip_skb_dst_mtu' which follows.

I don't see a problem with that. Hannes?

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Hannes Frederic Sowa]

On 11.07.2016 04:15, Florian Westphal wrote:
> Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
>> OK with the general idea; However I don't want to abuse IPSKB_FORWARDED
>> as the skb is not really "ip forwarded", and it might have undesirable
>> affects, such as in 'ip_skb_dst_mtu' which follows.
> 
> I don't see a problem with that. Hannes?

If we segment/fragment along the way we need to use the
non-pmtu/interface-mtu, so this change would be correct in regard to the
mtu selection.

Bye,
Hannes

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

On Sat, 9 Jul 2016 15:22:30 +0200 Florian Westphal <fw@strlen.de> wrote:
> Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> > I'd appreciate any suggestion how to determine traffic is local OTHER
> > THAN testing IPSKB_FORWARDED; If we have such a way, there wouldn't be an
> > impact on local traffic.
> >   
> > > What about setting IPCB FORWARD flag in iptunnel_xmit if
> > > skb->skb_iif != 0... instead?  

I've came up with a suggestion that does not abuse IPSKB_FORWARDED,
while properly addressing the use case (and similar ones), without
introducing the cost of entering 'skb_gso_validate_mtu' in the local
case.

How about:

@@ -220,12 +220,15 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk,
 				struct sk_buff *skb, unsigned int mtu)
 {
 	netdev_features_t features;
+	int local_trusted_gso;
 	struct sk_buff *segs;
 	int ret = 0;
 
-	/* common case: locally created skb or seglen is <= mtu */
-	if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||
-	      skb_gso_validate_mtu(skb, mtu))
+	local_trusted_gso = (IPCB(skb)->flags & IPSKB_FORWARDED) == 0 &&
+			    !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY);
+	/* common case: locally created skb from a trusted gso source or
+	 * seglen is <= mtu */
+	if (local_trusted_gso || skb_gso_validate_mtu(skb, mtu))
 		return ip_finish_output2(net, sk, skb);
 
 	/* Slowpath -  GSO segment length is exceeding the dst MTU.

This well addresses the usecase where we have gso-skb arriving from an
untrusted source, thus its gso_size is out of our control (e.g. tun/tap,
macvtap, af_packet, xen-netfront...).

Locally "gso trusted" skbs (the common case) will NOT suffer the
additional (possibly costy) call to 'skb_gso_validate_mtu'.

Also, if IPSKB_FORWARDED is true, behavior stays exactly the same.

Regards,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

Hi Florian, Hannes,

On Tue, 12 Jul 2016 08:56:56 +0300 Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> On Sat, 9 Jul 2016 15:22:30 +0200 Florian Westphal <fw@strlen.de> wrote:
> > >     
> > > > What about setting IPCB FORWARD flag in iptunnel_xmit if
> > > > skb->skb_iif != 0... instead?    
> 
> I've came up with a suggestion that does not abuse IPSKB_FORWARDED,
> while properly addressing the use case (and similar ones), without
> introducing the cost of entering 'skb_gso_validate_mtu' in the local
> case.
> 
> How about:
> 
> @@ -220,12 +220,15 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk,
>  				struct sk_buff *skb, unsigned int mtu)
>  {
>  	netdev_features_t features;
> +	int local_trusted_gso;
>  	struct sk_buff *segs;
>  	int ret = 0;
>  
> -	/* common case: locally created skb or seglen is <= mtu */
> -	if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||
> -	      skb_gso_validate_mtu(skb, mtu))
> +	local_trusted_gso = (IPCB(skb)->flags & IPSKB_FORWARDED) == 0 &&
> +			    !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY);
> +	/* common case: locally created skb from a trusted gso source or
> +	 * seglen is <= mtu */
> +	if (local_trusted_gso || skb_gso_validate_mtu(skb, mtu))
>  		return ip_finish_output2(net, sk, skb);
>  
>  	/* Slowpath -  GSO segment length is exceeding the dst MTU.
> 
> This well addresses the usecase where we have gso-skb arriving from an
> untrusted source, thus its gso_size is out of our control (e.g. tun/tap,
> macvtap, af_packet, xen-netfront...).
> 
> Locally "gso trusted" skbs (the common case) will NOT suffer the
> additional (possibly costy) call to 'skb_gso_validate_mtu'.
> 
> Also, if IPSKB_FORWARDED is true, behavior stays exactly the same.

Any commnets regarding the latest suggestion above?
I'd like to post it as v2 - if it is in the right direction.

It handles the problem of gso_size values which are not in host's
control, it addresses the usecase described, and has a benefit of not
overloading IPSKB_FORWARDED with a new semantic that might be hard to
maintain.

PS:
Also, if we'd like to pinpoint it even further, we can:

local_trusted_gso = (IPCB(skb)->flags & IPSKB_FORWARDED) == 0 &&
		    (!sk || !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY));

Which ensures only the following conditions go to the expensive
skb_gso_validate_mtu:

1. IPSKB_FORWARDED is on
2. IPSKB_FORWARDED is off, but sk exists and gso_size is untrusted.
   Meaning: we have a packet arriving from higher layers (sk is set)
   with a gso_size out of host's control.

This fine-tuining leaves standard l2 bridging case (e.g 2x taps bridged)
of a gso skb unaffected, as sk would be NULL.

Many thanks,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Hannes Frederic Sowa]

Hello Shmulik,

On Wed, Jul 13, 2016, at 16:00, Shmulik Ladkani wrote:
> Hi Florian, Hannes,
> 
> On Tue, 12 Jul 2016 08:56:56 +0300 Shmulik Ladkani
> <shmulik.ladkani@ravellosystems.com> wrote:
> > On Sat, 9 Jul 2016 15:22:30 +0200 Florian Westphal <fw@strlen.de> wrote:
> > > >     
> > > > > What about setting IPCB FORWARD flag in iptunnel_xmit if
> > > > > skb->skb_iif != 0... instead?    
> > 
> > I've came up with a suggestion that does not abuse IPSKB_FORWARDED,
> > while properly addressing the use case (and similar ones), without
> > introducing the cost of entering 'skb_gso_validate_mtu' in the local
> > case.
> > 
> > How about:
> > 
> > @@ -220,12 +220,15 @@ static int ip_finish_output_gso(struct net *net, struct sock *sk,
> >  				struct sk_buff *skb, unsigned int mtu)
> >  {
> >  	netdev_features_t features;
> > +	int local_trusted_gso;
> >  	struct sk_buff *segs;
> >  	int ret = 0;
> >  
> > -	/* common case: locally created skb or seglen is <= mtu */
> > -	if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||
> > -	      skb_gso_validate_mtu(skb, mtu))
> > +	local_trusted_gso = (IPCB(skb)->flags & IPSKB_FORWARDED) == 0 &&
> > +			    !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY);
> > +	/* common case: locally created skb from a trusted gso source or
> > +	 * seglen is <= mtu */
> > +	if (local_trusted_gso || skb_gso_validate_mtu(skb, mtu))
> >  		return ip_finish_output2(net, sk, skb);
> >  
> >  	/* Slowpath -  GSO segment length is exceeding the dst MTU.
> > 
> > This well addresses the usecase where we have gso-skb arriving from an
> > untrusted source, thus its gso_size is out of our control (e.g. tun/tap,
> > macvtap, af_packet, xen-netfront...).
> > 
> > Locally "gso trusted" skbs (the common case) will NOT suffer the
> > additional (possibly costy) call to 'skb_gso_validate_mtu'.
> > 
> > Also, if IPSKB_FORWARDED is true, behavior stays exactly the same.

Sorry for the late reply, I am right now travelling and can't review
that closely.

> Any commnets regarding the latest suggestion above?
> I'd like to post it as v2 - if it is in the right direction.
> 
> It handles the problem of gso_size values which are not in host's
> control, it addresses the usecase described, and has a benefit of not
> overloading IPSKB_FORWARDED with a new semantic that might be hard to
> maintain.

I liked the fact that setting IPSKB_FORWARDED was only contained in
vxlan and as such wouldn't have as much impact. It was more logically
easy to review for me actually.

> PS:
> Also, if we'd like to pinpoint it even further, we can:
> 
> local_trusted_gso = (IPCB(skb)->flags & IPSKB_FORWARDED) == 0 &&
> 		    (!sk || !(skb_shinfo(skb)->gso_type & SKB_GSO_DODGY));

This also looks valid but too random. It seems to be a mix of random
conditions to make it work. ;)

> 
> Which ensures only the following conditions go to the expensive
> skb_gso_validate_mtu:
> 
> 1. IPSKB_FORWARDED is on
> 2. IPSKB_FORWARDED is off, but sk exists and gso_size is untrusted.
>    Meaning: we have a packet arriving from higher layers (sk is set)
>    with a gso_size out of host's control.

When can this really happen? In general we don't want to refragment gso
skb's and I think we can only make an exception for vxlan or udp.

> This fine-tuining leaves standard l2 bridging case (e.g 2x taps bridged)
> of a gso skb unaffected, as sk would be NULL.

Bridging does not in general orphan the socket, no?

Bye,
Hannes

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

Hi,

On Thu, 14 Jul 2016 15:12:07 +0200, hannes@stressinduktion.org wrote:
> I liked the fact that setting IPSKB_FORWARDED was only contained in
> vxlan and as such wouldn't have as much impact. It was more logically
> easy to review for me actually.

I agree here. It is rather safe and to the point.

I'm trying to exaust other alternatives because it has one potential
drawback: the name IPSKB_FORWARDED suggests ipv4 forwarding had
happened. Indeed, current setters of IPSKB_FORWARDED are ip_forward and
ip_mr_forward.

If we set IPSKB_FORWARDED in iptunnel_xmit, with packet not being ipv4
forwarded (e.g. bridged from some ingress device to a tunnel device), it
presents a nuance whose impact is yet to be determined.

For example, what about a packet that gets encapsulated and sent to a
multicast destination? The condition controlling mc loop-back in
ip_mc_output is affected by the flag.

> > Which ensures only the following conditions go to the expensive
> > skb_gso_validate_mtu:
> > 
> > 1. IPSKB_FORWARDED is on
> > 2. IPSKB_FORWARDED is off, but sk exists and gso_size is untrusted.
> >    Meaning: we have a packet arriving from higher layers (sk is set)
> >    with a gso_size out of host's control.
> 
> When can this really happen? In general we don't want to refragment gso
> skb's and I think we can only make an exception for vxlan or udp.

When IPSKB_FORWARDED is off, we'll get SKB_GSO_DODGY if packet
originally arrived from tap/macvtap/packet and it did NOT pass ipv4
forwarding (e.g bridges: tap0 to eth0 bridge, or tap0 to vxlan0 bridge).

The rationale: in the SKB_GSO_DODGY cases, the gso_size is given by
the user's virtio-net header, which is not in kernel's control.

This exactly resembles the usecase: tap0 gives packets with gso_size
unsuitable for encapsulation and segmentation. I have no control on
the source that gives those packets.

If (1) it does not make sense, or (2) considered too broad-spectrum to
asses, then we can go with the safer IPSKB_FORWARDED approach.

Let me know.

Regards,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Hannes Frederic Sowa]

Hello,

On Thu, Jul 14, 2016, at 16:13, Shmulik Ladkani wrote:
> Hi,
> 
> On Thu, 14 Jul 2016 15:12:07 +0200, hannes@stressinduktion.org wrote:
> > I liked the fact that setting IPSKB_FORWARDED was only contained in
> > vxlan and as such wouldn't have as much impact. It was more logically
> > easy to review for me actually.
> 
> I agree here. It is rather safe and to the point.
> 
> I'm trying to exaust other alternatives because it has one potential
> drawback: the name IPSKB_FORWARDED suggests ipv4 forwarding had
> happened. Indeed, current setters of IPSKB_FORWARDED are ip_forward and
> ip_mr_forward.
> 
> If we set IPSKB_FORWARDED in iptunnel_xmit, with packet not being ipv4
> forwarded (e.g. bridged from some ingress device to a tunnel device), it
> presents a nuance whose impact is yet to be determined.

The reason why I rather don't want to see a general solution is that
currently gre and ipip are pmtu-safe and I rather would like to keep the
old behavior that gre and ipip packets don't get treated alike. I
couldn't check the current throughly but it seems they would also be
affected by this change.

My idea was to put those IPSKB_FORWARD just into the vxlan and geneve
endpoints which could be seen as UDP endpoints and don't forward and
care about pmtu events.

> For example, what about a packet that gets encapsulated and sent to a
> multicast destination? The condition controlling mc loop-back in
> ip_mc_output is affected by the flag.

I have to look at that more closely after my trip.

What about adding a new flag with a proper name. It shouldn't affect
performance in any way if you check for two bits instead of just the
FORWARDED one.

> > > Which ensures only the following conditions go to the expensive
> > > skb_gso_validate_mtu:
> > > 
> > > 1. IPSKB_FORWARDED is on
> > > 2. IPSKB_FORWARDED is off, but sk exists and gso_size is untrusted.
> > >    Meaning: we have a packet arriving from higher layers (sk is set)
> > >    with a gso_size out of host's control.
> > 
> > When can this really happen? In general we don't want to refragment gso
> > skb's and I think we can only make an exception for vxlan or udp.
> 
> When IPSKB_FORWARDED is off, we'll get SKB_GSO_DODGY if packet
> originally arrived from tap/macvtap/packet and it did NOT pass ipv4
> forwarding (e.g bridges: tap0 to eth0 bridge, or tap0 to vxlan0 bridge).
> 
> The rationale: in the SKB_GSO_DODGY cases, the gso_size is given by
> the user's virtio-net header, which is not in kernel's control.

But we can assume it being correct based on the mtu of the interface
which is not in control of the current namespace instance or in another
vm. We should act accordingly to normal ethernet bridging here, IMHO.

> This exactly resembles the usecase: tap0 gives packets with gso_size
> unsuitable for encapsulation and segmentation. I have no control on
> the source that gives those packets.

My argumentation is that vxlan and geneve can be seen as endpoints and
don't have proper pmtu handling. Thus I would be fine with adding hacks
to just make it working. For GRE I would like to keep strict internet
pmtu handling and also keep the normal ethernet semantics in place, that
we should drop packets if another interface did transmit on an ethernet
bridge with a too big frame size.

> If (1) it does not make sense, or (2) considered too broad-spectrum to
> asses, then we can go with the safer IPSKB_FORWARDED approach.

Please have a look at my reasoning. I probably can follow-up more deeply
from Sunday on.

Thanks,
Hannes

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Shmulik Ladkani]

On Sat, 9 Jul 2016 15:30:17 +0300 Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> On Sat, 9 Jul 2016 11:00:20 +0200 Florian Westphal <fw@strlen.de> wrote:
> > I am worried about this patch, skb_gso_validate_mtu is more costly than
> > the ->flags & FORWARD check; everyone pays this extra cost.  
> 
> I can get back with numbers regarding the impact on local traffic.

Florian, I've repeatedly tested how this affects locally generated
traffic and it seems there's no impact (or at least too negligible for
netperf workload to notice):

- veth to veth (netns separated), both UFO enabled
- UDP_RR, with Request Size (udp payload) of 1500, to ensure UFO in place
  (sender reaches ip_finish_output_gso)
- Before: stable v4.6.3
- After:  stable v4.6.3 + suggested fix (kill flags&IPSKB_FORWARDED check)

# netperf -T1,2 -c -C -L 192.168.13.2 -H 192.168.13.1 -l 20 -I 99,2 -i 10 -t UDP_RR -- -P 10002,10001 -r 1500,1

MIGRATED UDP REQUEST/RESPONSE TEST from 192.168.13.2 () port 10002 AF_INET to 192.168.13.1 () port 10001 AF_INET : +/-1.000% @ 99% conf.  : demo : first burst 0 : cpu bind
Local /Remote
Socket Size   Request Resp.  Elapsed Trans.   CPU    CPU    S.dem   S.dem
Send   Recv   Size    Size   Time    Rate     local  remote local   remote
bytes  bytes  bytes   bytes  secs.   per sec  % S    % S    us/Tr   us/Tr

[before]
212992 212992 1500    1      20.00   45368.10   20.56  20.56  18.131  18.131
[after]
212992 212992 1500    1      20.00   45376.09   20.74  20.74  18.287  18.287

Therefore it seems trying to optimize this by setting IPSKB_FORWARDED
(or introducing a different mark) in 'iptunnel_xmit' would offer no
genuine benefit.

WDYT?

Were you thinking of different workloads that we should assess the
impact on?

Thanks,
Shmulik

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Florian Westphal]

Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> On Sat, 9 Jul 2016 15:30:17 +0300 Shmulik Ladkani <shmulik.ladkani@ravellosystems.com> wrote:
> > On Sat, 9 Jul 2016 11:00:20 +0200 Florian Westphal <fw@strlen.de> wrote:
> > > I am worried about this patch, skb_gso_validate_mtu is more costly than
> > > the ->flags & FORWARD check; everyone pays this extra cost.  
> > 
> > I can get back with numbers regarding the impact on local traffic.
> 
> Florian, I've repeatedly tested how this affects locally generated
> traffic and it seems there's no impact (or at least too negligible for
> netperf workload to notice):
> 
> - veth to veth (netns separated), both UFO enabled
> - UDP_RR, with Request Size (udp payload) of 1500, to ensure UFO in place
>   (sender reaches ip_finish_output_gso)
> - Before: stable v4.6.3
> - After:  stable v4.6.3 + suggested fix (kill flags&IPSKB_FORWARDED check)
> 
> # netperf -T1,2 -c -C -L 192.168.13.2 -H 192.168.13.1 -l 20 -I 99,2 -i 10 -t UDP_RR -- -P 10002,10001 -r 1500,1
> 
> MIGRATED UDP REQUEST/RESPONSE TEST from 192.168.13.2 () port 10002 AF_INET to 192.168.13.1 () port 10001 AF_INET : +/-1.000% @ 99% conf.  : demo : first burst 0 : cpu bind
> Local /Remote
> Socket Size   Request Resp.  Elapsed Trans.   CPU    CPU    S.dem   S.dem
> Send   Recv   Size    Size   Time    Rate     local  remote local   remote
> bytes  bytes  bytes   bytes  secs.   per sec  % S    % S    us/Tr   us/Tr
> 
> [before]
> 212992 212992 1500    1      20.00   45368.10   20.56  20.56  18.131  18.131
> [after]
> 212992 212992 1500    1      20.00   45376.09   20.74  20.74  18.287  18.287
> 
> Therefore it seems trying to optimize this by setting IPSKB_FORWARDED
> (or introducing a different mark) in 'iptunnel_xmit' would offer no
> genuine benefit.
> 
> WDYT?

I think that this just shows that on this machine and this workload
there is no impact.  E.g. for sctp we now skb_walk_frags() for
every gso packet.

We also have two more function calls per skb.

> Were you thinking of different workloads that we should assess the
> impact on?

I still think that just marking those packets as being forwarded from
tunnel code is the right thing to do; linux runs on so many platforms
that its very difficult to asses that something "has no overhead".

Long term we should just adjust gso size to not have to do skb software
segmentation anymore.

I remember that Eric Dumazet suggested the same thing back when this
gso forward checking was added but I recall there were corner cases in
the segmentation code that caused BUG_ON crashes.

Re: [PATCH] net: ip_finish_output_gso: If skb_gso_network_seglen exceeds MTU, do segmentation even for non IPSKB_FORWARDED skbs

[Hannes Frederic Sowa]

On 09.07.2016 05:00, Florian Westphal wrote:
> I am worried about this patch, skb_gso_validate_mtu is more costly than
> the ->flags & FORWARD check; everyone pays this extra cost.
> 
> What about setting IPCB FORWARD flag in iptunnel_xmit if
> skb->skb_iif != 0... instead?

That came to my mind first, too.

> Yet another possibility would be to reduce gso_size but that violates
> gro/gso symmetry...

If you see vxlan (or any other UDP encap protocol) as an in-kernel
tunnel solution/program acting as an end point it is actually legal to
modify gso_size in my opinion. Adding headers to an skb can also not be
symmetric in this case. I would not see anything bad happening because
of doing so. Do you? It is a matter of implementation. vxlan could also
eat all data and splice it anew onto a socket. It already doesn't care
about end-to-end pmtu consistency, so I can't see anything wrong with it.

To make this all safe one needs to handle the ttl in vxlan much better.
It needs to be inherited from the inner header, checked and properly
decremented on the outer header, so that vxlan itself acts as a full
blown router by itself. Otherwise endless loops are possible, as the
packet will always fit the size.

Bye,
Hannes