linux-kernel - Re: [PATCH net v2] team: Move team device type change at the end of team_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <gbashnabbmnsqwwno5rc4piiwkluriytfcnvfdejn4abwovfkl@furcaobquk5t>
Date: Wed, 19 Nov 2025 17:51:42 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: "Nikola Z. Ivanov" <zlatistiv@...il.com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, david.hunter.linux@...il.com, 
	khalid@...nel.org, linux-kernel-mentees@...ts.linuxfoundation.org, 
	syzbot+a2a3b519de727b0f7903@...kaller.appspotmail.com
Subject: Re: [PATCH net v2] team: Move team device type change at the end of
 team_port_add

Thu, Nov 13, 2025 at 10:11:42PM +0100, zlatistiv@...il.com wrote:
>Attempting to add a port device that is already up will expectedly fail,
>but not before modifying the team device header_ops.
>
>In the case of the syzbot reproducer the gre0 device is
>already in state UP when it attempts to add it as a
>port device of team0, this fails but before that
>header_ops->create of team0 is changed from eth_header to ipgre_header
>in the call to team_dev_type_check_change.
>
>Later when we end up in ipgre_header() struct *ip_tunnel points to nonsense
>as the private data of the device still holds a struct team.
>
>Example sequence of iproute commands to reproduce the hang/BUG():
>ip link add dev team0 type team
>ip link add dev gre0 type gre
>ip link set dev gre0 up
>ip link set dev gre0 master team0
>ip link set dev team0 up
>ping -I team0 1.1.1.1
>
>Move team_dev_type_check_change down where all other checks have passed
>as it changes the dev type with no way to restore it in case
>one of the checks that follow it fail.
>
>Also make sure to preserve the origial mtu assignment:
>  - If port_dev is not the same type as dev, dev takes mtu from port_dev
>  - If port_dev is the same type as dev, port_dev takes mtu from dev
>
>Testing:
>  - team device driver in-tree selftests
>  - Add/remove various devices as slaves of team device
>  - syzbot
>
>Reported-by: syzbot+a2a3b519de727b0f7903@...kaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=a2a3b519de727b0f7903
>Fixes: 1d76efe1577b ("team: add support for non-ethernet devices")
>Signed-off-by: Nikola Z. Ivanov <zlatistiv@...il.com>
>---
>Changes since v1:
>  - Add a "Fixes" tag
>  - Add a simple reproducer in the commit log
>  https://lore.kernel.org/netdev/20251111171341.4c6d69be@kernel.org/T/#u
>
> drivers/net/team/team_core.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
>index 29dc04c299a3..94c149e89231 100644
>--- a/drivers/net/team/team_core.c
>+++ b/drivers/net/team/team_core.c
>@@ -1134,10 +1134,6 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
> 		return -EPERM;
> 	}
> 
>-	err = team_dev_type_check_change(dev, port_dev);
>-	if (err)
>-		return err;
>-
> 	if (port_dev->flags & IFF_UP) {
> 		NL_SET_ERR_MSG(extack, "Device is up. Set it down before adding it as a team port");
> 		netdev_err(dev, "Device %s is up. Set it down before adding it as a team port\n",
>@@ -1155,10 +1151,12 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
> 	INIT_LIST_HEAD(&port->qom_list);
> 
> 	port->orig.mtu = port_dev->mtu;
>-	err = dev_set_mtu(port_dev, dev->mtu);
>-	if (err) {
>-		netdev_dbg(dev, "Error %d calling dev_set_mtu\n", err);
>-		goto err_set_mtu;
>+	if (dev->type == port_dev->type) {
>+		err = dev_set_mtu(port_dev, dev->mtu);

I'm probably missing something. Why you made this conditional? Didn't
find mentioning that in patch description.



>+		if (err) {
>+			netdev_dbg(dev, "Error %d calling dev_set_mtu\n", err);
>+			goto err_set_mtu;
>+		}
> 	}
> 
> 	memcpy(port->orig.dev_addr, port_dev->dev_addr, port_dev->addr_len);
>@@ -1233,6 +1231,10 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
> 		}
> 	}
> 
>+	err = team_dev_type_check_change(dev, port_dev);
>+	if (err)
>+		goto err_set_dev_type;
>+
> 	if (dev->flags & IFF_UP) {
> 		netif_addr_lock_bh(dev);
> 		dev_uc_sync_multiple(port_dev, dev);
>@@ -1251,6 +1253,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
> 
> 	return 0;
> 
>+err_set_dev_type:
> err_set_slave_promisc:
> 	__team_option_inst_del_port(team, port);
> 
>-- 
>2.51.0
>