[PATCH net-next] net: remove useless check in napi_gro_frags()

[PATCH net-next] net: remove useless check in napi_gro_frags()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Checking if skb is NULL in napi_gro_frags() is too late.

If skb was NULL, we would crash earlier in napi_frags_skb()

Drivers normally catch napi_get_frags() NULL return value
before calling napi_gro_frags()

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 41cef3e3f558b857a9093a83f6b0c73f32b8b45f..8d8d34ff68f5800dbcb2958b6a937b9db478e359 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4465,9 +4465,6 @@ gro_result_t napi_gro_frags(struct napi_struct *napi)
 {
 	struct sk_buff *skb = napi_frags_skb(napi);
 
-	if (!skb)
-		return GRO_DROP;
-
 	trace_napi_gro_frags_entry(skb);
 
 	return napi_frags_finish(napi, skb, dev_gro_receive(napi, skb));

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[Alexei Starovoitov]

On Thu, Nov 19, 2015 at 11:42:50AM -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Checking if skb is NULL in napi_gro_frags() is too late.
> 
> If skb was NULL, we would crash earlier in napi_frags_skb()
> 
> Drivers normally catch napi_get_frags() NULL return value
> before calling napi_gro_frags()
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/dev.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 41cef3e3f558b857a9093a83f6b0c73f32b8b45f..8d8d34ff68f5800dbcb2958b6a937b9db478e359 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4465,9 +4465,6 @@ gro_result_t napi_gro_frags(struct napi_struct *napi)
>  {
>  	struct sk_buff *skb = napi_frags_skb(napi);
>  
> -	if (!skb)
> -		return GRO_DROP;
> -

I don't get it. napi_frags_skb() can return valid NULL.
Should it be fixed there as well?
Must be missing something...

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[Eric Dumazet]

On Thu, 2015-11-19 at 11:54 -0800, Alexei Starovoitov wrote:
> On Thu, Nov 19, 2015 at 11:42:50AM -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Checking if skb is NULL in napi_gro_frags() is too late.
> > 
> > If skb was NULL, we would crash earlier in napi_frags_skb()
> > 
> > Drivers normally catch napi_get_frags() NULL return value
> > before calling napi_gro_frags()
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/core/dev.c |    3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 41cef3e3f558b857a9093a83f6b0c73f32b8b45f..8d8d34ff68f5800dbcb2958b6a937b9db478e359 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4465,9 +4465,6 @@ gro_result_t napi_gro_frags(struct napi_struct *napi)
> >  {
> >  	struct sk_buff *skb = napi_frags_skb(napi);
> >  
> > -	if (!skb)
> > -		return GRO_DROP;
> > -
> 
> I don't get it. napi_frags_skb() can return valid NULL.
> Should it be fixed there as well?
> Must be missing something...


How many times should we crash before napi_frags_skb() returns NULL ?

At least one time, and this is enough really ...


static struct sk_buff *napi_frags_skb(struct napi_struct *napi)
{
        struct sk_buff *skb = napi->skb;
        const struct ethhdr *eth;
        unsigned int hlen = sizeof(*eth);

        napi->skb = NULL;

        skb_reset_mac_header(skb);
        skb_gro_reset_offset(skb);

        eth = skb_gro_header_fast(skb, 0);
        if (unlikely(skb_gro_header_hard(skb, hlen))) {
                eth = skb_gro_header_slow(skb, hlen, 0);
                if (unlikely(!eth)) {
                        napi_reuse_skb(napi, skb);
                        return NULL;
                }
        } else {
                gro_pull_from_frag0(skb, hlen);
                NAPI_GRO_CB(skb)->frag0 += hlen;
                NAPI_GRO_CB(skb)->frag0_len -= hlen;
        }
        __skb_pull(skb, hlen);
        /*
         * This works because the only protocols we care about don't require
         * special handling.
         * We'll fix it up properly in napi_frags_finish()
         */
        skb->protocol = eth->h_proto;

        return skb;
}

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[Bjørn Mork]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> How many times should we crash before napi_frags_skb() returns NULL ?
..
>                         return NULL;

Huh?  Now I'm lost here, too.


Bjørn

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[Eric Dumazet]

On Thu, 2015-11-19 at 21:20 +0100, Bjørn Mork wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
> > How many times should we crash before napi_frags_skb() returns NULL ?
> ..
> >                         return NULL;
> 
> Huh?  Now I'm lost here, too.


Well, Ethernet drivers should not feed GRO with frames with less than 14 bytes.

( eth_type_trans() would crash the same )

Lets fix buggy drivers instead of adding defensive code all over the stack.

napi_gro_frags() is used by exactly 10 drivers, and I am pretty sure they are OK.

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[Aaron Conole]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Thu, 2015-11-19 at 21:20 +0100, Bjørn Mork wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> 
>> > How many times should we crash before napi_frags_skb() returns NULL ?
>> ..
>> >                         return NULL;
>> 
>> Huh?  Now I'm lost here, too.
>
>
> Well, Ethernet drivers should not feed GRO with frames with less than 14 bytes.
>
> ( eth_type_trans() would crash the same )
>
> Lets fix buggy drivers instead of adding defensive code all over the stack.
>
> napi_gro_frags() is used by exactly 10 drivers, and I am pretty sure
> they are OK.
>

Would the following be an appropriate change in addition to the one
you've posted, then? If so I can repost as a formal patch, if you'd
like. At present, there's only one user of napi_frags_skb(), and your
patch removes the NULL check. If this can really only be the result of
buggy driver, then perhaps we should just call out the bug?

diff --git a/net/core/dev.c b/net/core/dev.c
index 41cef3e..b71c8b4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4440,10 +4440,7 @@ static struct sk_buff *napi_frags_skb(struct napi_struct *napi)
 	eth = skb_gro_header_fast(skb, 0);
 	if (unlikely(skb_gro_header_hard(skb, hlen))) {
 		eth = skb_gro_header_slow(skb, hlen, 0);
-		if (unlikely(!eth)) {
-			napi_reuse_skb(napi, skb);
-			return NULL;
-		}
+		BUG_ON(!eth);
 	} else {
 		gro_pull_from_frag0(skb, hlen);
 		NAPI_GRO_CB(skb)->frag0 += hlen;

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[Eric Dumazet]

On Thu, 2015-11-19 at 16:06 -0500, Aaron Conole wrote:

> >
> 
> Would the following be an appropriate change in addition to the one
> you've posted, then? If so I can repost as a formal patch, if you'd
> like. At present, there's only one user of napi_frags_skb(), and your
> patch removes the NULL check. If this can really only be the result of
> buggy driver, then perhaps we should just call out the bug?

Lets mark my patch as "premature" optimization, and revisit whole thing
after audit of the 10 drivers using this interface ;)

Thanks.

Re: [PATCH net-next] net: remove useless check in napi_gro_frags()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Nov 2015 13:43:45 -0800

> On Thu, 2015-11-19 at 16:06 -0500, Aaron Conole wrote:
> 
>> >
>> 
>> Would the following be an appropriate change in addition to the one
>> you've posted, then? If so I can repost as a formal patch, if you'd
>> like. At present, there's only one user of napi_frags_skb(), and your
>> patch removes the NULL check. If this can really only be the result of
>> buggy driver, then perhaps we should just call out the bug?
> 
> Lets mark my patch as "premature" optimization, and revisit whole thing
> after audit of the 10 drivers using this interface ;)

Also BUG_ON() is way too large a hammer.

An attempt to continue should be made in some way, so that person
inspecting the message and still have a network and work on fixing
the driver after the check triggers :-)