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

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

On Sun, Mar 15, 2026 at 11:56 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> 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.
>

Sorry - was a bit loaded with work. So finally tried this idea - see
attached. Comments:

The bigger challenge is when we encounter a phony action (one that
does not exist). This gets exacerbated when you have a list of actions
and the phony one is in the middle.
 One attempt to resolve this involves keeping the transaction state of
the last failed action loading; see the variable rtnl_failed_act_num
(another global variable).

The global variables leave a stench (and we should not even consider
such an approach), but this seems resolvable by returning back a
"retry context".

tbh, I am not sure about this approach; while we can make it work, we
are adding unnecessary complexity to an already complex piece of code.

cheers,
jamal

> 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: