Re: [Devel] [PATCH VZ10 v2 1/4] connector: annotate ve->cn with __rcu

[Konstantin Khorenko <khorenko@virtuozzo.com>]

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@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;
>