Re: [Devel] [PATCH VZ10 v2 2/4] connector: free the per-VE connector state after an RCU grace period

[Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>]

On 8/25/26 1:47 PM, Konstantin Khorenko wrote:
> On 8/18/26 17:10, Vasileios Almpanis wrote:
>> cn_fini_ve() tears down everything the proc event delivery path uses
>> and only clears ve->cn at the very end. A reader that fetched ve->cn
>> right before the teardown dereferences freed memory afterwards:
>> the only guard on the delivery path is the ve->cn check in
>> proc_event_num_listeners() and nothing keeps the state alive once the
>> check has passed.
>>
>> Today the window is not reachable: the per-VE delivery path is only
>> entered for a task alive in this VE, a live task keeps the VE pid
>> namespace busy, so zap_pid_ns_processes() -> ve_exit_ns() ->
>> cn_fini_ve() cannot run in parallel. A subsequent patch will make
>> proc_exit_connector() deliver the exit event with a VE reference
>> pinned before exit_notify(), i.e. possibly after the task was reaped
>> and stopped pinning the pid namespace. This will make the teardown able
>> to run in parallel with the delivery.
>>
>> Clear ve->cn and wait for an RCU grace period before freeing anything
>> reachable from it, so that the delivery path can safely use the state
>> it observed within a single RCU read-side critical section. The clearing
>> is done in cn_proc_fini_ve(): it has to happen before the first thing
>> the delivery path uses (local_event) is freed and everything else is
>> freed later in cn_fini_ve().
>>
>> 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   | 11 +++++++++++
>>   drivers/connector/connector.c |  2 +-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
>> index d4ce1697dd0b..6095c7def7ce 100644
>> --- a/drivers/connector/cn_proc.c
>> +++ b/drivers/connector/cn_proc.c
>> @@ -535,5 +535,16 @@ void cn_proc_fini_ve(struct ve_struct *ve)
>>   				       ve_is_super(ve));
>>   
>>   	cn_del_callback_ve(ve, &cn_proc_event_id);
>> +
>> +	/*
>> +	 * Hide the connector state from the proc event delivery path,
>> +	 * which dereferences ve->cn under rcu_read_lock(), and wait for
>> +	 * the readers to finish before anything reachable from it is
>> +	 * freed: the percpu local_event here, the callback device, the
>> +	 * netlink socket and the state itself in cn_fini_ve().
>> +	 */
>> +	RCU_INIT_POINTER(ve->cn, NULL);
>> +	synchronize_rcu();
>> +
> [Severity: Medium]
> Can this crash the netlink receive path while the VE is stopping?
>
> Clearing ve->cn now happens long before the kernel netlink socket is
> released in cn_fini_ve(): the window covers this synchronize_rcu(),
> free_percpu(), remove_proc_entry() and cn_queue_free_dev(). Within
> that window dev->nls is still alive and accepts messages, and a send
> to it ends up in:
>
> drivers/connector/connector.c:cn_call_callback() {
>          ...
>          struct cn_dev *dev = get_cdev(sock_net(skb->sk)->owner_ve);
>          ...
>          spin_lock_bh(&dev->cbdev->queue_lock);
>          ...
> }
>
> Since the previous patch get_cdev() returns NULL once ve->cn is
> cleared, so this dereferences a NULL dev.
>
> All in-VE tasks are dead at this point, but the VE netns can still be
> entered from the host (nsenter, criu and vzctl network tooling), and a
> NETLINK_CONNECTOR socket created there delivers straight into
> cn_rx_skb() -> cn_call_callback() in the sender's context.
>
> Before this patch the pointer was cleared only after
> netlink_kernel_release(), which quiesces the input path first, so this
> window did not exist. The next patch adds a NULL check to the send
> side (cn_netlink_send_mult_ve()) but not to the receive side.
>
> Would a NULL check in cn_call_callback(), or clearing ve->cn only
> after netlink_kernel_release() in cn_fini_ve() while keeping the
> grace period before the frees, close this?
I think a NULL check on the receive side is the better option. The send
path uses dev->nls under RCU, so the socket must outlive the grace period,
which means the clear must precede the release. I'll add it in v3
>
> =============================================================
> ? Scenario 1 - NULL deref (fetch after the clear)
>
>    Precondition: the container is stopping - all CT tasks are dead, the container init is finishing zap_pid_ns_processes().
>    On the host there is a process (criu / vzctl tooling / nsenter) that has opened a NETLINK_CONNECTOR socket in this CT's
>    netns beforehand (the socket keeps the netns alive).
>
>    CPU0: container init (last CT task)            CPU1: host process with a socket in the CT netns
>    =========================================      ============================================
>    do_exit()
>     exit_notify()
>      zap_pid_ns_processes()   /* pid_ns is empty */
>       ve_exit_ns()
>        down_write(&ve->op_sem)
>        ve_hook_iterate_fini()
>         cn_fini_ve()
>          cn = rcu_dereference_protected(ve->cn)
>          cn->cn_already_initialized = 0
>          cn_proc_fini_ve()
>           cn_del_callback_ve()
>           RCU_INIT_POINTER(ve->cn, NULL)  <--- pointer is hidden
>           synchronize_rcu()               <--- sleeps for milliseconds,
>            ...                                 dev->nls is still ALIVE
>            ...                                 sendmsg(nl_sock, msg)
>            ...                                  netlink_sendmsg()
>            ...                                   netlink_unicast()
>            ...                                    netlink_unicast_kernel()
>            ...                                     nlk->netlink_rcv = cn_rx_skb()
>            ...                                      cn_call_callback()
>            ...                                       dev = get_cdev(net->owner_ve)
>            ...                                        rcu_dereference_check(ve->cn, 1)
>            ...                                        /* ve->cn == NULL */
>            ...                                        return NULL;    <--- patch 1
>            ...                                       spin_lock_bh(&dev->cbdev->queue_lock)
>            ...                                       /* dev == NULL */
>            ...                                       *** oops: NULL + offsetof(cbdev) ***
>           free_percpu(cn->local_event)
>          remove_proc_entry("connector", ...)
>          cn_queue_free_dev(dev->cbdev)
>          netlink_kernel_release(dev->nls)  <--- only HERE does the input path die,
>          kfree(cn)                              but it is too late
>
>    The key point: the window between RCU_INIT_POINTER(ve->cn, NULL) and netlink_kernel_release() is now wide (it contains a
>    whole synchronize_rcu()), the kernel socket keeps accepting messages all that time, and cn_call_callback() on the receive
>    side has no NULL check. Before patch 2 the order was reversed: netlink_kernel_release() quiesced the input path first, and
>    only then ve->cn = NULL - within that window CPU1 simply could not reach cn_call_callback().
>
>
> ? Scenario 2 - UAF (fetch before the clear, use after the free)
>
>    Same setup, but CPU1 fetched the pointer before the clear and got preempted. synchronize_rcu() does not wait for it -
>    cn_call_callback() reads ve->cn without rcu_read_lock():
>
>    CPU0: container init                           CPU1: host-side sender
>    =========================================      ============================================
>                                                   cn_rx_skb()
>                                                    cn_call_callback()
>                                                     dev = get_cdev(...)   /* cn != NULL, ok */
>                                                     <-- preempted -->
>    cn_proc_fini_ve()
>     RCU_INIT_POINTER(ve->cn, NULL)
>     synchronize_rcu()   /* CPU1 is not in an RCU
>                            section - it will NOT
>                            be waited for */
>     free_percpu(cn->local_event)
>    cn_queue_free_dev(dev->cbdev)   <--- cbdev is freed
>    netlink_kernel_release(dev->nls)
>    kfree(cn)                       <--- cn is freed
>                                                   <-- resumes -->
>                                                   spin_lock_bh(&dev->cbdev->queue_lock)
>                                                   *** use-after-free: dev = &cn->cdev,
>                                                       cn and cbdev already kfree()d ***
>
>    Scenario 2 is exactly why a bare if (!dev) return in cn_call_callback() is not enough: it only fixes scenario 1. The
>    proposed patch takes rcu_read_lock() around the fetch plus the queue walk - then in scenario 2 CPU1 is inside an RCU
>    section, synchronize_rcu() on CPU0 has to wait for it and all the frees move past the end of the section; and in scenario
>    1 the NULL check kicks in.
> =============================================================
>
>
> suggested fix:
>
>    diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
>    --- a/drivers/connector/connector.c
>    +++ b/drivers/connector/connector.c
>    @@ -154,7 +154,7 @@ static int cn_call_callback(struct sk_buff *skb)
>     {
>          struct nlmsghdr *nlh;
>          struct cn_callback_entry *i, *cbq = NULL;
>    -     struct cn_dev *dev = get_cdev(sock_net(skb->sk)->owner_ve);
>    +     struct cn_dev *dev;
>          struct cn_msg *msg = nlmsg_data(nlmsg_hdr(skb));
>          struct netlink_skb_parms *nsp = &NETLINK_CB(skb);
>          int err = -ENODEV;
>    @@ -164,6 +164,19 @@ static int cn_call_callback(struct sk_buff *skb)
>          if (nlh->nlmsg_len < NLMSG_HDRLEN + sizeof(struct cn_msg) + msg->len)
>                  return -EINVAL;
>
>    +     /*
>    +      * The kernel socket outlives ve->cn on VE stop: cn_proc_fini_ve()
>    +      * clears the pointer and waits for a grace period long before
>    +      * cn_fini_ve() releases the socket, so a message sent from the
>    +      * host into the dying VE netns can still get here. Fetch ve->cn
>    +      * and walk the callback queue under rcu_read_lock(): once the
>    +      * pointer is observed, the grace period in cn_proc_fini_ve()
>    +      * keeps the callback device alive until we are done with it.
>    +      */
>    +     rcu_read_lock();
>    +     dev = get_cdev(sock_net(skb->sk)->owner_ve);
>    +     if (!dev) {
>    +             rcu_read_unlock();
>    +             return -ENODEV;
>    +     }
>    +
>          spin_lock_bh(&dev->cbdev->queue_lock);
>          list_for_each_entry(i, &dev->cbdev->queue_list, callback_entry) {
>                  if (cn_cb_equal(&i->id.id, &msg->id)) {
>    @@ -173,6 +186,7 @@ static int cn_call_callback(struct sk_buff *skb)
>                  }
>          }
>          spin_unlock_bh(&dev->cbdev->queue_lock);
>    +     rcu_read_unlock();
>
>          if (cbq != NULL) {
>                  cbq->callback(msg, nsp);
>
>>   	free_percpu(cn->local_event);
>>   }
>> diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
>> index bf4a83f3f870..6483e15b888e 100644
>> --- a/drivers/connector/connector.c
>> +++ b/drivers/connector/connector.c
>> @@ -394,7 +394,7 @@ static void cn_fini_ve(void *data)
>>   	cn_queue_free_dev(dev->cbdev);
>>   	netlink_kernel_release(dev->nls);
>>   
>> -	RCU_INIT_POINTER(ve->cn, NULL);
>> +	/* ve->cn was cleared by cn_proc_fini_ve() before the grace period */
>>   	kfree(cn);
>>   }
>>   
>>
-- 
Best regards, Vasileios Almpanis
Software Developer, Virtuozzo.