Re: [PATCH net v2 1/1] openvswitch: Fix CT limit teardown use-after-free
From: Ilya Maximets <i.maximets@ovn.org>
Date: 2026-07-22 20:59:15
On 7/22/26 6:40 PM, Ren Wei wrote:
quoted hunk ↗ jump to hunk
From: Yuqi Xu <redacted> 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. Replace the pointer before freeing the CT limit state. Wait for in-flight RCU readers before freeing its contents. Serialize CT limit netlink operations with teardown for the full lifetime of their state accesses. Fixes: 11efd5cb04a1 ("openvswitch: Support conntrack zone limit") Cc: stable@vger.kernel.org Reported-by: Vega <redacted> Assisted-by: Codex:GPT-5.4 Co-developed-by: Nan Li <redacted> Signed-off-by: Nan Li <redacted> Signed-off-by: Yuqi Xu <redacted> Signed-off-by: Ren Wei <redacted> --- changes in v2: - Sort local declarations modified by this patch in reverse Christmas-tree order. - v1 Link: https://lore.kernel.org/all/aa8a1d8dcbac8a13dbdf077a642a66f4c5d81e4b.1784355642.git.xuyuqiabc@gmail.com/ (local) net/openvswitch/conntrack.c | 79 +++++++++++++++++++++++-------------- net/openvswitch/datapath.h | 2 +- 2 files changed, 51 insertions(+), 30 deletions(-)diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index 95697d4e16e6..5c9a607d27a7 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c@@ -933,10 +933,14 @@ static int ovs_ct_check_limit(struct net *net, const struct ovs_conntrack_info *info) { struct ovs_net *ovs_net = net_generic(net, ovs_net_id); - const struct ovs_ct_limit_info *ct_limit_info = ovs_net->ct_limit_info; + const struct ovs_ct_limit_info *ct_limit_info; u32 per_zone_limit, connections; u32 conncount_key; + ct_limit_info = rcu_dereference(ovs_net->ct_limit_info); + if (!ct_limit_info) + return 0; + conncount_key = info->zone.id; per_zone_limit = ct_limit_get(ct_limit_info, info->zone.id);@@ -1585,40 +1589,47 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) static int ovs_ct_limit_init(struct net *net, struct ovs_net *ovs_net) { + struct ovs_ct_limit_info *info; int i, err; - ovs_net->ct_limit_info = kmalloc_obj(*ovs_net->ct_limit_info); - if (!ovs_net->ct_limit_info) + info = kmalloc_obj(*info); + if (!info) return -ENOMEM; - ovs_net->ct_limit_info->default_limit = OVS_CT_LIMIT_DEFAULT; - ovs_net->ct_limit_info->limits = + info->default_limit = OVS_CT_LIMIT_DEFAULT; + info->limits = kmalloc_objs(struct hlist_head, CT_LIMIT_HASH_BUCKETS); - if (!ovs_net->ct_limit_info->limits) { - kfree(ovs_net->ct_limit_info); + if (!info->limits) { + kfree(info); return -ENOMEM; } for (i = 0; i < CT_LIMIT_HASH_BUCKETS; i++) - INIT_HLIST_HEAD(&ovs_net->ct_limit_info->limits[i]); + INIT_HLIST_HEAD(&info->limits[i]); - ovs_net->ct_limit_info->data = nf_conncount_init(net, sizeof(u32)); + info->data = nf_conncount_init(net, sizeof(u32)); - if (IS_ERR(ovs_net->ct_limit_info->data)) { - err = PTR_ERR(ovs_net->ct_limit_info->data); - kfree(ovs_net->ct_limit_info->limits); - kfree(ovs_net->ct_limit_info); + if (IS_ERR(info->data)) { + err = PTR_ERR(info->data); + kfree(info->limits); + kfree(info); pr_err("openvswitch: failed to init nf_conncount %d\n", err); return err; } + rcu_assign_pointer(ovs_net->ct_limit_info, info); return 0; } static void ovs_ct_limit_exit(struct net *net, struct ovs_net *ovs_net) { - const struct ovs_ct_limit_info *info = ovs_net->ct_limit_info; + const struct ovs_ct_limit_info *info; int i; + info = rcu_replace_pointer(ovs_net->ct_limit_info, NULL, + lockdep_ovsl_is_held()); + /* ovs_ct_check_limit() accesses the info under the datapath RCU lock. */
'datapath RCU lock' is a strange wording. 'RCU read lock' may be better. But also, the comment is not particularly useful. It could say 'wait for other CPUs to stop using ct limits', or something like that instead.
quoted hunk ↗ jump to hunk
+ synchronize_rcu(); + nf_conncount_destroy(net, info->data); for (i = 0; i < CT_LIMIT_HASH_BUCKETS; ++i) { struct hlist_head *head = &info->limits[i];@@ -1678,9 +1689,7 @@ static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, while (rem >= sizeof(*zone_limit)) { if (unlikely(zone_limit->zone_id == OVS_ZONE_LIMIT_DEFAULT_ZONE)) { - ovs_lock(); info->default_limit = zone_limit->limit; - ovs_unlock();
Enlarging the critical section for the ovs_mutex() doesn't sound nice.
There are multiple memory allocations and other things in the path
that could take a decent amount of time holding the lock. Might be
better to pass ovs_net into set and del funtions instead and do the
rcu dereferencing right here after taking the lock. It will be done
in 4 places in the code, but you're doing it in 3 places right now,
so code-wise it's not that different. E.g.:
---
ovs_lock();
+ info = ovsl_dereference(ovs_net->ct_limit_info);
+ if (!info) {
+ ovs_unlock();
+ return -ENOENT;
+ }
info->default_limit = zone_limit->limit;
ovs_unlock();
---
If we do so, then ovs_net should likely be the first argument of the
function.
quoted hunk ↗ jump to hunk
} else if (unlikely(!check_zone_id( zone_limit->zone_id, &zone))) { OVS_NLERR(true, "zone id is out of range");@@ -1694,9 +1703,7 @@ static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, ct_limit->zone = zone; ct_limit->limit = zone_limit->limit; - ovs_lock(); ct_limit_set(info, ct_limit); - ovs_unlock(); } rem -= NLA_ALIGN(sizeof(*zone_limit)); zone_limit = (struct ovs_zone_limit *)((u8 *)zone_limit +@@ -1722,16 +1729,12 @@ static int ovs_ct_limit_del_zone_limit(struct nlattr *nla_zone_limit, while (rem >= sizeof(*zone_limit)) { if (unlikely(zone_limit->zone_id == OVS_ZONE_LIMIT_DEFAULT_ZONE)) { - ovs_lock(); info->default_limit = OVS_CT_LIMIT_DEFAULT; - ovs_unlock(); } else if (unlikely(!check_zone_id( zone_limit->zone_id, &zone))) { OVS_NLERR(true, "zone id is out of range"); } else { - ovs_lock(); ct_limit_del(info, zone); - ovs_unlock(); } rem -= NLA_ALIGN(sizeof(*zone_limit)); zone_limit = (struct ovs_zone_limit *)((u8 *)zone_limit +@@ -1850,7 +1853,7 @@ static int ovs_ct_limit_cmd_set(struct sk_buff *skb, struct genl_info *info) struct sk_buff *reply; struct ovs_header *ovs_reply_header; struct ovs_net *ovs_net = net_generic(sock_net(skb->sk), ovs_net_id); - struct ovs_ct_limit_info *ct_limit_info = ovs_net->ct_limit_info; + struct ovs_ct_limit_info *ct_limit_info; int err; reply = ovs_ct_limit_cmd_reply_start(info, OVS_CT_LIMIT_CMD_SET,@@ -1863,16 +1866,22 @@ static int ovs_ct_limit_cmd_set(struct sk_buff *skb, struct genl_info *info) goto exit_err; } + ovs_lock(); + ct_limit_info = rcu_dereference_protected(ovs_net->ct_limit_info, + lockdep_ovsl_is_held());
Should use ovsl_dereference() instead of open-coding it. And also, we should probably check that the result is not NULL, for consistency.
err = ovs_ct_limit_set_zone_limit(a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT], ct_limit_info); if (err) - goto exit_err; + goto exit_unlock; static_branch_enable(&ovs_ct_limit_enabled); genlmsg_end(reply, ovs_reply_header); + ovs_unlock();
There is no point holding ovs_mutex for the entire reply message preparation and also the static branch setting. See above.
quoted hunk ↗ jump to hunk
return genlmsg_reply(reply, info); +exit_unlock: + ovs_unlock(); exit_err: nlmsg_free(reply); return err;@@ -1884,7 +1893,7 @@ static int ovs_ct_limit_cmd_del(struct sk_buff *skb, struct genl_info *info) struct sk_buff *reply; struct ovs_header *ovs_reply_header; struct ovs_net *ovs_net = net_generic(sock_net(skb->sk), ovs_net_id); - struct ovs_ct_limit_info *ct_limit_info = ovs_net->ct_limit_info; + struct ovs_ct_limit_info *ct_limit_info; int err; reply = ovs_ct_limit_cmd_reply_start(info, OVS_CT_LIMIT_CMD_DEL,@@ -1897,14 +1906,20 @@ static int ovs_ct_limit_cmd_del(struct sk_buff *skb, struct genl_info *info) goto exit_err; } + ovs_lock(); + ct_limit_info = rcu_dereference_protected(ovs_net->ct_limit_info, + lockdep_ovsl_is_held()); err = ovs_ct_limit_del_zone_limit(a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT], ct_limit_info); if (err) - goto exit_err; + goto exit_unlock; genlmsg_end(reply, ovs_reply_header); + ovs_unlock();
Same here.
quoted hunk ↗ jump to hunk
return genlmsg_reply(reply, info); +exit_unlock: + ovs_unlock(); exit_err: nlmsg_free(reply); return err;@@ -1918,7 +1933,7 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, struct genl_info *info) struct ovs_header *ovs_reply_header; struct net *net = sock_net(skb->sk); struct ovs_net *ovs_net = net_generic(net, ovs_net_id); - struct ovs_ct_limit_info *ct_limit_info = ovs_net->ct_limit_info; + struct ovs_ct_limit_info *ct_limit_info; int err; reply = ovs_ct_limit_cmd_reply_start(info, OVS_CT_LIMIT_CMD_GET,@@ -1932,23 +1947,29 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, struct genl_info *info) goto exit_err; } + ovs_lock();
Why are we locking the get() path? Is simple rcu_dereference not enough?
quoted hunk ↗ jump to hunk
+ ct_limit_info = rcu_dereference_protected(ovs_net->ct_limit_info, + lockdep_ovsl_is_held()); if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) { err = ovs_ct_limit_get_zone_limit( net, a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT], ct_limit_info, reply); if (err) - goto exit_err; + goto exit_unlock; } else { err = ovs_ct_limit_get_all_zone_limit(net, ct_limit_info, reply); if (err) - goto exit_err; + goto exit_unlock; } nla_nest_end(reply, nla_reply); genlmsg_end(reply, ovs_reply_header); + ovs_unlock(); return genlmsg_reply(reply, info); +exit_unlock: + ovs_unlock(); exit_err: nlmsg_free(reply); return err;diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h index 696640e88fa7..93e11e468d17 100644 --- a/net/openvswitch/datapath.h +++ b/net/openvswitch/datapath.h@@ -172,7 +172,7 @@ struct ovs_net { struct work_struct dp_notify_work; struct delayed_work masks_rebalance; #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT) - struct ovs_ct_limit_info *ct_limit_info; + struct ovs_ct_limit_info __rcu *ct_limit_info; #endif bool xt_label; };