From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khorenko Date: Tue, 25 Aug 2026 13:58:25 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 1/4] connector: annotate ve->cn with __rcu In-Reply-To: <20260818-connectors-v2-1-88c5d9049e6f@virtuozzo.com> References: <20260818-connectors-v2-0-88c5d9049e6f@virtuozzo.com> <20260818-connectors-v2-1-88c5d9049e6f@virtuozzo.com> Message-ID: <31c6c4fc-5890-4de1-aa54-8aeeea4518b0@virtuozzo.com> List-Id: 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 > > 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; >