On 12/11/24 09:11, Hangbin Liu wrote:
> The active-backup bonding mode supports XFRM ESP offload. However, when
> a bond is added using command like `ip link add bond0 type bond mode 1
> miimon 100`, the `ethtool -k` command shows that the XFRM ESP offload is
> disabled. This occurs because, in bond_newlink(), we change bond link
> first and register bond device later. So the XFRM feature update in
> bond_option_mode_set() is not called as the bond device is not yet
> registered, leading to the offload feature not being set successfully.
>
> To resolve this issue, we can modify the code order in bond_newlink() to
> ensure that the bond device is registered first before changing the bond
> link parameters. This change will allow the XFRM ESP offload feature to be
> correctly enabled.
>
> Fixes: 007ab5345545 ("bonding: fix feature flag setting at init time")
> Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxx>
> ---
> drivers/net/bonding/bond_main.c | 2 +-
> drivers/net/bonding/bond_netlink.c | 17 ++++++++++-------
> include/net/bonding.h | 1 +
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 49dd4fe195e5..7daeab67e7b5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4389,7 +4389,7 @@ void bond_work_init_all(struct bonding *bond)
> INIT_DELAYED_WORK(&bond->slave_arr_work, bond_slave_arr_handler);
> }
>
> -static void bond_work_cancel_all(struct bonding *bond)
> +void bond_work_cancel_all(struct bonding *bond)
> {
> cancel_delayed_work_sync(&bond->mii_work);
> cancel_delayed_work_sync(&bond->arp_work);
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 2a6a424806aa..7fe8c62366eb 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -568,18 +568,21 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
> struct nlattr *tb[], struct nlattr *data[],
> struct netlink_ext_ack *extack)
> {
> + struct bonding *bond = netdev_priv(bond_dev);
> int err;
>
> - err = bond_changelink(bond_dev, tb, data, extack);
> - if (err < 0)
> + err = register_netdevice(bond_dev);
> + if (err)
> return err;
>
> - err = register_netdevice(bond_dev);
> - if (!err) {
> - struct bonding *bond = netdev_priv(bond_dev);
> + netif_carrier_off(bond_dev);
> + bond_work_init_all(bond);
>
> - netif_carrier_off(bond_dev);
> - bond_work_init_all(bond);
> + err = bond_changelink(bond_dev, tb, data, extack);
> + if (err) {
> + bond_work_cancel_all(bond);
> + netif_carrier_on(bond_dev);
The patch looks good, but I'm curious why the carrier on here?
> + unregister_netdevice(bond_dev);
> }
>
> return err;
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 8bb5f016969f..e5e005cd2e17 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -707,6 +707,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
> void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay);
> void bond_work_init_all(struct bonding *bond);
> +void bond_work_cancel_all(struct bonding *bond);
>
> #ifdef CONFIG_PROC_FS
> void bond_create_proc_entry(struct bonding *bond);
