Re: [PATCH net v2] net/smc: fix one NULL pointer dereference in smc_ib_is_sg_need_sync()
From: "D. Wythe" <alibuda@linux.alibaba.com >
Date: 2026-01-28 05:51:00
Also in:
linux-rdma, linux-s390
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:quoted
On Tue, Sep 09, 2025 at 12:45:32PM +0300, Leon Romanovsky wrote:quoted
On Thu, Aug 28, 2025 at 08:41:17PM +0800, Liu Jian wrote:quoted
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 <redacted> --- 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; ThanksHi 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
quoted
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. Thanksquoted
Best wishes, D. Wythe