| 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: <kpcqanotwzhukk6oemmopw77m5v6lvbubikmk6gotfmwhezbnq@ofz2e2zq4mot>
Date: Wed, 19 Nov 2025 20:36:57 +0200
From: "Nikola Z. Ivanov" <zlatistiv@...il.com>
To: Jiri Pirko <jiri@...nulli.us>
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
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.
>
>
> >+ 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