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] [day] [month] [year] [list]

Date: Fri, 21 Jul 2023 11:43:39 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Paolo Abeni <pabeni@...hat.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, Jul 20, 2023 at 12:40:56PM +0200, Paolo Abeni wrote:
> > 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/

This looks like a team bug here. Team will not inherited port mtu if
they are some dev type. This would cause adding vxlan to team failed.

But if we change the team dev type and then adding vxlan. Team will
reset the mtu and add vxlan success.

With my patch, if we reset team to 1500, the later adding will failed.
So, as consistency, you suggestion is right.

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

Thanks, this looks good to me. I will update the patch.

Hangbin