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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMArcTXzGXBBjkhefM3iFiwYWxtsQ70QTRtVLSv_+hAERtaQKA@mail.gmail.com>
Date: Thu, 20 Feb 2025 12:33:20 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Daniel Xu <dxu@...uu.xyz>, davem@...emloft.net, netdev@...r.kernel.org, 
	edumazet@...gle.com, pabeni@...hat.com, andrew+netdev@...n.ch, 
	horms@...nel.org, daniel@...earbox.net, hawk@...nel.org, 
	john.fastabend@...il.com, michael.chan@...adcom.com, 
	pavan.chebbi@...adcom.com
Subject: Re: [PATCH net 1/2] bnxt: don't reject XDP installation when HDS
 isn't forced on

On Thu, Feb 20, 2025 at 11:14 AM Jakub Kicinski <kuba@...nel.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@...uu.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