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

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

[Nikola Z. Ivanov]

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.

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
Signed-off-by: Nikola Z. Ivanov <zlatistiv@gmail.com>
---
 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] team: Move team device type change at the end of team_port_add

[Jakub Kicinski]

On Wed, 12 Nov 2025 02:34:44 +0200 Nikola Z. Ivanov 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.
> 
> 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.

Since this is a bug fix it must have a Fixes tag pointing to first
commit where the issue could be reproduced.

Please make sure to have a quick read of (at least the tl;dr of)
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
before reposting.
-- 
pw-bot: cr

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

[Paolo Abeni]

On 11/12/25 1:34 AM, Nikola Z. Ivanov wrote:
> @@ -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;

Please don't add unneeded new labels, instead reuse the exiting
`err_set_slave_promisc`.

Thanks,

Paolo

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

[Jiri Pirko]

Tue, Nov 18, 2025 at 12:46:36PM +0100, pabeni@redhat.com wrote:
>On 11/12/25 1:34 AM, Nikola Z. Ivanov wrote:
>> @@ -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;
>
>Please don't add unneeded new labels, instead reuse the exiting
>`err_set_slave_promisc`.

Well, that is how error labels are done in team. "action" and
"err_action" is always paired. Why to break this consistent pattern?

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

[Nikola Z. Ivanov]

On Wed, Nov 19, 2025 at 05:10:15PM +0100, Jiri Pirko wrote:
> Tue, Nov 18, 2025 at 12:46:36PM +0100, pabeni@redhat.com wrote:
> >On 11/12/25 1:34 AM, Nikola Z. Ivanov wrote:
> >> @@ -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;
> >
> >Please don't add unneeded new labels, instead reuse the exiting
> >`err_set_slave_promisc`.
> 
> Well, that is how error labels are done in team. "action" and
> "err_action" is always paired. Why to break this consistent pattern?

Hi Jiri,

This pattern is already broken in the same function by this:

        /* set promiscuity level to new slave */
        if (dev->flags & IFF_PROMISC) {
                err = dev_set_promiscuity(port_dev, 1);
                if (err)
                        goto err_set_slave_promisc;
        }

        /* set allmulti level to new slave */
        if (dev->flags & IFF_ALLMULTI) {
                err = dev_set_allmulti(port_dev, 1);
                if (err) {
                        if (dev->flags & IFF_PROMISC)
                                dev_set_promiscuity(port_dev, -1);
                        goto err_set_slave_promisc;
                }
        }

So I guess I should also "break" it or do it as you've just
suggested and add another label "err_set_slave_allmulti"
so that we are at least consistent with this.

Thank you!

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

[Jiri Pirko]

Wed, Nov 19, 2025 at 05:24:37PM +0100, zlatistiv@gmail.com wrote:
>On Wed, Nov 19, 2025 at 05:10:15PM +0100, Jiri Pirko wrote:
>> Tue, Nov 18, 2025 at 12:46:36PM +0100, pabeni@redhat.com wrote:
>> >On 11/12/25 1:34 AM, Nikola Z. Ivanov wrote:
>> >> @@ -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;
>> >
>> >Please don't add unneeded new labels, instead reuse the exiting
>> >`err_set_slave_promisc`.
>> 
>> Well, that is how error labels are done in team. "action" and
>> "err_action" is always paired. Why to break this consistent pattern?
>
>Hi Jiri,
>
>This pattern is already broken in the same function by this:
>
>        /* set promiscuity level to new slave */
>        if (dev->flags & IFF_PROMISC) {
>                err = dev_set_promiscuity(port_dev, 1);
>                if (err)
>                        goto err_set_slave_promisc;
>        }
>
>        /* set allmulti level to new slave */
>        if (dev->flags & IFF_ALLMULTI) {
>                err = dev_set_allmulti(port_dev, 1);
>                if (err) {
>                        if (dev->flags & IFF_PROMISC)
>                                dev_set_promiscuity(port_dev, -1);
>                        goto err_set_slave_promisc;
>                }
>        }
>
>So I guess I should also "break" it or do it as you've just
>suggested and add another label "err_set_slave_allmulti"
>so that we are at least consistent with this.

:( I think it would be fine to fix it, by another patch targetting
   net-next tree. Thanks!

>
>Thank you!