diff mbox series

[net-next] team: prevent adding a device which is already a team device lower

Message ID 20241230205647.1338900-1-tavip@google.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [net-next] team: prevent adding a device which is already a team device lower | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 2 this patch: 2
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch warning WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-01-01--09-00 (tests: 880)

Commit Message

Octavian Purdila Dec. 30, 2024, 8:56 p.m. UTC 
Prevent adding a device which is already a team device lower,
e.g. adding veth0 if vlan1 was already added and veth0 is a lower of
vlan1.

This is not useful in practice and can lead to recursive locking:

$ ip link add veth0 type veth peer name veth1
$ ip link set veth0 up
$ ip link set veth1 up
$ ip link add link veth0 name veth0.1 type vlan protocol 802.1Q id 1
$ ip link add team0 type team
$ ip link set veth0.1 down
$ ip link set veth0.1 master team0
team0: Port device veth0.1 added
$ ip link set veth0 down
$ ip link set veth0 master team0

============================================
WARNING: possible recursive locking detected
6.13.0-rc2-virtme-00441-ga14a429069bb #46 Not tainted
--------------------------------------------
ip/7684 is trying to acquire lock:
ffff888016848e00 (team->team_lock_key){+.+.}-{4:4}, at: team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)

but task is already holding lock:
ffff888016848e00 (team->team_lock_key){+.+.}-{4:4}, at: team_add_slave (drivers/net/team/team_core.c:1147 drivers/net/team/team_core.c:1977)

other info that might help us debug this:
Possible unsafe locking scenario:

CPU0
----
lock(team->team_lock_key);
lock(team->team_lock_key);

*** DEADLOCK ***

May be due to missing lock nesting notation

2 locks held by ip/7684:

stack backtrace:
CPU: 3 UID: 0 PID: 7684 Comm: ip Not tainted 6.13.0-rc2-virtme-00441-ga14a429069bb #46
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl (lib/dump_stack.c:122)
print_deadlock_bug.cold (kernel/locking/lockdep.c:3040)
__lock_acquire (kernel/locking/lockdep.c:3893 kernel/locking/lockdep.c:5226)
? netlink_broadcast_filtered (net/netlink/af_netlink.c:1548)
lock_acquire.part.0 (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:5851)
? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
? trace_lock_acquire (./include/trace/events/lock.h:24 (discriminator 2))
? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
? lock_acquire (kernel/locking/lockdep.c:5822)
? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
__mutex_lock (kernel/locking/mutex.c:587 kernel/locking/mutex.c:735)
? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
? fib_sync_up (net/ipv4/fib_semantics.c:2167)
? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
notifier_call_chain (kernel/notifier.c:85)
call_netdevice_notifiers_info (net/core/dev.c:1996)
__dev_notify_flags (net/core/dev.c:8993)
? __dev_change_flags (net/core/dev.c:8975)
dev_change_flags (net/core/dev.c:9027)
vlan_device_event (net/8021q/vlan.c:85 net/8021q/vlan.c:470)
? br_device_event (net/bridge/br.c:143)
notifier_call_chain (kernel/notifier.c:85)
call_netdevice_notifiers_info (net/core/dev.c:1996)
dev_open (net/core/dev.c:1519 net/core/dev.c:1505)
team_add_slave (drivers/net/team/team_core.c:1219 drivers/net/team/team_core.c:1977)
? __pfx_team_add_slave (drivers/net/team/team_core.c:1972)
do_set_master (net/core/rtnetlink.c:2917)
do_setlink.isra.0 (net/core/rtnetlink.c:3117)

Reported-by: syzbot+3c47b5843403a45aef57@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3c47b5843403a45aef57
Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device")
Signed-off-by: Octavian Purdila <tavip@google.com>
---
 drivers/net/team/team_core.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Hangbin Liu Jan. 2, 2025, 8:50 a.m. UTC | #1 
On Mon, Dec 30, 2024 at 12:56:47PM -0800, Octavian Purdila wrote:
> Prevent adding a device which is already a team device lower,
> e.g. adding veth0 if vlan1 was already added and veth0 is a lower of
> vlan1.
> 
> This is not useful in practice and can lead to recursive locking:
> 
> $ ip link add veth0 type veth peer name veth1
> $ ip link set veth0 up
> $ ip link set veth1 up
> $ ip link add link veth0 name veth0.1 type vlan protocol 802.1Q id 1
> $ ip link add team0 type team
> $ ip link set veth0.1 down
> $ ip link set veth0.1 master team0
> team0: Port device veth0.1 added
> $ ip link set veth0 down
> $ ip link set veth0 master team0
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.13.0-rc2-virtme-00441-ga14a429069bb #46 Not tainted
> --------------------------------------------
> ip/7684 is trying to acquire lock:
> ffff888016848e00 (team->team_lock_key){+.+.}-{4:4}, at: team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> 
> but task is already holding lock:
> ffff888016848e00 (team->team_lock_key){+.+.}-{4:4}, at: team_add_slave (drivers/net/team/team_core.c:1147 drivers/net/team/team_core.c:1977)
> 
> other info that might help us debug this:
> Possible unsafe locking scenario:
> 
> CPU0
> ----
> lock(team->team_lock_key);
> lock(team->team_lock_key);
> 
> *** DEADLOCK ***
> 
> May be due to missing lock nesting notation
> 
> 2 locks held by ip/7684:
> 
> stack backtrace:
> CPU: 3 UID: 0 PID: 7684 Comm: ip Not tainted 6.13.0-rc2-virtme-00441-ga14a429069bb #46
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl (lib/dump_stack.c:122)
> print_deadlock_bug.cold (kernel/locking/lockdep.c:3040)
> __lock_acquire (kernel/locking/lockdep.c:3893 kernel/locking/lockdep.c:5226)
> ? netlink_broadcast_filtered (net/netlink/af_netlink.c:1548)
> lock_acquire.part.0 (kernel/locking/lockdep.c:467 kernel/locking/lockdep.c:5851)
> ? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> ? trace_lock_acquire (./include/trace/events/lock.h:24 (discriminator 2))
> ? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> ? lock_acquire (kernel/locking/lockdep.c:5822)
> ? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> __mutex_lock (kernel/locking/mutex.c:587 kernel/locking/mutex.c:735)
> ? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> ? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> ? fib_sync_up (net/ipv4/fib_semantics.c:2167)
> ? team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> team_device_event (drivers/net/team/team_core.c:2928 drivers/net/team/team_core.c:2951 drivers/net/team/team_core.c:2973)
> notifier_call_chain (kernel/notifier.c:85)
> call_netdevice_notifiers_info (net/core/dev.c:1996)
> __dev_notify_flags (net/core/dev.c:8993)
> ? __dev_change_flags (net/core/dev.c:8975)
> dev_change_flags (net/core/dev.c:9027)
> vlan_device_event (net/8021q/vlan.c:85 net/8021q/vlan.c:470)
> ? br_device_event (net/bridge/br.c:143)
> notifier_call_chain (kernel/notifier.c:85)
> call_netdevice_notifiers_info (net/core/dev.c:1996)
> dev_open (net/core/dev.c:1519 net/core/dev.c:1505)
> team_add_slave (drivers/net/team/team_core.c:1219 drivers/net/team/team_core.c:1977)
> ? __pfx_team_add_slave (drivers/net/team/team_core.c:1972)
> do_set_master (net/core/rtnetlink.c:2917)
> do_setlink.isra.0 (net/core/rtnetlink.c:3117)
> 
> Reported-by: syzbot+3c47b5843403a45aef57@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=3c47b5843403a45aef57
> Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device")
> Signed-off-by: Octavian Purdila <tavip@google.com>
> ---
>  drivers/net/team/team_core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
> index c7690adec8db..dc7cbd6a9798 100644
> --- a/drivers/net/team/team_core.c
> +++ b/drivers/net/team/team_core.c
> @@ -1175,6 +1175,13 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
>  		return -EBUSY;
>  	}
>  
> +	if (netdev_has_upper_dev(port_dev, dev)) {
> +		NL_SET_ERR_MSG(extack, "Device is already a lower device of the team interface");
> +		netdev_err(dev, "Device %s is already a lower device of the team interface\n",
> +			   portname);
> +		return -EBUSY;
> +	}
> +
>  	if (port_dev->features & NETIF_F_VLAN_CHALLENGED &&
>  	    vlan_uses_dev(dev)) {
>  		NL_SET_ERR_MSG(extack, "Device is VLAN challenged and team device has VLAN set up");
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
Hangbin Liu Jan. 2, 2025, 8:53 a.m. UTC | #2 
On Thu, Jan 02, 2025 at 08:50:42AM +0000, Hangbin Liu wrote:
> On Mon, Dec 30, 2024 at 12:56:47PM -0800, Octavian Purdila wrote:
> > Prevent adding a device which is already a team device lower,
> > e.g. adding veth0 if vlan1 was already added and veth0 is a lower of
> > vlan1.
> > 
> > This is not useful in practice and can lead to recursive locking:
> > 
> > $ ip link add veth0 type veth peer name veth1
> > $ ip link set veth0 up
> > $ ip link set veth1 up
> > $ ip link add link veth0 name veth0.1 type vlan protocol 802.1Q id 1
> > $ ip link add team0 type team
> > $ ip link set veth0.1 down
> > $ ip link set veth0.1 master team0
> > team0: Port device veth0.1 added
> > $ ip link set veth0 down
> > $ ip link set veth0 master team0
> > 

I didn't test, what if enslave veth0 first and then add enslave veth0.1 later.

Thanks
Hangbin
diff mbox series

Patch

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index c7690adec8db..dc7cbd6a9798 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -1175,6 +1175,13 @@  static int team_port_add(struct team *team, struct net_device *port_dev,
 		return -EBUSY;
 	}
 
+	if (netdev_has_upper_dev(port_dev, dev)) {
+		NL_SET_ERR_MSG(extack, "Device is already a lower device of the team interface");
+		netdev_err(dev, "Device %s is already a lower device of the team interface\n",
+			   portname);
+		return -EBUSY;
+	}
+
 	if (port_dev->features & NETIF_F_VLAN_CHALLENGED &&
 	    vlan_uses_dev(dev)) {
 		NL_SET_ERR_MSG(extack, "Device is VLAN challenged and team device has VLAN set up");