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: <2cdcad89-2677-4526-8ab5-3624d0300b7f@blackwall.org>
Date: Tue, 15 Oct 2024 12:53:08 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Daniel Borkmann <daniel@...earbox.net>, Hangbin Liu
 <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc: "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 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@...hat.com>
>> 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@...il.com>
>> ---
>>   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)