On Mon, Nov 15, 2021 at 05:56:36PM +0100, Greg Kroah-Hartman wrote:
> From: Vladimir Oltean <[email protected]>
>
> [ 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/[email protected]/
>
> 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 <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> 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.
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 <[email protected]>
>>
>> [ 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/[email protected]/
>>
>> 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 <[email protected]>
>> Signed-off-by: David S. Miller <[email protected]>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>> 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