Re: [PATCH 5.15 301/917] net: dsa: flush switchdev workqueue when leaving the bridge

[Sasha Levin]

On Mon, Nov 15, 2021 at 11:33:59PM +0000, Vladimir Oltean wrote:
> On Mon, Nov 15, 2021 at 05:56:36PM +0100, Greg Kroah-Hartman wrote:
> > From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> >
> > [ Upstream commit d7d0d423dbaa73fd0506e25971dfdab6bf185d00 ]
> >
> > DSA is preparing to offer switch drivers an API through which they can
> > associate each FDB entry with a struct net_device *bridge_dev. This can
> > be used to perform FDB isolation (the FDB lookup performed on the
> > ingress of a standalone, or bridged port, should not find an FDB entry
> > that is present in the FDB of another bridge).
> >
> > In preparation of that work, DSA needs to ensure that by the time we
> > call the switch .port_fdb_add and .port_fdb_del methods, the
> > dp->bridge_dev pointer is still valid, i.e. the port is still a bridge
> > port.
> >
> > This is not guaranteed because the SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE API
> > requires drivers that must have sleepable context to handle those events
> > to schedule the deferred work themselves. DSA does this through the
> > dsa_owq.
> >
> > It can happen that a port leaves a bridge, del_nbp() flushes the FDB on
> > that port, SWITCHDEV_FDB_DEL_TO_DEVICE is notified in atomic context,
> > DSA schedules its deferred work, but del_nbp() finishes unlinking the
> > bridge as a master from the port before DSA's deferred work is run.
> >
> > Fundamentally, the port must not be unlinked from the bridge until all
> > FDB deletion deferred work items have been flushed. The bridge must wait
> > for the completion of these hardware accesses.
> >
> > An attempt has been made to address this issue centrally in switchdev by
> > making SWITCHDEV_FDB_DEL_TO_DEVICE deferred (=> blocking) at the switchdev
> > level, which would offer implicit synchronization with del_nbp:
> >
> > https://patchwork.kernel.org/project/netdevbpf/cover/20210820115746.3701811-1-vladimir.oltean@xxxxxxx/
> >
> > but it seems that any attempt to modify switchdev's behavior and make
> > the events blocking there would introduce undesirable side effects in
> > other switchdev consumers.
> >
> > The most undesirable behavior seems to be that
> > switchdev_deferred_process_work() takes the rtnl_mutex itself, which
> > would be worse off than having the rtnl_mutex taken individually from
> > drivers which is what we have now (except DSA which has removed that
> > lock since commit 0faf890fc519 ("net: dsa: drop rtnl_lock from
> > dsa_slave_switchdev_event_work")).
> >
> > So to offer the needed guarantee to DSA switch drivers, I have come up
> > with a compromise solution that does not require switchdev rework:
> > we already have a hook at the last moment in time when the bridge is
> > still an upper of ours: the NETDEV_PRECHANGEUPPER handler. We can flush
> > the dsa_owq manually from there, which makes all FDB deletions
> > synchronous.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> > Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > ---
> >  net/dsa/port.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index 616330a16d319..3947537ed46ba 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -380,6 +380,8 @@ void dsa_port_pre_bridge_leave(struct dsa_port *dp, struct net_device *br)
> >  	switchdev_bridge_port_unoffload(brport_dev, dp,
> >  					&dsa_slave_switchdev_notifier,
> >  					&dsa_slave_switchdev_blocking_notifier);
> > +
> > +	dsa_flush_workqueue();
> >  }
> >
> >  void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
> > --
> > 2.33.0
>
> This patch represents preparation work for a new feature. Unless it
> constitutes a dependency for some other bugfix patches (which I doubt),
> my suggestion is to not backport it. Thanks.

Dropped, thanks!

--
Thanks,
Sasha