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

[Yuan, Linyu (NSB - CN/Shanghai)]

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.

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@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 <da...@davemloft.net>
> wrote:
> > From: Tonghao Zhang <xiangxia.m@gmail.com>
> > Date: Wed,  9 Aug 2017 05:04:38 -0700
> >
> >> 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 <xiangxia.m@gmail.com>
> >> ---
> >>  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.

[Tonghao Zhang]

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  wrote:
> From: Tonghao Zhang 
> Date: Wed,  9 Aug 2017 05:04:38 -0700
>
>> 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 
>> ---
>>  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.

[David Miller]

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

> 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 
> ---
>  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.