We should not enable netmem TX for drivers that don't declare support.

Check for driver netmem TX support during devmem TX binding and fail if
the driver does not have the functionality.

Check for driver support in validate_xmit_skb as well.

Signed-off-by: Mina Almasry <almasrymina@google.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>

---

v4:
- New patch

---
 net/core/dev.c         | 3 +++
 net/core/netdev-genl.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3f525278a871..80636c569cf4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3921,6 +3921,9 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
 
 	skb = validate_xmit_xfrm(skb, features, again);
 
+	if (!skb_frags_readable(skb) && !dev->netmem_tx)
+		goto out_kfree_skb;
+
 	return skb;
 
 out_kfree_skb:
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 6e7cd6a5c177..6c5d62df0d65 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -972,6 +972,13 @@ int netdev_nl_bind_tx_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_unlock;
 	}
 
+	if (!netdev->netmem_tx) {
+		err = -EOPNOTSUPP;
+		NL_SET_ERR_MSG(info->extack,
+			       "Driver does not support netmem TX");
+		goto err_unlock;
+	}
+
 	binding = net_devmem_bind_dmabuf(netdev, DMA_TO_DEVICE, dmabuf_fd,
 					 info->extack);
 	if (IS_ERR(binding)) {
-- 
2.48.1.658.g4767266eb4-goog

On Thu, 27 Feb 2025 04:12:08 +0000 Mina Almasry wrote:
> +	if (!skb_frags_readable(skb) && !dev->netmem_tx)

How do you know it's for _this_ device tho?
The driver doesn't seem to check the DMA mapping belongs to it either.

Remind me, how do we prevent the unreadable skbs from getting into the
Tx path today?

On Fri, Feb 28, 2025 at 4:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 27 Feb 2025 04:12:08 +0000 Mina Almasry wrote:
> > +     if (!skb_frags_readable(skb) && !dev->netmem_tx)
>
> How do you know it's for _this_ device tho?

Maybe a noob question, but how do we end up here with an skb that is
not targeted for the 'dev' device? We are checking in
tcp_sendmsg_locked that we're targeting the appropriate device before
creating the skb. Is this about a packet arriving on a dmabuf bound to
a device and then being forwarded through another device that doesn't
own the mapping, bypassing the check?

> The driver doesn't seem to check the DMA mapping belongs to it either.
>
> Remind me, how do we prevent the unreadable skbs from getting into the
> Tx path today?

I'm not sure if this is about forwarding, or if there is some other
way for unreadable skbs to end up in the XT path that you have in
mind. At some point in this thread[1] we had talked about preventing
MP bound devices from being lower devices at all to side step this
entirely but you mentioned that may not be enough, and we ended up
sidestepping only XDP entirely.

[1] https://lore.kernel.org/bpf/20240821153049.7dc983db@kernel.org/


--
Thanks,
Mina

On Fri, 28 Feb 2025 17:53:24 -0800 Mina Almasry wrote:
> On Fri, Feb 28, 2025 at 4:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 27 Feb 2025 04:12:08 +0000 Mina Almasry wrote:  
> > > +     if (!skb_frags_readable(skb) && !dev->netmem_tx)  
> >
> > How do you know it's for _this_ device tho?  
> 
> Maybe a noob question, but how do we end up here with an skb that is
> not targeted for the 'dev' device? We are checking in
> tcp_sendmsg_locked that we're targeting the appropriate device before
> creating the skb. Is this about a packet arriving on a dmabuf bound to
> a device and then being forwarded through another device that doesn't
> own the mapping, bypassing the check?

Forwarded or just redirected by nft/bpf/tc

> > The driver doesn't seem to check the DMA mapping belongs to it either.
> >
> > Remind me, how do we prevent the unreadable skbs from getting into the
> > Tx path today?  
> 
> I'm not sure if this is about forwarding, or if there is some other
> way for unreadable skbs to end up in the XT path that you have in
> mind. At some point in this thread[1] we had talked about preventing
> MP bound devices from being lower devices at all to side step this
> entirely but you mentioned that may not be enough, and we ended up
> sidestepping only XDP entirely.
> 
> [1] https://lore.kernel.org/bpf/20240821153049.7dc983db@kernel.org/

Upper devices and BPF access is covered I think, by the skbuff checks.
But I think we missed adding a check in validate_xmit_skb() to protect
the xmit paths of HW|virt drivers. You can try to add a TC rule which
forwards all traffic from your devmem flow back out to the device and
see if it crashes on net-next ?

On Mon, Mar 3, 2025 at 4:29 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 28 Feb 2025 17:53:24 -0800 Mina Almasry wrote:
> > On Fri, Feb 28, 2025 at 4:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Thu, 27 Feb 2025 04:12:08 +0000 Mina Almasry wrote:
> > > > +     if (!skb_frags_readable(skb) && !dev->netmem_tx)
> > >
> > > How do you know it's for _this_ device tho?
> >
> > Maybe a noob question, but how do we end up here with an skb that is
> > not targeted for the 'dev' device? We are checking in
> > tcp_sendmsg_locked that we're targeting the appropriate device before
> > creating the skb. Is this about a packet arriving on a dmabuf bound to
> > a device and then being forwarded through another device that doesn't
> > own the mapping, bypassing the check?
>
> Forwarded or just redirected by nft/bpf/tc
>
> > > The driver doesn't seem to check the DMA mapping belongs to it either.
> > >
> > > Remind me, how do we prevent the unreadable skbs from getting into the
> > > Tx path today?
> >
> > I'm not sure if this is about forwarding, or if there is some other
> > way for unreadable skbs to end up in the XT path that you have in
> > mind. At some point in this thread[1] we had talked about preventing
> > MP bound devices from being lower devices at all to side step this
> > entirely but you mentioned that may not be enough, and we ended up
> > sidestepping only XDP entirely.
> >
> > [1] https://lore.kernel.org/bpf/20240821153049.7dc983db@kernel.org/
>
> Upper devices and BPF access is covered I think, by the skbuff checks.
> But I think we missed adding a check in validate_xmit_skb() to protect
> the xmit paths of HW|virt drivers. You can try to add a TC rule which
> forwards all traffic from your devmem flow back out to the device and
> see if it crashes on net-next ?

No crash, but by adding debug logs I'm detecting that we're passing
unreadable netmem dma-addresses to the dma_unmap_*() APIs, which is
known to be unsafe. I just can't reproduce an issue because my
platform has the IOMMU disabled.

I guess I do need to send the hunk from validate_xmit_skb() as a fix
to net and CC stable.

Another thing I'm worried about is ip_forward() inserting an
unreadable skb into the tx path somewhere higher up the stack which
calls more code that isn't expecting unreadable skbs? Specifically
worried about skb_frag_ref/unref. Does this sound like a concern as
well? Or is it a similar code path to tc?

--
Thanks,
Mina

On Mon, 3 Mar 2025 19:53:44 -0800 Mina Almasry wrote:
> > Upper devices and BPF access is covered I think, by the skbuff checks.
> > But I think we missed adding a check in validate_xmit_skb() to protect
> > the xmit paths of HW|virt drivers. You can try to add a TC rule which
> > forwards all traffic from your devmem flow back out to the device and
> > see if it crashes on net-next ?  
> 
> No crash, but by adding debug logs I'm detecting that we're passing
> unreadable netmem dma-addresses to the dma_unmap_*() APIs, which is
> known to be unsafe. I just can't reproduce an issue because my
> platform has the IOMMU disabled.
> 
> I guess I do need to send the hunk from validate_xmit_skb() as a fix
> to net and CC stable.
> 
> Another thing I'm worried about is ip_forward() inserting an
> unreadable skb into the tx path somewhere higher up the stack which
> calls more code that isn't expecting unreadable skbs? Specifically
> worried about skb_frag_ref/unref. Does this sound like a concern as
> well? Or is it a similar code path to tc?

I'd say similar to tc. We can protect drivers with a check in
validate_xmit_skb(). The second API surface we need to filter on
is skb / skb_frag helpers. The third is socket API / opt-in for
in-kernel socket readers.

Driver and socket should be "easy" to cover with an explicit
opt in. You already covered skb APIs but it's less centralized
and there may be some abuses we are not aware of. Which is why
patch 1 worries me a little.