[PATCHv3 net 0/2] Fix up dev flags when add P2P down link

[PATCHv3 net 0/2] Fix up dev flags when add P2P down link

[Hangbin Liu]

When adding p2p interfaces to bond/team. The POINTOPOINT, NOARP flags are
not inherit to up devices. Which will trigger IPv6 DAD. Since there is
no ethernet MAC address for P2P devices. This will cause unexpected DAD
failures.

v3: add function team_ether_setup to reset team back to ethernet. Thanks Paolo
v2: Add the missed {} after if checking. Thanks Nikolay.

Hangbin Liu (2):
  bonding: reset bond's flags when down link is P2P device
  team: reset team's flags when down link is P2P device

 drivers/net/bonding/bond_main.c |  5 +++++
 drivers/net/team/team.c         | 23 ++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCHv3 net 1/2] bonding: reset bond's flags when down link is P2P device

[Hangbin Liu]

When adding a point to point downlink to the bond, we neglected to reset
the bond'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 the bonding.

To address this issue, let's reset the bond's flags for P2P interfaces.

Before fix:
7: gre0@NONE: <POINTOPOINT,NOARP,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UNKNOWN group default qlen 1000
    link/gre6 2006:70:10::1 peer 2006:70:10::2 permaddr 167f:18:f188::
8: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/gre6 2006:70:10::1 brd 2006:70:10::2
    inet6 fe80::200:ff:fe00:0/64 scope link
       valid_lft forever preferred_lft forever

After fix:
7: gre0@NONE: <POINTOPOINT,NOARP,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond2 state UNKNOWN group default qlen 1000
    link/gre6 2006:70:10::1 peer 2006:70:10::2 permaddr c29e:557a:e9d9::
8: bond0: <POINTOPOINT,NOARP,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/gre6 2006:70:10::1 peer 2006:70:10::2
    inet6 fe80::1/64 scope link
       valid_lft forever preferred_lft forever

Reported-by: Liang Li <liali@redhat.com>
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2221438
Fixes: 872254dd6b1f ("net/bonding: Enable bonding to enslave non ARPHRD_ETHER")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v3: no change.
v2: Add the missed {} after if checking.
---
 drivers/net/bonding/bond_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7a0f25301f7e..484c9e3e5e82 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1508,6 +1508,11 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
 
 	memcpy(bond_dev->broadcast, slave_dev->broadcast,
 		slave_dev->addr_len);
+
+	if (slave_dev->flags & IFF_POINTOPOINT) {
+		bond_dev->flags &= ~(IFF_BROADCAST | IFF_MULTICAST);
+		bond_dev->flags |= (IFF_POINTOPOINT | IFF_NOARP);
+	}
 }
 
 /* On bonding slaves other than the currently active slave, suppress
-- 
2.38.1

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

[Hangbin Liu]

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@redhat.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@gmail.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;
 }
 
 static int team_dev_type_check_change(struct net_device *dev,
@@ -2158,7 +2172,14 @@ static int team_dev_type_check_change(struct net_device *dev,
 	}
 	dev_uc_flush(dev);
 	dev_mc_flush(dev);
-	team_setup_by_port(dev, port_dev);
+
+	if (port_dev->type != ARPHRD_ETHER) {
+		team_setup_by_port(dev, port_dev);
+	} else {
+		team_ether_setup(dev);
+		eth_hw_addr_inherit(dev, port_dev);
+	}
+
 	call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
 	return 0;
 }
-- 
2.38.1

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

[Paolo Abeni]

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@redhat.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@gmail.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

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

[Hangbin Liu]

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.

> 
> 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().

Thanks
Hangbin

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

[Paolo Abeni]

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,

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

[Hangbin Liu]

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