netdev - Re: [PATCH net] bpf: xdp: fallback to SKB mode if DRV flag is absent.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zw5GNHSjgut12LEV@fedora>
Date: Tue, 15 Oct 2024 10:38:44 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>
Cc: Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Andrii Nakryiko <andriin@...com>, Jussi Maki <joamaki@...il.com>,
	Jay Vosburgh <jv@...sburgh.net>, linux-kernel@...r.kernel.org,
	bpf@...r.kernel.org, Liang Li <liali@...hat.com>
Subject: Re: [PATCH net] bpf: xdp: fallback to SKB mode if DRV flag is absent.

On Tue, Oct 15, 2024 at 12:53:08PM +0300, Nikolay Aleksandrov wrote:
> On 15/10/2024 11:17, Daniel Borkmann wrote:
> > On 10/15/24 5:36 AM, Hangbin Liu wrote:
> >> After commit c8a36f1945b2 ("bpf: xdp: Fix XDP mode when no mode flags
> >> specified"), the mode is automatically set to XDP_MODE_DRV if the driver
> >> implements the .ndo_bpf function. However, for drivers like bonding, which
> >> only support native XDP for specific modes, this may result in an
> >> "unsupported" response.
> >>
> >> In such cases, let's fall back to SKB mode if the user did not explicitly
> >> request DRV mode.
> >>
> 
> So behaviour changed once, now it's changing again.. 

This should not be a behaviour change, it just follow the fallback rules.

> IMO it's better to explicitly
> error out and let the user decide how to resolve the situation. 

The user feels confused and reported a bug. Because cmd
`ip link set bond0 xdp obj xdp_dummy.o section xdp` failed with "Operation
not supported" in stead of fall back to xdpgeneral mode.

> The above commit
> is 4 years old, surely everyone is used to the behaviour by now. If you insist
> to do auto-fallback, then at least I'd go with Daniel's suggestion and do it
> in the bonding device. Maybe it can return -EFALLBACK, or some other way to
> signal the caller and change the mode, but you assume that's what the user
> would want, maybe it is and maybe it's not - that is why I'd prefer the
> explicit error so conscious action can be taken to resolve the situation.
> 
> That being said, I don't have a strong preference, just my few cents. :)
> 
> >> Fixes: c8a36f1945b2 ("bpf: xdp: Fix XDP mode when no mode flags specified")
> >> Reported-by: Liang Li <liali@...hat.com>
> >> Closes: https://issues.redhat.com/browse/RHEL-62339
> > 
> > nit: The link is not accessible to the public.

I made it public now.

> > 
> > Also, this breaks BPF CI with regards to existing bonding selftest :
> > 
> >   https://github.com/kernel-patches/bpf/actions/runs/11340153361/job/31536275257

The following should fix the selftest error.

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 18d1314fa797..0c380558a25d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5705,7 +5705,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
                if (dev_xdp_prog_count(slave_dev) > 0) {
                        SLAVE_NL_ERR(dev, slave_dev, extack,
                                     "Slave has XDP program loaded, please unload before enslaving");
-                       err = -EOPNOTSUPP;
+                       err = -EEXIST;
                        goto err;
                }

But it doesn't solve the problem if the slave has xdp program loaded while
using an unsupported bond mode, which will return too early.

If there is not other driver has this problem. I can try fix this on
bonding side.

Thanks
Hangbin