[Cake] Re: [PATCH net] net/sched: Mark qdisc for deletion if graft cannot delete

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Sat, Mar 14, 2026 at 11:00 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 Mar 2026 15:36:28 -0400 Jamal Hadi Salim wrote:
> > In this specific example, the issue is that the classifier code path
> > can't release the rtnl_lock while the qdisc's refcnt is bigger than 1.
> >
> > Does this make more sense?
> > The reason we went with the "mark for delete" approach is at time x+3
> > the "qdisc add" wont be able to find this qdisc. This is the common
> > observed pattern - for example described in the commit message where
> > we get have a slightly different flow with "qdisc del" before "filter
> > add".
>
> Maybe a (completely untested) diff will help illustrate my thinking
> better than words:
>

Dont have much time today - but will try in the upcoming days.

cheers,
jamal

> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 99ac747b7906..c13c9e8619e4 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -27,6 +27,9 @@ void unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
>  #define NET_CLS_ALIAS_PREFIX "net-cls-"
>  #define MODULE_ALIAS_NET_CLS(kind)     MODULE_ALIAS(NET_CLS_ALIAS_PREFIX kind)
>
> +extern char *rtnl_load_mod;
> +void rtnl_load_mod_check(void);
> +
>  struct tcf_block_ext_info {
>         enum flow_block_binder_type binder_type;
>         tcf_chain_head_change_t *chain_head_change;
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 332fd9695e54..c21dd2e36592 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1368,11 +1368,15 @@ struct tc_action_ops *tc_action_load_ops(struct nlattr *nla, u32 flags,
>  #ifdef CONFIG_MODULES
>                 bool rtnl_held = !(flags & TCA_ACT_FLAGS_NO_RTNL);
>
> -               if (rtnl_held)
> -                       rtnl_unlock();
> +               if (rtnl_held) {
> +                       if (WARN_ON_ONCE(rtnl_load_mod))
> +                               return ERR_PTR(-EINVAL);
> +                       rtnl_load_mod = kasprintf(GFP_KERNEL,
> +                                                 NET_ACT_ALIAS_PREFIX "%s",
> +                                                 act_name);
> +                       return ERR_PTR(-EAGAIN);
> +               }
>                 request_module(NET_ACT_ALIAS_PREFIX "%s", act_name);
> -               if (rtnl_held)
> -                       rtnl_lock();
>
>                 a_o = tc_lookup_action_n(act_name);
>
> @@ -2107,6 +2111,9 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>                                       &attr_size, flags, 0, extack);
>                 if (ret != -EAGAIN)
>                         break;
> +               rtnl_unlock();
> +               rtnl_load_mod_check();
> +               rtnl_lock();
>         }
>
>         if (ret < 0)
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 4829c27446e3..1b0f762d6e4b 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -46,6 +46,19 @@
>  /* The list of all installed classifier types */
>  static LIST_HEAD(tcf_proto_base);
>
> +char *rtnl_load_mod;
> +
> +void rtnl_load_mod_check(void)
> +{
> +       char *mod = rtnl_load_mod;
> +
> +       if (mod) {
> +               rtnl_load_mod = NULL;
> +               request_module("%s", mod);
> +               kfree(mod);
> +       }
> +}
> +
>  /* Protects list of registered TC modules. It is pure SMP lock. */
>  static DEFINE_RWLOCK(cls_mod_lock);
>
> @@ -255,17 +268,15 @@ tcf_proto_lookup_ops(const char *kind, bool rtnl_held,
>         if (ops)
>                 return ops;
>  #ifdef CONFIG_MODULES
> -       if (rtnl_held)
> -               rtnl_unlock();
> +       if (rtnl_held) {
> +               if (WARN_ON_ONCE(rtnl_load_mod))
> +                       return ERR_PTR(-EINVAL);
> +               rtnl_load_mod = kasprintf(GFP_KERNEL,
> +                                         NET_CLS_ALIAS_PREFIX "%s", kind);
> +               return ERR_PTR(-EAGAIN);
> +       }
>         request_module(NET_CLS_ALIAS_PREFIX "%s", kind);
> -       if (rtnl_held)
> -               rtnl_lock();
>         ops = __tcf_proto_lookup_ops(kind);
> -       /* We dropped the RTNL semaphore in order to perform
> -        * the module load. So, even if we succeeded in loading
> -        * the module we have to replay the request. We indicate
> -        * this using -EAGAIN.
> -        */
>         if (ops) {
>                 module_put(ops->owner);
>                 return ERR_PTR(-EAGAIN);
> @@ -2459,6 +2470,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>                  * of target chain.
>                  */
>                 rtnl_held = true;
> +               rtnl_load_mod_check();
>                 /* Replay the request. */
>                 goto replay;
>         }
> @@ -3230,9 +3242,13 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
>         tcf_chain_put(chain);
>  errout_block:
>         tcf_block_release(q, block, true);
> -       if (err == -EAGAIN)
> +       if (err == -EAGAIN) {
> +               rtnl_unlock();
> +               rtnl_load_mod_check();
> +               rtnl_lock();
>                 /* Replay the request. */
>                 goto replay;
> +       }
>         return err;
>
>  errout_block_locked: