netdev - Re: [PATCH net-next 1/3] bonding: return detailed error when loading native XDP fails

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: <87ebd401-ddb3-4cb8-9e62-424b5497c33e@blackwall.org>
Date: Wed, 16 Oct 2024 11:13:19 +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>,
 Andy Gospodarek <andy@...yhouse.net>, Jonathan Corbet <corbet@....net>,
 Andrew Lunn <andrew+netdev@...n.ch>, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] bonding: return detailed error when loading
 native XDP fails

On 16/10/2024 10:59, Daniel Borkmann wrote:
> On 10/16/24 5:16 AM, Hangbin Liu wrote:
>> Bonding only supports native XDP for specific modes, which can lead to
>> confusion for users regarding why XDP loads successfully at times and
>> fails at others. This patch enhances error handling by returning detailed
>> error messages, providing users with clearer insights into the specific
>> reasons for the failure when loading native XDP.
>>
>> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
>> ---
>>   drivers/net/bonding/bond_main.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b1bffd8e9a95..f0f76b6ac8be 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -5676,8 +5676,11 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>>         ASSERT_RTNL();
>>   -    if (!bond_xdp_check(bond))
>> +    if (!bond_xdp_check(bond)) {
>> +        BOND_NL_ERR(dev, extack,
>> +                "No native XDP support for the current bonding mode");
>>           return -EOPNOTSUPP;
>> +    }
>>         old_prog = bond->xdp_prog;
>>       bond->xdp_prog = prog;
> 
> LGTM, but independent of these I was more thinking whether something like this
> could do the trick (only compile tested). That way you also get the fallback
> without changing anything in the core XDP code.
> 
> Thanks,
> Daniel
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index b1bffd8e9a95..2861b3a895ff 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5915,6 +5915,10 @@ static const struct ethtool_ops bond_ethtool_ops = {
>      .get_ts_info        = bond_ethtool_get_ts_info,
>  };
>  
> +static const struct device_type bond_type = {
> +    .name = "bond",
> +};
> +
>  static const struct net_device_ops bond_netdev_ops = {
>      .ndo_init        = bond_init,
>      .ndo_uninit        = bond_uninit,
> @@ -5951,9 +5955,20 @@ static const struct net_device_ops bond_netdev_ops = {
>      .ndo_hwtstamp_set    = bond_hwtstamp_set,
>  };
>  
> -static const struct device_type bond_type = {
> -    .name = "bond",
> -};
> +static struct net_device_ops bond_netdev_ops_noxdp __ro_after_init;
> +
> +static void __init bond_setup_noxdp_ops(void)
> +{
> +    memcpy(&bond_netdev_ops_noxdp, &bond_netdev_ops,
> +           sizeof(bond_netdev_ops));
> +
> +    /* Used for bond device mode which does not support XDP
> +     * yet, see also bond_xdp_check().
> +     */
> +    bond_netdev_ops_noxdp.ndo_bpf = NULL;
> +    bond_netdev_ops_noxdp.ndo_xdp_xmit = NULL;
> +    bond_netdev_ops_noxdp.ndo_xdp_get_xmit_slave = NULL;
> +}
>  
>  static void bond_destructor(struct net_device *bond_dev)
>  {
> @@ -5978,7 +5993,9 @@ void bond_setup(struct net_device *bond_dev)
>      /* Initialize the device entry points */
>      ether_setup(bond_dev);
>      bond_dev->max_mtu = ETH_MAX_MTU;
> -    bond_dev->netdev_ops = &bond_netdev_ops;
> +    bond_dev->netdev_ops = bond_xdp_check(bond) ?
> +                   &bond_netdev_ops :
> +                   &bond_netdev_ops_noxdp;

This will have to be done safely on bond mode change as well.
If all slaves are released we can switch modes without destroying
the device.

>      bond_dev->ethtool_ops = &bond_ethtool_ops;
>  
>      bond_dev->needs_free_netdev = true;
> @@ -6591,6 +6608,8 @@ static int __init bonding_init(void)
>      int i;
>      int res;
>  
> +    bond_setup_noxdp_ops();
> +
>      res = bond_check_params(&bonding_defaults);
>      if (res)
>          goto out;