[PATCH] introduce put_pid_rcu() to fix unsafe put_pid(vc->vt_pid)

[PATCH] introduce put_pid_rcu() to fix unsafe put_pid(vc->vt_pid)

[Oleg Nesterov]

Compile tested.

drivers/char/vt_ioctl.c changes vc->vt_pid doing

	put_pid(xchg(&vc->vt_pid, ...));

This is unsafe, put_pid() can actually free the memory while vc->vt_pid is
still used by kill_pid(vc->vt_pid).

Add a new helper, put_pid_rcu(), which frees "struct pid" via rcu callback
and convert vt_ioctl.c to use it.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

 include/linux/pid.h     |    2 ++
 kernel/pid.c            |   18 ++++++++++++++++++
 drivers/char/vt_ioctl.c |   22 ++++++++++++++++++----
 3 files changed, 38 insertions(+), 4 deletions(-)

--- 19-rc6/include/linux/pid.h~vt_pid	2006-10-22 18:24:02.000000000 +0400
+++ 19-rc6/include/linux/pid.h	2006-12-02 01:38:59.000000000 +0300
@@ -64,6 +64,8 @@ static inline struct pid *get_pid(struct
 }
 
 extern void FASTCALL(put_pid(struct pid *pid));
+extern void put_pid_rcu(struct pid *pid);
+
 extern struct task_struct *FASTCALL(pid_task(struct pid *pid, enum pid_type));
 extern struct task_struct *FASTCALL(get_pid_task(struct pid *pid,
 						enum pid_type));
--- 19-rc6/kernel/pid.c~vt_pid	2006-10-22 18:24:03.000000000 +0400
+++ 19-rc6/kernel/pid.c	2006-12-02 01:35:15.000000000 +0300
@@ -196,6 +196,24 @@ fastcall void free_pid(struct pid *pid)
 	call_rcu(&pid->rcu, delayed_put_pid);
 }
 
+static void delayed_free_pid(struct rcu_head *rhp)
+{
+	struct pid *pid = container_of(rhp, struct pid, rcu);
+	kmem_cache_free(pid_cachep, pid);
+}
+
+void put_pid_rcu(struct pid *pid)
+{
+	if (!pid)
+		return;
+	/*
+	 * ->count can't go to 0 unless delayed_put_pid() was already
+	 * called (RCU owns a reference), so we can re-use pid->rcu.
+	 */
+	if (atomic_dec_and_test(&pid->count))
+		call_rcu(&pid->rcu, delayed_free_pid);
+}
+
 struct pid *alloc_pid(void)
 {
 	struct pid *pid;
--- 19-rc6/drivers/char/vt_ioctl.c~vt_pid	2006-10-22 18:23:58.000000000 +0400
+++ 19-rc6/drivers/char/vt_ioctl.c	2006-12-02 02:44:48.000000000 +0300
@@ -358,6 +358,20 @@ do_unimap_ioctl(int cmd, struct unimapde
 	return 0;
 }
 
+static inline void set_vt_pid(struct vc_data *vc, struct pid *pid)
+{
+	put_pid_rcu(xchg(&vc->vt_pid, pid));
+}
+
+static int kill_vt_pid(struct vc_data *vc, int sig)
+{
+	int ret;
+	rcu_read_lock();
+	ret = kill_pid(rcu_dereference(vc->vt_pid), sig, 1);
+	rcu_read_unlock();
+	return ret;
+}
+
 /*
  * We handle the console-specific ioctl's here.  We allow the
  * capability to modify any console, not just the fg_console. 
@@ -672,7 +686,7 @@ int vt_ioctl(struct tty_struct *tty, str
 		vc->vt_mode = tmp;
 		/* the frsig is ignored, so we set it to 0 */
 		vc->vt_mode.frsig = 0;
-		put_pid(xchg(&vc->vt_pid, get_pid(task_pid(current))));
+		set_vt_pid(vc, get_pid(task_pid(current)));
 		/* no switch is required -- saw@shade.msu.ru */
 		vc->vt_newvt = -1;
 		release_console_sem();
@@ -1063,7 +1077,7 @@ void reset_vc(struct vc_data *vc)
 	vc->vt_mode.relsig = 0;
 	vc->vt_mode.acqsig = 0;
 	vc->vt_mode.frsig = 0;
-	put_pid(xchg(&vc->vt_pid, NULL));
+	set_vt_pid(vc, NULL);
 	vc->vt_newvt = -1;
 	if (!in_interrupt())    /* Via keyboard.c:SAK() - akpm */
 		reset_palette(vc);
@@ -1114,7 +1128,7 @@ static void complete_change_console(stru
 		 * tell us if the process has gone or something else
 		 * is awry
 		 */
-		if (kill_pid(vc->vt_pid, vc->vt_mode.acqsig, 1) != 0) {
+		if (kill_vt_pid(vc, vc->vt_mode.acqsig) != 0) {
 		/*
 		 * The controlling process has died, so we revert back to
 		 * normal operation. In this case, we'll also change back
@@ -1174,7 +1188,7 @@ void change_console(struct vc_data *new_
 		 * tell us if the process has gone or something else
 		 * is awry
 		 */
-		if (kill_pid(vc->vt_pid, vc->vt_mode.relsig, 1) == 0) {
+		if (kill_vt_pid(vc, vc->vt_mode.relsig) == 0) {
 			/*
 			 * It worked. Mark the vt to switch to and
 			 * return. The process needs to send us a

Re: [PATCH] introduce put_pid_rcu() to fix unsafe put_pid(vc->vt_pid)

[Andrew Morton]

On Sat, 2 Dec 2006 02:48:26 +0300
Oleg Nesterov <oleg@tv-sign.ru> wrote:

> drivers/char/vt_ioctl.c changes vc->vt_pid doing
> 
> 	put_pid(xchg(&vc->vt_pid, ...));
> 
> This is unsafe, put_pid() can actually free the memory while vc->vt_pid is
> still used by kill_pid(vc->vt_pid).
> 
> Add a new helper, put_pid_rcu(), which frees "struct pid" via rcu callback
> and convert vt_ioctl.c to use it.
> 


I'm a bit reluctant to go adding more tricky infrastructure (especially
100% undocumented infrastructure) on behalf of a single usage site in a
place as creepy as the VT ioctl code.

If we envisage future users of this infrastructure (and if it gets
documented) then OK.  Otherwise I'd rather just stick another bandaid into
the vt code.  Can we add some locking there, or change it to use a
task_struct* or something?

Re: [PATCH] introduce put_pid_rcu() to fix unsafe put_pid(vc->vt_pid)

[Oleg Nesterov]

On 12/03, Andrew Morton wrote:
>
> On Sat, 2 Dec 2006 02:48:26 +0300
> Oleg Nesterov <oleg@tv-sign.ru> wrote:
> 
> > drivers/char/vt_ioctl.c changes vc->vt_pid doing
> > 
> > 	put_pid(xchg(&vc->vt_pid, ...));
> > 
> > This is unsafe, put_pid() can actually free the memory while vc->vt_pid is
> > still used by kill_pid(vc->vt_pid).
> > 
> > Add a new helper, put_pid_rcu(), which frees "struct pid" via rcu callback
> > and convert vt_ioctl.c to use it.
> > 
> 
> 
> I'm a bit reluctant to go adding more tricky infrastructure (especially
> 100% undocumented infrastructure) on behalf of a single usage site in a
> place as creepy as the VT ioctl code.
> If we envisage future users of this infrastructure (and if it gets
> documented) then OK.

It is a shame we can't use "struct pid*" lockless, note that "struct pid"
itself is rcu-protected. I hope we can find another usage for put_pid_rcu
(in fact I suggested it before, but didn't have a reason). However, I don't
see any other example immediately.

>                       Otherwise I'd rather just stick another bandaid into
> the vt code.  Can we add some locking there,

Yes, this is possible, and probably we should do just this.

>                                               or change it to use a
> task_struct* or something?

I don't think this is good. It was converted from task_struct* to pid*.

Eric, what do you think?

Oleg.

Re: [PATCH] introduce put_pid_rcu() to fix unsafe put_pid(vc->vt_pid)

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

>> task_struct* or something?
>
> I don't think this is good. It was converted from task_struct* to pid*.
>
> Eric, what do you think?

I think I have a fix that uses the proper locking sitting in my queue that
I haven't pushed because I have been got to look at just about every
irq but present in 2.6.19-rcX.  Then for some reason I had this stupid
usb debug cable sitting on my desk and since I can't stand useful
things going unused I just wrote a driver for that :)

Anyway with a little luck I should be working on the pid namespace and
this stuff later today so I will try and send out the proper patch.

Not that I'm really opposed to this infrastructure but I'd like to
avoid it until we really need it.

Eric

Re: [PATCH] introduce put_pid_rcu() to fix unsafe put_pid(vc->vt_pid)

[Oleg Nesterov]

On 12/03, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > Eric, what do you think?
> 
> Anyway with a little luck I should be working on the pid namespace and
> this stuff later today so I will try and send out the proper patch.
> 
> Not that I'm really opposed to this infrastructure but I'd like to
> avoid it until we really need it.

Great! please CC me :)

Oleg.