diff mbox

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

Message ID f7tfv01pulb.fsf@aconole.bos.csb
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Aaron Conole Nov. 19, 2015, 9:06 p.m. UTC 
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?

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

Comments

Eric Dumazet Nov. 19, 2015, 9:43 p.m. UTC | #1 
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 20, 2015, 7:59 p.m. UTC | #2 
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 :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;