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

[Nikolay Aleksandrov]

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.. IMO it's better to explicitly
error out and let the user decide how to resolve the situation. 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.
> 
> Also, this breaks BPF CI with regards to existing bonding selftest :
> 
>   https://github.com/kernel-patches/bpf/actions/runs/11340153361/job/31536275257
> 
> Given this issue is related to only bonding driver, could this be fixed
> there instead?
> 
>> Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxx>
>> ---
>>   net/core/dev.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index ea5fbcd133ae..e32069d81cd7 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -9579,6 +9579,7 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
>>         /* don't call drivers if the effective program didn't change */
>>       if (new_prog != cur_prog) {
>> +reinstall:
>>           bpf_op = dev_xdp_bpf_op(dev, mode);
>>           if (!bpf_op) {
>>               NL_SET_ERR_MSG(extack, "Underlying driver does not support XDP in native mode");
>> @@ -9586,8 +9587,17 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
>>           }
>>             err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog);
>> -        if (err)
>> +        if (err) {
>> +            /* The driver returns not supported even .ndo_bpf
>> +             * implemented, fall back to SKB mode.
>> +             */
>> +            if (err == -EOPNOTSUPP && mode == XDP_MODE_DRV &&
>> +                !(flags & XDP_FLAGS_DRV_MODE)) {
>> +                mode = XDP_MODE_SKB;
>> +                goto reinstall;
>> +            }
>>               return err;
>> +        }
>>       }
>>         if (link)