| Message ID | 20230718101741.2751799-3-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix up dev flags when add P2P down link | expand |
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
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
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,
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
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; }
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(-)