[PATCH net 1/2] bnxt: don't reject XDP installation when HDS isn't forced on

[PATCH net 1/2] bnxt: don't reject XDP installation when HDS isn't forced on

[Jakub Kicinski]

HDS flag is often set because GRO-HW is enabled. But we call
bnxt_set_rx_skb_mode() later, which will clear it. So make
sure we reject XDP when user asked for HDS, not when it's
enabled for other reasons.

Fixes: 87c8f8496a05 ("bnxt_en: add support for tcp-data-split ethtool command")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: daniel@iogearbox.net
CC: hawk@kernel.org
CC: john.fastabend@gmail.com
CC: michael.chan@broadcom.com
CC: pavan.chebbi@broadcom.com
CC: ap420073@gmail.com
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index e6c64e4bd66c..ff208c4b7d70 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -11,10 +11,12 @@
 #include <linux/pci.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/if_vlan.h>
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/filter.h>
+#include <net/netdev_queues.h>
 #include <net/page_pool/helpers.h>
 #include "bnxt_hsi.h"
 #include "bnxt.h"
@@ -395,7 +397,7 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
 			    bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
 		return -EOPNOTSUPP;
 	}
-	if (prog && bp->flags & BNXT_FLAG_HDS) {
+	if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
 		netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
 		return -EOPNOTSUPP;
 	}
-- 
2.48.1

[PATCH net 2/2] selftests: drv-net: test installing XDP with HDS set to auto

[Jakub Kicinski]

Testing bnxt:

  # NETIF=eth0  ./ksft-net-drv/drivers/net/hds.py
  KTAP version 1
  1..9
  ok 1 hds.get_hds
  ok 2 hds.get_hds_thresh
  ok 3 hds.set_hds_disable # SKIP disabling of HDS not supported by the device
  ok 4 hds.set_hds_enable
  ok 5 hds.set_hds_thresh_zero
  ok 6 hds.set_hds_thresh_max
  ok 7 hds.set_hds_thresh_gt
  ok 8 hds.set_xdp
  ok 9 hds.set_xdp_enabled
  # Totals: pass:8 fail:0 xfail:0 xpass:0 skip:1 error:0

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: hawk@kernel.org
CC: petrm@nvidia.com
CC: willemb@google.com
CC: jstancek@redhat.com
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/lib/Makefile      |  3 +
 .../testing/selftests/net/lib/xdp_dummy.bpf.c | 13 +++++
 tools/testing/selftests/drivers/net/hds.py    | 55 ++++++++++++++++++-
 3 files changed, 68 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/net/lib/xdp_dummy.bpf.c

diff --git a/tools/testing/selftests/net/lib/Makefile b/tools/testing/selftests/net/lib/Makefile
index bc6b6762baf3..c22623b9a2a5 100644
--- a/tools/testing/selftests/net/lib/Makefile
+++ b/tools/testing/selftests/net/lib/Makefile
@@ -9,7 +9,10 @@ TEST_FILES := ../../../../../Documentation/netlink/specs
 TEST_FILES += ../../../../net/ynl
 
 TEST_GEN_FILES += csum
+TEST_GEN_FILES += $(patsubst %.c,%.o,$(wildcard *.bpf.c))
 
 TEST_INCLUDES := $(wildcard py/*.py sh/*.sh)
 
 include ../../lib.mk
+
+include ../bpf.mk
diff --git a/tools/testing/selftests/net/lib/xdp_dummy.bpf.c b/tools/testing/selftests/net/lib/xdp_dummy.bpf.c
new file mode 100644
index 000000000000..d988b2e0cee8
--- /dev/null
+++ b/tools/testing/selftests/net/lib/xdp_dummy.bpf.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define KBUILD_MODNAME "xdp_dummy"
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp")
+int xdp_dummy_prog(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
index 394971b25c0b..cd1a0eea39a8 100755
--- a/tools/testing/selftests/drivers/net/hds.py
+++ b/tools/testing/selftests/drivers/net/hds.py
@@ -2,17 +2,26 @@
 # SPDX-License-Identifier: GPL-2.0
 
 import errno
+import os
 from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_raises, KsftSkipEx
-from lib.py import EthtoolFamily, NlError
+from lib.py import CmdExitFailure, EthtoolFamily, NlError
 from lib.py import NetDrvEnv
+from lib.py import defer, ip
 
-def get_hds(cfg, netnl) -> None:
+
+def _get_hds_mode(cfg, netnl) -> str:
     try:
         rings = netnl.rings_get({'header': {'dev-index': cfg.ifindex}})
     except NlError as e:
         raise KsftSkipEx('ring-get not supported by device')
     if 'tcp-data-split' not in rings:
         raise KsftSkipEx('tcp-data-split not supported by device')
+    return rings['tcp-data-split']
+
+
+def get_hds(cfg, netnl) -> None:
+    _get_hds_mode(cfg, netnl)
+
 
 def get_hds_thresh(cfg, netnl) -> None:
     try:
@@ -104,6 +113,44 @@ from lib.py import NetDrvEnv
         netnl.rings_set({'header': {'dev-index': cfg.ifindex}, 'hds-thresh': hds_gt})
     ksft_eq(e.exception.nl_msg.error, -errno.EINVAL)
 
+
+def set_xdp(cfg, netnl) -> None:
+    """
+    Enable single-buffer XDP on the device.
+    When HDS is in "auto" / UNKNOWN mode, XDP installation should work.
+    """
+    mode = _get_hds_mode(cfg, netnl)
+    if mode == 'enabled':
+        netnl.rings_set({'header': {'dev-index': cfg.ifindex},
+                         'tcp-data-split': 'unknown'})
+
+    test_dir = os.path.dirname(os.path.realpath(__file__))
+    prog = test_dir + "/../../net/lib/xdp_dummy.bpf.o"
+    ip(f"link set dev %s xdp obj %s sec xdp" %
+        (cfg.ifname, prog))
+    ip(f"link set dev %s xdp off" % cfg.ifname)
+
+
+def set_xdp_enabled(cfg, netnl) -> None:
+    """
+    Enable single-buffer XDP on the device.
+    When HDS is in "enabled" mode, XDP installation should not work.
+    """
+    _get_hds_mode(cfg, netnl)
+    netnl.rings_set({'header': {'dev-index': cfg.ifindex},
+                     'tcp-data-split': 'enabled'})
+
+    defer(netnl.rings_set, {'header': {'dev-index': cfg.ifindex},
+                            'tcp-data-split': 'unknown'})
+
+    test_dir = os.path.dirname(os.path.realpath(__file__))
+    prog = test_dir + "/../../net/lib/xdp_dummy.bpf.o"
+    with ksft_raises(CmdExitFailure) as e:
+        ip(f"link set dev %s xdp obj %s sec xdp" %
+            (cfg.ifname, prog))
+        ip(f"link set dev %s xdp off" % cfg.ifname)
+
+
 def main() -> None:
     with NetDrvEnv(__file__, queue_count=3) as cfg:
         ksft_run([get_hds,
@@ -112,7 +159,9 @@ from lib.py import NetDrvEnv
                   set_hds_enable,
                   set_hds_thresh_zero,
                   set_hds_thresh_max,
-                  set_hds_thresh_gt],
+                  set_hds_thresh_gt,
+                  set_xdp,
+                  set_xdp_enabled],
                  args=(cfg, EthtoolFamily()))
     ksft_exit()
 
-- 
2.48.1

Re: [PATCH net 1/2] bnxt: don't reject XDP installation when HDS isn't forced on

[Daniel Xu]

On Wed, Feb 19, 2025 at 04:53:17PM -0800, Jakub Kicinski wrote:
> HDS flag is often set because GRO-HW is enabled. But we call
> bnxt_set_rx_skb_mode() later, which will clear it. So make
> sure we reject XDP when user asked for HDS, not when it's
> enabled for other reasons.
> 
> Fixes: 87c8f8496a05 ("bnxt_en: add support for tcp-data-split ethtool command")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: daniel@iogearbox.net
> CC: hawk@kernel.org
> CC: john.fastabend@gmail.com
> CC: michael.chan@broadcom.com
> CC: pavan.chebbi@broadcom.com
> CC: ap420073@gmail.com
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index e6c64e4bd66c..ff208c4b7d70 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> @@ -11,10 +11,12 @@
>  #include <linux/pci.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
> +#include <linux/ethtool_netlink.h>
>  #include <linux/if_vlan.h>
>  #include <linux/bpf.h>
>  #include <linux/bpf_trace.h>
>  #include <linux/filter.h>
> +#include <net/netdev_queues.h>
>  #include <net/page_pool/helpers.h>
>  #include "bnxt_hsi.h"
>  #include "bnxt.h"
> @@ -395,7 +397,7 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
>  			    bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
>  		return -EOPNOTSUPP;
>  	}
> -	if (prog && bp->flags & BNXT_FLAG_HDS) {
> +	if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
>  		netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
>  		return -EOPNOTSUPP;
>  	}
> -- 
> 2.48.1
> 

Nice, that fixed it.

Tested-by: Daniel Xu <dxu@dxuuu.xyz>

Re: [PATCH net 1/2] bnxt: don't reject XDP installation when HDS isn't forced on

[Jakub Kicinski]

On Wed, 19 Feb 2025 18:58:02 -0700 Daniel Xu wrote:
> > @@ -395,7 +397,7 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
> >  			    bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
> >  		return -EOPNOTSUPP;
> >  	}
> > -	if (prog && bp->flags & BNXT_FLAG_HDS) {
> > +	if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
> >  		netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> >  		return -EOPNOTSUPP;
> >  	}
> > -- 
> > 2.48.1
> >   
> 
> Nice, that fixed it.
> 
> Tested-by: Daniel Xu <dxu@dxuuu.xyz>

I looked again after sending because it wasn't sitting 100% well with
me. As the commit message says this will work, because it forces all
flags to off. But the driver is also only setting its internal flag
when user requested. So why does it get set in the first place..

I think the real fix may be:

@@ -2071,6 +2072,8 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
 
        dev->ethtool_ops->get_ringparam(dev, &max, &kernel_ringparam, NULL);
 
+       kernel_ringparam.tcp_data_split = dev->cfg->hds_config;
+
        /* ensure new ring parameters are within the maximums */
        if (ringparam.rx_pending > max.rx_max_pending ||
            ringparam.rx_mini_pending > max.rx_mini_max_pending ||

This is the legacy / ioctl path. We don't hit it in testing, but you
probably hit it via systemd.

At least that's my current theory, waiting for the test kernel 
to deploy.  Sorry for the flip flop..

Re: [PATCH net 1/2] bnxt: don't reject XDP installation when HDS isn't forced on

[Taehee Yoo]

On Thu, Feb 20, 2025 at 11:14 AM Jakub Kicinski <kuba@kernel.org> wrote:

Hi Jakub, Thank you so much for this fix!

>
> On Wed, 19 Feb 2025 18:58:02 -0700 Daniel Xu wrote:
> > > @@ -395,7 +397,7 @@ static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
> > >                         bp->dev->mtu, BNXT_MAX_PAGE_MODE_MTU);
> > >             return -EOPNOTSUPP;
> > >     }
> > > -   if (prog && bp->flags & BNXT_FLAG_HDS) {
> > > +   if (prog && dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
> > >             netdev_warn(dev, "XDP is disallowed when HDS is enabled.\n");
> > >             return -EOPNOTSUPP;
> > >     }
> > > --
> > > 2.48.1
> > >
> >
> > Nice, that fixed it.
> >
> > Tested-by: Daniel Xu <dxu@dxuuu.xyz>
>
> I looked again after sending because it wasn't sitting 100% well with
> me. As the commit message says this will work, because it forces all
> flags to off. But the driver is also only setting its internal flag
> when user requested. So why does it get set in the first place..
>
> I think the real fix may be:
>
> @@ -2071,6 +2072,8 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
>
>         dev->ethtool_ops->get_ringparam(dev, &max, &kernel_ringparam, NULL);
>
> +       kernel_ringparam.tcp_data_split = dev->cfg->hds_config;
> +
>         /* ensure new ring parameters are within the maximums */
>         if (ringparam.rx_pending > max.rx_max_pending ||
>             ringparam.rx_mini_pending > max.rx_mini_max_pending ||
>
> This is the legacy / ioctl path. We don't hit it in testing, but you
> probably hit it via systemd.
>
> At least that's my current theory, waiting for the test kernel
> to deploy.  Sorry for the flip flop..

As you mentioned, I tested it with legacy/ioctl path.

How to reproduce:
ethtool -K eth0 lro on gro on
ethtool --disable-netlink -G eth0 rx 512
ip link set eth0 xdp obj xdp.o

With this change, I can't see this bug anymore.

Thanks a lot!
Taehee Yoo