[PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.

Subsystems: networking [general], the rest

STALE3355d

9 messages, 4 authors, 2017-08-11 · open the first message on its own page

[PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.

From: Tonghao Zhang <hidden>
Date: 2017-08-09 12:05:05

This patch reverts the commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"),
because we removed the UFO support.

Cc: Eric Dumazet <redacted>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <redacted>
---
 net/core/dev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1d75499add72..1024d3741d12 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2731,8 +2731,7 @@ EXPORT_SYMBOL(skb_mac_gso_segment);
 static inline bool skb_needs_check(struct sk_buff *skb, bool tx_path)
 {
 	if (tx_path)
-		return skb->ip_summed != CHECKSUM_PARTIAL &&
-		       skb->ip_summed != CHECKSUM_NONE;
+		return skb->ip_summed != CHECKSUM_PARTIAL;
 
 	return skb->ip_summed == CHECKSUM_NONE;
 }

-- 
2.13.4

[PATCH net-next] skbuff: Add BUG_ON in skb_init.

From: Tonghao Zhang <hidden>
Date: 2017-08-09 12:05:08

When initializing the skbuff SLAB cache, we should make
sure it is successful. Adding BUG_ON to check it and
init_inodecache() is in the same case.

Signed-off-by: Tonghao Zhang <redacted>
---
 net/core/skbuff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 42b62c716a33..9513de519870 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -3904,6 +3904,8 @@ void __init skb_init(void)
 						0,
 						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 						NULL);
+	BUG_ON(skbuff_head_cache == NULL);
+	BUG_ON(skbuff_fclone_cache == NULL);
 }
 
 static int

-- 
2.13.4

Re: [PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2017-08-09 18:31:16

On Wed, Aug 9, 2017 at 5:04 AM, Tonghao Zhang [off-list ref] wrote:

This patch reverts the commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"),
because we removed the UFO support.

Cc: Eric Dumazet <redacted>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <redacted>


I would wait until net is merged into net-next. This will cause a conflict.

Also, while logically equivalent, it is not a real revert (as in `git
revert $SHA1`) of that patch.

Aside from those concerns, I agree that the original patch is no
longer needed now that UFO is reverted.

Re: [PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.

From: Tonghao Zhang <hidden>
Date: 2017-08-10 01:02:54

Thanks for your work.

On Thu, Aug 10, 2017 at 2:30 AM, Willem de Bruijn
[off-list ref] wrote:

On Wed, Aug 9, 2017 at 5:04 AM, Tonghao Zhang [off-list ref] wrote:

quoted

This patch reverts the commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"),
because we removed the UFO support.

Cc: Eric Dumazet <redacted>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <redacted>


I would wait until net is merged into net-next. This will cause a conflict.

Also, while logically equivalent, it is not a real revert (as in `git
revert $SHA1`) of that patch.

Aside from those concerns, I agree that the original patch is no
longer needed now that UFO is reverted.

Re: [PATCH net-next] skbuff: Add BUG_ON in skb_init.

From: David Miller <davem@davemloft.net>
Date: 2017-08-10 05:49:40

From: Tonghao Zhang <redacted>
Date: Wed,  9 Aug 2017 05:04:38 -0700

quoted hunk

When initializing the skbuff SLAB cache, we should make
sure it is successful. Adding BUG_ON to check it and
init_inodecache() is in the same case.

Signed-off-by: Tonghao Zhang <redacted>
---
 net/core/skbuff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 42b62c716a33..9513de519870 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -3904,6 +3904,8 @@ void __init skb_init(void)
 						0,
 						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 						NULL);
+	BUG_ON(skbuff_head_cache == NULL);
+	BUG_ON(skbuff_fclone_cache == NULL);
 }

I know you guys want every allocation to be explicitly checked so that
everything is consistent for static code analysis checkers.

But this is just wasted code.

The first allocation will take a NULL dereference and the backtrace
will make it completely clear which SLAB cache was NULL and couldn't
be allocated.

So there is no real value to adding these checks.

So I'm not applying this, sorry.

The same logic goes for your other patch of this nature.

Re: [PATCH net-next] skbuff: Add BUG_ON in skb_init.

From: Tonghao Zhang <hidden>
Date: 2017-08-10 06:10:14

Thanks a lot. I found it when reviewing this codes. and BUG_ON is
added  in the init_inodecache(). We may remove it or not ?

diff --git a/net/socket.c b/net/socket.c
index b332d1e..ebee3ee 100644
--- a/net/socket.c
+++ b/net/socket.c

@@ -296,7 +296,6 @@ static void init_inodecache(void)
                                               SLAB_RECLAIM_ACCOUNT |
                                               SLAB_MEM_SPREAD | SLAB_ACCOUNT),
                                              init_once);
-       BUG_ON(sock_inode_cachep == NULL);
 }

 static const struct super_operations sockfs_ops = {

On Thu, Aug 10, 2017 at 1:49 PM, David Miller [off-list ref] wrote:

From: Tonghao Zhang <redacted>
Date: Wed,  9 Aug 2017 05:04:38 -0700

quoted

When initializing the skbuff SLAB cache, we should make
sure it is successful. Adding BUG_ON to check it and
init_inodecache() is in the same case.

Signed-off-by: Tonghao Zhang <redacted>
---
 net/core/skbuff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 42b62c716a33..9513de519870 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -3904,6 +3904,8 @@ void __init skb_init(void)
                                              0,
                                              SLAB_HWCACHE_ALIGN|SLAB_PANIC,
                                              NULL);
+     BUG_ON(skbuff_head_cache == NULL);
+     BUG_ON(skbuff_fclone_cache == NULL);
 }

I know you guys want every allocation to be explicitly checked so that
everything is consistent for static code analysis checkers.

But this is just wasted code.

The first allocation will take a NULL dereference and the backtrace
will make it completely clear which SLAB cache was NULL and couldn't
be allocated.

So there is no real value to adding these checks.

So I'm not applying this, sorry.

The same logic goes for your other patch of this nature.

RE: [PATCH net-next] skbuff: Add BUG_ON in skb_init.

From: Yuan, Linyu (NSB - CN/Shanghai) <hidden>
Date: 2017-08-10 06:56:54

Original this function have return value but not used, and check allocate result internal, 
When I change this function to void return, I add this BUG_ON.

quoted hunk

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Tonghao Zhang
Sent: Thursday, August 10, 2017 2:10 PM
To: David Miller
Cc: Linux Kernel Network Developers
Subject: Re: [PATCH net-next] skbuff: Add BUG_ON in skb_init.

Thanks a lot. I found it when reviewing this codes. and BUG_ON is
added  in the init_inodecache(). We may remove it or not ?

diff --git a/net/socket.c b/net/socket.c
index b332d1e..ebee3ee 100644
--- a/net/socket.c
+++ b/net/socket.c

@@ -296,7 +296,6 @@ static void init_inodecache(void)

SLAB_RECLAIM_ACCOUNT |
                                               SLAB_MEM_SPREAD
| SLAB_ACCOUNT),
                                              init_once);
-       BUG_ON(sock_inode_cachep == NULL);
 }

 static const struct super_operations sockfs_ops = {

On Thu, Aug 10, 2017 at 1:49 PM, David Miller [off-list ref]
wrote:

quoted

From: Tonghao Zhang <redacted>
Date: Wed,  9 Aug 2017 05:04:38 -0700

quoted

When initializing the skbuff SLAB cache, we should make
sure it is successful. Adding BUG_ON to check it and
init_inodecache() is in the same case.

Signed-off-by: Tonghao Zhang <redacted>
---
 net/core/skbuff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 42b62c716a33..9513de519870 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -3904,6 +3904,8 @@ void __init skb_init(void)
                                              0,

SLAB_HWCACHE_ALIGN|SLAB_PANIC,

quoted

                                              NULL);
+     BUG_ON(skbuff_head_cache == NULL);
+     BUG_ON(skbuff_fclone_cache == NULL);
 }

I know you guys want every allocation to be explicitly checked so that
everything is consistent for static code analysis checkers.

But this is just wasted code.

The first allocation will take a NULL dereference and the backtrace
will make it completely clear which SLAB cache was NULL and couldn't
be allocated.

So there is no real value to adding these checks.

So I'm not applying this, sorry.

The same logic goes for your other patch of this nature.

Re: [PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2017-08-10 17:03:57

On Wed, Aug 9, 2017 at 6:02 PM, Tonghao Zhang [off-list ref] wrote:

Thanks for your work.

You, too.

On Thu, Aug 10, 2017 at 2:30 AM, Willem de Bruijn
[off-list ref] wrote:

quoted

On Wed, Aug 9, 2017 at 5:04 AM, Tonghao Zhang [off-list ref] wrote:

quoted

This patch reverts the commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"),
because we removed the UFO support.

Cc: Eric Dumazet <redacted>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <redacted>


I would wait until net is merged into net-next. This will cause a conflict.

Also, while logically equivalent, it is not a real revert (as in `git
revert $SHA1`) of that patch.

Aside from those concerns, I agree that the original patch is no
longer needed now that UFO is reverted.

Please do resubmit the revert patch once net has been merged into net-next.

Re: [PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.

From: Tonghao Zhang <hidden>
Date: 2017-08-11 02:29:57

Hi Willem, because we change the CHECKSUM_NONE to CHECKSUM_UNNECESSARY
in skb_needs_check(), I can't revert 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx").  I change it
directly and resubmit it.

On Fri, Aug 11, 2017 at 1:03 AM, Willem de Bruijn
[off-list ref] wrote:

On Wed, Aug 9, 2017 at 6:02 PM, Tonghao Zhang [off-list ref] wrote:

quoted

Thanks for your work.

You, too.

quoted

On Thu, Aug 10, 2017 at 2:30 AM, Willem de Bruijn
[off-list ref] wrote:

quoted

On Wed, Aug 9, 2017 at 5:04 AM, Tonghao Zhang [off-list ref] wrote:

quoted

This patch reverts the commit 6e7bc478c9a0
("net: skb_needs_check() accepts CHECKSUM_NONE for tx"),
because we removed the UFO support.

Cc: Eric Dumazet <redacted>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Tonghao Zhang <redacted>


I would wait until net is merged into net-next. This will cause a conflict.

Also, while logically equivalent, it is not a real revert (as in `git
revert $SHA1`) of that patch.

Aside from those concerns, I agree that the original patch is no
longer needed now that UFO is reverted.

Please do resubmit the revert patch once net has been merged into net-next.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help