Re: [PATCH net v3 1/1] openvswitch: Fix CT limit teardown use-after-free
Ilya Maximets <[email protected]> Mon, 3 Aug 2026 22:04:58 +0200
| Newsgroups | org.kernel.vger.netdev |
|---|---|
| Message-ID | <[email protected]> |
On 8/3/26 7:30 PM, Ren Wei wrote: > From: Yuqi Xu <[email protected]> > > 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, and wait for RCU > readers before freeing its contents. Keep ovs_mutex only around CT limit > updates, while GET uses the RCU read-side lock for the lifetime of the > lookup. > > Fixes: 11efd5cb04a1 ("openvswitch: Support conntrack zone limit") > Cc: [email protected] > Reported-by: Vega <[email protected]> > Assisted-by: Codex:GPT-5.4 > Co-developed-by: Nan Li <[email protected]> > Signed-off-by: Nan Li <[email protected]> > Signed-off-by: Yuqi Xu <[email protected]> > Signed-off-by: Ren Wei <[email protected]> > --- > Changes in v3: > > - Use RCU dereference and a NULL check for CT limit GET requests. > - Limit ovs_mutex to CT limit updates; do not hold it while preparing replies. > - Use ovsl_dereference() for update paths and clarify the RCU grace-period comment. > - v2 Link: https://lore.kernel.org/all/[email protected] > > 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/ > > net/openvswitch/conntrack.c | 109 +++++++++++++++++++++--------------- > net/openvswitch/datapath.h | 2 +- > 2 files changed, 65 insertions(+), 46 deletions(-) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 95697d4e16e6..aa71a254fe24 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()); > + /* Wait for RCU readers to stop using the CT limits. */ > + synchronize_rcu(); > + > nf_conncount_destroy(net, info->data); > for (i = 0; i < CT_LIMIT_HASH_BUCKETS; ++i) { > struct hlist_head *head = &info->limits[i]; > @@ -1665,22 +1676,28 @@ static bool check_zone_id(int zone_id, u16 *pzone) > return false; > } > > -static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, > - struct ovs_ct_limit_info *info) > +static int ovs_ct_limit_set_zone_limit(struct ovs_net *ovs_net, > + struct nlattr *nla_zone_limit) > { > + struct ovs_ct_limit_info *info; Reverse xmass tree, please. > struct ovs_zone_limit *zone_limit; > int rem; > u16 zone; > > + ovs_lock(); > + info = ovsl_dereference(ovs_net->ct_limit_info); > + if (!info) { > + ovs_unlock(); > + return -ENOENT; > + } > + This is not really what I asked for. I asked to keep the lock/unlock per entry as they are today and just add dereference + check under that lock. Something like this: @@ -1679,7 +1691,9 @@ static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, if (unlikely(zone_limit->zone_id == OVS_ZONE_LIMIT_DEFAULT_ZONE)) { ovs_lock(); - info->default_limit = zone_limit->limit; + info = ovsl_dereference(ovs_net->ct_limit_info); + if (likely(info)) + info->default_limit = zone_limit->limit; ovs_unlock(); } else if (unlikely(!check_zone_id( zone_limit->zone_id, &zone))) { @@ -1695,8 +1709,13 @@ static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, ct_limit->limit = zone_limit->limit; ovs_lock(); - ct_limit_set(info, ct_limit); + info = ovsl_dereference(ovs_net->ct_limit_info); + if (likley(info)) + ct_limit_set(info, ct_limit); ovs_unlock(); + + if (unlikley(!info)) + kfree(ct_limit); } rem -= NLA_ALIGN(sizeof(*zone_limit)); zone_limit = (struct ovs_zone_limit *)((u8 *)zone_limit + --- This may memory allocations are kept outside of the lock and we're also not holding the lock for a long time in case of a long list of entries. Note that this version of my suggestion doesn't return on error. This should be fine, since the only way info is NULL is if the namespace is being destroyed anyway. And we're not returning errors from this code when the id is out of range, so not returning an error here is not that different. But, please, note that in the commit message. Also, returning ENOENT for the SET command is kind of weird, as it is supposed to create the entries when they do not exist. > rem = NLA_ALIGN(nla_len(nla_zone_limit)); > zone_limit = (struct ovs_zone_limit *)nla_data(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(); > } else if (unlikely(!check_zone_id( > zone_limit->zone_id, &zone))) { > OVS_NLERR(true, "zone id is out of range"); > @@ -1688,15 +1705,15 @@ static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, > struct ovs_ct_limit *ct_limit; > > ct_limit = kmalloc_obj(*ct_limit, GFP_KERNEL_ACCOUNT); > - if (!ct_limit) > + if (!ct_limit) { > + ovs_unlock(); > return -ENOMEM; > + } > > 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 + > @@ -1706,32 +1723,37 @@ static int ovs_ct_limit_set_zone_limit(struct nlattr *nla_zone_limit, > if (rem) > OVS_NLERR(true, "set zone limit has %d unknown bytes", rem); > > + ovs_unlock(); > return 0; > } > > -static int ovs_ct_limit_del_zone_limit(struct nlattr *nla_zone_limit, > - struct ovs_ct_limit_info *info) > +static int ovs_ct_limit_del_zone_limit(struct ovs_net *ovs_net, > + struct nlattr *nla_zone_limit) > { > + struct ovs_ct_limit_info *info; Reverse xmass tree. > struct ovs_zone_limit *zone_limit; > int rem; > u16 zone; > > + ovs_lock(); > + info = ovsl_dereference(ovs_net->ct_limit_info); > + if (!info) { > + ovs_unlock(); > + return -ENOENT; > + } > + Same thing here. We should just deref and check under the existing locks inside the while loop: @@ -1723,14 +1742,18 @@ static int ovs_ct_limit_del_zone_limit(struct nlattr *nla_zone_limit, if (unlikely(zone_limit->zone_id == OVS_ZONE_LIMIT_DEFAULT_ZONE)) { ovs_lock(); - info->default_limit = OVS_CT_LIMIT_DEFAULT; + info = ovsl_dereference(ovs_net->ct_limit_info); + if (likely(info)) + 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); + info = ovsl_dereference(ovs_net->ct_limit_info); + if (likely(info)) + ct_limit_del(info, zone); ovs_unlock(); } rem -= NLA_ALIGN(sizeof(*zone_limit)); --- The code doesn't return any errors when the zone was not found, so we shouldn't return ENOENT when the info is not found either. > rem = NLA_ALIGN(nla_len(nla_zone_limit)); > zone_limit = (struct ovs_zone_limit *)nla_data(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 + > @@ -1741,6 +1763,7 @@ static int ovs_ct_limit_del_zone_limit(struct nlattr *nla_zone_limit, > if (rem) > OVS_NLERR(true, "del zone limit has %d unknown bytes", rem); > > + ovs_unlock(); > return 0; > } > > @@ -1796,12 +1819,10 @@ static int ovs_ct_limit_get_zone_limit(struct net *net, Should add a comment above the function that it is called under RCU read lock. > &zone))) { > OVS_NLERR(true, "zone id is out of range"); > } else { > - rcu_read_lock(); > limit = ct_limit_get(info, zone); > > err = __ovs_ct_limit_get_zone_limit( > net, info->data, zone, limit, reply); > - rcu_read_unlock(); > if (err) > return err; > } > @@ -1828,19 +1849,16 @@ static int ovs_ct_limit_get_all_zone_limit(struct net *net, Same here. > if (err) > return err; > > - rcu_read_lock(); > for (i = 0; i < CT_LIMIT_HASH_BUCKETS; ++i) { > head = &info->limits[i]; > hlist_for_each_entry_rcu(ct_limit, head, hlist_node) { > err = __ovs_ct_limit_get_zone_limit(net, info->data, > ct_limit->zone, ct_limit->limit, reply); > if (err) > - goto exit_err; > + return err; > } > } > > -exit_err: > - rcu_read_unlock(); > return err; > } > > @@ -1850,7 +1868,6 @@ 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; > int err; > > reply = ovs_ct_limit_cmd_reply_start(info, OVS_CT_LIMIT_CMD_SET, > @@ -1863,8 +1880,8 @@ static int ovs_ct_limit_cmd_set(struct sk_buff *skb, struct genl_info *info) > goto exit_err; > } > > - err = ovs_ct_limit_set_zone_limit(a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT], > - ct_limit_info); > + err = ovs_ct_limit_set_zone_limit(ovs_net, > + a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]); > if (err) > goto exit_err; > > @@ -1884,7 +1901,6 @@ 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; > int err; > > reply = ovs_ct_limit_cmd_reply_start(info, OVS_CT_LIMIT_CMD_DEL, > @@ -1897,8 +1913,8 @@ static int ovs_ct_limit_cmd_del(struct sk_buff *skb, struct genl_info *info) > goto exit_err; > } > > - err = ovs_ct_limit_del_zone_limit(a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT], > - ct_limit_info); > + err = ovs_ct_limit_del_zone_limit(ovs_net, > + a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]); > if (err) > goto exit_err; > > @@ -1918,7 +1934,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,18 +1948,21 @@ static int ovs_ct_limit_cmd_get(struct sk_buff *skb, struct genl_info *info) > goto exit_err; > } > > - if (a[OVS_CT_LIMIT_ATTR_ZONE_LIMIT]) { > + rcu_read_lock(); > + ct_limit_info = rcu_dereference(ovs_net->ct_limit_info); > + if (!ct_limit_info) { > + err = -ENOENT; > + } else 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; > } else { > err = ovs_ct_limit_get_all_zone_limit(net, ct_limit_info, > reply); > - if (err) > - goto exit_err; > } > + rcu_read_unlock(); > + if (err) > + goto exit_err; > > nla_nest_end(reply, nla_reply); > genlmsg_end(reply, ovs_reply_header); > 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; > };