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

[Patch net v2] 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 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: 17b85d29e82c ("net/core: revert "net: fix __netdev_update_features return.." and add comment")
Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.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 | 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

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

[Nikolay Aleksandrov]

On 06/05/2020 22:46, 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: 17b85d29e82c ("net/core: revert "net: fix __netdev_update_features return.." and add comment")
> Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
> Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
> Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.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>
> ---

The patch looks good, but note that __netdev_update_features() used to return -1
before the commit in the Fixes tag above (between 6cb6a27c45ce and 00ee59271777).
It only restored that behaviour.

>  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);
>  		}
>  	}
>  }
> 

Re: [Patch net v2] 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
>
>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: 17b85d29e82c ("net/core: revert "net: fix __netdev_update_features return.." and add comment")
>Reported-by: syzbot+e73ceacfd8560cc8a3ca@syzkaller.appspotmail.com
>Reported-by: syzbot+c2fb6f9ddcea95ba49b5@syzkaller.appspotmail.com
>Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.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>

Reviewed-by: Jay Vosburgh <jay.vosburgh@canonical.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
>

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

[Cong Wang]

On Wed, May 6, 2020 at 1:31 PM Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> The patch looks good, but note that __netdev_update_features() used to return -1
> before the commit in the Fixes tag above (between 6cb6a27c45ce and 00ee59271777).
> It only restored that behaviour.

Good point! But commit fd867d51f889 is the one which started
using netdev_update_features() in netdev_sync_lower_features(),
your commits 00ee59271777 and 17b85d29e82c are both after it,
and returning whatever doesn't matter before commit fd867d51f889,
therefore, commit fd867d51f889 is the right one to blame?

I will send V3 to just update this Fixes tag.

Thanks!

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

[Nikolay Aleksandrov]

On 07/05/2020 21:45, Cong Wang wrote:
> On Wed, May 6, 2020 at 1:31 PM Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com> wrote:
>> The patch looks good, but note that __netdev_update_features() used to return -1
>> before the commit in the Fixes tag above (between 6cb6a27c45ce and 00ee59271777).
>> It only restored that behaviour.
> 
> Good point! But commit fd867d51f889 is the one which started
> using netdev_update_features() in netdev_sync_lower_features(),
> your commits 00ee59271777 and 17b85d29e82c are both after it,
> and returning whatever doesn't matter before commit fd867d51f889,
> therefore, commit fd867d51f889 is the right one to blame?
> 

Right, that should be the one.

> I will send V3 to just update this Fixes tag.
> 
> Thanks!
> 

Cheers,
 Nik