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

[Jakub Kicinski <kuba@kernel.org>]

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:

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: