From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasileios Almpanis Date: Tue, 25 Aug 2026 14:43:32 +0200 Subject: Re: [Devel] [PATCH VZ10 v2 2/4] connector: free the per-VE connector state after an RCU grace period In-Reply-To: References: <20260818-connectors-v2-0-88c5d9049e6f@virtuozzo.com> <20260818-connectors-v2-2-88c5d9049e6f@virtuozzo.com> Message-ID: <9991b0c3-9683-48b2-88c6-33d842062004@virtuozzo.com> List-Id: 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 >> >> 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.