* [Patch net v3] net: fix a potential recursive NETDEV_FEAT_CHANGE
@ 2020-05-07 19:19 Cong Wang
2020-05-07 19:35 ` Nikolay Aleksandrov
2020-05-08 1:19 ` David Miller
0 siblings, 2 replies; 3+ messages in thread
From: Cong Wang @ 2020-05-07 19:19 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, syzbot+e73ceacfd8560cc8a3ca,
syzbot+c2fb6f9ddcea95ba49b5, Jarod Wilson, Nikolay Aleksandrov,
Josh Poimboeuf, Jann Horn, Jay Vosburgh
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 when the LRO feature fails to change,
so it goes back and forth recursively until the kernel stack is
exhausted.
Commit 17b85d29e82c intentionally lets __netdev_update_features()
return -1 for such a failure case, so we have to just rely on
the existing check inside netdev_sync_lower_features() and skip
NETDEV_FEAT_CHANGE event only for this specific failure case.
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: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Jann Horn <jannh@google.com>
Reviewed-by: Jay Vosburgh <jay.vosburgh@canonical.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/core/dev.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 522288177bbd..6d327b7aa813 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_features_change(lower);
}
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Patch net v3] net: fix a potential recursive NETDEV_FEAT_CHANGE
2020-05-07 19:19 [Patch net v3] net: fix a potential recursive NETDEV_FEAT_CHANGE Cong Wang
@ 2020-05-07 19:35 ` Nikolay Aleksandrov
2020-05-08 1:19 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: Nikolay Aleksandrov @ 2020-05-07 19:35 UTC (permalink / raw)
To: Cong Wang, netdev
Cc: syzbot+e73ceacfd8560cc8a3ca, syzbot+c2fb6f9ddcea95ba49b5,
Jarod Wilson, Josh Poimboeuf, Jann Horn, Jay Vosburgh
On 07/05/2020 22:19, 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 when the LRO feature fails to change,
> so it goes back and forth recursively until the kernel stack is
> exhausted.
>
> Commit 17b85d29e82c intentionally lets __netdev_update_features()
> return -1 for such a failure case, so we have to just rely on
> the existing check inside netdev_sync_lower_features() and skip
> NETDEV_FEAT_CHANGE event only for this specific failure case.
>
> 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: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jann Horn <jannh@google.com>
> Reviewed-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> net/core/dev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 522288177bbd..6d327b7aa813 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_features_change(lower);
> }
> }
> }
>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Patch net v3] net: fix a potential recursive NETDEV_FEAT_CHANGE
2020-05-07 19:19 [Patch net v3] net: fix a potential recursive NETDEV_FEAT_CHANGE Cong Wang
2020-05-07 19:35 ` Nikolay Aleksandrov
@ 2020-05-08 1:19 ` David Miller
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2020-05-08 1:19 UTC (permalink / raw)
To: xiyou.wangcong
Cc: netdev, syzbot+e73ceacfd8560cc8a3ca, syzbot+c2fb6f9ddcea95ba49b5,
jarod, nikolay, jpoimboe, jannh, jay.vosburgh
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 7 May 2020 12:19:03 -0700
> 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 when the LRO feature fails to change,
> so it goes back and forth recursively until the kernel stack is
> exhausted.
>
> Commit 17b85d29e82c intentionally lets __netdev_update_features()
> return -1 for such a failure case, so we have to just rely on
> the existing check inside netdev_sync_lower_features() and skip
> NETDEV_FEAT_CHANGE event only for this specific failure case.
>
> 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: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Jann Horn <jannh@google.com>
> Reviewed-by: Jay Vosburgh <jay.vosburgh@canonical.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied and queued up for -stable, thanks Cong.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-08 1:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07 19:19 [Patch net v3] net: fix a potential recursive NETDEV_FEAT_CHANGE Cong Wang
2020-05-07 19:35 ` Nikolay Aleksandrov
2020-05-08 1:19 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.