1. Patchew
  2. linux
  3. View series

[PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value

Mina Almasry posted 1 patch 1 month ago 
net/core/netdev-genl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
Posted by Mina Almasry 1 month ago 
netdev_nl_get_dma_dev can return NULL. This happens in the unlikely
scenario that netdev->dev.parent is NULL, or all the calls to the
ndo_queue_get_dma_dev return NULL from the driver.

Current code doesn't NULL check the return value, so it may be passed to
net_devmem_bind_dmabuf, which AFAICT will eventually hit
WARN_ON(!dmabuf || !dev) in dma_buf_dynamic_attach and do a kernel
splat. Avoid this scenario by using IS_ERR_OR_NULL in place of IS_ERR.

Found by code inspection.

Note that this was a problem even before the fixes patch, since we
passed netdev->dev.parent to net_devmem_bind_dmabuf before NULL checking
it anyway :( But that code got removed in the fixes patch (and retained
the bug).

Fixes: b8aab4bb9585 ("net: devmem: allow binding on rx queues with same DMA devices")
Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 net/core/netdev-genl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 470fabbeacd9..779bcdb5653d 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -1098,7 +1098,7 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
 	dma_dev = netdev_queue_get_dma_dev(netdev, 0);
 	binding = net_devmem_bind_dmabuf(netdev, dma_dev, DMA_TO_DEVICE,
 					 dmabuf_fd, priv, info->extack);
-	if (IS_ERR(binding)) {
+	if (IS_ERR_OR_NULL(binding)) {
 		err = PTR_ERR(binding);
 		goto err_unlock_netdev;
 	}

base-commit: 4f54dff818d7b5b1d84becd5d90bc46e6233c0d7
-- 
2.51.0.318.gd7df087d1a-goog
Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
Posted by Dan Carpenter 1 month ago 
Hi Mina,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Mina-Almasry/net-devmem-NULL-check-netdev_nl_get_dma_dev-return-value/20250830-060251
base:   4f54dff818d7b5b1d84becd5d90bc46e6233c0d7
patch link:    https://lore.kernel.org/r/20250829220003.3310242-1-almasrymina%40google.com
patch subject: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
config: x86_64-randconfig-161-20250830 (https://download.01.org/0day-ci/archive/20250831/202508310205.YQmsa9gY-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202508310205.YQmsa9gY-lkp@intel.com/

New smatch warnings:
net/core/netdev-genl.c:1102 netdev_nl_bind_tx_doit() warn: passing zero to 'PTR_ERR'

vim +/PTR_ERR +1102 net/core/netdev-genl.c

8802087d20c0e1 Stanislav Fomichev 2025-05-08  1046  int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
8802087d20c0e1 Stanislav Fomichev 2025-05-08  1047  {
bd61848900bff5 Mina Almasry       2025-05-08  1048  	struct net_devmem_dmabuf_binding *binding;
bd61848900bff5 Mina Almasry       2025-05-08  1049  	struct netdev_nl_sock *priv;
bd61848900bff5 Mina Almasry       2025-05-08  1050  	struct net_device *netdev;
512c88fb0e884c Dragos Tatulea     2025-08-27  1051  	struct device *dma_dev;
bd61848900bff5 Mina Almasry       2025-05-08  1052  	u32 ifindex, dmabuf_fd;
bd61848900bff5 Mina Almasry       2025-05-08  1053  	struct sk_buff *rsp;
bd61848900bff5 Mina Almasry       2025-05-08  1054  	int err = 0;
bd61848900bff5 Mina Almasry       2025-05-08  1055  	void *hdr;
bd61848900bff5 Mina Almasry       2025-05-08  1056  
bd61848900bff5 Mina Almasry       2025-05-08  1057  	if (GENL_REQ_ATTR_CHECK(info, NETDEV_A_DEV_IFINDEX) ||
bd61848900bff5 Mina Almasry       2025-05-08  1058  	    GENL_REQ_ATTR_CHECK(info, NETDEV_A_DMABUF_FD))
bd61848900bff5 Mina Almasry       2025-05-08  1059  		return -EINVAL;
bd61848900bff5 Mina Almasry       2025-05-08  1060  
bd61848900bff5 Mina Almasry       2025-05-08  1061  	ifindex = nla_get_u32(info->attrs[NETDEV_A_DEV_IFINDEX]);
bd61848900bff5 Mina Almasry       2025-05-08  1062  	dmabuf_fd = nla_get_u32(info->attrs[NETDEV_A_DMABUF_FD]);
bd61848900bff5 Mina Almasry       2025-05-08  1063  
bd61848900bff5 Mina Almasry       2025-05-08  1064  	priv = genl_sk_priv_get(&netdev_nl_family, NETLINK_CB(skb).sk);
bd61848900bff5 Mina Almasry       2025-05-08  1065  	if (IS_ERR(priv))
bd61848900bff5 Mina Almasry       2025-05-08  1066  		return PTR_ERR(priv);
bd61848900bff5 Mina Almasry       2025-05-08  1067  
bd61848900bff5 Mina Almasry       2025-05-08  1068  	rsp = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
bd61848900bff5 Mina Almasry       2025-05-08  1069  	if (!rsp)
bd61848900bff5 Mina Almasry       2025-05-08  1070  		return -ENOMEM;
bd61848900bff5 Mina Almasry       2025-05-08  1071  
bd61848900bff5 Mina Almasry       2025-05-08  1072  	hdr = genlmsg_iput(rsp, info);
bd61848900bff5 Mina Almasry       2025-05-08  1073  	if (!hdr) {
bd61848900bff5 Mina Almasry       2025-05-08  1074  		err = -EMSGSIZE;
bd61848900bff5 Mina Almasry       2025-05-08  1075  		goto err_genlmsg_free;
bd61848900bff5 Mina Almasry       2025-05-08  1076  	}
bd61848900bff5 Mina Almasry       2025-05-08  1077  
bd61848900bff5 Mina Almasry       2025-05-08  1078  	mutex_lock(&priv->lock);
bd61848900bff5 Mina Almasry       2025-05-08  1079  
bd61848900bff5 Mina Almasry       2025-05-08  1080  	netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
bd61848900bff5 Mina Almasry       2025-05-08  1081  	if (!netdev) {
bd61848900bff5 Mina Almasry       2025-05-08  1082  		err = -ENODEV;
bd61848900bff5 Mina Almasry       2025-05-08  1083  		goto err_unlock_sock;
bd61848900bff5 Mina Almasry       2025-05-08  1084  	}
bd61848900bff5 Mina Almasry       2025-05-08  1085  
bd61848900bff5 Mina Almasry       2025-05-08  1086  	if (!netif_device_present(netdev)) {
bd61848900bff5 Mina Almasry       2025-05-08  1087  		err = -ENODEV;
bd61848900bff5 Mina Almasry       2025-05-08  1088  		goto err_unlock_netdev;
bd61848900bff5 Mina Almasry       2025-05-08  1089  	}
bd61848900bff5 Mina Almasry       2025-05-08  1090  
ae28cb114727dd Mina Almasry       2025-05-08  1091  	if (!netdev->netmem_tx) {
ae28cb114727dd Mina Almasry       2025-05-08  1092  		err = -EOPNOTSUPP;
ae28cb114727dd Mina Almasry       2025-05-08  1093  		NL_SET_ERR_MSG(info->extack,
ae28cb114727dd Mina Almasry       2025-05-08  1094  			       "Driver does not support netmem TX");
ae28cb114727dd Mina Almasry       2025-05-08  1095  		goto err_unlock_netdev;
ae28cb114727dd Mina Almasry       2025-05-08  1096  	}
ae28cb114727dd Mina Almasry       2025-05-08  1097  
512c88fb0e884c Dragos Tatulea     2025-08-27  1098  	dma_dev = netdev_queue_get_dma_dev(netdev, 0);
512c88fb0e884c Dragos Tatulea     2025-08-27  1099  	binding = net_devmem_bind_dmabuf(netdev, dma_dev, DMA_TO_DEVICE,
512c88fb0e884c Dragos Tatulea     2025-08-27  1100  					 dmabuf_fd, priv, info->extack);
991dbef67e2c35 Mina Almasry       2025-08-29  1101  	if (IS_ERR_OR_NULL(binding)) {
bd61848900bff5 Mina Almasry       2025-05-08 @1102  		err = PTR_ERR(binding);
bd61848900bff5 Mina Almasry       2025-05-08  1103  		goto err_unlock_netdev;

net_devmem_bind_dmabuf() can't return NULL.  See my blog for more
details:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

bd61848900bff5 Mina Almasry       2025-05-08  1104  	}
bd61848900bff5 Mina Almasry       2025-05-08  1105  
bd61848900bff5 Mina Almasry       2025-05-08  1106  	nla_put_u32(rsp, NETDEV_A_DMABUF_ID, binding->id);
bd61848900bff5 Mina Almasry       2025-05-08  1107  	genlmsg_end(rsp, hdr);
bd61848900bff5 Mina Almasry       2025-05-08  1108  
bd61848900bff5 Mina Almasry       2025-05-08  1109  	netdev_unlock(netdev);
bd61848900bff5 Mina Almasry       2025-05-08  1110  	mutex_unlock(&priv->lock);
bd61848900bff5 Mina Almasry       2025-05-08  1111  
bd61848900bff5 Mina Almasry       2025-05-08  1112  	return genlmsg_reply(rsp, info);
bd61848900bff5 Mina Almasry       2025-05-08  1113  
bd61848900bff5 Mina Almasry       2025-05-08  1114  err_unlock_netdev:
bd61848900bff5 Mina Almasry       2025-05-08  1115  	netdev_unlock(netdev);
bd61848900bff5 Mina Almasry       2025-05-08  1116  err_unlock_sock:
bd61848900bff5 Mina Almasry       2025-05-08  1117  	mutex_unlock(&priv->lock);
bd61848900bff5 Mina Almasry       2025-05-08  1118  err_genlmsg_free:
bd61848900bff5 Mina Almasry       2025-05-08  1119  	nlmsg_free(rsp);
bd61848900bff5 Mina Almasry       2025-05-08  1120  	return err;
8802087d20c0e1 Stanislav Fomichev 2025-05-08  1121  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
Posted by Jakub Kicinski 1 month ago 
On Fri, 29 Aug 2025 21:59:38 +0000 Mina Almasry wrote:
> netdev_nl_get_dma_dev can return NULL. This happens in the unlikely
> scenario that netdev->dev.parent is NULL, or all the calls to the
> ndo_queue_get_dma_dev return NULL from the driver.

I probably have Friday brain but I don't see what you mean..
In net-next net_devmem_bind_dmabuf() gets a dma_dev and returns
-EOPNOTSUPP PTR if its NULL.

> Current code doesn't NULL check the return value, so it may be passed to
> net_devmem_bind_dmabuf, which AFAICT will eventually hit
> WARN_ON(!dmabuf || !dev) in dma_buf_dynamic_attach and do a kernel
> splat. Avoid this scenario by using IS_ERR_OR_NULL in place of IS_ERR.
> 
> Found by code inspection.
> 
> Note that this was a problem even before the fixes patch, since we
> passed netdev->dev.parent to net_devmem_bind_dmabuf before NULL checking
> it anyway :( But that code got removed in the fixes patch (and retained
> the bug).

If the bug exists in net please send a fix for net, and ignore net-next.
Maintainers will cope with the merge.
Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
Posted by Stanislav Fomichev 1 month ago 
On 08/29, Jakub Kicinski wrote:
> On Fri, 29 Aug 2025 21:59:38 +0000 Mina Almasry wrote:
> > netdev_nl_get_dma_dev can return NULL. This happens in the unlikely
> > scenario that netdev->dev.parent is NULL, or all the calls to the
> > ndo_queue_get_dma_dev return NULL from the driver.
> 
> I probably have Friday brain but I don't see what you mean..
> In net-next net_devmem_bind_dmabuf() gets a dma_dev and returns
> -EOPNOTSUPP PTR if its NULL.

+1, the description and the fix are confusing.

Unless I'm missing something, the intent seems to be to avoid hitting
a WARN_ON in dma_buf_attach (really dma_buf_dynamic_attach) when the dma_dev
is NULL. Mina, can we do this in the callers of netdev_queue_get_dma_dev?
Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
Posted by Mina Almasry 1 month ago 
On Tue, Sep 2, 2025 at 8:54 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 08/29, Jakub Kicinski wrote:
> > On Fri, 29 Aug 2025 21:59:38 +0000 Mina Almasry wrote:
> > > netdev_nl_get_dma_dev can return NULL. This happens in the unlikely
> > > scenario that netdev->dev.parent is NULL, or all the calls to the
> > > ndo_queue_get_dma_dev return NULL from the driver.
> >
> > I probably have Friday brain but I don't see what you mean..
> > In net-next net_devmem_bind_dmabuf() gets a dma_dev and returns
> > -EOPNOTSUPP PTR if its NULL.
>
> +1, the description and the fix are confusing.
>
> Unless I'm missing something, the intent seems to be to avoid hitting
> a WARN_ON in dma_buf_attach (really dma_buf_dynamic_attach) when the dma_dev
> is NULL. Mina, can we do this in the callers of netdev_queue_get_dma_dev?

Yes looks like I was the one with the Friday brain. I missed the NULL
check in net_devmem_bind_dmabuf. I'll check if this issue is on net
and send a fix there if needed.

-- 
Thanks,
Mina
Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
Posted by Mina Almasry 1 month ago 
Missed ccing Dragos. Adding manually.

On Fri, Aug 29, 2025 at 3:00 PM Mina Almasry <almasrymina@google.com> wrote:
>
> netdev_nl_get_dma_dev can return NULL. This happens in the unlikely
> scenario that netdev->dev.parent is NULL, or all the calls to the
> ndo_queue_get_dma_dev return NULL from the driver.
>
> Current code doesn't NULL check the return value, so it may be passed to
> net_devmem_bind_dmabuf, which AFAICT will eventually hit
> WARN_ON(!dmabuf || !dev) in dma_buf_dynamic_attach and do a kernel
> splat. Avoid this scenario by using IS_ERR_OR_NULL in place of IS_ERR.
>
> Found by code inspection.
>
> Note that this was a problem even before the fixes patch, since we
> passed netdev->dev.parent to net_devmem_bind_dmabuf before NULL checking
> it anyway :( But that code got removed in the fixes patch (and retained
> the bug).
>
> Fixes: b8aab4bb9585 ("net: devmem: allow binding on rx queues with same DMA devices")
> Signed-off-by: Mina Almasry <almasrymina@google.com>
>
> ---
>  net/core/netdev-genl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 470fabbeacd9..779bcdb5653d 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -1098,7 +1098,7 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
>         dma_dev = netdev_queue_get_dma_dev(netdev, 0);
>         binding = net_devmem_bind_dmabuf(netdev, dma_dev, DMA_TO_DEVICE,
>                                          dmabuf_fd, priv, info->extack);
> -       if (IS_ERR(binding)) {
> +       if (IS_ERR_OR_NULL(binding)) {
>                 err = PTR_ERR(binding);
>                 goto err_unlock_netdev;
>         }
>
> base-commit: 4f54dff818d7b5b1d84becd5d90bc46e6233c0d7
> --
> 2.51.0.318.gd7df087d1a-goog
>


-- 
Thanks,
Mina

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.