| 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
| ||
|
Message-ID: <bm4uszrqfszm5sgigrtmo2piowoaxzsprwxuezfze4lgbt22ki@rn2w2sncivv3>
Date: Wed, 9 Jul 2025 19:53:54 +0000
From: Dragos Tatulea <dtatulea@...dia.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: asml.silence@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Jens Axboe <axboe@...nel.dk>, Simona Vetter <simona.vetter@...ll.ch>,
Willem de Bruijn <willemb@...gle.com>, Kaiyuan Zhang <kaiyuanz@...gle.com>, cratiu@...dia.com,
parav@...dia.com, Tariq Toukan <tariqt@...dia.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, io-uring@...r.kernel.org
Subject: Re: [PATCH net] net: Allow non parent devices to be used for ZC DMA
On Wed, Jul 09, 2025 at 12:29:22PM -0700, Mina Almasry wrote:
> On Wed, Jul 9, 2025 at 5:46 AM Dragos Tatulea <dtatulea@...dia.com> wrote:
> >
> > For zerocopy (io_uring, devmem), there is an assumption that the
> > parent device can do DMA. However that is not always the case:
> > ScalableFunction devices have the DMA device in the grandparent.
> >
> > This patch adds a helper for getting the DMA device for a netdev from
> > its parent or grandparent if necessary. The NULL case is handled in the
> > callers.
> >
> > devmem and io_uring are updated accordingly to use this helper instead
> > of directly using the parent.
> >
> > Signed-off-by: Dragos Tatulea <dtatulea@...dia.com>
> > Fixes: 170aafe35cb9 ("netdev: support binding dma-buf to netdevice")
>
> nit: This doesn't seem like a fix? The current code supports all
> devices that are not SF well enough, right? And in the case of SF
> devices, I expect net_devmem_bind_dmabuf() to fail gracefully as the
> dma mapping of a device that doesn't support it, I think, would fail
> gracefully. So to me this seems like an improvement rather than a bug
> fix.
>
dma_buf_map_attachment_unlocked() will return a sg_table with 0 nents.
That is graceful. However this will result in page_pools that will
always be returning errors further down the line which is very confusing
regarding the motives that caused it.
I am also fine to not make it a fix btw. Especially since the mlx5
devmem code was just accepted.
> > Reviewed-by: Tariq Toukan <tariqt@...dia.com>
> > ---
> > Changes in v1:
> > - Upgraded from RFC status.
> > - Dropped driver specific bits for generic solution.
> > - Implemented single patch as a fix as requested in RFC.
> > - Handling of multi-PF netdevs will be handled in a subsequent patch
> > series.
> >
> > RFC: https://lore.kernel.org/all/20250702172433.1738947-2-dtatulea@nvidia.com/
> > ---
> > include/linux/netdevice.h | 14 ++++++++++++++
> > io_uring/zcrx.c | 2 +-
> > net/core/devmem.c | 10 +++++++++-
> > 3 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 5847c20994d3..1cbde7193c4d 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -5560,4 +5560,18 @@ extern struct net_device *blackhole_netdev;
> > atomic_long_add((VAL), &(DEV)->stats.__##FIELD)
> > #define DEV_STATS_READ(DEV, FIELD) atomic_long_read(&(DEV)->stats.__##FIELD)
> >
> > +static inline struct device *netdev_get_dma_dev(const struct net_device *dev)
> > +{
> > + struct device *dma_dev = dev->dev.parent;
> > +
> > + if (!dma_dev)
> > + return NULL;
> > +
> > + /* Some devices (e.g. SFs) have the dma device as a grandparent. */
> > + if (!dma_dev->dma_mask)
>
> I was able to confirm that !dev->dma_mask means "this device doesn't
> support dma". Multiple existing places in the code seem to use this
> check.
>
Ack. That was my understanding as well.
> > + dma_dev = dma_dev->parent;
> > +
> > + return (dma_dev && dma_dev->dma_mask) ? dma_dev : NULL;
>
> This may be a noob question, but are we sure that !dma_dev->dma_mask
> && dma_dev->parent->dma_mask != NULL means that the parent is the
> dma-device that we should use? I understand SF devices work that way
> but it's not immediately obvious to me that this is generically true.
>
This is what I gathered from Parav's answer.
> For example pavel came up with the case where for veth,
> netdev->dev.parent == NULL , I wonder if there are weird devices in
> the wild where netdev->dev.parent->dma_mask == NULL but that doesn't
> necessarily mean that the grandparent is the dma-device that we should
> use.
>
Yep.
> I guess to keep my long question short: what makes you think this is
> generically safe to do? Or is it not, but we think most devices behave
> this way and we're going to handle more edge cases in follow up
> patches?
>
It is just what we know so far about SFs. See end of mail.
> > +}
> > +
> > #endif /* _LINUX_NETDEVICE_H */
> > diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
> > index 797247a34cb7..93462e5b2207 100644
> > --- a/io_uring/zcrx.c
> > +++ b/io_uring/zcrx.c
> > @@ -584,7 +584,7 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx,
> > goto err;
> > }
> >
> > - ifq->dev = ifq->netdev->dev.parent;
> > + ifq->dev = netdev_get_dma_dev(ifq->netdev);
>
> nit: this hunk will not apply when backporting this to trees that only
> have the Fixes commit... which makes it more weird that this is
> considered a fix for that, but I'm fine either way.
>
Ouch, indeed. Should have thought of that.
Big picture view:
Maybe after all it is more generic to have queue ops that
can provide this info. For zcrx it is trivial (see below hunk).
For devmem I was thinking of calling netdev_queue_get_dma_dev()
for every bound queue (before the mapping) and return an error
only when we find different . It will make netdev_nl_bind_rx_doit()
a bit icky, but the idea is not complicated. What do you think?
---
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index 6e835972abd1..04c69f39558d 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h
@@ -127,6 +127,9 @@ void netdev_stat_queue_sum(struct net_device *netdev,
* @ndo_queue_stop: Stop the RX queue at the specified index. The stopped
* queue's memory is written at the specified address.
*
+ * @ndo_queue_get_dma_dev: When set, the driver can provide the DMA device to
+ * be used for the given queue.
+ *
* Note that @ndo_queue_mem_alloc and @ndo_queue_mem_free may be called while
* the interface is closed. @ndo_queue_start and @ndo_queue_stop will only
* be called for an interface which is open.
@@ -144,6 +147,8 @@ struct netdev_queue_mgmt_ops {
int (*ndo_queue_stop)(struct net_device *dev,
void *per_queue_mem,
int idx);
+ struct device * (*ndo_queue_get_dma_dev)(const struct net_device *dev,
+ int idx);
};
/**
@@ -321,4 +326,15 @@ static inline void netif_subqueue_sent(const struct net_device *dev,
get_desc, start_thrs); \
})
+static inline struct device *netdev_queue_get_dma_dev(const struct net_device *dev,
+ int idx)
+{
+ const struct netdev_queue_mgmt_ops *qops = dev->queue_mgmt_ops;
+
+ if (qops && qops->ndo_queue_get_dma_dev)
+ return qops->ndo_queue_get_dma_dev(dev, idx);
+
+ return netdev_get_dma_dev(dev);
+}
+
#endif
diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c
index 93462e5b2207..478693a6d325 100644
--- a/io_uring/zcrx.c
+++ b/io_uring/zcrx.c
@@ -12,6 +12,7 @@
#include <net/page_pool/helpers.h>
#include <net/page_pool/memory_provider.h>
#include <net/netlink.h>
+#include <net/netdev_queues.h>
#include <net/netdev_rx_queue.h>
#include <net/tcp.h>
#include <net/rps.h>
@@ -584,7 +585,7 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx,
goto err;
}
- ifq->dev = netdev_get_dma_dev(ifq->netdev);
+ ifq->dev = netdev_queue_get_dma_dev(ifq->netdev, reg.if_rxq);
if (!ifq->dev) {
ret = -EOPNOTSUPP;
goto err;
---
Thanks,
Dragos
Powered by blists - more mailing lists