| Message ID | 20241230205647.1338900-1-tavip@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3fff5da4ca2164bb4d0f1e6cd33f6eb8a0e73e50 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] team: prevent adding a device which is already a team device lower | expand |
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>
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
On Thu, Jan 2, 2025 at 12:53 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > 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. > Hi Hangbin, Thanks for the review! I was not able to reproduce the crash with this scenario. I think this is because adding veth0.1 does not affect the link state for veth0, while in the original scenario bringing up veth0 would also bring up veth0.1. Regardless, allowing this setup seems risky and syzkaller may find other ways to trigger it, so maybe a more generic check like below would be better? list_for_each_entry(tmp, &team->port_list, list) { if (netdev_has_upper_dev(tmp->dev, port_dev) || netdev_has_upper_dev(port_dev, tmp->dev)) { NL_SET_ERR_MSG(extack, "Device is a lower or upper of an enslaved device"); netdev_err(dev, "Device %s is a lower or upper device of enslaved device %s\n", portname, tmp->dev->name); return -EBUSY; } } Although I am not sure if there are legitimate use-cases that this may restrict?
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Mon, 30 Dec 2024 12:56:47 -0800 you 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 > > [...] Here is the summary with links: - [net-next] team: prevent adding a device which is already a team device lower https://git.kernel.org/netdev/net-next/c/3fff5da4ca21 You are awesome, thank you!
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");
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(+)