netdev - Re: [PATCH net v2] net/smc: fix one NULL pointer dereference in smc_ib_is_sg_need

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20260128125033.GA116672@j66a10360.sqa.eu95>
Date: Wed, 28 Jan 2026 20:50:33 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com    >
To: Leon Romanovsky <leon@...nel.org>
Cc: "D. Wythe" <alibuda@...ux.alibaba.com>, Liu Jian <liujian56@...wei.com>,
	dust.li@...ux.alibaba.com, sidraya@...ux.ibm.com,
	wenjia@...ux.ibm.com, mjambigi@...ux.ibm.com,
	tonylu@...ux.alibaba.com, guwen@...ux.alibaba.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org,
	guangguan.wang@...ux.alibaba.com, linux-rdma@...r.kernel.org,
	linux-s390@...r.kernel.org, netdev@...r.kernel.org,
	Christoph Hellwig <hch@....de>, Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH net v2] net/smc: fix one NULL pointer dereference in
 smc_ib_is_sg_need_sync()

On Wed, Jan 28, 2026 at 01:21:22PM +0200, Leon Romanovsky wrote:
> On Wed, Jan 28, 2026 at 01:50:51PM +0800, D. Wythe wrote:
> > On Tue, Jan 27, 2026 at 02:00:04PM +0200, Leon Romanovsky wrote:
> > > 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@...wei.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?
> > > 
> > 
> > You are right. I just noticed that the DMA has already introduced a
> > similar optimization.
> > 
> > The check in SMC was added back in 2022, while the DMA introduced
> > the internal "skip redundant sync" mechanism in 2024 (commit f406c8e4b770).
> > 
> > Given this, it might be better to simply remove this redundant check
> > from SMC now, which would also eliminate the need for direct access to
> > ibdev->dma_device. I need to perform some tests to confirm this.
> 
> It may also be worth looking at this series from Chuck, which reuses the
> recently introduced DMA API to remove the SG conversions:
> https://lore.kernel.org/linux-rdma/20260128005400.25147-1-cel@kernel.org/
> 
> You might be able to apply a similar approach and drop SG from your
> datapath entirely.
> 
> Thanks.
> 

Thanks for the pointer. I'll take some time to look into this series and
see if we can adopt a similar approach.

> > 
> > Thanks for your feedback.
> > 
> > D. Wythe
> > 
> > > > 
> > > > 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
> > > > 
> > > > 
> >