* [PATCH net-next] team: prevent adding a device which is already a team device lower
@ 2024-12-30 20:56 Octavian Purdila
2025-01-02 8:50 ` Hangbin Liu
2025-01-03 12:00 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 6+ messages in thread
From: Octavian Purdila @ 2024-12-30 20:56 UTC (permalink / raw)
To: jiri, andrew+netdev
Cc: edumazet, kuba, pabeni, netdev, Octavian Purdila,
syzbot+3c47b5843403a45aef57
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] team: prevent adding a device which is already a team device lower
2024-12-30 20:56 [PATCH net-next] team: prevent adding a device which is already a team device lower Octavian Purdila
@ 2025-01-02 8:50 ` Hangbin Liu
2025-01-02 8:53 ` Hangbin Liu
2025-01-03 12:00 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 6+ messages in thread
From: Hangbin Liu @ 2025-01-02 8:50 UTC (permalink / raw)
To: Octavian Purdila
Cc: jiri, andrew+netdev, edumazet, kuba, pabeni, netdev,
syzbot+3c47b5843403a45aef57
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] team: prevent adding a device which is already a team device lower
2025-01-02 8:50 ` Hangbin Liu
@ 2025-01-02 8:53 ` Hangbin Liu
2025-01-02 19:50 ` Octavian Purdila
0 siblings, 1 reply; 6+ messages in thread
From: Hangbin Liu @ 2025-01-02 8:53 UTC (permalink / raw)
To: Octavian Purdila
Cc: jiri, andrew+netdev, edumazet, kuba, pabeni, netdev,
syzbot+3c47b5843403a45aef57
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] team: prevent adding a device which is already a team device lower
2025-01-02 8:53 ` Hangbin Liu
@ 2025-01-02 19:50 ` Octavian Purdila
2025-01-06 3:46 ` Hangbin Liu
0 siblings, 1 reply; 6+ messages in thread
From: Octavian Purdila @ 2025-01-02 19:50 UTC (permalink / raw)
To: Hangbin Liu
Cc: jiri, andrew+netdev, edumazet, kuba, pabeni, netdev,
syzbot+3c47b5843403a45aef57
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?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] team: prevent adding a device which is already a team device lower
2025-01-02 19:50 ` Octavian Purdila
@ 2025-01-06 3:46 ` Hangbin Liu
0 siblings, 0 replies; 6+ messages in thread
From: Hangbin Liu @ 2025-01-06 3:46 UTC (permalink / raw)
To: Octavian Purdila
Cc: jiri, andrew+netdev, edumazet, kuba, pabeni, netdev,
syzbot+3c47b5843403a45aef57
On Thu, Jan 02, 2025 at 11:50:25AM -0800, Octavian Purdila wrote:
> > 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?
The logic makes sense to me. Let's see if Jiri has any comments.
Thanks
Hangbin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] team: prevent adding a device which is already a team device lower
2024-12-30 20:56 [PATCH net-next] team: prevent adding a device which is already a team device lower Octavian Purdila
2025-01-02 8:50 ` Hangbin Liu
@ 2025-01-03 12:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-01-03 12:00 UTC (permalink / raw)
To: Octavian Purdila
Cc: jiri, andrew+netdev, edumazet, kuba, pabeni, netdev,
syzbot+3c47b5843403a45aef57
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-06 3:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-30 20:56 [PATCH net-next] team: prevent adding a device which is already a team device lower Octavian Purdila
2025-01-02 8:50 ` Hangbin Liu
2025-01-02 8:53 ` Hangbin Liu
2025-01-02 19:50 ` Octavian Purdila
2025-01-06 3:46 ` Hangbin Liu
2025-01-03 12:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).