| Message ID | 20241211071127.38452-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bond: fix xfrm offload feature during init | expand |
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@gmail.com> > --- > 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);
On Thu, Dec 12, 2024 at 11:19:33AM +0200, Nikolay Aleksandrov wrote: > > 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? The current code set netif_carrier_off(bond_dev) after register_netdevice() success, So I make it on if register failed. Thanks hangbin > > > + unregister_netdevice(bond_dev); > > } > > > > return err;
On 12/12/24 11:39, Hangbin Liu wrote: > On Thu, Dec 12, 2024 at 11:19:33AM +0200, Nikolay Aleksandrov wrote: >>> 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? > > The current code set netif_carrier_off(bond_dev) after register_netdevice() > success, So I make it on if register failed. > > Thanks > hangbin I don't like adding code just for symmetry alone, I think you should drop it unless there is an actual reason to turn carrier on. >> >>> + unregister_netdevice(bond_dev); >>> } >>> >>> return err;
On Thu, Dec 12, 2024 at 11:43:15AM +0200, Nikolay Aleksandrov wrote: > >>> --- 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? > > > > The current code set netif_carrier_off(bond_dev) after register_netdevice() > > success, So I make it on if register failed. > > > > Thanks > > hangbin > > I don't like adding code just for symmetry alone, I think you should drop it > unless there is an actual reason to turn carrier on. OK, I will drop it. Thanks Hangbin
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); + 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);
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@gmail.com> --- 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(-)