netdev - Re: [PATCHv3 net 2/2] team: reset team's flags when down link is P2P device

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: <dcb23aaea64e5a890dd3c819dd6ba1ab2799dbfd.camel@redhat.com>
Date: Thu, 20 Jul 2023 12:40:56 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>, "David S .
 Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Eric
 Dumazet <edumazet@...gle.com>,  Liang Li <liali@...hat.com>, Jiri Pirko
 <jiri@...dia.com>, Nikolay Aleksandrov <razor@...ckwall.org>
Subject: Re: [PATCHv3 net 2/2] team: reset team's flags when down link is
 P2P device

On Thu, 2023-07-20 at 17:46 +0800, Hangbin Liu wrote:
> On Thu, Jul 20, 2023 at 10:29:19AM +0200, Paolo Abeni wrote:
> > > +static void team_ether_setup(struct net_device *dev)
> > > +{
> > > +	unsigned int flags = dev->flags & IFF_UP;
> > > +
> > > +	ether_setup(dev);
> > > +	dev->flags |= flags;
> > > +	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> > 
> > I think we can't do the above. e.g. ether_setup() sets dev->mtu to
> > ethernet default, while prior to this patch dev inherited mtu from the
> > slaved device. The change may affect the user-space in bad ways.
> 
> Hi Paolo,
> 
> I don't see the reason why we should inherited a none ethernet dev's mtu
> to an ethernet dev (i.e. add a none ethernet dev to team, then delete it and
> re-add a ethernet dev to team). I think the dev type has changed, so the
> mtu should also be updated.
> 
> BTW, after adding the port, team will also set port's mtu to team's mtu.

Let suppose the user has set the lower dev MTU to some N (< 1500) for
whatever reason (e.g. lower is a vxlan tunnel). After this change, team
will use mtu = 1500 breaking the connectivity in such scenario/

> > I think we just need an 'else' branch in the point2point check above,
> > restoring the bcast/mcast flags as needed.
> 
> Reset the flags is not enough. All the dev header_ops, type, etc are
> all need to be reset back, that's why we need to call ether_setup().

As far as I can see team_setup_by_port() takes care of that, inheriting
such infor from the current slave. What I mean is something alike the
following.

Cheers,

Paolo

---
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 555b0b1e9a78..17c8056adbe9 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2135,6 +2135,15 @@ static void team_setup_by_port(struct net_device *dev,
 	dev->mtu = port_dev->mtu;
 	memcpy(dev->broadcast, port_dev->broadcast, port_dev->addr_len);
 	eth_hw_addr_inherit(dev, port_dev);
+
+	if (port_dev->flags & IFF_POINTOPOINT) {
+		dev->flags &= ~(IFF_BROADCAST | IFF_MULTICAST);
+		dev->flags |= (IFF_POINTOPOINT | IFF_NOARP);
+	} else if ((port_dev->flags & (IFF_BROADCAST | IFF_MULTICAST)) ==
+		   (IFF_BROADCAST | IFF_MULTICAST)) {
+		dev->flags |= IFF_BROADCAST | IFF_MULTICAST;
+		dev->flags &= ~(IFF_POINTOPOINT | IFF_NOARP)
+	}
 }

 static int team_dev_type_check_change(struct net_device *dev,