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: <8729016553E3654398EA69218DA29EEF15C681AA@cnshjmbx02>
Date:   Thu, 10 Aug 2017 06:41:49 +0000
From:   "Yuan, Linyu (NSB - CN/Shanghai)" <linyu.yuan@...ia-sbell.com>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>,
        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.

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-owner@...r.kernel.org [mailto:netdev-owner@...r.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 <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.
> >