netdev - Re: [PATCH net-next] skbuff: Add BUG_ON in skb

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]

Message-ID: <CAMDZJNVFP=1f1r4fQN=dnQ4Ve8+VzGfY7cHUe6Vv8tsECUmraw@mail.gmail.com>
Date:   Thu, 10 Aug 2017 14:10:12 +0800
From:   Tonghao Zhang <xiangxia.m.yue@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
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 <davem@...emloft.net> wrote:
> From: Tonghao Zhang <xiangxia.m.yue@...il.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.yue@...il.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.
>