On 7/22/26 10:59 PM, Ilya Maximets wrote:
> On 7/22/26 6:40 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. 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: [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 v2:
>> - Sort local declarations modified by this patch in reverse
>> Christmas-tree order.
>> - v1 Link:
>> https://lore.kernel.org/all/aa8a1d8dcbac8a13dbdf077a642a66f4c5d81e4b.1784355642.git.xuyuqi...@gmail.com/
>>
>> 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.
>
>> + 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.
>
>> } 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.
>
>> 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.
>
>> 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?
Under rcu lock, of course.
>
>> + 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;
>> };
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
