1. Patchew
  2. linux
  3. View series

[PATCH net v2] team: Move team device type change at the end of team_port_add

Nikola Z. Ivanov posted 1 patch 2 months, 3 weeks ago
There is a newer version of this series
drivers/net/team/team_core.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
[PATCH net v2] team: Move team device type change at the end of team_port_add
Posted by Nikola Z. Ivanov 2 months, 3 weeks ago 
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@syzkaller.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@gmail.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);
+		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
Re: [PATCH net v2] team: Move team device type change at the end of team_port_add
Posted by Jiri Pirko 2 months, 2 weeks ago 
Thu, Nov 13, 2025 at 10:11:42PM +0100, zlatistiv@gmail.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@syzkaller.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@gmail.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
>
Re: [PATCH net v2] team: Move team device type change at the end of team_port_add
Posted by Nikola Z. Ivanov 2 months, 2 weeks ago 
On Wed, Nov 19, 2025 at 05:51:42PM +0100, Jiri Pirko wrote:
> Thu, Nov 13, 2025 at 10:11:42PM +0100, zlatistiv@gmail.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@syzkaller.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@gmail.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.
> 

Hi Jiri,

This is related to keeping the original MTU assignment logic.

Basically I see 2 cases:

if dev->type == port_dev->type then team_dev_type_check_change will
do nothing and dev_set_mtu will later assign port_dev->mtu = dev->mtu

if dev->type != port_dev->type then team_dev_type_check_change will
assign dev->mtu = port_dev->mtu and dev_set_mtu will do nothing
as they will already be the same.

Now since the patch moves the call to team_dev_type_check_change
past dev_set_mtu it changes this logic.

The conditional is there to maintain the original behavior.

If necessary I can update the commit log to address this in
more detail.

> 
> 
> >+		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
> >
Re: [PATCH net v2] team: Move team device type change at the end of team_port_add
Posted by Jiri Pirko 2 months, 2 weeks ago 
Wed, Nov 19, 2025 at 07:36:57PM +0100, zlatistiv@gmail.com wrote:
>On Wed, Nov 19, 2025 at 05:51:42PM +0100, Jiri Pirko wrote:
>> Thu, Nov 13, 2025 at 10:11:42PM +0100, zlatistiv@gmail.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@syzkaller.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@gmail.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.
>> 
>
>Hi Jiri,
>
>This is related to keeping the original MTU assignment logic.
>
>Basically I see 2 cases:
>
>if dev->type == port_dev->type then team_dev_type_check_change will
>do nothing and dev_set_mtu will later assign port_dev->mtu = dev->mtu
>
>if dev->type != port_dev->type then team_dev_type_check_change will
>assign dev->mtu = port_dev->mtu and dev_set_mtu will do nothing
>as they will already be the same.
>
>Now since the patch moves the call to team_dev_type_check_change
>past dev_set_mtu it changes this logic.
>
>The conditional is there to maintain the original behavior.
>
>If necessary I can update the commit log to address this in
>more detail.

Please do. Also, a brief comment at this check in the code it self would
be certainly nice for the reader.

Thanks!


>
>> 
>> 
>> >+		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
>> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.