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: <20260128055051.GA79132@j66a10360.sqa.eu95>
Date: Wed, 28 Jan 2026 13:50:51 +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 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.

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