[PATCH v2 1/2] pid: deinline kill_cad_pid

[PATCH v2 1/2] pid: deinline kill_cad_pid

[Cen Zhang (Microsoft)]

Move kill_cad_pid() out of the header without changing behavior. This
prepares for taking a reference to cad_pid under RCU in the following
fix.

Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Suggested-by: Bradley Morgan <include@grrlz.net>
Link: https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
v2:
  - New preparatory patch to deinline kill_cad_pid().

 include/linux/sched/signal.h | 5 +----
 kernel/pid.c                 | 5 +++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 584ae88b435e..d45a5476b97d 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -562,10 +562,7 @@ static inline sigset_t *sigmask_to_save(void)
 	return res;
 }
 
-static inline int kill_cad_pid(int sig, int priv)
-{
-	return kill_pid(cad_pid, sig, priv);
-}
+int kill_cad_pid(int sig, int priv);
 
 /* These can be the second arg to send_sig_info/send_group_sig_info.  */
 #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
diff --git a/kernel/pid.c b/kernel/pid.c
index f55189a3d07d..234ebee29375 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -557,6 +557,11 @@ pid_t pid_vnr(struct pid *pid)
 }
 EXPORT_SYMBOL_GPL(pid_vnr);
 
+int kill_cad_pid(int sig, int priv)
+{
+	return kill_pid(cad_pid, sig, priv);
+}
+
 pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
 			struct pid_namespace *ns)
 {
-- 
2.53.0

[PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Cen Zhang (Microsoft)]

proc_do_cad_pid() reads the global cad_pid pointer and passes it to
pid_vnr() without protecting the lifetime of the referenced struct pid.
A concurrent writer can replace cad_pid and drop the final reference to
the old struct pid after the reader has loaded the pointer but before
pid_vnr() has finished dereferencing it, causing a use-after-free.

The sysctl is mode 0600, but access is checked against the owning user
namespace, so an unprivileged user can reach it via userns, pidns, and a
proc mount.

Fix this by treating cad_pid as an RCU-protected pointer at both read
sites and by waiting for a grace period before dropping the old reference
on the write side.

KASAN crash stack:
  kernel/pid.c:545 pid_nr_ns()        # reads freed pid->level
  kernel/pid.c:556 pid_vnr()
  kernel/pid.c:775 proc_do_cad_pid()
  fs/proc/proc_sysctl.c proc_sys_call_handler()
  fs/read_write.c vfs_read()
  fs/read_write.c __x64_sys_pread64()

Fixes: 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")
Reported-by: AutonomousCodeSecurity@microsoft.com
Closes: https://lore.kernel.org/all/20260717210143.4734-1-blbllhy@gmail.com/
Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Reviewed-by: Bradley Morgan <include@grrlz.net>
Cc: stable@vger.kernel.org
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
v2:
  - Split out kill_cad_pid() deinline into a preparatory patch.
  - Annotate cad_pid as __rcu and use rcu_dereference().
  - Protect kill_cad_pid() by taking a pid reference under RCU.
  - Add a comment explaining why synchronize_rcu() is used instead of
    call_rcu().

 include/linux/sched.h |  2 +-
 init/main.c           |  2 +-
 kernel/pid.c          | 26 +++++++++++++++++++++++---
 kernel/reboot.c       |  2 +-
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 373bcc0598d1..31ce72b1233c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1767,7 +1767,7 @@ static inline bool is_lazy_mmu_mode_active(void)
 }
 #endif
 
-extern struct pid *cad_pid;
+extern struct pid __rcu *cad_pid;
 
 /*
  * Per process flags
diff --git a/init/main.c b/init/main.c
index e363232b428b..19a10d0c2760 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1636,7 +1636,7 @@ static noinline void __init kernel_init_freeable(void)
 	 */
 	set_mems_allowed(node_states[N_MEMORY]);
 
-	cad_pid = get_pid(task_pid(current));
+	rcu_assign_pointer(cad_pid, get_pid(task_pid(current)));
 
 	smp_prepare_cpus(setup_max_cpus);
 
diff --git a/kernel/pid.c b/kernel/pid.c
index 234ebee29375..bffc5f765080 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -559,7 +559,17 @@ EXPORT_SYMBOL_GPL(pid_vnr);
 
 int kill_cad_pid(int sig, int priv)
 {
-	return kill_pid(cad_pid, sig, priv);
+	struct pid *pid;
+	int ret;
+
+	rcu_read_lock();
+	pid = get_pid(rcu_dereference(cad_pid));
+	rcu_read_unlock();
+
+	ret = kill_pid(pid, sig, priv);
+	put_pid(pid);
+
+	return ret;
 }
 
 pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
@@ -773,11 +783,15 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
 		size_t *lenp, loff_t *ppos)
 {
 	struct pid *new_pid;
+	struct pid *old_pid;
 	pid_t tmp_pid;
 	int r;
 	struct ctl_table tmp_table = *table;
 
-	tmp_pid = pid_vnr(cad_pid);
+	rcu_read_lock();
+	tmp_pid = pid_vnr(rcu_dereference(cad_pid));
+	rcu_read_unlock();
+
 	tmp_table.data = &tmp_pid;
 
 	r = proc_dointvec(&tmp_table, write, buffer, lenp, ppos);
@@ -788,7 +802,13 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
 	if (!new_pid)
 		return -ESRCH;
 
-	put_pid(xchg(&cad_pid, new_pid));
+	old_pid = unrcu_pointer(xchg(&cad_pid, RCU_INITIALIZER(new_pid)));
+	/*
+	 * Wait for cad_pid readers before put_pid().  We cannot use
+	 * call_rcu() here because free_pid() already owns pid->rcu.
+	 */
+	synchronize_rcu();
+	put_pid(old_pid);
 	return 0;
 }
 
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 695c33e75efd..fc191a48c0e9 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -24,7 +24,7 @@
  */
 
 static int C_A_D = 1;
-struct pid *cad_pid;
+struct pid __rcu *cad_pid;
 EXPORT_SYMBOL(cad_pid);
 
 #if defined(CONFIG_ARM)
-- 
2.53.0

Re: [PATCH v2 1/2] pid: deinline kill_cad_pid

[Bradley Morgan]

On July 18, 2026 4:19:59 AM GMT+01:00, "Cen Zhang (Microsoft)"
<blbllhy@gmail.com> wrote:
>Move kill_cad_pid() out of the header without changing behavior. This
>prepares for taking a reference to cad_pid under RCU in the following
>fix.
>
>Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
>Suggested-by: Bradley Morgan <include@grrlz.net>
>Link: https://lore.kernel.org/all/CAGudoHH0vz=1m97EDHQFLLwGJmDPmqWJ+444b7rMiD059drEwQ@mail.gmail.com/
>Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>

LGTM! Please add

Reviewed-by: Bradley Morgan <include@grrlz.net>

>---
>v2:
>  - New preparatory patch to deinline kill_cad_pid().
>
> include/linux/sched/signal.h | 5 +----
> kernel/pid.c                 | 5 +++++
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
>index 584ae88b435e..d45a5476b97d 100644
>--- a/include/linux/sched/signal.h
>+++ b/include/linux/sched/signal.h
>@@ -562,10 +562,7 @@ static inline sigset_t *sigmask_to_save(void)
> 	return res;
> }
> 
>-static inline int kill_cad_pid(int sig, int priv)
>-{
>-	return kill_pid(cad_pid, sig, priv);
>-}
>+int kill_cad_pid(int sig, int priv);
> 
> /* These can be the second arg to send_sig_info/send_group_sig_info.  */
> #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
>diff --git a/kernel/pid.c b/kernel/pid.c
>index f55189a3d07d..234ebee29375 100644
>--- a/kernel/pid.c
>+++ b/kernel/pid.c
>@@ -557,6 +557,11 @@ pid_t pid_vnr(struct pid *pid)
> }
> EXPORT_SYMBOL_GPL(pid_vnr);
> 
>+int kill_cad_pid(int sig, int priv)
>+{
>+	return kill_pid(cad_pid, sig, priv);
>+}
>+
> pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
> 			struct pid_namespace *ns)
> {
>

Thanks!

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Bradley Morgan]

On July 18, 2026 4:20:00 AM GMT+01:00, "Cen Zhang (Microsoft)"
<blbllhy@gmail.com> wrote:
>proc_do_cad_pid() reads the global cad_pid pointer and passes it to
>pid_vnr() without protecting the lifetime of the referenced struct pid.
>A concurrent writer can replace cad_pid and drop the final reference to
>the old struct pid after the reader has loaded the pointer but before
>pid_vnr() has finished dereferencing it, causing a use-after-free.
>
>The sysctl is mode 0600, but access is checked against the owning user
>namespace, so an unprivileged user can reach it via userns, pidns, and a
>proc mount.
>
>Fix this by treating cad_pid as an RCU-protected pointer at both read
>sites and by waiting for a grace period before dropping the old reference
>on the write side.
>
>KASAN crash stack:
>  kernel/pid.c:545 pid_nr_ns()        # reads freed pid->level
>  kernel/pid.c:556 pid_vnr()
>  kernel/pid.c:775 proc_do_cad_pid()
>  fs/proc/proc_sysctl.c proc_sys_call_handler()
>  fs/read_write.c vfs_read()
>  fs/read_write.c __x64_sys_pread64()
>
>Fixes: 9ec52099e4b8 ("[PATCH] replace cad_pid by a struct pid")
>Reported-by: AutonomousCodeSecurity@microsoft.com
>Closes: https://lore.kernel.org/all/20260717210143.4734-1-blbllhy@gmail.com/
>Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
>Reviewed-by: Bradley Morgan <include@grrlz.net>

This tag remains, no need to revoke.

Reviewed-by: Bradley Morgan <include@grrlz.net>


>Cc: stable@vger.kernel.org
>Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
>---
>v2:
>  - Split out kill_cad_pid() deinline into a preparatory patch.
>  - Annotate cad_pid as __rcu and use rcu_dereference().
>  - Protect kill_cad_pid() by taking a pid reference under RCU.
>  - Add a comment explaining why synchronize_rcu() is used instead of
>    call_rcu().
>
> include/linux/sched.h |  2 +-
> init/main.c           |  2 +-
> kernel/pid.c          | 26 +++++++++++++++++++++++---
> kernel/reboot.c       |  2 +-
> 4 files changed, 26 insertions(+), 6 deletions(-)
>
>diff --git a/include/linux/sched.h b/include/linux/sched.h
>index 373bcc0598d1..31ce72b1233c 100644
>--- a/include/linux/sched.h
>+++ b/include/linux/sched.h
>@@ -1767,7 +1767,7 @@ static inline bool is_lazy_mmu_mode_active(void)
> }
> #endif
> 
>-extern struct pid *cad_pid;
>+extern struct pid __rcu *cad_pid;
> 
> /*
>  * Per process flags
>diff --git a/init/main.c b/init/main.c
>index e363232b428b..19a10d0c2760 100644
>--- a/init/main.c
>+++ b/init/main.c
>@@ -1636,7 +1636,7 @@ static noinline void __init kernel_init_freeable(void)
> 	 */
> 	set_mems_allowed(node_states[N_MEMORY]);
> 
>-	cad_pid = get_pid(task_pid(current));
>+	rcu_assign_pointer(cad_pid, get_pid(task_pid(current)));
> 
> 	smp_prepare_cpus(setup_max_cpus);
> 
>diff --git a/kernel/pid.c b/kernel/pid.c
>index 234ebee29375..bffc5f765080 100644
>--- a/kernel/pid.c
>+++ b/kernel/pid.c
>@@ -559,7 +559,17 @@ EXPORT_SYMBOL_GPL(pid_vnr);
> 
> int kill_cad_pid(int sig, int priv)
> {
>-	return kill_pid(cad_pid, sig, priv);
>+	struct pid *pid;
>+	int ret;
>+
>+	rcu_read_lock();
>+	pid = get_pid(rcu_dereference(cad_pid));
>+	rcu_read_unlock();
>+
>+	ret = kill_pid(pid, sig, priv);
>+	put_pid(pid);
>+
>+	return ret;
> }
> 
> pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
>@@ -773,11 +783,15 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
> 		size_t *lenp, loff_t *ppos)
> {
> 	struct pid *new_pid;
>+	struct pid *old_pid;
> 	pid_t tmp_pid;
> 	int r;
> 	struct ctl_table tmp_table = *table;
> 
>-	tmp_pid = pid_vnr(cad_pid);
>+	rcu_read_lock();
>+	tmp_pid = pid_vnr(rcu_dereference(cad_pid));
>+	rcu_read_unlock();
>+
> 	tmp_table.data = &tmp_pid;
> 
> 	r = proc_dointvec(&tmp_table, write, buffer, lenp, ppos);
>@@ -788,7 +802,13 @@ static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffe
> 	if (!new_pid)
> 		return -ESRCH;
> 
>-	put_pid(xchg(&cad_pid, new_pid));
>+	old_pid = unrcu_pointer(xchg(&cad_pid, RCU_INITIALIZER(new_pid)));
>+	/*
>+	 * Wait for cad_pid readers before put_pid().  We cannot use
>+	 * call_rcu() here because free_pid() already owns pid->rcu.
>+	 */
>+	synchronize_rcu();
>+	put_pid(old_pid);
> 	return 0;
> }
> 
>diff --git a/kernel/reboot.c b/kernel/reboot.c
>index 695c33e75efd..fc191a48c0e9 100644
>--- a/kernel/reboot.c
>+++ b/kernel/reboot.c
>@@ -24,7 +24,7 @@
>  */
> 
> static int C_A_D = 1;
>-struct pid *cad_pid;
>+struct pid __rcu *cad_pid;
> EXPORT_SYMBOL(cad_pid);
> 
> #if defined(CONFIG_ARM)
>

Thanks!

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Oleg Nesterov]

On 07/17, Cen Zhang (Microsoft) wrote:
>
>  int kill_cad_pid(int sig, int priv)
>  {
> -	return kill_pid(cad_pid, sig, priv);
> +	struct pid *pid;
> +	int ret;
> +
> +	rcu_read_lock();
> +	pid = get_pid(rcu_dereference(cad_pid));
> +	rcu_read_unlock();
> +
> +	ret = kill_pid(pid, sig, priv);
> +	put_pid(pid);

Hmm. Why do we need get/put_pid() ? I think that

	rcu_read_lock();
	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
	rcu_read_unlock();

	return ret;

should work just fine?

Oleg.

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Bradley Morgan]

On July 18, 2026 2:19:57 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com>
wrote:
>On 07/17, Cen Zhang (Microsoft) wrote:
>>
>>  int kill_cad_pid(int sig, int priv)
>>  {
>> -	return kill_pid(cad_pid, sig, priv);
>> +	struct pid *pid;
>> +	int ret;
>> +
>> +	rcu_read_lock();
>> +	pid = get_pid(rcu_dereference(cad_pid));
>> +	rcu_read_unlock();
>> +
>> +	ret = kill_pid(pid, sig, priv);
>> +	put_pid(pid);
>
>Hmm. Why do we need get/put_pid() ? I think that
>
>	rcu_read_lock();
>	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
>	rcu_read_unlock();
>
>	return ret;
>
>should work just fine?
>
>Oleg.
>
>

On 07/18, Oleg Nesterov wrote:
> Hmm. Why do we need get/put_pid() ? I think that
>
> 	rcu_read_lock();
> 	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
> 	rcu_read_unlock();
>
> 	return ret;
>
> should work just fine?

youre right that the get/put is redundant. kill_pid() doesnt sleep, it just
grabs tasklist_lock, so doing it inside the read section is fine and the
extra ref buys nothing.

but i dont think either form is actually safe, and thats the bit id like
Cen to address. rcu_read_lock() only protects the pid if the freeing side
defers to a grace period. on the cad_pid path it doesnt:

proc_do_cad_pid():
	put_pid(xchg(&cad_pid, new_pid));

put_pid():
	if (refcount_dec_and_test(&pid->count)) {
		pidfs_free_pid(pid);
		kmem_cache_free(ns->pid_cachep, pid);	/* synchronous */
		put_pid_ns(ns);
	}

thats a straight kmem_cache_free, no call_rcu. only free_pid() goes through
call_rcu(delayed_put_pid), and thats dropping the hash reference, not the
reference cad_pid holds. and the pid cache isnt SLAB_TYPESAFE_BY_RCU:

	kmem_cache_create("pid", ...,
		SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT, NULL);

so:

	cpu0 kill_cad_pid		cpu1 proc_do_cad_pid
	rcu_read_lock()
	p = rcu_dereference(cad_pid)
					put_pid(old) // last ref, freed and reused
	kill_pid(p, ...)		// UAF
	rcu_read_unlock()

your form and Cens both hit this. Cens get_pid() is no safer either, its
a refcount_inc on the same p thats already freed, so its a UAF too, not a
more careful variant.

to make the rcu_read_lock() mean anything the writer has to defer. either
proc_do_cad_pid() drops the old pid via call_rcu instead of a bare
put_pid(), or the pid cache goes SLAB_TYPESAFE_BY_RCU and the reader uses a
not zero tryget. with the writer fixed your clean form is exactly right and
the ref stays unnecessary.

its reachable when cad_pid isnt init: root writes a pid, that task exits,
cad_pid ends up holding the last ref, and the next write frees it while a
signal is in flight.


Thanks!

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Oleg Nesterov]

On 07/18, Bradley Morgan wrote:
>
> On July 18, 2026 2:19:57 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com>
>
> > Hmm. Why do we need get/put_pid() ? I think that
> >
> > 	rcu_read_lock();
> > 	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
> > 	rcu_read_unlock();
> >
> > 	return ret;
> >
> > should work just fine?
>
> youre right that the get/put is redundant. kill_pid() doesnt sleep, it just
> grabs tasklist_lock,

(no it doesn't take tasklist)

> but i dont think either form is actually safe, and thats the bit id like
> Cen to address. rcu_read_lock() only protects the pid if the freeing side
> defers to a grace period. on the cad_pid path it doesnt:
>
> proc_do_cad_pid():
> 	put_pid(xchg(&cad_pid, new_pid));

I must have missed something.

But this patch adds synchronize_rcu() before the final put_pid() ?

And. If kill_pid(rcu_dereference(cad_pid)) was not safe under rcu_read_lock(),
then get_pid(rcu_dereference(cad_pid)) would be equally unsafe?

Oleg.

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Bradley Morgan]

On July 18, 2026 3:13:37 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com>
wrote:
>On 07/18, Bradley Morgan wrote:
>>
>> On July 18, 2026 2:19:57 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com>
>>
>> > Hmm. Why do we need get/put_pid() ? I think that
>> >
>> > 	rcu_read_lock();
>> > 	ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
>> > 	rcu_read_unlock();
>> >
>> > 	return ret;
>> >
>> > should work just fine?
>>
>> youre right that the get/put is redundant. kill_pid() doesnt sleep, it
>just
>> grabs tasklist_lock,
>
>(no it doesn't take tasklist)
>
>> but i dont think either form is actually safe, and thats the bit id like
>> Cen to address. rcu_read_lock() only protects the pid if the freeing
>side
>> defers to a grace period. on the cad_pid path it doesnt:
>>
>> proc_do_cad_pid():
>> 	put_pid(xchg(&cad_pid, new_pid));
>
>I must have missed something.
>
>But this patch adds synchronize_rcu() before the final put_pid() ?
>
>And. If kill_pid(rcu_dereference(cad_pid)) was not safe under
>rcu_read_lock(),
>then get_pid(rcu_dereference(cad_pid)) would be equally unsafe?
>
>Oleg.
>
>


oops, my bad. Brainfog.

Thanks!

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Mateusz Guzik]

On Sat, Jul 18, 2026 at 3:20 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 07/17, Cen Zhang (Microsoft) wrote:
> >
> >  int kill_cad_pid(int sig, int priv)
> >  {
> > -     return kill_pid(cad_pid, sig, priv);
> > +     struct pid *pid;
> > +     int ret;
> > +
> > +     rcu_read_lock();
> > +     pid = get_pid(rcu_dereference(cad_pid));
> > +     rcu_read_unlock();
> > +
> > +     ret = kill_pid(pid, sig, priv);
> > +     put_pid(pid);
>
> Hmm. Why do we need get/put_pid() ? I think that
>
>         rcu_read_lock();
>         ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
>         rcu_read_unlock();
>
>         return ret;
>
> should work just fine?
>

I suggested taking the ref as a future-proofing measure to *not*
impose any requirements on kill_pid.

Admittedly when responding to v1 I blindly assumed pid is always
rcu-freed, my bad.

Given the arcane nature of the proc file at hand and 0 performance
concerns, I think the easiest way out is to give it a spinlock to
protect access to cad_pid as opposed to trying to play any rcu and
lockless games.

The separate patch which deinlines kill_cad_pid should be folded into
the actual fix, no point having 2 patches for this one.

Re: [PATCH v2 2/2] pid: fix cad_pid use-after-free race

[Oleg Nesterov]

On 07/18, Mateusz Guzik wrote:
>
> On Sat, Jul 18, 2026 at 3:20 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 07/17, Cen Zhang (Microsoft) wrote:
> > >
> > >  int kill_cad_pid(int sig, int priv)
> > >  {
> > > -     return kill_pid(cad_pid, sig, priv);
> > > +     struct pid *pid;
> > > +     int ret;
> > > +
> > > +     rcu_read_lock();
> > > +     pid = get_pid(rcu_dereference(cad_pid));
> > > +     rcu_read_unlock();
> > > +
> > > +     ret = kill_pid(pid, sig, priv);
> > > +     put_pid(pid);
> >
> > Hmm. Why do we need get/put_pid() ? I think that
> >
> >         rcu_read_lock();
> >         ret = kill_pid(rcu_dereference(cad_pid), sig, priv);
> >         rcu_read_unlock();
> >
> >         return ret;
> >
> > should work just fine?
> >
>
> I suggested taking the ref as a future-proofing measure to *not*
> impose any requirements on kill_pid.
>
> Admittedly when responding to v1 I blindly assumed pid is always
> rcu-freed, my bad.
>
> Given the arcane nature of the proc file at hand and 0 performance
> concerns, I think the easiest way out is to give it a spinlock to
> protect access to cad_pid as opposed to trying to play any rcu and
> lockless games.

Just in case, I am fine either way. I'd personaly prefer to play rcu
games in this case, but I won't insist.

I've sent my comment only because get_pid + put_pid + kill_pid() outside
of rcu_read_lock() look really confusing to me.

As for requirements on kill_pid... See for example kill_pid_info().
kill_pid/kill_pid_info/etc are all rcu safe wrt "struct pid *pid" arg.

Oleg.