[Devel] [PATCH VZ10 v2 0/4] connector: do not lose exit events for in-CT listeners

[Devel] [PATCH VZ10 v2 0/4] connector: do not lose exit events for in-CT listeners

[Vasileios Almpanis]

The proc connector reports the exit event after exit_notify(), so the
parent may already have reaped the exiting task. release_task() ->
exit_ve_namespace() then resets tsk->task_ve to ve0, and if that wins
the race the event is delivered to the host listeners only. A listener
inside the container never sees the exit event. LTP catches this as the
flaky cn_pec exit-test failure causing the test to hang for as long as 8
hours on GCOV kernels.

Patch 4 fixes this by pinning the task's VE for the exit notification
before exit_notify(). This means the delivery path can now run against
a VE whose last task is already reaped, i.e. concurrently with the
per-VE connector teardown, which was impossible before. Patches 1-3
prepare for that: annotate ve->cn with __rcu, free the per-VE connector
state only after an RCU grace period, and deliver per-VE events under
rcu_read_lock().

Testing:
KASAN, PROVE_LOCKING/PROVE_RCU, DEBUG_OBJECTS, kmemleak=on; HCI VM with one
container):
 - LTP cn_pec inside a CT: unpatched kernel loses exit events in 3 of
   10 runs (-n 50); patched kernel passes 30 of 30 runs.
 - Teardown stress: 15 cycles of an active in-CT pec_listener plus a
   200-process exit storm with vzctl stop/start while events are in
   flight: no KASAN/lockdep/RCU splats, kmemleak scans report 0
   unreferenced objects.

https://virtuozzo.atlassian.net/browse/VSTOR-140421

---
Changes in v2:
- Turned the single patch into a series: with the exit event now
  delivered for a possibly already-reaped task, the per-VE delivery
  path can race with the connector teardown in cn_fini_ve(). New
  patches 1-3 make that safe: annotate ve->cn with __rcu, free the
  per-VE connector state only after an RCU grace period, and run the
  delivery under rcu_read_lock().
- cn_proc.h: forward-declare struct ve_struct instead of including
  linux/ve.h.
- Link to v1: https://lists.openvz.org/pipermail/devel/2026-August/084581.html

---
Vasileios Almpanis (4):
      connector: annotate ve->cn with __rcu
      connector: free the per-VE connector state after an RCU grace period
      connector: deliver per-VE proc events under an RCU read lock
      proc connector: pin task VE for the exit event notification

 drivers/connector/cn_proc.c   | 105 ++++++++++++++++++++++++++++++++++--------
 drivers/connector/connector.c |  47 +++++++++++++------
 include/linux/cn_proc.h       |   8 +++-
 include/linux/ve.h            |   2 +-
 kernel/exit.c                 |   5 +-
 5 files changed, 131 insertions(+), 36 deletions(-)
---
base-commit: 209f11f2c454a88761fb8f0a820fa22b6feae720
change-id: 20260813-connectors-3571be4ae57b

-- 
Best regards, Vasileios Almpanis
Software Developer, Virtuozzo.

[Devel] [PATCH VZ10 v2 1/4] connector: annotate ve->cn with __rcu

[Vasileios Almpanis]

The following patches will clear ve->cn on container stop and free the
per-VE connector state only after an RCU grace period, so that the
proc event delivery path can use it under rcu_read_lock() even when
the reported task no longer pins the VE.

Prepare for that: annotate ve->cn with __rcu and switch all accesses
to the RCU accessors so sparse can verify the protocol. All current
users run either in the context of a task alive in the VE or from
the VE start/stop hooks under ve->op_sem (or on ve0), so plain readers
use rcu_dereference_check(ve->cn, 1) with a comment and the start/stop
hooks use rcu_dereference_protected() with the proper lockdep
condition.

No functional change intended

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   | 48 +++++++++++++++++++++++++++++++------------
 drivers/connector/connector.c | 43 +++++++++++++++++++++++++-------------
 include/linux/ve.h            |  2 +-
 3 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index d41c8aca1866..d4ce1697dd0b 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -84,6 +84,8 @@ static int cn_filter(struct sock *dsk, struct sk_buff *skb, void *data)
 
 static inline void send_msg_ve(struct ve_struct *ve, struct cn_msg *msg)
 {
+	/* See the comment in proc_event_num_listeners() */
+	struct cn_private *cn = rcu_dereference_check(ve->cn, 1);
 	struct local_event *le_ptr;
 	__u32 filter_data[2];
 
@@ -93,9 +95,9 @@ static inline void send_msg_ve(struct ve_struct *ve, struct cn_msg *msg)
 	 * so be of the safe side.
 	 */
 	BUILD_BUG_ON(offsetof(struct local_event, lock) != 0);
-	local_lock(&ve->cn->local_event->lock);
+	local_lock(&cn->local_event->lock);
 
-	le_ptr = this_cpu_ptr(ve->cn->local_event);
+	le_ptr = this_cpu_ptr(cn->local_event);
 	msg->seq = le_ptr->count++;
 	((struct proc_event *)msg->data)->cpu = smp_processor_id();
 
@@ -116,7 +118,7 @@ static inline void send_msg_ve(struct ve_struct *ve, struct cn_msg *msg)
 	cn_netlink_send_mult_ve(ve, msg, msg->len, 0, CN_IDX_PROC, GFP_NOWAIT,
 				cn_filter, (void *)filter_data);
 
-	local_unlock(&ve->cn->local_event->lock);
+	local_unlock(&cn->local_event->lock);
 }
 
 static struct cn_msg *cn_msg_fill(__u8 *buffer, struct ve_struct *ve,
@@ -147,8 +149,15 @@ static struct cn_msg *cn_msg_fill(__u8 *buffer, struct ve_struct *ve,
 
 static int proc_event_num_listeners(struct ve_struct *ve)
 {
-	if (ve->cn)
-		return atomic_read(&ve->cn->proc_event_num_listeners);
+	/*
+	 * Callers not under rcu_read_lock() are pinned by a live task
+	 * of this VE (or run on ve0 whose connector state lives as long
+	 * as the connector itself).
+	 */
+	struct cn_private *cn = rcu_dereference_check(ve->cn, 1);
+
+	if (cn)
+		return atomic_read(&cn->proc_event_num_listeners);
 	return 0;
 }
 
@@ -414,6 +423,8 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
 	enum proc_cn_event ev_type = 0;
 	int err = 0, initial = 0;
 	struct sock *sk = NULL;
+	/* current is a live task of this VE, it cannot be stopped under us */
+	struct cn_private *cn = rcu_dereference_check(ve->cn, 1);
 
 	/* 
 	 * Events are reported with respect to the initial pid
@@ -467,11 +478,11 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg,
 	switch (mc_op) {
 	case PROC_CN_MCAST_LISTEN:
 		if (initial || (prev_mc_op != PROC_CN_MCAST_LISTEN))
-			atomic_inc(&ve->cn->proc_event_num_listeners);
+			atomic_inc(&cn->proc_event_num_listeners);
 		break;
 	case PROC_CN_MCAST_IGNORE:
 		if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE))
-			atomic_dec(&ve->cn->proc_event_num_listeners);
+			atomic_dec(&cn->proc_event_num_listeners);
 		((struct proc_input *)(sk->sk_user_data))->event_type =
 			PROC_EVENT_NONE;
 		break;
@@ -488,13 +499,18 @@ int cn_proc_init_ve(struct ve_struct *ve)
 {
 	int err, cpu;
 	struct local_event *le_ptr;
+	struct cn_private *cn;
 
-	ve->cn->local_event = alloc_percpu(struct local_event);
-	if (!ve->cn->local_event)
+	cn = rcu_dereference_protected(ve->cn,
+				       lockdep_is_held(&ve->op_sem) ||
+				       ve_is_super(ve));
+
+	cn->local_event = alloc_percpu(struct local_event);
+	if (!cn->local_event)
 		return -ENOMEM;
 
 	for_each_possible_cpu(cpu) {
-		le_ptr = per_cpu_ptr(ve->cn->local_event, cpu);
+		le_ptr = per_cpu_ptr(cn->local_event, cpu);
 		local_lock_init(&le_ptr->lock);
 	}
 
@@ -503,15 +519,21 @@ int cn_proc_init_ve(struct ve_struct *ve)
 				  &cn_proc_mcast_ctl);
 	if (err) {
 		pr_warn("VE#%d: cn_proc failed to register\n", ve->veid);
-		free_percpu(ve->cn->local_event);
+		free_percpu(cn->local_event);
 		return err;
 	}
-	atomic_set(&ve->cn->proc_event_num_listeners, 0);
+	atomic_set(&cn->proc_event_num_listeners, 0);
 	return 0;
 }
 
 void cn_proc_fini_ve(struct ve_struct *ve)
 {
+	struct cn_private *cn;
+
+	cn = rcu_dereference_protected(ve->cn,
+				       lockdep_is_held(&ve->op_sem) ||
+				       ve_is_super(ve));
+
 	cn_del_callback_ve(ve, &cn_proc_event_id);
-	free_percpu(ve->cn->local_event);
+	free_percpu(cn->local_event);
 }
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 39a697803d0f..bf4a83f3f870 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -29,7 +29,13 @@ MODULE_ALIAS_NET_PF_PROTO(PF_NETLINK, NETLINK_CONNECTOR);
 
 static struct cn_dev *get_cdev(struct ve_struct *ve)
 {
-	return &ve->cn->cdev;
+	/*
+	 * Callers not under rcu_read_lock() are pinned by a live task
+	 * of this VE or run from the VE start/stop hooks.
+	 */
+	struct cn_private *cn = rcu_dereference_check(ve->cn, 1);
+
+	return cn ? &cn->cdev : NULL;
 }
 
 /*
@@ -230,12 +236,13 @@ int cn_add_callback_ve(struct ve_struct *ve,
 		       void (*callback)(struct cn_msg *,
 					struct netlink_skb_parms *))
 {
-	struct cn_dev *dev = get_cdev(ve);
+	/* See the comment in get_cdev() */
+	struct cn_private *cn = rcu_dereference_check(ve->cn, 1);
 
-	if (!ve->cn->cn_already_initialized)
+	if (!cn || !cn->cn_already_initialized)
 		return -EAGAIN;
 
-	return cn_queue_add_callback(dev->cbdev, name, id, callback);
+	return cn_queue_add_callback(cn->cdev.cbdev, name, id, callback);
 }
 
 /*
@@ -297,6 +304,7 @@ static int __maybe_unused cn_proc_show(struct seq_file *m, void *v)
 static int cn_init_ve(void *data)
 {
 	struct ve_struct *ve = data;
+	struct cn_private *cn;
 	struct cn_dev *dev;
 	struct netlink_kernel_cfg cfg = {
 		.groups	= CN_NETLINK_USERS + 0xf,
@@ -308,11 +316,12 @@ static int cn_init_ve(void *data)
 	struct net *net;
 	int err;
 
-	ve->cn = kzalloc(sizeof(*ve->cn), GFP_KERNEL);
-	if (!ve->cn)
+	cn = kzalloc(sizeof(*cn), GFP_KERNEL);
+	if (!cn)
 		return -ENOMEM;
+	rcu_assign_pointer(ve->cn, cn);
 
-	dev = &ve->cn->cdev;
+	dev = &cn->cdev;
 
 	/*
 	 * This is a hook, hooks are called under a single lock, so ve_nsproxy will
@@ -331,7 +340,7 @@ static int cn_init_ve(void *data)
 		goto netlink_release;
 	}
 
-	ve->cn->cn_already_initialized = 1;
+	cn->cn_already_initialized = 1;
 
 	if (!proc_ve_create_single("connector", S_IRUGO, net->proc_net,
 				cn_proc_show)) {
@@ -353,18 +362,24 @@ static int cn_init_ve(void *data)
 netlink_release:
 	netlink_kernel_release(dev->nls);
 free_cn:
-	kfree(ve->cn);
-	ve->cn = NULL;
+	RCU_INIT_POINTER(ve->cn, NULL);
+	kfree(cn);
 	goto net_unlock;
 }
 
 static void cn_fini_ve(void *data)
 {
 	struct ve_struct *ve = data;
-	struct cn_dev *dev = get_cdev(ve);
+	struct cn_private *cn;
+	struct cn_dev *dev;
 	struct net *net;
 
-	ve->cn->cn_already_initialized = 0;
+	cn = rcu_dereference_protected(ve->cn,
+				       lockdep_is_held(&ve->op_sem) ||
+				       ve_is_super(ve));
+	dev = &cn->cdev;
+
+	cn->cn_already_initialized = 0;
 
 	cn_proc_fini_ve(ve);
 
@@ -379,8 +394,8 @@ static void cn_fini_ve(void *data)
 	cn_queue_free_dev(dev->cbdev);
 	netlink_kernel_release(dev->nls);
 
-	kfree(ve->cn);
-	ve->cn = NULL;
+	RCU_INIT_POINTER(ve->cn, NULL);
+	kfree(cn);
 }
 
 #ifdef CONFIG_VE
diff --git a/include/linux/ve.h b/include/linux/ve.h
index cba827260d07..3334bd1e9517 100644
--- a/include/linux/ve.h
+++ b/include/linux/ve.h
@@ -94,7 +94,7 @@ struct ve_struct {
 	char			core_pattern[CORENAME_MAX_SIZE];
 #endif
 #ifdef CONFIG_CONNECTOR
-	struct cn_private	*cn;
+	struct cn_private __rcu	*cn;
 #endif
 
 	struct kthread_worker	*kthreadd_worker;

-- 
2.43.0

[Devel] [PATCH VZ10 v2 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
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();
+
 	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);
 }
 

-- 
2.43.0

[Devel] [PATCH VZ10 v2 3/4] connector: deliver per-VE proc events under an RCU read lock

[Vasileios Almpanis]

proc_event_connector_ve() dereferences ve->cn several times and
fill_exit_event() dereferences ve->ve_nsproxy assuming the VE cannot
stop in the middle of the delivery. This holds while the reported task
is alive in the VE: a live task keeps the VE pid namespace busy, so
zap_pid_ns_processes() -> ve_exit_ns() cannot start.

The next patch makes proc_exit_connector() deliver the exit event with
a VE reference pinned before exit_notify(). Once the task is reaped,
its pid no longer keeps the pid namespace busy: the container init may
be woken up by free_pid() from release_task(), finish
zap_pid_ns_processes() and run ve_exit_ns() while the exit event is
still being delivered:

    cpu0: exiting task                cpu1: container init
    do_exit()
      exit_notify()
        release_task()
          free_pid()  ------------->  wakes zap_pid_ns_processes()
      proc_exit_connector()           ve_exit_ns()
        proc_event_connector_ve()       cn_fini_ve()      /* ve->cn */
          fill_exit_event()             ve_drop_context() /* ve_nsproxy */
            ve->ve_nsproxy->...

Deliver the event under rcu_read_lock() and recheck the pointers: the
previous patch guarantees everything reachable from ve->cn stays alive
for the whole read-side critical section once observed, and
ve_drop_context() already waits for a grace period before dropping
ve_nsproxy. Bail out if the VE is being stopped: its listeners are
dead anyway, there is nobody to deliver to.

The whole delivery path runs with GFP_NOWAIT and never sleeps, so it
is legal inside an RCU read-side critical section.

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   | 36 +++++++++++++++++++++++++++++++++---
 drivers/connector/connector.c |  4 ++++
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 6095c7def7ce..608430808548 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -89,6 +89,10 @@ static inline void send_msg_ve(struct ve_struct *ve, struct cn_msg *msg)
 	struct local_event *le_ptr;
 	__u32 filter_data[2];
 
+	/* The VE is being stopped, so are its listeners: nothing to do */
+	if (!cn)
+		return;
+
 	/*
 	 * The following hack with local_event->lock address works only
 	 * till the "lock" is the first field in the local_event struct,
@@ -172,15 +176,27 @@ static void proc_event_connector_ve(struct task_struct *task,
 	struct cn_msg *msg;
 	__u8 buffer[CN_PROC_MSG_SIZE] __aligned(8);
 
+	/*
+	 * The exit event may be delivered when the reported task no
+	 * longer pins the VE (see proc_exit_connector()), so the VE may
+	 * be stopping concurrently. cn_proc_fini_ve() waits for an RCU
+	 * grace period before the connector state is freed, take the RCU
+	 * read lock to make the state observed here stay valid for the
+	 * whole delivery. The path below never sleeps (GFP_NOWAIT).
+	 */
+	rcu_read_lock();
+
 	if (proc_event_num_listeners(ve) < 1)
-		return;
+		goto out_unlock;
 
 	msg = cn_msg_fill(buffer, ve, task, what, cookie, fill_event);
 	if (!msg)
-		return;
+		goto out_unlock;
 
 	/*  If cn_netlink_send() failed, the data is not sent */
 	send_msg_ve(ve, msg);
+out_unlock:
+	rcu_read_unlock();
 }
 
 static void proc_event_connector(struct task_struct *task,
@@ -350,9 +366,23 @@ void proc_coredump_connector(struct task_struct *task)
 static bool fill_exit_event(struct proc_event *ev, struct ve_struct *ve,
 			    struct task_struct *task, long cookie_pids)
 {
-	struct pid_namespace *pid_ns = ve->ve_nsproxy->pid_ns_for_children;
+	struct pid_namespace *pid_ns;
 	struct task_struct *parent;
 	struct pids *pids = (struct pids *)cookie_pids;
+	struct nsproxy *nsproxy;
+
+	/*
+	 * Unlike all other events, the exit event may be delivered after
+	 * the task was reaped, when nothing keeps the VE pid namespace
+	 * busy anymore and the VE may be stopping concurrently.
+	 * ve_drop_context() clears ve_nsproxy and waits for an RCU grace
+	 * period before dropping it; we are called under rcu_read_lock().
+	 * The VE is dead, so are its listeners: skip the event.
+	 */
+	nsproxy = rcu_dereference(ve->ve_nsproxy);
+	if (!nsproxy)
+		return false;
+	pid_ns = nsproxy->pid_ns_for_children;
 
 	ev->event_data.exit.process_pid = pid_nr_ns(pids->pid, pid_ns);
 	ev->event_data.exit.process_tgid = pid_nr_ns(pids->tgid, pid_ns);
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 6483e15b888e..23c66d80b6b9 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -79,6 +79,10 @@ int cn_netlink_send_mult_ve(struct ve_struct *ve, struct cn_msg *msg, u16 len,
 	u32 group = 0;
 	int found = 0;
 
+	/* The VE is being stopped, see proc_event_connector_ve() */
+	if (!dev)
+		return -ENODEV;
+
 	if (portid || __group) {
 		group = __group;
 	} else {

-- 
2.43.0

[Devel] [PATCH VZ10 v2 4/4] proc connector: pin task VE for the exit event notification

[Vasileios Almpanis]

The exit event is reported after exit_notify(), so the parent might have
been woken up and reaped the exiting task via wait() -> release_task()
-> exit_ve_namespace(), which resets tsk->task_ve to ve0. If that wins
the race against the exiting task then the event will only be delivered
to the host VE listeners and the in-VE listeners will be skipped. For a
task inside a container this means the listener never receives an exit
event.

The issue is caught by the LTP suite_kernel_misc.exec.cn_pec_sh test:
pec_listener terminates upon receiving the exit event of the pid given
via -p. When the lost exit event is a child's one, the test fails:

  cn_pec 3 TFAIL: Event was not detected by the event listener:
                  exit pid: 58388 exit_code: 0 exit_signal: 17

and when it is the event generator's own exit event, the listener
polls the netlink socket forever and the test hangs until the LTP
timeout kills it (~8h on coverage kernels):

22:51:37 cn_pec 2 TINFO: Testing exec event (nevents=10)
07:11:37 Test timed out, sending SIGTERM!

The race was captured using kprobes on the connector send path:
  p:cnp/pexit proc_exit_connector task=$arg1:x64
  p:cnp/vexit exit_ve_namespace task=$arg1:x64
  p:cnp/pevcve proc_event_connector_ve what=$arg3:u32
  r:cnp/cnsend cn_netlink_send_mult_ve ret=$retval:s64

A normal exit looks like:
  pexit -> pevcve(ve) -> cnsend ret=0 -> pevcve(ve0)

The lost event (task 0xffff89a506d73980 is the exiting child, reaped by
its parent pid 465820 in between):

 465831 [001] pexit: (proc_exit_connector) task=0xffff89a506d73980
 465820 [002] vexit: (exit_ve_namespace) task=0xffff89a506d73980
 465831 [001] pevcve: (proc_event_connector_ve) what=2147483648

Only one proc_event_connector_ve() call fires (ve0, no listeners) and
cn_netlink_send_mult_ve() is never reached: the event is dropped.

This is the same race window that commit c565cc211694 ("proc
connector: report proper pid/tgid of an exited process") closed for
the task pid/tgid, but nothing pins the VE.

Reproducer (fails within ~50 iterations in a CT on a coverage kernel):

  cd /opt/ltp/testcases/bin/
  export PATH=$PATH:/opt/ltp/testcases/bin
  for i in $(seq 1 1024); do cn_pec.sh >/dev/null 2>&1 || break; done

Solve this the same way. Pin the VE in do_exit() before exit_notify()
is called and use it in proc_exit_connector() instead of re-reading
task->task_ve.

https://virtuozzo.atlassian.net/browse/VSTOR-140421
Fixes: 95fa2f096b72 ("ve: Introduce VE namespace")
Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

Feature: ve: ve generic structures
---
 drivers/connector/cn_proc.c | 10 ++++++++--
 include/linux/cn_proc.h     |  8 ++++++--
 kernel/exit.c               |  5 ++++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 608430808548..8d8c538cc3cd 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -401,9 +401,15 @@ static bool fill_exit_event(struct proc_event *ev, struct ve_struct *ve,
 	return true;
 }
 
-void proc_exit_connector(struct task_struct *task, struct pids *pids)
+void proc_exit_connector(struct task_struct *task, struct pids *pids,
+			 struct ve_struct *ve)
 {
-	proc_event_connector(task, PROC_EVENT_EXIT, (long)pids, fill_exit_event);
+	if (!ve_is_super(ve))
+		proc_event_connector_ve(task, ve, PROC_EVENT_EXIT, (long)pids,
+					fill_exit_event);
+
+	proc_event_connector_ve(task, get_ve0(), PROC_EVENT_EXIT, (long)pids,
+				fill_exit_event);
 }
 
 /*
diff --git a/include/linux/cn_proc.h b/include/linux/cn_proc.h
index 9701c13d82df..aa606192b5c2 100644
--- a/include/linux/cn_proc.h
+++ b/include/linux/cn_proc.h
@@ -19,6 +19,8 @@
 
 #include <uapi/linux/cn_proc.h>
 
+struct ve_struct;
+
 /*
  * The struct is used solely for pinning task pids for proc connector
  * notification on process exit.
@@ -36,7 +38,8 @@ void proc_sid_connector(struct task_struct *task);
 void proc_ptrace_connector(struct task_struct *task, int which_id);
 void proc_comm_connector(struct task_struct *task);
 void proc_coredump_connector(struct task_struct *task);
-void proc_exit_connector(struct task_struct *task, struct pids *pids);
+void proc_exit_connector(struct task_struct *task, struct pids *pids,
+			 struct ve_struct *ve);
 #else
 static inline void proc_fork_connector(struct task_struct *task)
 {}
@@ -61,7 +64,8 @@ static inline void proc_ptrace_connector(struct task_struct *task,
 static inline void proc_coredump_connector(struct task_struct *task)
 {}
 
-static inline void proc_exit_connector(struct task_struct *task, struct pids *pids)
+static inline void proc_exit_connector(struct task_struct *task, struct pids *pids,
+				       struct ve_struct *ve)
 {}
 #endif	/* CONFIG_PROC_EVENTS */
 #endif	/* CN_PROC_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index 448a734270a7..94d9bddae2b8 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -936,6 +936,7 @@ void __noreturn do_exit(long code)
 	struct task_struct *tsk = current;
 	int group_dead;
 	struct pids pids;
+	struct ve_struct *ve;
 
 	WARN_ON(irqs_disabled());
 
@@ -1021,8 +1022,10 @@ void __noreturn do_exit(long code)
 	exit_tasks_rcu_start();
 	pids.pid = get_pid(task_pid(tsk));
 	pids.tgid = get_pid(task_tgid(tsk));
+	ve = get_task_ve(tsk);
 	exit_notify(tsk, group_dead);
-	proc_exit_connector(tsk, &pids);
+	proc_exit_connector(tsk, &pids, ve);
+	put_ve(ve);
 	put_pid(pids.tgid);
 	put_pid(pids.pid);
 	mpol_put_task_policy(tsk);

-- 
2.43.0

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

[Konstantin Khorenko]

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?

=============================================================
? 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);
>  }
>  
> 

Re: [Devel] [PATCH VZ10 v2 1/4] connector: annotate ve->cn with __rcu

[Konstantin Khorenko]

On 8/18/26 17:10, Vasileios Almpanis wrote:
> The following patches will clear ve->cn on container stop and free the
> per-VE connector state only after an RCU grace period, so that the
> proc event delivery path can use it under rcu_read_lock() even when
> the reported task no longer pins the VE.
> 
> Prepare for that: annotate ve->cn with __rcu and switch all accesses
> to the RCU accessors so sparse can verify the protocol. All current
> users run either in the context of a task alive in the VE or from
> the VE start/stop hooks under ve->op_sem (or on ve0), so plain readers
> use rcu_dereference_check(ve->cn, 1) with a comment and the start/stop
> hooks use rcu_dereference_protected() with the proper lockdep
> condition.
> 
> No functional change intended
> 
> 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   | 48 +++++++++++++++++++++++++++++++------------
>  drivers/connector/connector.c | 43 +++++++++++++++++++++++++-------------
>  include/linux/ve.h            |  2 +-
>  3 files changed, 65 insertions(+), 28 deletions(-)
> 
...

> @@ -297,6 +304,7 @@ static int __maybe_unused cn_proc_show(struct seq_file *m, void *v)
>  static int cn_init_ve(void *data)
>  {
>  	struct ve_struct *ve = data;
> +	struct cn_private *cn;
>  	struct cn_dev *dev;
>  	struct netlink_kernel_cfg cfg = {
>  		.groups	= CN_NETLINK_USERS + 0xf,
> @@ -308,11 +316,12 @@ static int cn_init_ve(void *data)
>  	struct net *net;
>  	int err;
>  
> -	ve->cn = kzalloc(sizeof(*ve->cn), GFP_KERNEL);
> -	if (!ve->cn)
> +	cn = kzalloc(sizeof(*cn), GFP_KERNEL);
> +	if (!cn)
>  		return -ENOMEM;
> +	rcu_assign_pointer(ve->cn, cn);
>  
> -	dev = &ve->cn->cdev;
> +	dev = &cn->cdev;
>  
>  	/*
>  	 * This is a hook, hooks are called under a single lock, so ve_nsproxy will
> @@ -331,7 +340,7 @@ static int cn_init_ve(void *data)
>  		goto netlink_release;
>  	}
>  
> -	ve->cn->cn_already_initialized = 1;
> +	cn->cn_already_initialized = 1;
>  
>  	if (!proc_ve_create_single("connector", S_IRUGO, net->proc_net,
>  				cn_proc_show)) {
> @@ -353,18 +362,24 @@ static int cn_init_ve(void *data)
>  netlink_release:
>  	netlink_kernel_release(dev->nls);
>  free_cn:
> -	kfree(ve->cn);
> -	ve->cn = NULL;
> +	RCU_INIT_POINTER(ve->cn, NULL);
> +	kfree(cn);

  The attribution logic:

  - The error path itself (RCU_INIT_POINTER(ve->cn, NULL); kfree(cn) in cn_init_ve()) appeared in patch 1, where it
    faithfully preserves the base behavior (ve->cn = NULL; kfree) - the "no functional change" claim holds.
  - Before patch 4, the only possible reader of a container VE's ve->cn is a live task of that VE. While cn_init_ve() runs
    (VE start, under op_sem) there are no tasks in the VE yet - no reader can exist, so the immediate kfree is safe.
  - Patch 4 creates a new kind of reader: the exit event delivery with a pinned ve_struct, which can run after the VE has
    fully stopped - and therefore also concurrently with its restart (it is the very same ve_struct, the cgroup stays
    alive). Bisect-wise the crash only becomes reachable starting with patch 4 - hence the question sits in its
    review-inline.

  The scenario:

  CPU0: exiting task (old CT incarnation)        CPU1: vzctl start (restart of the same ve)
  =========================================      ========================================
  do_exit()
   ve = get_task_ve(tsk)      /* pins ve */
   exit_notify()              /* reaped, pid
                                 freed */
   <-- preempted; the CT manages to stop
       completely: cn_fini_ve() done,
       ve->cn == NULL -->
                                                 ve_start_container()  /* op_sem */
                                                  cn_init_ve()
                                                   cn = kzalloc(...)
                                                   rcu_assign_pointer(ve->cn, cn)  <-- published
   proc_exit_connector(tsk, &pids, ve)
    proc_event_connector_ve(task, ve, ...)
     rcu_read_lock()
     proc_event_num_listeners(ve)
      cn = rcu_dereference_check(ve->cn, 1)
      /* sees the FRESH cn from CPU1 */
                                                   netlink_kernel_create() -> fail
                                                  free_cn:
                                                   RCU_INIT_POINTER(ve->cn, NULL)
                                                   kfree(cn)          <-- NO grace period
      atomic_read(&cn->proc_event_num_listeners)
      *** use-after-free ***

  The reader dutifully sits under rcu_read_lock(), but unlike cn_proc_fini_ve() the error path does not wait for a grace
  period - patch 2's RCU contract ("everything reachable from ve->cn is freed only after a grace period") was not extended
  to this freeing site.

  That said, the right place to fix it is patch 2 rather than patch 4 - patch 2 owns the contract, and with the fix in place
  patch 4 lands on a fully safe base. The minimal variant, symmetric to cn_proc_fini_ve():


  --- a/drivers/connector/connector.c
  +++ b/drivers/connector/connector.c
  @@ static int cn_init_ve(void *data)
   free_cn:
        RCU_INIT_POINTER(ve->cn, NULL);
  +     /*
  +      * The pointer was published, so a pinned-VE exit event delivery
  +      * may already be looking at it under rcu_read_lock(): wait for
  +      * the readers before freeing, same as cn_proc_fini_ve() does.
  +      */
  +     synchronize_rcu();
        kfree(cn);
        goto net_unlock;

  The context is sleepable (GFP_KERNEL right nearby), and sleeping under op_sem is fine - ve_stop_ns()/cn_proc_fini_ve()
  already do exactly that.

  The architecturally cleaner alternative is to publish ve->cn only after the initialization fully succeeds (then a reader
  can never see a half-constructed state and the error path stays a plain kfree). But that is a bigger refactor:
  cn_proc_init_ve() and cn_add_callback_ve() dereference ve->cn themselves, so they would need to take cn as an argument.
  For this series the one-line synchronize_rcu() in the error path looks more appropriate.


>  	goto net_unlock;
>  }
>  
>  static void cn_fini_ve(void *data)
>  {
>  	struct ve_struct *ve = data;
> -	struct cn_dev *dev = get_cdev(ve);
> +	struct cn_private *cn;
> +	struct cn_dev *dev;
>  	struct net *net;
>  
> -	ve->cn->cn_already_initialized = 0;
> +	cn = rcu_dereference_protected(ve->cn,
> +				       lockdep_is_held(&ve->op_sem) ||
> +				       ve_is_super(ve));
> +	dev = &cn->cdev;
> +
> +	cn->cn_already_initialized = 0;
>  
>  	cn_proc_fini_ve(ve);
>  
> @@ -379,8 +394,8 @@ static void cn_fini_ve(void *data)
>  	cn_queue_free_dev(dev->cbdev);
>  	netlink_kernel_release(dev->nls);
>  
> -	kfree(ve->cn);
> -	ve->cn = NULL;
> +	RCU_INIT_POINTER(ve->cn, NULL);
> +	kfree(cn);
>  }
>  
>  #ifdef CONFIG_VE
> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index cba827260d07..3334bd1e9517 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -94,7 +94,7 @@ struct ve_struct {
>  	char			core_pattern[CORENAME_MAX_SIZE];
>  #endif
>  #ifdef CONFIG_CONNECTOR
> -	struct cn_private	*cn;
> +	struct cn_private __rcu	*cn;
>  #endif
>  
>  	struct kthread_worker	*kthreadd_worker;
> 

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

[Vasileios Almpanis]

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.