* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 17:30 ` [PATCH net v2] " Jarod Wilson
@ 2020-12-02 17:41 ` Ivan Vecera
2020-12-02 17:53 ` Jakub Kicinski
` (2 subsequent siblings)
3 siblings, 0 replies; 21+ messages in thread
From: Ivan Vecera @ 2020-12-02 17:41 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jakub Kicinski, Thomas Davis, netdev
On Wed, 2 Dec 2020 12:30:53 -0500
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.
>
> 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 */
Tested-by: Ivan Vecera <ivecera@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 17:30 ` [PATCH net v2] " Jarod Wilson
2020-12-02 17:41 ` Ivan Vecera
@ 2020-12-02 17:53 ` Jakub Kicinski
2020-12-02 19:03 ` Jarod Wilson
2020-12-02 17:55 ` Jay Vosburgh
2020-12-03 0:43 ` [PATCH net v3] " Jarod Wilson
3 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2020-12-02 17:53 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, netdev
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?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 17:53 ` Jakub Kicinski
@ 2020-12-02 19:03 ` Jarod Wilson
2020-12-02 19:22 ` Jakub Kicinski
0 siblings, 1 reply; 21+ messages in thread
From: Jarod Wilson @ 2020-12-02 19:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: LKML, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, Netdev
On Wed, Dec 2, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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?
Seemed cleaner to prevent an extra level of indentation of the code
following the goto and before the label, but I'm not that attached to
it if it's not wanted for coding style reasons.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 19:03 ` Jarod Wilson
@ 2020-12-02 19:22 ` Jakub Kicinski
2020-12-02 19:39 ` Jarod Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2020-12-02 19:22 UTC (permalink / raw)
To: Jarod Wilson
Cc: LKML, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, Netdev
On Wed, 2 Dec 2020 14:03:53 -0500 Jarod Wilson wrote:
> On Wed, Dec 2, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > 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?
>
> Seemed cleaner to prevent an extra level of indentation of the code
> following the goto and before the label, but I'm not that attached to
> it if it's not wanted for coding style reasons.
Yes, please don't use gotos where a normal if statement is sufficient.
If you must avoid the indentation move the code to a helper.
Also - this patch did not apply to net, please make sure you're
developing on the correct base.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 19:22 ` Jakub Kicinski
@ 2020-12-02 19:39 ` Jarod Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Jarod Wilson @ 2020-12-02 19:39 UTC (permalink / raw)
To: Jakub Kicinski
Cc: LKML, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, Netdev
On Wed, Dec 2, 2020 at 2:23 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 2 Dec 2020 14:03:53 -0500 Jarod Wilson wrote:
> > On Wed, Dec 2, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > 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?
> >
> > Seemed cleaner to prevent an extra level of indentation of the code
> > following the goto and before the label, but I'm not that attached to
> > it if it's not wanted for coding style reasons.
>
> Yes, please don't use gotos where a normal if statement is sufficient.
> If you must avoid the indentation move the code to a helper.
>
> Also - this patch did not apply to net, please make sure you're
> developing on the correct base.
Argh, I must have been working in net-next instead of net, apologies.
Okay, I'll clarify the description per what Jay pointed out and adjust
the code to not include a goto, then make it on the right branch.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 17:30 ` [PATCH net v2] " Jarod Wilson
2020-12-02 17:41 ` Ivan Vecera
2020-12-02 17:53 ` Jakub Kicinski
@ 2020-12-02 17:55 ` Jay Vosburgh
2020-12-02 19:23 ` Jarod Wilson
2020-12-03 0:43 ` [PATCH net v3] " Jarod Wilson
3 siblings, 1 reply; 21+ messages in thread
From: Jay Vosburgh @ 2020-12-02 17:55 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, Ivan Vecera, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jakub Kicinski, Thomas Davis, netdev
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 17:55 ` Jay Vosburgh
@ 2020-12-02 19:23 ` Jarod Wilson
2020-12-02 20:17 ` Jay Vosburgh
0 siblings, 1 reply; 21+ messages in thread
From: Jarod Wilson @ 2020-12-02 19:23 UTC (permalink / raw)
To: Jay Vosburgh
Cc: LKML, Ivan Vecera, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jakub Kicinski, Thomas Davis, Netdev
On Wed, Dec 2, 2020 at 12:55 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> 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).
Sorry, yeah, this is not actually a race condition, just an ordering
issue, bond_check_params() gets called at init time, which leads to
bond_option_mode_set() being called, and does so prior to
bond_create() running, which is where we actually call
register_netdevice().
> 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?
Correct. Later on, when the bonding driver is already loaded, and
parameter changes are made, bond_option_mode_set() gets called and if
the mode changes to or from active-backup, we do need/want this code
to run to update wanted and features flags properly.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 19:23 ` Jarod Wilson
@ 2020-12-02 20:17 ` Jay Vosburgh
2020-12-02 20:54 ` Jarod Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Jay Vosburgh @ 2020-12-02 20:17 UTC (permalink / raw)
To: Jarod Wilson
Cc: LKML, Ivan Vecera, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jakub Kicinski, Thomas Davis, Netdev
Jarod Wilson <jarod@redhat.com> wrote:
>On Wed, Dec 2, 2020 at 12:55 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>>
>> 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).
>
>Sorry, yeah, this is not actually a race condition, just an ordering
>issue, bond_check_params() gets called at init time, which leads to
>bond_option_mode_set() being called, and does so prior to
>bond_create() running, which is where we actually call
>register_netdevice().
So this only happens if there's a "mode" module parameter? That
doesn't sound like the call path that Ivan described (coming in via
bond_newlink).
-J
>> 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?
>
>Correct. Later on, when the bonding driver is already loaded, and
>parameter changes are made, bond_option_mode_set() gets called and if
>the mode changes to or from active-backup, we do need/want this code
>to run to update wanted and features flags properly.
>
>
>--
>Jarod Wilson
>jarod@redhat.com
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v2] bonding: fix feature flag setting at init time
2020-12-02 20:17 ` Jay Vosburgh
@ 2020-12-02 20:54 ` Jarod Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Jarod Wilson @ 2020-12-02 20:54 UTC (permalink / raw)
To: Jay Vosburgh
Cc: LKML, Ivan Vecera, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jakub Kicinski, Thomas Davis, Netdev
On Wed, Dec 2, 2020 at 3:17 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> Jarod Wilson <jarod@redhat.com> wrote:
>
> >On Wed, Dec 2, 2020 at 12:55 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> >>
> >> 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).
> >
> >Sorry, yeah, this is not actually a race condition, just an ordering
> >issue, bond_check_params() gets called at init time, which leads to
> >bond_option_mode_set() being called, and does so prior to
> >bond_create() running, which is where we actually call
> >register_netdevice().
>
> So this only happens if there's a "mode" module parameter? That
> doesn't sound like the call path that Ivan described (coming in via
> bond_newlink).
Ah. I think there's actually two different pathways that can trigger
this. The first is for bonds created at module load time, which I was
describing, the second is for a new bond created via bond_newlink()
after the bonding module is already loaded, as described by Ivan. Both
have the problem of bond_option_mode_set() running prior to
register_netdevice(). Of course, that would suggest every bond
currently comes up with unintentionally neutered flags, which I
neglected to catch in earlier testing and development.
--
Jarod Wilson
jarod@redhat.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v3] bonding: fix feature flag setting at init time
2020-12-02 17:30 ` [PATCH net v2] " Jarod Wilson
` (2 preceding siblings ...)
2020-12-02 17:55 ` Jay Vosburgh
@ 2020-12-03 0:43 ` Jarod Wilson
2020-12-03 16:45 ` Jakub Kicinski
` (2 more replies)
3 siblings, 3 replies; 21+ messages in thread
From: Jarod Wilson @ 2020-12-03 0:43 UTC (permalink / raw)
To: linux-kernel
Cc: Jarod Wilson, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Jakub Kicinski, Thomas Davis,
netdev
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 */
/* don't cache arp_validate between modes */
--
2.28.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v3] bonding: fix feature flag setting at init time
2020-12-03 0:43 ` [PATCH net v3] " Jarod Wilson
@ 2020-12-03 16:45 ` Jakub Kicinski
2020-12-05 16:13 ` Jarod Wilson
2020-12-03 16:50 ` Jakub Kicinski
2020-12-05 17:22 ` [PATCH net v4] " Jarod Wilson
2 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2020-12-03 16:45 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, netdev
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 */
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH net v3] bonding: fix feature flag setting at init time
2020-12-03 16:45 ` Jakub Kicinski
@ 2020-12-05 16:13 ` Jarod Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Jarod Wilson @ 2020-12-05 16:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: LKML, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, Netdev
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v3] bonding: fix feature flag setting at init time
2020-12-03 0:43 ` [PATCH net v3] " Jarod Wilson
2020-12-03 16:45 ` Jakub Kicinski
@ 2020-12-03 16:50 ` Jakub Kicinski
2020-12-04 3:14 ` Jarod Wilson
2020-12-05 17:22 ` [PATCH net v4] " Jarod Wilson
2 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2020-12-03 16:50 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, netdev
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?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v3] bonding: fix feature flag setting at init time
2020-12-03 16:50 ` Jakub Kicinski
@ 2020-12-04 3:14 ` Jarod Wilson
2020-12-04 15:45 ` Jakub Kicinski
0 siblings, 1 reply; 21+ messages in thread
From: Jarod Wilson @ 2020-12-04 3:14 UTC (permalink / raw)
To: Jakub Kicinski
Cc: LKML, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, Netdev
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net v3] bonding: fix feature flag setting at init time
2020-12-04 3:14 ` Jarod Wilson
@ 2020-12-04 15:45 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2020-12-04 15:45 UTC (permalink / raw)
To: Jarod Wilson
Cc: LKML, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, Netdev
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net v4] bonding: fix feature flag setting at init time
2020-12-03 0:43 ` [PATCH net v3] " Jarod Wilson
2020-12-03 16:45 ` Jakub Kicinski
2020-12-03 16:50 ` Jakub Kicinski
@ 2020-12-05 17:22 ` Jarod Wilson
2020-12-08 19:27 ` Jakub Kicinski
2 siblings, 1 reply; 21+ messages in thread
From: Jarod Wilson @ 2020-12-05 17:22 UTC (permalink / raw)
To: linux-kernel
Cc: Jarod Wilson, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Jakub Kicinski, Thomas Davis,
netdev
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 both on post-module-load mode changes, as well as at module init
time, and when run at module init time, it is before register_netdevice()
has been called and filled 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.
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>
---
v2: rework based on further testing and suggestions from ivecera
v3: add helper function, remove goto
v4: drop hunk not directly related to fix, clean up ifdeffery
drivers/net/bonding/bond_options.c | 22 +++++++++++++++-------
include/net/bonding.h | 2 --
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 9abfaae1c6f7..a4e4e15f574d 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -745,6 +745,19 @@ const struct bond_option *bond_opt_get(unsigned int option)
return &bond_opts[option];
}
+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);
+}
+
static int bond_option_mode_set(struct bonding *bond,
const struct bond_opt_value *newval)
{
@@ -767,13 +780,8 @@ static int bond_option_mode_set(struct bonding *bond,
if (newval->value == BOND_MODE_ALB)
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);
-#endif /* CONFIG_XFRM_OFFLOAD */
+ if (bond->dev->reg_state == NETREG_REGISTERED)
+ bond_set_xfrm_features(bond->dev, newval->value);
/* don't cache arp_validate between modes */
bond->params.arp_validate = BOND_ARP_VALIDATE_NONE;
diff --git a/include/net/bonding.h b/include/net/bonding.h
index d9d0ff3b0ad3..adc3da776970 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -86,10 +86,8 @@
#define bond_for_each_slave_rcu(bond, pos, iter) \
netdev_for_each_lower_private_rcu((bond)->dev, pos, iter)
-#ifdef CONFIG_XFRM_OFFLOAD
#define BOND_XFRM_FEATURES (NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | \
NETIF_F_GSO_ESP)
-#endif /* CONFIG_XFRM_OFFLOAD */
#ifdef CONFIG_NET_POLL_CONTROLLER
extern atomic_t netpoll_block_tx;
--
2.28.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net v4] bonding: fix feature flag setting at init time
2020-12-05 17:22 ` [PATCH net v4] " Jarod Wilson
@ 2020-12-08 19:27 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2020-12-08 19:27 UTC (permalink / raw)
To: Jarod Wilson
Cc: linux-kernel, Ivan Vecera, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, David S. Miller, Thomas Davis, netdev
On Sat, 5 Dec 2020 12:22:29 -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 both on post-module-load mode changes, as well as at module init
> time, and when run at module init time, it is before register_netdevice()
> has been called and filled 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.
>
> Fixes: a3b658cfb664 ("bonding: allow xfrm offload setup post-module-load")
> Reported-by: Ivan Vecera <ivecera@redhat.com>
> Suggested-by: Ivan Vecera <ivecera@redhat.com>
Applied, thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread