netdev - Re: [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 28 Feb 2014 16:56:14 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	hkchu@...gle.com
Cc:	or.gerlitz@...il.com, edumazet@...gle.com, ogerlitz@...lanox.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path

From: Jerry Chu <hkchu@...gle.com>
Date: Thu, 27 Feb 2014 15:39:47 -0800

> On Thu, Feb 27, 2014 at 2:25 PM, Or Gerlitz <or.gerlitz@...il.com> wrote:
>>
>> On Thu, Feb 27, 2014 at 11:26 PM, H.K. Jerry Chu <hkchu@...gle.com> wrote:
>> > From: Jerry Chu <hkchu@...gle.com>
>> >
>> > I stumbled across a bug that was introduced by my own GRE-GRO
>> > patch (bf5a755f5e9186406bbf50f4087100af5bd68e40
>> > net-gre-gro: Add GRE support to the GRO stack) submitted a while
>> > back that breaks the forwarding path because various GSO related
>> > fields were not set. This will cause on the egress path either
>> > the GSO code to fail, or a GRE-TSO capable (NETIF_F_GSO_GRE)
>> > NICs to choke. The following fix has been tested for both cases.
>> >
>> > Signed-off-by: H.K. Jerry Chu <hkchu@...gle.com>
>> > ---
>> >  net/core/dev.c           | 2 ++
>> >  net/ipv4/af_inet.c       | 3 +++
>> >  net/ipv4/gre_offload.c   | 3 +++
>> >  net/ipv4/tcp_offload.c   | 2 +-
>> >  net/ipv6/tcpv6_offload.c | 2 +-
>> >  5 files changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/core/dev.c b/net/core/dev.c
>> > index b1b0c8d..75517d8 100644
>> > --- a/net/core/dev.c
>> > +++ b/net/core/dev.c
>> > @@ -4045,6 +4045,8 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
>> >         skb->vlan_tci = 0;
>> >         skb->dev = napi->dev;
>> >         skb->skb_iif = 0;
>> > +       skb->encapsulation = 0;
>> > +       skb_shinfo(skb)->gso_type = 0;
>> >
>> >         napi->skb = skb;
>> >  }
>> > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> > index ecd2c3f..68229ca 100644
>> > --- a/net/ipv4/af_inet.c
>> > +++ b/net/ipv4/af_inet.c
>> > @@ -1431,6 +1431,9 @@ static int inet_gro_complete(struct sk_buff *skb, int nhoff)
>> >         int proto = iph->protocol;
>> >         int err = -ENOSYS;
>> >
>> > +       if (skb->encapsulation)
>> > +               skb_set_inner_network_header(skb, nhoff);
>> > +
>>
>> Note that packets (e.g those who are UDP encapsulated) will pass here
>> twice, once for the external IP header and once for the internal, so
>> if the driver marks the encapsulation sign the inner header setting
>> will happen twice, is that what we want?
> 
> The inner most hdr will be the last to set. But I don't know
> all the rules regarding skb->encapsulation. E.g., in this patch
> napi_reuse_skb() will reset skb->encapsulation. If that's problematic
> then I could call skb_set_inner_network_header() inside gre_gro_receive()
> (but that will require checking against "type" in order to compute the right
> offset).

The topic of the skb->encapsulation semantics has come up several times in the
past few weeks.

We cannot move forward on any changes in this area until the semantics
are well defined, and documented.

Can someone work on a patch which documents skb->encapsulation properly,
and then we can come back to fixing this bug?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html