[PATCH net] team: grab team lock during team_change_rx_flags

[PATCH net] team: grab team lock during team_change_rx_flags

[Stanislav Fomichev]

Syzkaller reports the following issue:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:578

 netdev_lock include/linux/netdevice.h:2751 [inline]
 netdev_lock_ops include/net/netdev_lock.h:42 [inline]
 dev_set_promiscuity+0x10e/0x260 net/core/dev_api.c:285
 bond_set_promiscuity drivers/net/bonding/bond_main.c:922 [inline]
 bond_change_rx_flags+0x219/0x690 drivers/net/bonding/bond_main.c:4732
 dev_change_rx_flags net/core/dev.c:9145 [inline]
 __dev_set_promiscuity+0x3f5/0x590 net/core/dev.c:9189
 netif_set_promiscuity+0x50/0xe0 net/core/dev.c:9201
 dev_set_promiscuity+0x126/0x260 net/core/dev_api.c:286
 ^^ all of the above is under rcu lock
 team_change_rx_flags+0x1b3/0x330 drivers/net/team/team_core.c:1785
 dev_change_rx_flags net/core/dev.c:9145 [inline]
 __dev_set_promiscuity+0x3f5/0x590 net/core/dev.c:9189
 netif_set_promiscuity+0x50/0xe0 net/core/dev.c:9201
 dev_set_promiscuity+0x126/0x260 net/core/dev_api.c:286
 hsr_del_port+0x25e/0x2d0 net/hsr/hsr_slave.c:233
 hsr_netdev_notify+0x827/0xb60 net/hsr/hsr_main.c:104
 notifier_call_chain+0x1b3/0x3e0 kernel/notifier.c:85
 call_netdevice_notifiers_extack net/core/dev.c:2214 [inline]
 call_netdevice_notifiers net/core/dev.c:2228 [inline]
 unregister_netdevice_many_notify+0x15d8/0x2330 net/core/dev.c:11970
 rtnl_delete_link net/core/rtnetlink.c:3522 [inline]
 rtnl_dellink+0x488/0x710 net/core/rtnetlink.c:3564
 rtnetlink_rcv_msg+0x7cc/0xb70 net/core/rtnetlink.c:6955
 netlink_rcv_skb+0x219/0x490 net/netlink/af_netlink.c:2534
 netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
 netlink_unicast+0x758/0x8d0 net/netlink/af_netlink.c:1339
 netlink_sendmsg+0x805/0xb30 net/netlink/af_netlink.c:1883

team_change_rx_flags runs under rcu lock which means we can't grab
instance lock for the lower devices. Switch to team->lock, similar
to what we already do for team_set_mac_address and team_change_mtu.

Fixes: 78cd408356fe ("net: add missing instance lock to dev_set_promiscuity")
Reported-by: syzbot+53485086a41dbb43270a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=53485086a41dbb43270a
Link: https://lore.kernel.org/netdev/6822cc81.050a0220.f2294.00e8.GAE@google.com
Signed-off-by: Stanislav Fomichev <stfomichev@gmail.com>
---
 drivers/net/team/team_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index d8fc0c79745d..b75ceb90359f 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -1778,8 +1778,8 @@ static void team_change_rx_flags(struct net_device *dev, int change)
 	struct team_port *port;
 	int inc;
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(port, &team->port_list, list) {
+	mutex_lock(&team->lock);
+	list_for_each_entry(port, &team->port_list, list) {
 		if (change & IFF_PROMISC) {
 			inc = dev->flags & IFF_PROMISC ? 1 : -1;
 			dev_set_promiscuity(port->dev, inc);
@@ -1789,7 +1789,7 @@ static void team_change_rx_flags(struct net_device *dev, int change)
 			dev_set_allmulti(port->dev, inc);
 		}
 	}
-	rcu_read_unlock();
+	mutex_unlock(&team->lock);
 }
 
 static void team_set_rx_mode(struct net_device *dev)
-- 
2.49.0

Re: [PATCH net] team: grab team lock during team_change_rx_flags

[Jakub Kicinski]

On Wed, 14 May 2025 15:03:19 -0700 Stanislav Fomichev wrote:
> --- a/drivers/net/team/team_core.c
> +++ b/drivers/net/team/team_core.c
> @@ -1778,8 +1778,8 @@ static void team_change_rx_flags(struct net_device *dev, int change)
>  	struct team_port *port;
>  	int inc;
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(port, &team->port_list, list) {
> +	mutex_lock(&team->lock);
> +	list_for_each_entry(port, &team->port_list, list) {

I'm not sure if change_rx_flags is allowed to sleep.
Could you try to test it on a bond with a child without IFF_UNICAST_FLT,
add an extra unicast address to the bond and remove it?
That should flip promisc on and off IIUC.

Re: [PATCH net] team: grab team lock during team_change_rx_flags

[Stanislav Fomichev]

On 05/15, Jakub Kicinski wrote:
> On Wed, 14 May 2025 15:03:19 -0700 Stanislav Fomichev wrote:
> > --- a/drivers/net/team/team_core.c
> > +++ b/drivers/net/team/team_core.c
> > @@ -1778,8 +1778,8 @@ static void team_change_rx_flags(struct net_device *dev, int change)
> >  	struct team_port *port;
> >  	int inc;
> >  
> > -	rcu_read_lock();
> > -	list_for_each_entry_rcu(port, &team->port_list, list) {
> > +	mutex_lock(&team->lock);
> > +	list_for_each_entry(port, &team->port_list, list) {
> 
> I'm not sure if change_rx_flags is allowed to sleep.
> Could you try to test it on a bond with a child without IFF_UNICAST_FLT,
> add an extra unicast address to the bond and remove it?
> That should flip promisc on and off IIUC.

I see, looks like you're concerned about addr_list_lock spin lock in
dev_set_rx_mode? (or other callers of __dev_set_rx_mode) Let me try
to reproduce with your example, but seems like it's an issue, yes
and we have a lot of ndo_change_rx_flags callbacks that are sleepable :-(

Re: [PATCH net] team: grab team lock during team_change_rx_flags

[Stanislav Fomichev]

On 05/15, Stanislav Fomichev wrote:
> On 05/15, Jakub Kicinski wrote:
> > On Wed, 14 May 2025 15:03:19 -0700 Stanislav Fomichev wrote:
> > > --- a/drivers/net/team/team_core.c
> > > +++ b/drivers/net/team/team_core.c
> > > @@ -1778,8 +1778,8 @@ static void team_change_rx_flags(struct net_device *dev, int change)
> > >  	struct team_port *port;
> > >  	int inc;
> > >  
> > > -	rcu_read_lock();
> > > -	list_for_each_entry_rcu(port, &team->port_list, list) {
> > > +	mutex_lock(&team->lock);
> > > +	list_for_each_entry(port, &team->port_list, list) {
> > 
> > I'm not sure if change_rx_flags is allowed to sleep.
> > Could you try to test it on a bond with a child without IFF_UNICAST_FLT,
> > add an extra unicast address to the bond and remove it?
> > That should flip promisc on and off IIUC.
> 
> I see, looks like you're concerned about addr_list_lock spin lock in
> dev_set_rx_mode? (or other callers of __dev_set_rx_mode) Let me try
> to reproduce with your example, but seems like it's an issue, yes
> and we have a lot of ndo_change_rx_flags callbacks that are sleepable :-(

Hmm, both bond and team set IFF_UNICAST_FLT, so it seems adding/removing uc
address on the bonding device should not flip promisc. But still will
verify for real.

Re: [PATCH net] team: grab team lock during team_change_rx_flags

[Jay Vosburgh]

Stanislav Fomichev <stfomichev@gmail.com> wrote:

>On 05/15, Stanislav Fomichev wrote:
>> On 05/15, Jakub Kicinski wrote:
>> > On Wed, 14 May 2025 15:03:19 -0700 Stanislav Fomichev wrote:
>> > > --- a/drivers/net/team/team_core.c
>> > > +++ b/drivers/net/team/team_core.c
>> > > @@ -1778,8 +1778,8 @@ static void team_change_rx_flags(struct net_device *dev, int change)
>> > >  	struct team_port *port;
>> > >  	int inc;
>> > >  
>> > > -	rcu_read_lock();
>> > > -	list_for_each_entry_rcu(port, &team->port_list, list) {
>> > > +	mutex_lock(&team->lock);
>> > > +	list_for_each_entry(port, &team->port_list, list) {
>> > 
>> > I'm not sure if change_rx_flags is allowed to sleep.
>> > Could you try to test it on a bond with a child without IFF_UNICAST_FLT,
>> > add an extra unicast address to the bond and remove it?
>> > That should flip promisc on and off IIUC.
>> 
>> I see, looks like you're concerned about addr_list_lock spin lock in
>> dev_set_rx_mode? (or other callers of __dev_set_rx_mode) Let me try
>> to reproduce with your example, but seems like it's an issue, yes
>> and we have a lot of ndo_change_rx_flags callbacks that are sleepable :-(
>
>Hmm, both bond and team set IFF_UNICAST_FLT, so it seems adding/removing uc
>address on the bonding device should not flip promisc. But still will
>verify for real.

	I think Jakub is saying that adding a unicast address to the
bond would change promisc on the underlying device that's part of the
bond (a not-IFF_UNICAST_FLT interface), not on the bond itself.  The
question is whether that change of promisc in turn generates a sleeping
function warning.

	FWIW, I think an easy way to add a unicast MAC to a bond is to
configure a VLAN above the bond, then change the MAC address of the VLAN
interface (so it doesn't match the bond's).

	-J

---
	-Jay Vosburgh, jv@jvosburgh.net

Re: [PATCH net] team: grab team lock during team_change_rx_flags

[Stanislav Fomichev]

On 05/15, Jay Vosburgh wrote:
> Stanislav Fomichev <stfomichev@gmail.com> wrote:
> 
> >On 05/15, Stanislav Fomichev wrote:
> >> On 05/15, Jakub Kicinski wrote:
> >> > On Wed, 14 May 2025 15:03:19 -0700 Stanislav Fomichev wrote:
> >> > > --- a/drivers/net/team/team_core.c
> >> > > +++ b/drivers/net/team/team_core.c
> >> > > @@ -1778,8 +1778,8 @@ static void team_change_rx_flags(struct net_device *dev, int change)
> >> > >  	struct team_port *port;
> >> > >  	int inc;
> >> > >  
> >> > > -	rcu_read_lock();
> >> > > -	list_for_each_entry_rcu(port, &team->port_list, list) {
> >> > > +	mutex_lock(&team->lock);
> >> > > +	list_for_each_entry(port, &team->port_list, list) {
> >> > 
> >> > I'm not sure if change_rx_flags is allowed to sleep.
> >> > Could you try to test it on a bond with a child without IFF_UNICAST_FLT,
> >> > add an extra unicast address to the bond and remove it?
> >> > That should flip promisc on and off IIUC.
> >> 
> >> I see, looks like you're concerned about addr_list_lock spin lock in
> >> dev_set_rx_mode? (or other callers of __dev_set_rx_mode) Let me try
> >> to reproduce with your example, but seems like it's an issue, yes
> >> and we have a lot of ndo_change_rx_flags callbacks that are sleepable :-(
> >
> >Hmm, both bond and team set IFF_UNICAST_FLT, so it seems adding/removing uc
> >address on the bonding device should not flip promisc. But still will
> >verify for real.
> 
> 	I think Jakub is saying that adding a unicast address to the
> bond would change promisc on the underlying device that's part of the
> bond (a not-IFF_UNICAST_FLT interface), not on the bond itself.  The
> question is whether that change of promisc in turn generates a sleeping
> function warning.
> 
> 	FWIW, I think an easy way to add a unicast MAC to a bond is to
> configure a VLAN above the bond, then change the MAC address of the VLAN
> interface (so it doesn't match the bond's).

This seems to work (using teaming instead of bonding, but should not matter):

  ip link add name dummy1 type dummy
  ip link add name team0 type team
  #ip link set team0 down # hit team_port_add vs team_set_rx_mode
  ip link set dev dummy1 master team0 # promisc enabled here

  ip link set dummy1 up
  ip link set team0 up # or here (if was down previously)

  ip link add link team0 name team0.100 type vlan id 100
  ip link set dev team0.100 address 00:00:00:00:00:02
  ip link set team0.100 up

But mostly because promisc is enabled via team_port_add->dev_uc_sync_multiple
(when team is up when adding a port) or via
do_setlink->netif_change_flags->ndo_set_rx_mode->team_set_rx_mode (when
upping team). The subsequent calls to __dev_set_rx_mode are noops
because the device is already in promisc. IOW, I don't see how we can
reach team_change_rx_flags from here.

Re: [PATCH net] team: grab team lock during team_change_rx_flags

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 14 May 2025 15:03:19 -0700 you wrote:
> Syzkaller reports the following issue:
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:578
> 
>  netdev_lock include/linux/netdevice.h:2751 [inline]
>  netdev_lock_ops include/net/netdev_lock.h:42 [inline]
>  dev_set_promiscuity+0x10e/0x260 net/core/dev_api.c:285
>  bond_set_promiscuity drivers/net/bonding/bond_main.c:922 [inline]
>  bond_change_rx_flags+0x219/0x690 drivers/net/bonding/bond_main.c:4732
>  dev_change_rx_flags net/core/dev.c:9145 [inline]
>  __dev_set_promiscuity+0x3f5/0x590 net/core/dev.c:9189
>  netif_set_promiscuity+0x50/0xe0 net/core/dev.c:9201
>  dev_set_promiscuity+0x126/0x260 net/core/dev_api.c:286
>  ^^ all of the above is under rcu lock
>  team_change_rx_flags+0x1b3/0x330 drivers/net/team/team_core.c:1785
>  dev_change_rx_flags net/core/dev.c:9145 [inline]
>  __dev_set_promiscuity+0x3f5/0x590 net/core/dev.c:9189
>  netif_set_promiscuity+0x50/0xe0 net/core/dev.c:9201
>  dev_set_promiscuity+0x126/0x260 net/core/dev_api.c:286
>  hsr_del_port+0x25e/0x2d0 net/hsr/hsr_slave.c:233
>  hsr_netdev_notify+0x827/0xb60 net/hsr/hsr_main.c:104
>  notifier_call_chain+0x1b3/0x3e0 kernel/notifier.c:85
>  call_netdevice_notifiers_extack net/core/dev.c:2214 [inline]
>  call_netdevice_notifiers net/core/dev.c:2228 [inline]
>  unregister_netdevice_many_notify+0x15d8/0x2330 net/core/dev.c:11970
>  rtnl_delete_link net/core/rtnetlink.c:3522 [inline]
>  rtnl_dellink+0x488/0x710 net/core/rtnetlink.c:3564
>  rtnetlink_rcv_msg+0x7cc/0xb70 net/core/rtnetlink.c:6955
>  netlink_rcv_skb+0x219/0x490 net/netlink/af_netlink.c:2534
>  netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
>  netlink_unicast+0x758/0x8d0 net/netlink/af_netlink.c:1339
>  netlink_sendmsg+0x805/0xb30 net/netlink/af_netlink.c:1883
> 
> [...]

Here is the summary with links:
  - [net] team: grab team lock during team_change_rx_flags
    https://git.kernel.org/netdev/net/c/6b1d3c5f675c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html