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

[Nikolay Aleksandrov]

On 15/10/2024 13:38, Hangbin Liu wrote:
> 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.
> 

hm, what fallback rules? I see dev_xdp_attach() exits on many errors
with proper codes and extack messages, am I missing something, where's the
fallback?

>> 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.
> 

Where's the nice extack msg then? :)

We can tell them what's going on, maybe they'll want to change the bonding mode
and still use this mode rather than falling back to another mode silently.
That was my point, fallback is not the only solution.

>> 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@xxxxxxxxxx>
>>>> 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