diff mbox series

[net,v2] bonding: fix feature flag setting at init time

Message ID 20201202173053.13800-1-jarod@redhat.com
State New
Headers show
Series [net,v2] bonding: fix feature flag setting at init time | expand

Commit Message

Jarod Wilson Dec. 2, 2020, 5:30 p.m. UTC 
Don't try to adjust XFRM support flags if the bond device isn't yet
registered. Bad things can currently happen when netdev_change_features()
is called without having wanted_features fully filled in yet. Basically,
this code was racing against register_netdevice() filling in
wanted_features, and when it got there first, the empty wanted_features
led to features also getting emptied out, which was definitely not the
intended behavior, so prevent that from happening.

Originally, I'd hoped to stop adjusting wanted_features at all in the
bonding driver, as it's documented as being something only the network
core should touch, but we actually do need to do this to properly update
both the features and wanted_features fields when changing the bond type,
or we get to a situation where ethtool sees:

    esp-hw-offload: off [requested on]

I do think we should be using netdev_update_features instead of
netdev_change_features here though, so we only send notifiers when the
features actually changed.

v2: rework based on further testing and suggestions from ivecera

Fixes: a3b658cfb664 ("bonding: allow xfrm offload setup post-module-load")
Reported-by: Ivan Vecera <ivecera@redhat.com>
Suggested-by: Ivan Vecera <ivecera@redhat.com>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Veaceslav Falico <vfalico@gmail.com>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Thomas Davis <tadavis@lbl.gov>
Cc: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/net/bonding/bond_main.c    | 10 ++++------
 drivers/net/bonding/bond_options.c |  6 +++++-
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Jakub Kicinski Dec. 2, 2020, 5:53 p.m. UTC | #1 
On Wed,  2 Dec 2020 12:30:53 -0500 Jarod Wilson wrote:
> +	if (bond->dev->reg_state != NETREG_REGISTERED)
> +		goto noreg;
> +
>  	if (newval->value == BOND_MODE_ACTIVEBACKUP)
>  		bond->dev->wanted_features |= BOND_XFRM_FEATURES;
>  	else
>  		bond->dev->wanted_features &= ~BOND_XFRM_FEATURES;
> -	netdev_change_features(bond->dev);
> +	netdev_update_features(bond->dev);
> +noreg:

Why the goto?
Jay Vosburgh Dec. 2, 2020, 5:55 p.m. UTC | #2 
Jarod Wilson <jarod@redhat.com> wrote:

>Don't try to adjust XFRM support flags if the bond device isn't yet
>registered. Bad things can currently happen when netdev_change_features()
>is called without having wanted_features fully filled in yet. Basically,
>this code was racing against register_netdevice() filling in
>wanted_features, and when it got there first, the empty wanted_features
>led to features also getting emptied out, which was definitely not the
>intended behavior, so prevent that from happening.

	Is this an actual race?  Reading Ivan's prior message, it sounds
like it's an ordering problem (in that bond_newlink calls
register_netdevice after bond_changelink).

	The change to bond_option_mode_set tests against reg_state, so
presumably it wants to skip the first(?) time through, before the
register_netdevice call; is that right?

	-J

>Originally, I'd hoped to stop adjusting wanted_features at all in the
>bonding driver, as it's documented as being something only the network
>core should touch, but we actually do need to do this to properly update
>both the features and wanted_features fields when changing the bond type,
>or we get to a situation where ethtool sees:
>
>    esp-hw-offload: off [requested on]
>
>I do think we should be using netdev_update_features instead of
>netdev_change_features here though, so we only send notifiers when the
>features actually changed.
>
>v2: rework based on further testing and suggestions from ivecera
>
>Fixes: a3b658cfb664 ("bonding: allow xfrm offload setup post-module-load")
>Reported-by: Ivan Vecera <ivecera@redhat.com>
>Suggested-by: Ivan Vecera <ivecera@redhat.com>
>Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>Cc: Veaceslav Falico <vfalico@gmail.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Cc: "David S. Miller" <davem@davemloft.net>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Cc: Thomas Davis <tadavis@lbl.gov>
>Cc: netdev@vger.kernel.org
>Signed-off-by: Jarod Wilson <jarod@redhat.com>
>---
> drivers/net/bonding/bond_main.c    | 10 ++++------
> drivers/net/bonding/bond_options.c |  6 +++++-
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e0880a3840d7..5fe5232cc3f3 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4746,15 +4746,13 @@ void bond_setup(struct net_device *bond_dev)
> 				NETIF_F_HW_VLAN_CTAG_FILTER;
> 
> 	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
>-#ifdef CONFIG_XFRM_OFFLOAD
>-	bond_dev->hw_features |= BOND_XFRM_FEATURES;
>-#endif /* CONFIG_XFRM_OFFLOAD */
> 	bond_dev->features |= bond_dev->hw_features;
> 	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
> #ifdef CONFIG_XFRM_OFFLOAD
>-	/* Disable XFRM features if this isn't an active-backup config */
>-	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)
>-		bond_dev->features &= ~BOND_XFRM_FEATURES;
>+	bond_dev->hw_features |= BOND_XFRM_FEATURES;
>+	/* Only enable XFRM features if this is an active-backup config */
>+	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)
>+		bond_dev->features |= BOND_XFRM_FEATURES;
> #endif /* CONFIG_XFRM_OFFLOAD */
> }
> 
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 9abfaae1c6f7..19205cfac751 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -768,11 +768,15 @@ static int bond_option_mode_set(struct bonding *bond,
> 		bond->params.tlb_dynamic_lb = 1;
> 
> #ifdef CONFIG_XFRM_OFFLOAD
>+	if (bond->dev->reg_state != NETREG_REGISTERED)
>+		goto noreg;
>+
> 	if (newval->value == BOND_MODE_ACTIVEBACKUP)
> 		bond->dev->wanted_features |= BOND_XFRM_FEATURES;
> 	else
> 		bond->dev->wanted_features &= ~BOND_XFRM_FEATURES;
>-	netdev_change_features(bond->dev);
>+	netdev_update_features(bond->dev);
>+noreg:
>
> #endif /* CONFIG_XFRM_OFFLOAD */
> 
> 	/* don't cache arp_validate between modes */
>-- 
>2.28.0
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com
Jakub Kicinski Dec. 3, 2020, 4:45 p.m. UTC | #3 
On Wed,  2 Dec 2020 19:43:57 -0500 Jarod Wilson wrote:
> Don't try to adjust XFRM support flags if the bond device isn't yet

> registered. Bad things can currently happen when netdev_change_features()

> is called without having wanted_features fully filled in yet. This code

> runs on post-module-load mode changes, as well as at module init time

> and new bond creation time, and in the latter two scenarios, it is

> running prior to register_netdevice() having been called and

> subsequently filling in wanted_features. The empty wanted_features led

> to features also getting emptied out, which was definitely not the

> intended behavior, so prevent that from happening.

> 

> Originally, I'd hoped to stop adjusting wanted_features at all in the

> bonding driver, as it's documented as being something only the network

> core should touch, but we actually do need to do this to properly update

> both the features and wanted_features fields when changing the bond type,

> or we get to a situation where ethtool sees:

> 

>     esp-hw-offload: off [requested on]

> 

> I do think we should be using netdev_update_features instead of

> netdev_change_features here though, so we only send notifiers when the

> features actually changed.

> 

> v2: rework based on further testing and suggestions from ivecera

> v3: add helper function, remove goto, fix problem description

> 

> Fixes: a3b658cfb664 ("bonding: allow xfrm offload setup post-module-load")

> Reported-by: Ivan Vecera <ivecera@redhat.com>

> Suggested-by: Ivan Vecera <ivecera@redhat.com>

> Cc: Jay Vosburgh <j.vosburgh@gmail.com>

> Cc: Veaceslav Falico <vfalico@gmail.com>

> Cc: Andy Gospodarek <andy@greyhouse.net>

> Cc: "David S. Miller" <davem@davemloft.net>

> Cc: Jakub Kicinski <kuba@kernel.org>

> Cc: Thomas Davis <tadavis@lbl.gov>

> Cc: netdev@vger.kernel.org

> Signed-off-by: Jarod Wilson <jarod@redhat.com>

> ---

>  drivers/net/bonding/bond_main.c    | 10 ++++------

>  drivers/net/bonding/bond_options.c | 19 ++++++++++++++-----

>  2 files changed, 18 insertions(+), 11 deletions(-)

> 

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c

> index 47afc5938c26..7905534a763b 100644

> --- a/drivers/net/bonding/bond_main.c

> +++ b/drivers/net/bonding/bond_main.c

> @@ -4747,15 +4747,13 @@ void bond_setup(struct net_device *bond_dev)

>  				NETIF_F_HW_VLAN_CTAG_FILTER;

>  

>  	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;

> -#ifdef CONFIG_XFRM_OFFLOAD

> -	bond_dev->hw_features |= BOND_XFRM_FEATURES;

> -#endif /* CONFIG_XFRM_OFFLOAD */

>  	bond_dev->features |= bond_dev->hw_features;

>  	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;

>  #ifdef CONFIG_XFRM_OFFLOAD

> -	/* Disable XFRM features if this isn't an active-backup config */

> -	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)

> -		bond_dev->features &= ~BOND_XFRM_FEATURES;

> +	bond_dev->hw_features |= BOND_XFRM_FEATURES;

> +	/* Only enable XFRM features if this is an active-backup config */

> +	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)

> +		bond_dev->features |= BOND_XFRM_FEATURES;

>  #endif /* CONFIG_XFRM_OFFLOAD */

>  }

>  

> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c

> index 9abfaae1c6f7..1ae0e5ab8c67 100644

> --- a/drivers/net/bonding/bond_options.c

> +++ b/drivers/net/bonding/bond_options.c

> @@ -745,6 +745,18 @@ const struct bond_option *bond_opt_get(unsigned int option)

>  	return &bond_opts[option];

>  }

>  

> +#ifdef CONFIG_XFRM_OFFLOAD

> +static void bond_set_xfrm_features(struct net_device *bond_dev, u64 mode)

> +{

> +	if (mode == BOND_MODE_ACTIVEBACKUP)

> +		bond_dev->wanted_features |= BOND_XFRM_FEATURES;

> +	else

> +		bond_dev->wanted_features &= ~BOND_XFRM_FEATURES;

> +

> +	netdev_update_features(bond_dev);

> +}

> +#endif /* CONFIG_XFRM_OFFLOAD */

> +

>  static int bond_option_mode_set(struct bonding *bond,

>  				const struct bond_opt_value *newval)

>  {

> @@ -768,11 +780,8 @@ static int bond_option_mode_set(struct bonding *bond,

>  		bond->params.tlb_dynamic_lb = 1;

>  

>  #ifdef CONFIG_XFRM_OFFLOAD

> -	if (newval->value == BOND_MODE_ACTIVEBACKUP)

> -		bond->dev->wanted_features |= BOND_XFRM_FEATURES;

> -	else

> -		bond->dev->wanted_features &= ~BOND_XFRM_FEATURES;

> -	netdev_change_features(bond->dev);

> +	if (bond->dev->reg_state == NETREG_REGISTERED)

> +		bond_set_xfrm_features(bond->dev, newval->value);

>  #endif /* CONFIG_XFRM_OFFLOAD */


nit: let's narrow down the ifdef-enery

no need for the ifdef here, if the helper looks like this:

+static void bond_set_xfrm_features(struct net_device *bond_dev, u64 mode)
+{
+#ifdef CONFIG_XFRM_OFFLOAD
+	if (mode == BOND_MODE_ACTIVEBACKUP)
+		bond_dev->wanted_features |= BOND_XFRM_FEATURES;
+	else
+		bond_dev->wanted_features &= ~BOND_XFRM_FEATURES;
+
+	netdev_update_features(bond_dev);
+#endif /* CONFIG_XFRM_OFFLOAD */
+}

Even better:

+static void bond_set_xfrm_features(struct net_device *bond_dev, u64 mode)
+{
+	if (!IS_ENABLED(CONFIG_XFRM_OFFLOAD))
+		return;
+
+	if (mode == BOND_MODE_ACTIVEBACKUP)
+		bond_dev->wanted_features |= BOND_XFRM_FEATURES;
+	else
+		bond_dev->wanted_features &= ~BOND_XFRM_FEATURES;
+
+	netdev_update_features(bond_dev);
+}

(Assuming BOND_XFRM_FEATURES doesn't itself hide under an ifdef.)

>  

>  	/* don't cache arp_validate between modes */
Jakub Kicinski Dec. 3, 2020, 4:50 p.m. UTC | #4 
On Wed,  2 Dec 2020 19:43:57 -0500 Jarod Wilson wrote:
>  	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;

> -#ifdef CONFIG_XFRM_OFFLOAD

> -	bond_dev->hw_features |= BOND_XFRM_FEATURES;

> -#endif /* CONFIG_XFRM_OFFLOAD */

>  	bond_dev->features |= bond_dev->hw_features;

>  	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;

>  #ifdef CONFIG_XFRM_OFFLOAD

> -	/* Disable XFRM features if this isn't an active-backup config */

> -	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)

> -		bond_dev->features &= ~BOND_XFRM_FEATURES;

> +	bond_dev->hw_features |= BOND_XFRM_FEATURES;

> +	/* Only enable XFRM features if this is an active-backup config */

> +	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)

> +		bond_dev->features |= BOND_XFRM_FEATURES;

>  #endif /* CONFIG_XFRM_OFFLOAD */


This makes no functional change, or am I reading it wrong?
Jarod Wilson Dec. 4, 2020, 3:14 a.m. UTC | #5 
On Thu, Dec 3, 2020 at 11:50 AM Jakub Kicinski <kuba@kernel.org> wrote:
>

> On Wed,  2 Dec 2020 19:43:57 -0500 Jarod Wilson wrote:

> >       bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;

> > -#ifdef CONFIG_XFRM_OFFLOAD

> > -     bond_dev->hw_features |= BOND_XFRM_FEATURES;

> > -#endif /* CONFIG_XFRM_OFFLOAD */

> >       bond_dev->features |= bond_dev->hw_features;

> >       bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;

> >  #ifdef CONFIG_XFRM_OFFLOAD

> > -     /* Disable XFRM features if this isn't an active-backup config */

> > -     if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)

> > -             bond_dev->features &= ~BOND_XFRM_FEATURES;

> > +     bond_dev->hw_features |= BOND_XFRM_FEATURES;

> > +     /* Only enable XFRM features if this is an active-backup config */

> > +     if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)

> > +             bond_dev->features |= BOND_XFRM_FEATURES;

> >  #endif /* CONFIG_XFRM_OFFLOAD */

>

> This makes no functional change, or am I reading it wrong?


You are correct, there's ultimately no functional change there, it
primarily just condenses the code down to a single #ifdef block, and
doesn't add and then remove BOND_XFRM_FEATURES from
bond_dev->features, instead omitting it initially and only adding it
when in AB mode. I'd poked at the code in that area while trying to
get to the bottom of this, thought it made it more understandable, so
I left it in, but ultimately, it's not necessary to fix the problem
here.

-- 
Jarod Wilson
jarod@redhat.com
Jakub Kicinski Dec. 4, 2020, 3:45 p.m. UTC | #6 
On Thu, 3 Dec 2020 22:14:12 -0500 Jarod Wilson wrote:
> On Thu, Dec 3, 2020 at 11:50 AM Jakub Kicinski <kuba@kernel.org> wrote:

> >

> > On Wed,  2 Dec 2020 19:43:57 -0500 Jarod Wilson wrote:  

> > >       bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;

> > > -#ifdef CONFIG_XFRM_OFFLOAD

> > > -     bond_dev->hw_features |= BOND_XFRM_FEATURES;

> > > -#endif /* CONFIG_XFRM_OFFLOAD */

> > >       bond_dev->features |= bond_dev->hw_features;

> > >       bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;

> > >  #ifdef CONFIG_XFRM_OFFLOAD

> > > -     /* Disable XFRM features if this isn't an active-backup config */

> > > -     if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)

> > > -             bond_dev->features &= ~BOND_XFRM_FEATURES;

> > > +     bond_dev->hw_features |= BOND_XFRM_FEATURES;

> > > +     /* Only enable XFRM features if this is an active-backup config */

> > > +     if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)

> > > +             bond_dev->features |= BOND_XFRM_FEATURES;

> > >  #endif /* CONFIG_XFRM_OFFLOAD */  

> >

> > This makes no functional change, or am I reading it wrong?  

> 

> You are correct, there's ultimately no functional change there, it

> primarily just condenses the code down to a single #ifdef block, and

> doesn't add and then remove BOND_XFRM_FEATURES from

> bond_dev->features, instead omitting it initially and only adding it

> when in AB mode. I'd poked at the code in that area while trying to

> get to the bottom of this, thought it made it more understandable, so

> I left it in, but ultimately, it's not necessary to fix the problem

> here.


Makes sense, but please split it out and send separately to net-next.
Jarod Wilson Dec. 5, 2020, 4:13 p.m. UTC | #7 
On Thu, Dec 3, 2020 at 11:45 AM Jakub Kicinski <kuba@kernel.org> wrote:
...
> nit: let's narrow down the ifdef-enery

>

> no need for the ifdef here, if the helper looks like this:

>

> +static void bond_set_xfrm_features(struct net_device *bond_dev, u64 mode)

> +{

> +#ifdef CONFIG_XFRM_OFFLOAD

> +       if (mode == BOND_MODE_ACTIVEBACKUP)

> +               bond_dev->wanted_features |= BOND_XFRM_FEATURES;

> +       else

> +               bond_dev->wanted_features &= ~BOND_XFRM_FEATURES;

> +

> +       netdev_update_features(bond_dev);

> +#endif /* CONFIG_XFRM_OFFLOAD */

> +}

>

> Even better:

>

> +static void bond_set_xfrm_features(struct net_device *bond_dev, u64 mode)

> +{

> +       if (!IS_ENABLED(CONFIG_XFRM_OFFLOAD))

> +               return;

> +

> +       if (mode == BOND_MODE_ACTIVEBACKUP)

> +               bond_dev->wanted_features |= BOND_XFRM_FEATURES;

> +       else

> +               bond_dev->wanted_features &= ~BOND_XFRM_FEATURES;

> +

> +       netdev_update_features(bond_dev);

> +}

>

> (Assuming BOND_XFRM_FEATURES doesn't itself hide under an ifdef.)


It is, but doesn't need to be. I can mix these changes in as well.

-- 
Jarod Wilson
jarod@redhat.com
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e0880a3840d7..5fe5232cc3f3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4746,15 +4746,13 @@  void bond_setup(struct net_device *bond_dev)
 				NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
-#ifdef CONFIG_XFRM_OFFLOAD
-	bond_dev->hw_features |= BOND_XFRM_FEATURES;
-#endif /* CONFIG_XFRM_OFFLOAD */
 	bond_dev->features |= bond_dev->hw_features;
 	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
 #ifdef CONFIG_XFRM_OFFLOAD
-	/* Disable XFRM features if this isn't an active-backup config */
-	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)
-		bond_dev->features &= ~BOND_XFRM_FEATURES;
+	bond_dev->hw_features |= BOND_XFRM_FEATURES;
+	/* Only enable XFRM features if this is an active-backup config */
+	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP)
+		bond_dev->features |= BOND_XFRM_FEATURES;
 #endif /* CONFIG_XFRM_OFFLOAD */
 }
 
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9abfaae1c6f7..19205cfac751 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -768,11 +768,15 @@  static int bond_option_mode_set(struct bonding *bond,
 		bond->params.tlb_dynamic_lb = 1;
 
 #ifdef CONFIG_XFRM_OFFLOAD
+	if (bond->dev->reg_state != NETREG_REGISTERED)
+		goto noreg;
+
 	if (newval->value == BOND_MODE_ACTIVEBACKUP)
 		bond->dev->wanted_features |= BOND_XFRM_FEATURES;
 	else
 		bond->dev->wanted_features &= ~BOND_XFRM_FEATURES;
-	netdev_change_features(bond->dev);
+	netdev_update_features(bond->dev);
+noreg:
 #endif /* CONFIG_XFRM_OFFLOAD */
 
 	/* don't cache arp_validate between modes */