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: <b223add3-169a-4753-bdac-9f4cfc95eb97@iogearbox.net>
Date: Wed, 16 Oct 2024 09:59:01 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: 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,
 Nikolay Aleksandrov <razor@...ckwall.org>
Subject: Re: [PATCH net-next 1/3] bonding: return detailed error when loading
 native XDP fails

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