Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Jan 2021 13:13:53 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Raju Rangoju <rajur@chelsio.com>
Cc:     Yang Li <abaci-bugfix@linux.alibaba.com>, davem@davemloft.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] cxgb4: remove redundant NULL check
Message-ID: <20210127131353.5fc141da@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20210127063426.GC21071@chelsio.com>
References: <1611629413-81373-1-git-send-email-abaci-bugfix@linux.alibaba.com>
        <20210127063426.GC21071@chelsio.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linux.alibaba.com>
> > Signed-off-by: Yang Li <abaci-bugfix@linux.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.