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

[PATCHv2 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.

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         | 5 +++++
 2 files changed, 10 insertions(+)

-- 
2.38.1

[PATCHv2 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>
Links: 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>
---
 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

[PATCHv2 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.

Reported-by: Liang Li <liali@redhat.com>
Links: https://bugzilla.redhat.com/show_bug.cgi?id=2221438
Fixes: 1d76efe1577b ("team: add support for non-ethernet devices")
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/team/team.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 555b0b1e9a78..9104e373c8cb 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2135,6 +2135,11 @@ 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 int team_dev_type_check_change(struct net_device *dev,
-- 
2.38.1

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

[Paolo Abeni]

Hi,

On Fri, 2023-07-14 at 16:13 +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.
> 
> Reported-by: Liang Li <liali@redhat.com>
> Links: https://bugzilla.redhat.com/show_bug.cgi?id=2221438
> Fixes: 1d76efe1577b ("team: add support for non-ethernet devices")
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/team/team.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 555b0b1e9a78..9104e373c8cb 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -2135,6 +2135,11 @@ 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);
> +	}

It's unclear to me what happens with the following sequence of events:

* p2p dev is enslaved to team (IFF_BROADCAST cleared)
* p2p dev is removed from team
* plain ether device is enslaved to team.

I don't see where/when IFF_BROADCAST is set again. Could you please
point it out?

Thanks!

Paolo

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

[Hangbin Liu]

On Tue, Jul 18, 2023 at 10:01:27AM +0200, Paolo Abeni wrote:
> > diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> > index 555b0b1e9a78..9104e373c8cb 100644
> > --- a/drivers/net/team/team.c
> > +++ b/drivers/net/team/team.c
> > @@ -2135,6 +2135,11 @@ 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);
> > +	}
> 
> It's unclear to me what happens with the following sequence of events:
> 
> * p2p dev is enslaved to team (IFF_BROADCAST cleared)
> * p2p dev is removed from team
> * plain ether device is enslaved to team.
> 
> I don't see where/when IFF_BROADCAST is set again. Could you please
> point it out?

Hmm, you are right. Bonding will call bond_ether_setup(), ether_setup() to
reset the dev flags. But team didn't do that. I will fix it.

Thanks
Hangbin