From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konstantin Khorenko Date: Tue, 25 Aug 2026 13:47:20 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 2/4] connector: free the per-VE connector state after an RCU grace period In-Reply-To: <20260818-connectors-v2-2-88c5d9049e6f@virtuozzo.com> References: <20260818-connectors-v2-0-88c5d9049e6f@virtuozzo.com> <20260818-connectors-v2-2-88c5d9049e6f@virtuozzo.com> Message-ID: List-Id: 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 > > 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? ============================================================= ? 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); > } > >