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]

Date: Thu, 20 Jul 2023 10:29:19 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc: 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 Tue, 2023-07-18 at 18:17 +0800, Hangbin Liu wrote:
> When adding a point to point downlink to team device, we neglected to reset
> the team's flags, which were still using flags like BROADCAST and
> MULTICAST. Consequently, this would initiate ARP/DAD for P2P downlink
> interfaces, such as when adding a GRE device to team device. Fix this by
> remove multicast/broadcast flags and add p2p and noarp flags.
> 
> After removing the none ethernet interface and adding an ethernet interface
> to team, we need to reset team interface flags and hw address back. Unlike
> bonding interface, team do not need restore IFF_MASTER, IFF_SLAVE flags.
> 
> Reported-by: Liang Li <liali@...hat.com>
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2221438
> Fixes: 1d76efe1577b ("team: add support for non-ethernet devices")
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
> v3: add function team_ether_setup to reset team back to ethernet.
> v2: Add the missed {} after if checking.
> ---
>  drivers/net/team/team.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 555b0b1e9a78..2e124a3b81d1 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -2135,6 +2135,20 @@ 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);
> +	}
> +}
> +
> +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.

I think we just need an 'else' branch in the point2point check above,
restoring the bcast/mcast flags as needed.

Yes, we have slightly different behaviours between bond and team, but
we can't change that easily.

Cheers,

Paolo