[Devel] [PATCH VZ10 v2 2/4] connector: free the per-VE connector state after an RCU grace period
Vasileios Almpanis
vasileios.almpanis at virtuozzo.com
Tue Aug 25 15:43:32 MSK 2026
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 at 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.
More information about the Devel
mailing list