Re: [PATCH net v6 1/1] openvswitch: Fix CT limit teardown use-after-free

[Ilya Maximets <i.maximets@ovn.org>]

On 8/12/26 2:15 PM, Ilya Maximets wrote:
> On 8/12/26 11:59 AM, Yuqi Xu wrote:
>> Packet processing uses CT limit state under RCU, while netns teardown
>> frees that state under ovs_mutex. The CT limit pointer was neither removed
>> from readers nor protected by a grace period, allowing packet processing to
>> dereference the freed state.
>>
>> An unprivileged user can trigger this bug from a user and network
>> namespace, causing a slab-use-after-free in ovs_ct_execute() when the
>> netns is torn down.
>>
>> Publish the CT limit pointer through RCU, remove it before teardown, and
>> wait for readers before freeing its contents. Keep ovs_mutex around
>> individual CT limit updates, and use the RCU read-side lock while GET
>> traverses the RCU-protected limit lists.
>>
>> The netlink command handlers do not need NULL checks because the userspace
>> netlink socket holds an active reference to its network namespace while a
>> request is processed. The per-netns exit path therefore cannot run
>> concurrently with SET, DEL, or GET for that socket's namespace.
>>
>> The teardown path currently waits for the RCU grace period while holding
>> ovs_mutex. Moving synchronize_rcu() outside the mutex would require
>> restructuring the teardown path and is left for a separate change.
>>
>> Fixes: 11efd5cb04a1 ("openvswitch: Support conntrack zone limit")
>> Cc: stable@vger.kernel.org
>> Reported-by: Vega <vega@nebusec.ai>
>> Link: https://lore.kernel.org/all/cover.1784711445.git.xuyuqiabc@gmail.com
>> Assisted-by: Codex:GPT-5.4
>> Co-developed-by: Nan Li <tonanli66@gmail.com>
>> Signed-off-by: Nan Li <tonanli66@gmail.com>
>> Signed-off-by: Yuqi Xu <xuyuqiabc@gmail.com>
>> Reviewed-by: Ren Wei <enjou1224z@gmail.com>
>> ---
> Reviewed-by: Ilya Maximets <i.maximets@ovn.org>

Hmm.  LLMs keep complaining about the RCU synchronization under the mutex,
which is a valid concern at the end of a day.  So, maybe we should fold
something like this in instead of fixing it later:

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index cc6ea4014c16..e39724390654 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1620,13 +1620,20 @@ static int ovs_ct_limit_init(struct net *net, struct ovs_net *ovs_net)
 	return 0;
 }
 
-static void ovs_ct_limit_exit(struct net *net, struct ovs_net *ovs_net)
+static void *ovs_ct_limit_exit_start(struct ovs_net *ovs_net)
 {
-	const struct ovs_ct_limit_info *info;
+	return rcu_replace_pointer(ovs_net->ct_limit_info, NULL,
+				   lockdep_ovsl_is_held());
+}
+
+static void ovs_ct_limit_exit_finish(struct net *net, void *data)
+{
+	const struct ovs_ct_limit_info *info = data;
 	int i;
 
-	info = rcu_replace_pointer(ovs_net->ct_limit_info, NULL,
-				   lockdep_ovsl_is_held());
+	if (!info)
+		return;
+
 	/* Wait for RCU readers to stop using the CT limits. */
 	synchronize_rcu();
 
@@ -2029,12 +2036,27 @@ int ovs_ct_init(struct net *net)
 #endif
 }
 
-void ovs_ct_exit(struct net *net)
+/* Must be called with ovs_mutex held.  Detaches RCU-protected ct_limit_info
+ * and returns an opaque handle for ovs_ct_exit_finish() to complete teardown
+ * after the mutex is released.
+ */
+void *ovs_ct_exit_start(struct net *net __maybe_unused)
+{
+#if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
+	return ovs_ct_limit_exit_start(net_generic(net, ovs_net_id));
+#endif
+	return NULL;
+}
+
+/* Must be called without ovs_mutex held.  @data must be the opaque pointer
+ * returned by ovs_ct_exit_start().
+ */
+void ovs_ct_exit_finish(struct net *net, void *data __maybe_unused)
 {
 	struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
 
 #if	IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
-	ovs_ct_limit_exit(net, ovs_net);
+	ovs_ct_limit_exit_finish(net, data);
 #endif
 
 	if (ovs_net->xt_label)
diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
index 317e525c8a11..8c4aa7b8a563 100644
--- a/net/openvswitch/conntrack.h
+++ b/net/openvswitch/conntrack.h
@@ -14,7 +14,8 @@ enum ovs_key_attr;
 
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
 int ovs_ct_init(struct net *);
-void ovs_ct_exit(struct net *);
+void *ovs_ct_exit_start(struct net *);
+void ovs_ct_exit_finish(struct net *, void *data);
 bool ovs_ct_verify(struct net *, enum ovs_key_attr attr);
 int ovs_ct_copy_action(struct net *, const struct nlattr *,
 		       const struct sw_flow_key *, struct sw_flow_actions **,
@@ -40,7 +41,8 @@ void ovs_ct_free_action(const struct nlattr *a);
 
 static inline int ovs_ct_init(struct net *net) { return 0; }
 
-static inline void ovs_ct_exit(struct net *net) { }
+static inline void *ovs_ct_exit_start(struct net *net) { return NULL; }
+static inline void ovs_ct_exit_finish(struct net *net, void *data) { }
 
 static inline bool ovs_ct_verify(struct net *net, int attr)
 {
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ae69b2cabab9..c18dafa920b7 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -2758,15 +2758,16 @@ static void __net_exit list_vports_from_net(struct net *net, struct net *dnet,
 
 static void __net_exit ovs_exit_net(struct net *dnet)
 {
-	struct datapath *dp, *dp_next;
 	struct ovs_net *ovs_net = net_generic(dnet, ovs_net_id);
 	struct vport *vport, *vport_next;
+	struct datapath *dp, *dp_next;
+	void *ct_exit_data;
 	struct net *net;
 	LIST_HEAD(head);
 
 	ovs_lock();
 
-	ovs_ct_exit(dnet);
+	ct_exit_data = ovs_ct_exit_start(dnet);
 
 	list_for_each_entry_safe(dp, dp_next, &ovs_net->dps, list_node)
 		__dp_destroy(dp);
@@ -2784,6 +2785,8 @@ static void __net_exit ovs_exit_net(struct net *dnet)
 
 	ovs_unlock();
 
+	ovs_ct_exit_finish(dnet, ct_exit_data);
+
 	cancel_delayed_work_sync(&ovs_net->masks_rebalance);
 	cancel_work_sync(&ovs_net->dp_notify_work);
 }
-- 

WDYT?

Best regards, Ilya Maximets.