| 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
| ||
|
Message-ID: <tdtkaqqlfivf4r4wxgoztv6ogzmp4dprbcz32psoj2sbtgphkg@bvwiovmnxtgi>
Date: Thu, 20 Nov 2025 13:12:33 +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
Wed, Nov 19, 2025 at 07:36:57PM +0100, zlatistiv@...il.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@...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.
>>
>
>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
>> >
Powered by blists - more mailing lists