Re: [PATCH net-next v10 04/14] net: hold netdev instance lock during qdisc ndo_setup_tc

[Stanislav Fomichev <stfomichev@gmail.com>]

On 05/05, Cosmin Ratiu wrote:
> On Wed, 2025-03-05 at 08:37 -0800, Stanislav Fomichev wrote:
> > Qdisc operations that can lead to ndo_setup_tc might need
> > to have an instance lock. Add netdev_lock_ops/netdev_unlock_ops
> > invocations for all psched_rtnl_msg_handlers operations.
> 
> Sorry for resurrecting this thread, but it seems like a good place to
> ask a related question.
> 
> If qdisc operations that lead to .ndo_setup_tc() need to hold an
> instance lock, shouldn't all such callers acquire it?
> 
> In my testing, I found out that e.g., when unloading a device, there's
> this call path which ends up calling .ndo_setup_tc() unlocked:
> 
> devlink_reload -...-> device_del -...-> unregister_netdev -...->
> unregister_netdevice_many_notify -> dev_shutdown -> qdisc_put ->
> __qdisc_destroy -> mqprio_disable_offload -> .ndo_setup_tc
> 
> Many other qdiscs (other than mqprio) call .ndo_setup_tc() and would be
> in a similar situation.
> 
> Does it make sense to extend the netdev lock for dev_shutdown like in
> the patch below? After some basic testing, it seems safe but I haven't
> looked too deep into all possibilities.
> 
> Cosmin.
> 
> From e8b613dfd2b241a6c19bb89a829d598d6640b6f9 Mon Sep 17 00:00:00 2001
> From: Cosmin Ratiu <cratiu@nvidia.com>
> Date: Mon, 5 May 2025 15:34:53 +0300
> Subject: [PATCH 01/20] net/sched: Lock netdevices during dev_shutdown
> 
> Various qdiscs can end up calling into .ndo_setup_tc() and as such,
> might need the netdev instance lock.
> 
> Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
> ---
>  net/core/dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d1a8cad0c99c..134ceddf7fa5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -12020,9 +12020,9 @@ void unregister_netdevice_many_notify(struct
> list_head *head,
>                 struct sk_buff *skb = NULL;
>  
>                 /* Shutdown queueing discipline. */
> +               netdev_lock_ops(dev);
>                 dev_shutdown(dev);
>                 dev_tcx_uninstall(dev);

There is a synchronize_net hidden inside of dev_tcx_uninstall, so
let's ops-lock only dev_shutdown here? Other than that, don't see
anything wrong. Can you send this separately and target net tree?