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

[Vasileios Almpanis]

cn_fini_ve() tears down everything the proc event delivery path uses.
the percpu local_event, the callback device and the kernel netlink
socket. The 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, then the teardown can 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 (done by
the next patch). 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().

The kernel netlink socket outlives the ve->cn clearing: it is released
only later in cn_fini_ve(), after this grace period. A message sent
from the host into the dying VE netns (e.g. via nsenter) can therefore
still reach cn_call_callback() with ve->cn already NULL, so get_cdev()
now returns NULL there. Look the callback device up and walk the queue
under rcu_read_lock() and bail out on NULL, so the receive path is
covered by the same grace period as the delivery path.

cn_init_ve() publishes ve->cn early with rcu_assign_pointer(), before
the connector is fully set up, so its error path may already have an
RCU reader looking at the state. Wait for a grace period there too
before freeing it, symmetric to cn_proc_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 | 25 +++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

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();
+
 	free_percpu(cn->local_event);
 }
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index bf4a83f3f870..05c281bb321b 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -151,7 +151,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;
@@ -161,6 +161,21 @@ static int cn_call_callback(struct sk_buff *skb)
 	if (nlh->nlmsg_len < NLMSG_HDRLEN + sizeof(struct cn_msg) + msg->len)
 		return -EINVAL;
 
+	/*
+	 * On VE stop cn_proc_fini_ve() clears ve->cn and waits for a grace
+	 * period before the callback device is freed, but the kernel socket
+	 * is released only afterwards, so a message sent from the host into
+	 * the dying VE netns can still get here. Look the device up and walk
+	 * the callback queue under rcu_read_lock(); the callback itself may
+	 * sleep and is kept alive by the refcount taken here.
+	 */
+	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)) {
@@ -170,6 +185,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);
@@ -362,7 +378,12 @@ static int cn_init_ve(void *data)
 netlink_release:
 	netlink_kernel_release(dev->nls);
 free_cn:
+	/*
+	 * The pointer was published: wait for the readers before freeing,
+	 * same as cn_proc_fini_ve() does.
+	 */
 	RCU_INIT_POINTER(ve->cn, NULL);
+	synchronize_rcu();
 	kfree(cn);
 	goto net_unlock;
 }
@@ -394,7 +415,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);
 }
 

-- 
2.43.0