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)
