> -----Original Message----- > From: Steffen Klassert [mailto:steffen.klass...@secunet.com] > Sent: Wednesday, June 21, 2017 2:38 PM > To: Yossi Kuperman <yoss...@mellanox.com> > Cc: netdev@vger.kernel.org; Herbert Xu <herb...@gondor.apana.org.au>; > Yevgeny Kliteynik <klit...@mellanox.com>; Boris Pismenny > <bor...@mellanox.com>; Ilan Tayari <il...@mellanox.com> > Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in > xfrm6_transport_finish > > On Wed, Jun 21, 2017 at 11:24:05AM +0000, Yossi Kuperman wrote: > > > > > > > -----Original Message----- > > > From: Steffen Klassert [mailto:steffen.klass...@secunet.com] > > > Sent: Wednesday, June 21, 2017 8:37 AM > > > To: Yossi Kuperman <yoss...@mellanox.com> > > > Cc: netdev@vger.kernel.org; Herbert Xu > > > <herb...@gondor.apana.org.au>; Yevgeny Kliteynik > > > <klit...@mellanox.com>; Boris Pismenny <bor...@mellanox.com>; Ilan > > > Tayari <il...@mellanox.com> > > > Subject: Re: [PATCH net 1/2] xfrm6: Fix IPv6 payload_len in > > > xfrm6_transport_finish > > > > > > On Mon, Jun 19, 2017 at 11:33:20AM +0300, yoss...@mellanox.com > wrote: > > > > From: Yossi Kuperman <yoss...@mellanox.com> > > > > > > > > IPv6 payload length indicates the size of the payload, including > > > > any extension headers. In xfrm6_transport_finish, > > > > ipv6_hdr(skb)->payload_len is set to the payload size only, > > > > regardless of the presence of any extension headers. > > > > > > > > After ESP GRO transport mode decapsulation, ipv6_rcv trims the > > > > packet according to the wrong payload_len, thus corrupting the packet. > > > > > > > > Set payload_len to account for extension headers as well. > > > > > > > > Fixes: 716062fd4c2f ("[IPSEC]: Merge most of the input path") > > > > Signed-off-by: Yossi Kuperman <yoss...@mellanox.com> > > > > --- > > > > net/ipv6/xfrm6_input.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index > > > > 08a807b..3ef5d91 100644 > > > > --- a/net/ipv6/xfrm6_input.c > > > > +++ b/net/ipv6/xfrm6_input.c > > > > @@ -43,8 +43,8 @@ int xfrm6_transport_finish(struct sk_buff *skb, > > > > int > > > async) > > > > return 1; > > > > #endif > > > > > > > > - ipv6_hdr(skb)->payload_len = htons(skb->len); > > > > __skb_push(skb, skb->data - skb_network_header(skb)); > > > > + ipv6_hdr(skb)->payload_len = htons(skb->len - sizeof(struct > > > > +ipv6hdr)); > > > > > > You mentioned that the bug happens with ESP GRO. Does this bug also > > > happen in the standard codepath? If not, you might better move the > > > above line into the 'if' section below. > > > > > > > Yes, it happens only with ESP GRO. > > In this case your Fixes tag is wrong. > > > Normally, ipv6_rcv happens first which trims the packet correctly and then > comes xfrm6_transport_finish. > > Now, with ESP GRO introduced, xfrm6_transport_finish, which comes > > first, modifies the packet's payload_len, and followed by a call to ipv6_rcv > which trims it wrongly. > > > > If I understand you right, you suggest to move the new line under the "if" > section and restore the removed line? > > I just want to make sure that the standard codepath works with this cange. > Did you test this with the standard codepath too?
Sure, it works when GRO is disabled (esp6_offload is not loaded). Tested: ping, iperf (TCP stream) and injected manually created packets with extension headers.
