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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Jan 26, 2026 at 12:45:01PM +0800, D. Wythe wrote:
> 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.

I would like to challenge the use of dma_need_sync() in smc_ib.c. From
what I see, it is used to avoid calls to dma_sync_single_*_device()
which will be NOP anyway in that case.

Why did you copy that logic from XSK?

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

If we're discussing wrappers, it's likely better to provide a wrapper around
dma_sync_sg_for_cpu() for use in ib_dma_sync_sg_for_cpu(), rather than
open‑coding the logic.

Thanks

> 
> Best wishes,
> D. Wythe
> 
>