Re: [PATCH v2 6/6] RDMA/cxgb4: remove dead NULL checks after GFP_NOFAIL allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Double free of `mhp->dereg_skb` in MR allocation error paths (`c4iw_get_dma_mr`).
- [Medium] `write_adapter_mem()` passes an already freed skb to `_c4iw_write_mem_inline()` if `_c4iw_write_mem_dma()` fails.
- [Medium] Leaked `mhp->dereg_skb` in `c4iw_dereg_mr()` during device fatal error.
--

--- Patch [6]: [PATCH v2 6/6] RDMA/cxgb4: remove dead NULL checks after GFP_NOFAIL allocations ---
commit 8aa6275ceca7a8b20e9109cfd6059d14769af15d
Author: Gou Hao <gouhao@uniontech.com>

RDMA/cxgb4: remove dead NULL checks after GFP_NOFAIL allocations

alloc_skb() with the __GFP_NOFAIL flag will never return NULL,
so the subsequent NULL checks and error handling are unreachable
dead code. Remove them.

> diff --git a/drivers/infiniband/hw/cxgb4/mem.c b/drivers/infiniband/hw/cxgb4/mem.c
> index cd1b010141984..08631de17c0a3 100644
> --- a/drivers/infiniband/hw/cxgb4/mem.c
> +++ b/drivers/infiniband/hw/cxgb4/mem.c
> @@ -74,11 +74,8 @@ static int _c4iw_write_mem_dma_aligned(struct c4iw_rdev *rdev, u32 addr,
>  		c4iw_init_wr_wait(wr_waitp);
>  	wr_len = roundup(sizeof(*req) + sizeof(*sgl), 16);
>  
> -	if (!skb) {
> +	if (!skb)
>  		skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
> -		if (!skb)
> -			return -ENOMEM;
> -	}
>  	set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>  
>  	req = __skb_put_zero(skb, wr_len);
> @@ -134,11 +131,8 @@ static int _c4iw_write_mem_inline(struct c4iw_rdev *rdev, u32 addr, u32 len,
>  					 roundup(copy_len, T4_ULPTX_MIN_IO),
>  				 16);
>  
> -		if (!skb) {
> +		if (!skb)
>  			skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
> -			if (!skb)
> -				return -ENOMEM;
> -		}
>  		set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>  
>  		req = __skb_put_zero(skb, wr_len);

[ ... ]

> static int write_adapter_mem(struct c4iw_rdev *rdev, u32 addr, u32 len,
> 			     void *data, struct sk_buff *skb,
> 			     struct c4iw_wr_wait *wr_waitp)
> {
[ ... ]
> 	ret = _c4iw_write_mem_dma(rdev, addr, len, data, skb, wr_waitp);
> 	if (ret) {
> 		pr_warn_ratelimited("%s: dma map failure (non fatal)\n",
> 				    pci_name(rdev->lldi.pdev));
> 		ret = _c4iw_write_mem_inline(rdev, addr, len, data, skb,
> 					      wr_waitp);
> 	}
> out:
> 	return ret;
> 
> }

[Severity: Medium]
This isn't a bug introduced by this patch, but does the error handling path
here pass an already freed skb to _c4iw_write_mem_inline()?

If _c4iw_write_mem_dma() encounters a transmission error (e.g., c4iw_ofld_send
fails), the network layer will consume and free the skb.

Does write_adapter_mem() incorrectly assume any error from the DMA function is
a non-fatal mapping error, and fall back to _c4iw_write_mem_inline() passing
the exact same freed skb pointer?

[ ... ]

> struct ib_mr *c4iw_get_dma_mr(struct ib_pd *pd, int acc)
> {
[ ... ]
> 	ret = write_tpt_entry(&rhp->rdev, 0, &stag, 1, php->pdid,
> 			      FW_RI_STAG_NSMR, mhp->attr.perms,
> 			      mhp->attr.mw_bind_enable, 0, 0, ~0ULL, 0, 0, 0,
> 			      NULL, mhp->wr_waitp);
> 	if (ret)
> 		goto err_free_skb;
> 
> 	ret = finish_mem_reg(mhp, stag);
> 	if (ret)
> 		goto err_dereg_mem;
> 	return &mhp->ibmr;
> err_dereg_mem:
> 	dereg_mem(&rhp->rdev, mhp->attr.stag, mhp->attr.pbl_size,
> 		  mhp->attr.pbl_addr, mhp->dereg_skb, mhp->wr_waitp);
> err_free_skb:
> 	kfree_skb(mhp->dereg_skb);
> err_free_wr_wait:

[Severity: High]
This is a pre-existing issue, but does this error path cause a double-free
of mhp->dereg_skb?

In c4iw_get_dma_mr(), if finish_mem_reg() fails, the error handling path calls
dereg_mem() with the pre-allocated mhp->dereg_skb. dereg_mem() passes this skb
to the network stack, where functions like c4iw_ofld_send() assume ownership
and unconditionally consume and free the skb.

When dereg_mem() returns, the error path unconditionally falls through to the
err_free_skb label and executes kfree_skb(mhp->dereg_skb) on the already-freed
pointer.

[ ... ]

> int c4iw_dereg_mr(struct ib_mr *ib_mr, struct ib_udata *udata)
> {
[ ... ]
> 	dereg_mem(&rhp->rdev, mhp->attr.stag, mhp->attr.pbl_size,
> 		  mhp->attr.pbl_addr, mhp->dereg_skb, mhp->wr_waitp);
> 	if (mhp->attr.pbl_size)
> 		c4iw_pblpool_free(&mhp->rhp->rdev, mhp->attr.pbl_addr,
> 				  mhp->attr.pbl_size << 3);
> 	if (mhp->kva)
> 		kfree((void *) (unsigned long) mhp->kva);
> 	ib_umem_release(mhp->umem);
> 	pr_debug("mmid 0x%x ptr %p\n", mmid, mhp);
> 	c4iw_put_wr_wait(mhp->wr_waitp);
> 	kfree(mhp);
> 	return 0;
> }

[Severity: Medium]
This is a pre-existing issue, but does this function leak mhp->dereg_skb if
the device is in a fatal error state?

c4iw_dereg_mr() passes mhp->dereg_skb to dereg_mem() to deregister the memory
region, assuming the network stack will consume and free the skb.

However, if the device is in a fatal error state, write_tpt_entry()
immediately aborts and returns -EIO before the skb is handed to the network
layer, leaving it unconsumed:

drivers/infiniband/hw/cxgb4/mem.c:write_tpt_entry() {
	if (c4iw_fatal_error(rdev))
		return -EIO;
    ...
}

Because c4iw_dereg_mr() ignores the return value of dereg_mem() and
unconditionally frees the mhp structure, does it permanently leak the
unconsumed skb?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724022851.466017-1-gouhao@uniontech.com?part=6