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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Fallback error path and chunking in `_c4iw_write_mem_dma` and `write_adapter_mem` trigger a use-after-free on caller-provided SKBs.
- [High] Unconditional double-free of `mhp->dereg_skb` in the error path of `c4iw_alloc_mr`.
--

--- Patch [4]: [PATCH 4/4] RDMA/cxgb4: remove dead NULL checks after GFP_NOFAIL allocations ---
commit 2b81b1933eb1e926287199e29810f5bc991be785
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);

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
related to how these skb pointers are passed in from caller functions.

Looking at _c4iw_write_mem_dma(), the chunking loop reuses the caller's skb
for multiple offload sends:

drivers/infiniband/hw/cxgb4/mem.c:_c4iw_write_mem_dma() {
    ...
	while (remain > inline_threshold) {
		// ...
		ret = _c4iw_write_mem_dma_aligned(rdev, addr, dmalen, daddr,
						 skb, remain ? NULL : wr_waitp);
		// ...
	}
	if (remain)
		ret = _c4iw_write_mem_inline(rdev, addr, remain, data, skb,
					     wr_waitp);
    ...
}

Since the first call to _c4iw_write_mem_dma_aligned() (or
_c4iw_write_mem_inline()) hands the skb to c4iw_ofld_send() which consumes it,
doesn't reusing this same skb pointer in subsequent loop iterations or the
remainder fallback trigger a use-after-free?

Similarly, in write_adapter_mem(), the fallback logic reuses the skb pointer
after _c4iw_write_mem_dma() has potentially consumed it during a failed
offload send:

drivers/infiniband/hw/cxgb4/mem.c:write_adapter_mem() {
    ...
	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);
	}
    ...
}

Could this fallback also result in a use-after-free if the skb was consumed
before the error was returned?

>  
>  	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;
> -		}

[Severity: High]
This is also a pre-existing issue, but there appears to be a double-free
involving dereg_skb in the error path of c4iw_alloc_mr().

When finish_mem_reg() fails, it jumps to err_dereg_mem and calls dereg_mem()
with mhp->dereg_skb. This eventually passes the skb to c4iw_ofld_send()
which takes ownership of the skb.

drivers/infiniband/hw/cxgb4/mem.c:c4iw_alloc_mr() {
    ...
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);
    ...
}

After dereg_mem() returns, the error path unconditionally calls kfree_skb()
on mhp->dereg_skb.

Does this unconditionally double-free the skb after it has already been handed
off to the network stack?

>  		set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>  
>  		req = __skb_put_zero(skb, wr_len);

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