On Fri, Jul 02, 2021 at 02:16:51PM +0300, Nikolay Aleksandrov wrote:
> On 02/07/2021 11:26, Wolfgang Bumiller wrote:
> > Since commit 2796d0c648c9 ("bridge: Automatically manage
> > port promiscuous mode.")
> > bridges with `vlan_filtering 1` and only 1 auto-port don't
> > set IFF_PROMISC for unicast-filtering-capable ports.
> >
> > Normally on port changes `br_manage_promisc` is called to
> > update the promisc flags and unicast filters if necessary,
> > but it cannot distinguish between *new* ports and ones
> > losing their promisc flag, and new ports end up not
> > receiving the MAC address list.
> >
> > Fix this by calling `br_fdb_sync_static` in `br_add_if`
> > after the port promisc flags are updated and the unicast
> > filter was supposed to have been filled.
> >
> > Fixes: 2796d0c648c9 ("bridge: Automatically manage port promiscuous mode.")
> > Signed-off-by: Wolfgang Bumiller <w.bumil...@proxmox.com>
> > ---
> > Changes to v1:
> > * Added unsync to error case.
> > * Improved error message
> > * Added `Fixes` tag to commit message
> >
>
> Hi,
> One comment below..
>
> > net/bridge/br_if.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index f7d2f472ae24..2fd03a9742c8 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -652,6 +652,18 @@ int br_add_if(struct net_bridge *br, struct net_device
> > *dev,
> > list_add_rcu(&p->list, &br->port_list);
> >
> > nbp_update_port_count(br);
> > + if (!br_promisc_port(p) && (p->dev->priv_flags & IFF_UNICAST_FLT)) {
> > + /* When updating the port count we also update all ports'
> > + * promiscuous mode.
> > + * A port leaving promiscuous mode normally gets the bridge's
> > + * fdb synced to the unicast filter (if supported), however,
> > + * `br_port_clear_promisc` does not distinguish between
> > + * non-promiscuous ports and *new* ports, so we need to
> > + * sync explicitly here.
> > + */
> > + if (br_fdb_sync_static(br, p))
> > + netdev_err(dev, "failed to sync bridge static fdb
> > addresses to this port\n");
> > + }
> >
> > netdev_update_features(br->dev);
> >
> > @@ -701,6 +713,7 @@ int br_add_if(struct net_bridge *br, struct net_device
> > *dev,
> > return 0;
> >
> > err7:
> > + br_fdb_unsync_static(br, p);
>
> I don't think you should always unsync, but only if they were synced
> otherwise you
> might delete an entry that wasn't added by the bridge (e.g. promisc bond dev
> with mac A ->
> port mac A and if the bridge has that as static fdb it will delete it on
> error)
Right, sorry, I don't know why I missed that.
Conditional setup => conditional teardown, obviously >.>
>
> I've been thinking some more about this and obviously you can check if the
> sync happened,
> but you could avoid the error path if you move that sync after the vlan init
> (nbp_vlan_init())
> but before the port is STP enabled, that would avoid error handling
> altogether.
Yeah, that's true. Although it'll be easier for future changes
introducing another error case to forget about taking this into account.
Which way do you prefer?
