linux-kernel - Re: [PATCH net-next] cxgb4: remove redundant NULL check

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] [day] [month] [year] [list]

Message-ID: <20210127131353.5fc141da@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 27 Jan 2021 13:13:53 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Raju Rangoju <rajur@...lsio.com>
Cc:     Yang Li <abaci-bugfix@...ux.alibaba.com>, davem@...emloft.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] cxgb4: remove redundant NULL check

On Wed, 27 Jan 2021 12:04:27 +0530 Raju Rangoju wrote:
> On Tuesday, January 01/26/21, 2021 at 10:50:13 +0800, Yang Li wrote:
> > Fix below warnings reported by coccicheck:
> > ./drivers/net/ethernet/chelsio/cxgb4/clip_tbl.c:323:3-9: WARNING:
> > NULL check before some freeing functions is not needed.
> > ./drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c:3554:2-8: WARNING:
> > NULL check before some freeing functions is not needed.
> > ./drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.c:157:2-7: WARNING:
> > NULL check before some freeing functions is not needed.
> > ./drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c:525:3-9: WARNING:
> > NULL check before some freeing functions is not needed.
> > 
> > Reported-by: Abaci Robot <abaci@...ux.alibaba.com>
> > Signed-off-by: Yang Li <abaci-bugfix@...ux.alibaba.com>

> > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.c
> > index 77648e4..dd66b24 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.c
> > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_cudbg.c
> > @@ -157,8 +157,7 @@ static int cudbg_alloc_compress_buff(struct cudbg_init *pdbg_init)
> >  
> >  static void cudbg_free_compress_buff(struct cudbg_init *pdbg_init)
> >  {
> > -	if (pdbg_init->compress_buff)  
> 
> NAK. The above check is necessary.
> 
> pdbg_init->compress_buff may be NULL when Zlib is unavailable or when
> pdbg_init->compress_buff allocation fails, in which case we ignore error
> and continue without compression. Check is necessary before calling
> vfree().

Thanks taking a look! The point is that vfree() kfree() etc. all can be
fed NULL in which case they are a nop. E.g.:

/**
 * vfree - Release memory allocated by vmalloc()
 * @addr:  Memory base address
 *
 * Free the virtually continuous memory area starting at @addr, as obtained
 * from one of the vmalloc() family of APIs.  This will usually also free the
 * physical memory underlying the virtual allocation, but that memory is
 * reference counted, so it will not be freed until the last user goes away.
 *
   vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
>* If @addr is NULL, no operation is performed. <=
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 *
 * Context:
 * May sleep if called *not* from interrupt context.
 * Must not be called in NMI context (strictly speaking, it could be
 * if we have CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG, but making the calling
 * conventions for vfree() arch-depenedent would be a really bad idea).
 */

I don't think there is any advanced static analysis going on here if
that's what you assumed.

Yang, please respin, and explain in the patch message why removing
those conditions is safe.