Re: [PATCH net v2] net/smc: fix one NULL pointer dereference in smc_ib_is_sg_need_sync()

["D. Wythe" <alibuda@linux.alibaba.com >]

On Tue, Sep 09, 2025 at 12:45:32PM +0300, Leon Romanovsky wrote:
> On Thu, Aug 28, 2025 at 08:41:17PM +0800, Liu Jian wrote:
> > BUG: kernel NULL pointer dereference, address: 00000000000002ec
> > PGD 0 P4D 0
> > Oops: Oops: 0000 [#1] SMP PTI
> > CPU: 28 UID: 0 PID: 343 Comm: kworker/28:1 Kdump: loaded Tainted: G        OE       6.17.0-rc2+ #9 NONE
> > Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> > Workqueue: smc_hs_wq smc_listen_work [smc]
> > RIP: 0010:smc_ib_is_sg_need_sync+0x9e/0xd0 [smc]
> > ...
> > Call Trace:
> >  <TASK>
> >  smcr_buf_map_link+0x211/0x2a0 [smc]
> >  __smc_buf_create+0x522/0x970 [smc]
> >  smc_buf_create+0x3a/0x110 [smc]
> >  smc_find_rdma_v2_device_serv+0x18f/0x240 [smc]
> >  ? smc_vlan_by_tcpsk+0x7e/0xe0 [smc]
> >  smc_listen_find_device+0x1dd/0x2b0 [smc]
> >  smc_listen_work+0x30f/0x580 [smc]
> >  process_one_work+0x18c/0x340
> >  worker_thread+0x242/0x360
> >  kthread+0xe7/0x220
> >  ret_from_fork+0x13a/0x160
> >  ret_from_fork_asm+0x1a/0x30
> >  </TASK>
> > 
> > If the software RoCE device is used, ibdev->dma_device is a null pointer.
> > As a result, the problem occurs. Null pointer detection is added to
> > prevent problems.
> > 
> > Fixes: 0ef69e788411c ("net/smc: optimize for smc_sndbuf_sync_sg_for_device and smc_rmb_sync_sg_for_cpu")
> > Signed-off-by: Liu Jian <liujian56@huawei.com>
> > ---
> > v1->v2:
> > move the check outside of loop.
> >  net/smc/smc_ib.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
> > index 53828833a3f7..a42ef3f77b96 100644
> > --- a/net/smc/smc_ib.c
> > +++ b/net/smc/smc_ib.c
> > @@ -742,6 +742,9 @@ bool smc_ib_is_sg_need_sync(struct smc_link *lnk,
> >  	unsigned int i;
> >  	bool ret = false;
> >  
> > +	if (!lnk->smcibdev->ibdev->dma_device)
> > +		return ret;
> 
> Please use ib_uses_virt_dma() function for that.
> 
> It is clearly stated in the code:
>   2784 struct ib_device {
>   2785         /* Do not access @dma_device directly from ULP nor from HW drivers. */
>   2786         struct device                *dma_device;     
> 
> Thanks
> 
>

Hi Leon,

Sorry for the late reply, I just noticed this and thank you for your review.
I agree completely with your feedback: we should not be accessing ibdev->dma_device
directly. Following your advice, replacing the

	if (!lnk->smcibdev->ibdev->dma_device)

check with ib_uses_virt_dma() is straightforward and absolutely the correct
thing to do for that part of the logic.

However, this has led me to a further challenge. The main purpose of the
smc_ib_is_sg_need_sync() function is to get the result of dma_need_sync().
This means that even after correctly using ib_uses_virt_dma(), the function
still needs to call dma_need_sync(ibdev->dma_device, ...), which forces us
right back into the direct access pattern we are trying to eliminate.

Since the RDMA doesn't currently offer a helper for this "check" functionality,
I'd like to propose adding one. What are your thoughts on a new helper like
the one below, which would allow us to solve this cleanly?

/* ib_verbs.h */
static inline bool ib_dma_need_sync(struct ib_device *dev, dma_addr_t dma_addr) {
	return !ib_uses_virt_dma(dev) && dma_need_sync(dev->dma_device, dma_addr);
}

Best wishes,
D. Wythe