[Devel] [PATCH VZ10 v2 1/4] connector: annotate ve->cn with __rcu
Konstantin Khorenko
khorenko at virtuozzo.com
Tue Aug 25 14:58:25 MSK 2026
On 8/18/26 17:10, Vasileios Almpanis wrote:
> The following patches will clear ve->cn on container stop and free the
> per-VE connector state only after an RCU grace period, so that the
> proc event delivery path can use it under rcu_read_lock() even when
> the reported task no longer pins the VE.
>
> Prepare for that: annotate ve->cn with __rcu and switch all accesses
> to the RCU accessors so sparse can verify the protocol. All current
> users run either in the context of a task alive in the VE or from
> the VE start/stop hooks under ve->op_sem (or on ve0), so plain readers
> use rcu_dereference_check(ve->cn, 1) with a comment and the start/stop
> hooks use rcu_dereference_protected() with the proper lockdep
> condition.
>
> No functional change intended
>
> https://virtuozzo.atlassian.net/browse/VSTOR-140421
> Signed-off-by: Vasileios Almpanis <vasileios.almpanis at virtuozzo.com>
>
> Feature: ve: ve generic structures
> ---
> drivers/connector/cn_proc.c | 48 +++++++++++++++++++++++++++++++------------
> drivers/connector/connector.c | 43 +++++++++++++++++++++++++-------------
> include/linux/ve.h | 2 +-
> 3 files changed, 65 insertions(+), 28 deletions(-)
>
...
> @@ -297,6 +304,7 @@ static int __maybe_unused cn_proc_show(struct seq_file *m, void *v)
> static int cn_init_ve(void *data)
> {
> struct ve_struct *ve = data;
> + struct cn_private *cn;
> struct cn_dev *dev;
> struct netlink_kernel_cfg cfg = {
> .groups = CN_NETLINK_USERS + 0xf,
> @@ -308,11 +316,12 @@ static int cn_init_ve(void *data)
> struct net *net;
> int err;
>
> - ve->cn = kzalloc(sizeof(*ve->cn), GFP_KERNEL);
> - if (!ve->cn)
> + cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> + if (!cn)
> return -ENOMEM;
> + rcu_assign_pointer(ve->cn, cn);
>
> - dev = &ve->cn->cdev;
> + dev = &cn->cdev;
>
> /*
> * This is a hook, hooks are called under a single lock, so ve_nsproxy will
> @@ -331,7 +340,7 @@ static int cn_init_ve(void *data)
> goto netlink_release;
> }
>
> - ve->cn->cn_already_initialized = 1;
> + cn->cn_already_initialized = 1;
>
> if (!proc_ve_create_single("connector", S_IRUGO, net->proc_net,
> cn_proc_show)) {
> @@ -353,18 +362,24 @@ static int cn_init_ve(void *data)
> netlink_release:
> netlink_kernel_release(dev->nls);
> free_cn:
> - kfree(ve->cn);
> - ve->cn = NULL;
> + RCU_INIT_POINTER(ve->cn, NULL);
> + kfree(cn);
The attribution logic:
- The error path itself (RCU_INIT_POINTER(ve->cn, NULL); kfree(cn) in cn_init_ve()) appeared in patch 1, where it
faithfully preserves the base behavior (ve->cn = NULL; kfree) - the "no functional change" claim holds.
- Before patch 4, the only possible reader of a container VE's ve->cn is a live task of that VE. While cn_init_ve() runs
(VE start, under op_sem) there are no tasks in the VE yet - no reader can exist, so the immediate kfree is safe.
- Patch 4 creates a new kind of reader: the exit event delivery with a pinned ve_struct, which can run after the VE has
fully stopped - and therefore also concurrently with its restart (it is the very same ve_struct, the cgroup stays
alive). Bisect-wise the crash only becomes reachable starting with patch 4 - hence the question sits in its
review-inline.
The scenario:
CPU0: exiting task (old CT incarnation) CPU1: vzctl start (restart of the same ve)
========================================= ========================================
do_exit()
ve = get_task_ve(tsk) /* pins ve */
exit_notify() /* reaped, pid
freed */
<-- preempted; the CT manages to stop
completely: cn_fini_ve() done,
ve->cn == NULL -->
ve_start_container() /* op_sem */
cn_init_ve()
cn = kzalloc(...)
rcu_assign_pointer(ve->cn, cn) <-- published
proc_exit_connector(tsk, &pids, ve)
proc_event_connector_ve(task, ve, ...)
rcu_read_lock()
proc_event_num_listeners(ve)
cn = rcu_dereference_check(ve->cn, 1)
/* sees the FRESH cn from CPU1 */
netlink_kernel_create() -> fail
free_cn:
RCU_INIT_POINTER(ve->cn, NULL)
kfree(cn) <-- NO grace period
atomic_read(&cn->proc_event_num_listeners)
*** use-after-free ***
The reader dutifully sits under rcu_read_lock(), but unlike cn_proc_fini_ve() the error path does not wait for a grace
period - patch 2's RCU contract ("everything reachable from ve->cn is freed only after a grace period") was not extended
to this freeing site.
That said, the right place to fix it is patch 2 rather than patch 4 - patch 2 owns the contract, and with the fix in place
patch 4 lands on a fully safe base. The minimal variant, symmetric to cn_proc_fini_ve():
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ static int cn_init_ve(void *data)
free_cn:
RCU_INIT_POINTER(ve->cn, NULL);
+ /*
+ * The pointer was published, so a pinned-VE exit event delivery
+ * may already be looking at it under rcu_read_lock(): wait for
+ * the readers before freeing, same as cn_proc_fini_ve() does.
+ */
+ synchronize_rcu();
kfree(cn);
goto net_unlock;
The context is sleepable (GFP_KERNEL right nearby), and sleeping under op_sem is fine - ve_stop_ns()/cn_proc_fini_ve()
already do exactly that.
The architecturally cleaner alternative is to publish ve->cn only after the initialization fully succeeds (then a reader
can never see a half-constructed state and the error path stays a plain kfree). But that is a bigger refactor:
cn_proc_init_ve() and cn_add_callback_ve() dereference ve->cn themselves, so they would need to take cn as an argument.
For this series the one-line synchronize_rcu() in the error path looks more appropriate.
> goto net_unlock;
> }
>
> static void cn_fini_ve(void *data)
> {
> struct ve_struct *ve = data;
> - struct cn_dev *dev = get_cdev(ve);
> + struct cn_private *cn;
> + struct cn_dev *dev;
> struct net *net;
>
> - ve->cn->cn_already_initialized = 0;
> + cn = rcu_dereference_protected(ve->cn,
> + lockdep_is_held(&ve->op_sem) ||
> + ve_is_super(ve));
> + dev = &cn->cdev;
> +
> + cn->cn_already_initialized = 0;
>
> cn_proc_fini_ve(ve);
>
> @@ -379,8 +394,8 @@ static void cn_fini_ve(void *data)
> cn_queue_free_dev(dev->cbdev);
> netlink_kernel_release(dev->nls);
>
> - kfree(ve->cn);
> - ve->cn = NULL;
> + RCU_INIT_POINTER(ve->cn, NULL);
> + kfree(cn);
> }
>
> #ifdef CONFIG_VE
> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index cba827260d07..3334bd1e9517 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -94,7 +94,7 @@ struct ve_struct {
> char core_pattern[CORENAME_MAX_SIZE];
> #endif
> #ifdef CONFIG_CONNECTOR
> - struct cn_private *cn;
> + struct cn_private __rcu *cn;
> #endif
>
> struct kthread_worker *kthreadd_worker;
>
More information about the Devel
mailing list