[Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Cong Wang]

syzbot managed to trigger a recursive NETDEV_FEAT_CHANGE event
between bonding master and slave. I managed to find a reproducer
for this:

  ip li set bond0 up
  ifenslave bond0 eth0
  brctl addbr br0
  ethtool -K eth0 lro off
  brctl addif br0 bond0
  ip li set br0 up

When a NETDEV_FEAT_CHANGE event is triggered on a bonding slave,
it captures this and calls bond_compute_features() to fixup its
master's and other slaves' features. However, when syncing with
its lower devices by netdev_sync_lower_features() this event is
triggered again on slaves, so it goes back and forth recursively
until the kernel stack is exhausted.

It is unnecessary to trigger it for a second time, because when
we update the features from top down, we rely on each
dev->netdev_ops->ndo_fix_features() to do the job, each stacked
device should implement it. NETDEV_FEAT_CHANGE event is necessary
when we update from bottom up, like in existing stacked device
implementations.

Just calling __netdev_update_features() is sufficient to fix this
issue.

Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jay Vosburgh <j.vosburgh@gmail.com>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 522288177bbd..ece50ae346c3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
 				   &feature, lower->name);
 			lower->wanted_features &= ~feature;
-			netdev_update_features(lower);
+			__netdev_update_features(lower);
 
 			if (unlikely(lower->features & feature))
 				netdev_WARN(upper, "failed to disable %pNF on %s!\n",
-- 
2.26.2

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Michal Kubecek]

On Tue, May 05, 2020 at 02:58:19PM -0700, Cong Wang wrote:
> syzbot managed to trigger a recursive NETDEV_FEAT_CHANGE event
> between bonding master and slave. I managed to find a reproducer
> for this:
> 
>   ip li set bond0 up
>   ifenslave bond0 eth0
>   brctl addbr br0
>   ethtool -K eth0 lro off
>   brctl addif br0 bond0
>   ip li set br0 up
> 
> When a NETDEV_FEAT_CHANGE event is triggered on a bonding slave,
> it captures this and calls bond_compute_features() to fixup its
> master's and other slaves' features. However, when syncing with
> its lower devices by netdev_sync_lower_features() this event is
> triggered again on slaves, so it goes back and forth recursively
> until the kernel stack is exhausted.
> 
> It is unnecessary to trigger it for a second time, because when
> we update the features from top down, we rely on each
> dev->netdev_ops->ndo_fix_features() to do the job, each stacked
> device should implement it. NETDEV_FEAT_CHANGE event is necessary
> when we update from bottom up, like in existing stacked device
> implementations.
> 
> Just calling __netdev_update_features() is sufficient to fix this
> issue.
> 
> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
> Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
> Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
> Cc: Jarod Wilson <jarod@redhat.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 522288177bbd..ece50ae346c3 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
>  			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
>  				   &feature, lower->name);
>  			lower->wanted_features &= ~feature;
> -			netdev_update_features(lower);
> +			__netdev_update_features(lower);
>  
>  			if (unlikely(lower->features & feature))
>  				netdev_WARN(upper, "failed to disable %pNF on %s!\n",

Wouldn't this mean that when we disable LRO on a bond manually with
"ethtool -K", LRO will be also disabled on its slaves but no netlink
notification for them would be sent to userspace?

Michal

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Cong Wang]

On Tue, May 5, 2020 at 3:27 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Tue, May 05, 2020 at 02:58:19PM -0700, Cong Wang wrote:
> > syzbot managed to trigger a recursive NETDEV_FEAT_CHANGE event
> > between bonding master and slave. I managed to find a reproducer
> > for this:
> >
> >   ip li set bond0 up
> >   ifenslave bond0 eth0
> >   brctl addbr br0
> >   ethtool -K eth0 lro off
> >   brctl addif br0 bond0
> >   ip li set br0 up
> >
> > When a NETDEV_FEAT_CHANGE event is triggered on a bonding slave,
> > it captures this and calls bond_compute_features() to fixup its
> > master's and other slaves' features. However, when syncing with
> > its lower devices by netdev_sync_lower_features() this event is
> > triggered again on slaves, so it goes back and forth recursively
> > until the kernel stack is exhausted.
> >
> > It is unnecessary to trigger it for a second time, because when
> > we update the features from top down, we rely on each
> > dev->netdev_ops->ndo_fix_features() to do the job, each stacked
> > device should implement it. NETDEV_FEAT_CHANGE event is necessary
> > when we update from bottom up, like in existing stacked device
> > implementations.
> >
> > Just calling __netdev_update_features() is sufficient to fix this
> > issue.
> >
> > Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
> > Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
> > Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
> > Cc: Jarod Wilson <jarod@redhat.com>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Jay Vosburgh <j.vosburgh@gmail.com>
> > Cc: Jann Horn <jannh@google.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> > ---
> >  net/core/dev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 522288177bbd..ece50ae346c3 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
> >                       netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> >                                  &feature, lower->name);
> >                       lower->wanted_features &= ~feature;
> > -                     netdev_update_features(lower);
> > +                     __netdev_update_features(lower);
> >
> >                       if (unlikely(lower->features & feature))
> >                               netdev_WARN(upper, "failed to disable %pNF on %s!\n",
>
> Wouldn't this mean that when we disable LRO on a bond manually with
> "ethtool -K", LRO will be also disabled on its slaves but no netlink
> notification for them would be sent to userspace?

What netlink notification are you talking about?

When we change features from top down, ->ndo_fix_features()
does the work, in bonding case, it is bond_fix_features().
I see no netlink notification either in bond_compute_features()
or bond_fix_features().

Thanks.

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Jay Vosburgh]

Cong Wang <xiyou.wangcong@gmail.com> wrote:

>syzbot managed to trigger a recursive NETDEV_FEAT_CHANGE event
>between bonding master and slave. I managed to find a reproducer
>for this:
>
>  ip li set bond0 up
>  ifenslave bond0 eth0
>  brctl addbr br0
>  ethtool -K eth0 lro off
>  brctl addif br0 bond0
>  ip li set br0 up

	Presumably this is tied to the LRO feature being special in
netdev_sync_lower_features (via NETIF_F_UPPER_DISABLES), but why doesn't
LRO become disabled and stop the recursion once the test

		if (!(features & feature) && (lower->features & feature)) {

	no longer evalutes to true (in theory)?

	-J

>When a NETDEV_FEAT_CHANGE event is triggered on a bonding slave,
>it captures this and calls bond_compute_features() to fixup its
>master's and other slaves' features. However, when syncing with
>its lower devices by netdev_sync_lower_features() this event is
>triggered again on slaves, so it goes back and forth recursively
>until the kernel stack is exhausted.
>
>It is unnecessary to trigger it for a second time, because when
>we update the features from top down, we rely on each
>dev->netdev_ops->ndo_fix_features() to do the job, each stacked
>device should implement it. NETDEV_FEAT_CHANGE event is necessary
>when we update from bottom up, like in existing stacked device
>implementations.
>
>Just calling __netdev_update_features() is sufficient to fix this
>issue.
>
>Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
>Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
>Cc: Jarod Wilson <jarod@redhat.com>
>Cc: Josh Poimboeuf <jpoimboe@redhat.com>
>Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>Cc: Jann Horn <jannh@google.com>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>---
> net/core/dev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 522288177bbd..ece50ae346c3 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
> 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> 				   &feature, lower->name);
> 			lower->wanted_features &= ~feature;
>-			netdev_update_features(lower);
>+			__netdev_update_features(lower);
> 
> 			if (unlikely(lower->features & feature))
> 				netdev_WARN(upper, "failed to disable %pNF on %s!\n",
>-- 
>2.26.2
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Michal Kubecek]

On Tue, May 05, 2020 at 03:35:27PM -0700, Cong Wang wrote:
> On Tue, May 5, 2020 at 3:27 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > On Tue, May 05, 2020 at 02:58:19PM -0700, Cong Wang wrote:
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 522288177bbd..ece50ae346c3 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
> > >                       netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> > >                                  &feature, lower->name);
> > >                       lower->wanted_features &= ~feature;
> > > -                     netdev_update_features(lower);
> > > +                     __netdev_update_features(lower);
> > >
> > >                       if (unlikely(lower->features & feature))
> > >                               netdev_WARN(upper, "failed to disable %pNF on %s!\n",
> >
> > Wouldn't this mean that when we disable LRO on a bond manually with
> > "ethtool -K", LRO will be also disabled on its slaves but no netlink
> > notification for them would be sent to userspace?
> 
> What netlink notification are you talking about?

There is ethtool notification sent by ethnl_netdev_event() and rtnetlink
notification sent by rtnetlink_event(). Both are triggered by
NETDEV_FEAT_CHANGE notifier so unless I missed something, when you
suppress the notifier, there will be no netlink notifications to
userspace.

Michal

> When we change features from top down, ->ndo_fix_features()
> does the work, in bonding case, it is bond_fix_features().
> I see no netlink notification either in bond_compute_features()
> or bond_fix_features().
> 
> Thanks.

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Cong Wang]

On Tue, May 5, 2020 at 3:42 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> >syzbot managed to trigger a recursive NETDEV_FEAT_CHANGE event
> >between bonding master and slave. I managed to find a reproducer
> >for this:
> >
> >  ip li set bond0 up
> >  ifenslave bond0 eth0
> >  brctl addbr br0
> >  ethtool -K eth0 lro off
> >  brctl addif br0 bond0
> >  ip li set br0 up
>
>         Presumably this is tied to the LRO feature being special in
> netdev_sync_lower_features (via NETIF_F_UPPER_DISABLES), but why doesn't
> LRO become disabled and stop the recursion once the test
>
>                 if (!(features & feature) && (lower->features & feature)) {
>
>         no longer evalutes to true (in theory)?

Good point!

Actually the LRO feature fails to disable:

[   62.559537] netdevice: bond0: failed to disable 0x0000000000008000 on eth0!
...
[   78.312003] netdevice: eth0: failed to disable LRO!

It seems we should only skip netdev_update_features() for such case,
like below. Note __netdev_update_features() intentionally returns -1
for this failure, so I am afraid we just have to live with it.

diff --git a/net/core/dev.c b/net/core/dev.c
index 522288177bbd..8040b07214fa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8907,11 +8907,13 @@ static void netdev_sync_lower_features(struct
net_device *upper,
                        netdev_dbg(upper, "Disabling feature %pNF on
lower dev %s.\n",
                                   &feature, lower->name);
                        lower->wanted_features &= ~feature;
-                       netdev_update_features(lower);
+                       __netdev_update_features(lower);

                        if (unlikely(lower->features & feature))
                                netdev_WARN(upper, "failed to disable
%pNF on %s!\n",
                                            &feature, lower->name);
+                       else
+                               netdev_update_features(lower);
                }
        }
 }

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Cong Wang]

On Wed, May 6, 2020 at 11:46 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, May 5, 2020 at 3:42 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> >
> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > >syzbot managed to trigger a recursive NETDEV_FEAT_CHANGE event
> > >between bonding master and slave. I managed to find a reproducer
> > >for this:
> > >
> > >  ip li set bond0 up
> > >  ifenslave bond0 eth0
> > >  brctl addbr br0
> > >  ethtool -K eth0 lro off
> > >  brctl addif br0 bond0
> > >  ip li set br0 up
> >
> >         Presumably this is tied to the LRO feature being special in
> > netdev_sync_lower_features (via NETIF_F_UPPER_DISABLES), but why doesn't
> > LRO become disabled and stop the recursion once the test
> >
> >                 if (!(features & feature) && (lower->features & feature)) {
> >
> >         no longer evalutes to true (in theory)?
>
> Good point!
>
> Actually the LRO feature fails to disable:
>
> [   62.559537] netdevice: bond0: failed to disable 0x0000000000008000 on eth0!
> ...
> [   78.312003] netdevice: eth0: failed to disable LRO!
>
> It seems we should only skip netdev_update_features() for such case,
> like below. Note __netdev_update_features() intentionally returns -1
> for this failure, so I am afraid we just have to live with it.

Oops, I meant netdev_features_change() of course.

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Cong Wang]

On Tue, May 5, 2020 at 10:26 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Tue, May 05, 2020 at 03:35:27PM -0700, Cong Wang wrote:
> > On Tue, May 5, 2020 at 3:27 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > > On Tue, May 05, 2020 at 02:58:19PM -0700, Cong Wang wrote:
> > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > index 522288177bbd..ece50ae346c3 100644
> > > > --- a/net/core/dev.c
> > > > +++ b/net/core/dev.c
> > > > @@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
> > > >                       netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> > > >                                  &feature, lower->name);
> > > >                       lower->wanted_features &= ~feature;
> > > > -                     netdev_update_features(lower);
> > > > +                     __netdev_update_features(lower);
> > > >
> > > >                       if (unlikely(lower->features & feature))
> > > >                               netdev_WARN(upper, "failed to disable %pNF on %s!\n",
> > >
> > > Wouldn't this mean that when we disable LRO on a bond manually with
> > > "ethtool -K", LRO will be also disabled on its slaves but no netlink
> > > notification for them would be sent to userspace?
> >
> > What netlink notification are you talking about?
>
> There is ethtool notification sent by ethnl_netdev_event() and rtnetlink
> notification sent by rtnetlink_event(). Both are triggered by
> NETDEV_FEAT_CHANGE notifier so unless I missed something, when you
> suppress the notifier, there will be no netlink notifications to
> userspace.

Oh, interesting, why ethtool_notify() can't be called directly, for example,
in netdev_update_features()? To me, using a NETDEV_FEAT_CHANGE
event handler seems unnecessary for ethtool netlink.

BTW, as pointed out by Jay, actually we only need to skip
NETDEV_FEAT_CHANGE for failure case, so I will update my patch.

Thanks.

Re: [Patch net] net: fix a potential recursive NETDEV_FEAT_CHANGE

[Michal Kubecek]

On Wed, May 06, 2020 at 12:08:35PM -0700, Cong Wang wrote:
> On Tue, May 5, 2020 at 10:26 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> >
> > On Tue, May 05, 2020 at 03:35:27PM -0700, Cong Wang wrote:
> > > On Tue, May 5, 2020 at 3:27 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> > > > On Tue, May 05, 2020 at 02:58:19PM -0700, Cong Wang wrote:
> > > > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > > > index 522288177bbd..ece50ae346c3 100644
> > > > > --- a/net/core/dev.c
> > > > > +++ b/net/core/dev.c
> > > > > @@ -8907,7 +8907,7 @@ static void netdev_sync_lower_features(struct net_device *upper,
> > > > >                       netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> > > > >                                  &feature, lower->name);
> > > > >                       lower->wanted_features &= ~feature;
> > > > > -                     netdev_update_features(lower);
> > > > > +                     __netdev_update_features(lower);
> > > > >
> > > > >                       if (unlikely(lower->features & feature))
> > > > >                               netdev_WARN(upper, "failed to disable %pNF on %s!\n",
> > > >
> > > > Wouldn't this mean that when we disable LRO on a bond manually with
> > > > "ethtool -K", LRO will be also disabled on its slaves but no netlink
> > > > notification for them would be sent to userspace?
> > >
> > > What netlink notification are you talking about?
> >
> > There is ethtool notification sent by ethnl_netdev_event() and rtnetlink
> > notification sent by rtnetlink_event(). Both are triggered by
> > NETDEV_FEAT_CHANGE notifier so unless I missed something, when you
> > suppress the notifier, there will be no netlink notifications to
> > userspace.
> 
> Oh, interesting, why ethtool_notify() can't be called directly, for example,
> in netdev_update_features()? To me, using a NETDEV_FEAT_CHANGE
> event handler seems unnecessary for ethtool netlink.

It is certainly an option and as this is the only use of netdev
notifiers in ethtool code, it might even be more convenient. I rather
felt that when the call of notifier chain is in netdev_features_change()
already, it would be cleaner to use it. But my point rather was that the
NETDEV_FEAT_CHANGE event is used for more than only feature propagation
between upper/lower devices so that suppressing it may have unwanted
side effects (ethtool notifications were of course the first example
that came to my mind).

> BTW, as pointed out by Jay, actually we only need to skip
> NETDEV_FEAT_CHANGE for failure case, so I will update my patch.

Sounds like a good idea. I was wondering about this myself yesterday but
I didn't have time to look into it deeper so I only realized the problem
would probably be a difference between features and wanted_features.

Michal