* Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
2025-08-29 21:59 [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value Mina Almasry
@ 2025-08-29 22:02 ` Mina Almasry
2025-08-30 0:52 ` Jakub Kicinski
2025-09-01 11:35 ` Dan Carpenter
2 siblings, 0 replies; 6+ messages in thread
From: Mina Almasry @ 2025-08-29 22:02 UTC (permalink / raw)
To: netdev, linux-kernel, Dragos Tatulea
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Joe Damato, Stanislav Fomichev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
2025-08-29 21:59 [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value Mina Almasry
2025-08-29 22:02 ` Mina Almasry
@ 2025-08-30 0:52 ` Jakub Kicinski
2025-09-02 15:54 ` Stanislav Fomichev
2025-09-01 11:35 ` Dan Carpenter
2 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2025-08-30 0:52 UTC (permalink / raw)
To: Mina Almasry
Cc: netdev, linux-kernel, David S. Miller, Eric Dumazet, Paolo Abeni,
Simon Horman, Stanislav Fomichev
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
2025-08-30 0:52 ` Jakub Kicinski
@ 2025-09-02 15:54 ` Stanislav Fomichev
2025-09-02 16:14 ` Mina Almasry
0 siblings, 1 reply; 6+ messages in thread
From: Stanislav Fomichev @ 2025-09-02 15:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Mina Almasry, netdev, linux-kernel, David S. Miller, Eric Dumazet,
Paolo Abeni, Simon Horman, Stanislav Fomichev
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
2025-09-02 15:54 ` Stanislav Fomichev
@ 2025-09-02 16:14 ` Mina Almasry
0 siblings, 0 replies; 6+ messages in thread
From: Mina Almasry @ 2025-09-02 16:14 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Jakub Kicinski, netdev, linux-kernel, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, Stanislav Fomichev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value
2025-08-29 21:59 [PATCH net-next v1] net: devmem: NULL check netdev_nl_get_dma_dev return value Mina Almasry
2025-08-29 22:02 ` Mina Almasry
2025-08-30 0:52 ` Jakub Kicinski
@ 2025-09-01 11:35 ` Dan Carpenter
2 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2025-09-01 11:35 UTC (permalink / raw)
To: oe-kbuild, Mina Almasry, netdev, linux-kernel
Cc: lkp, oe-kbuild-all, Mina Almasry, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Joe Damato, Stanislav Fomichev
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
^ permalink raw reply [flat|nested] 6+ messages in thread