[PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Sergey Senozhatsky]

Commit 4221a9918e38b7494cee341dda7b7b4bb8c04bde "Add RCU check for 
find_task_by_vpid()" introduced rcu_lockdep_assert to find_task_by_pid_ns.
Assertion failed in sys_ioprio_get. The patch is fixing assertion
failure in ioprio_set as well. 

 ===================================================
 [ INFO: suspicious rcu_dereference_check() usage. ]
 ---------------------------------------------------
 kernel/pid.c:419 invoked rcu_dereference_check() without protection!
 
 rcu_scheduler_active = 1, debug_locks = 0
 1 lock held by iotop/4254:
 #0:  (tasklist_lock){.?.?..}, at: [<ffffffff811104b4>] sys_ioprio_get+0x22/0x2da
 
 stack backtrace:
 Pid: 4254, comm: iotop Not tainted
 Call Trace:
 [<ffffffff810656f2>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffff81053c67>] find_task_by_pid_ns+0x4f/0x68
 [<ffffffff81053c9d>] find_task_by_vpid+0x1d/0x1f
 [<ffffffff811104e2>] sys_ioprio_get+0x50/0x2da
 [<ffffffff81002182>] system_call_fastpath+0x16/0x1b
 

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

diff --git a/fs/ioprio.c b/fs/ioprio.c
index 748cfb9..666343d 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -113,8 +113,11 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
 		case IOPRIO_WHO_PROCESS:
 			if (!who)
 				p = current;
-			else
+			else {
+				rcu_read_lock();
 				p = find_task_by_vpid(who);
+				rcu_read_unlock();
+			}
 			if (p)
 				ret = set_task_ioprio(p, ioprio);
 			break;
@@ -202,8 +205,11 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
 		case IOPRIO_WHO_PROCESS:
 			if (!who)
 				p = current;
-			else
+			else {
+				rcu_read_lock();
 				p = find_task_by_vpid(who);
+				rcu_read_unlock();
+			}
 			if (p)
 				ret = get_task_ioprio(p);
 			break;

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Paul E. McKenney]

On Fri, Oct 29, 2010 at 03:55:50PM +0300, Sergey Senozhatsky wrote:
> Commit 4221a9918e38b7494cee341dda7b7b4bb8c04bde "Add RCU check for 
> find_task_by_vpid()" introduced rcu_lockdep_assert to find_task_by_pid_ns.
> Assertion failed in sys_ioprio_get. The patch is fixing assertion
> failure in ioprio_set as well. 
> 
>  ===================================================
>  [ INFO: suspicious rcu_dereference_check() usage. ]
>  ---------------------------------------------------
>  kernel/pid.c:419 invoked rcu_dereference_check() without protection!
> 
>  rcu_scheduler_active = 1, debug_locks = 0
>  1 lock held by iotop/4254:
>  #0:  (tasklist_lock){.?.?..}, at: [<ffffffff811104b4>] sys_ioprio_get+0x22/0x2da
> 
>  stack backtrace:
>  Pid: 4254, comm: iotop Not tainted
>  Call Trace:
>  [<ffffffff810656f2>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffff81053c67>] find_task_by_pid_ns+0x4f/0x68
>  [<ffffffff81053c9d>] find_task_by_vpid+0x1d/0x1f
>  [<ffffffff811104e2>] sys_ioprio_get+0x50/0x2da
>  [<ffffffff81002182>] system_call_fastpath+0x16/0x1b
> 
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> ---
> 
> diff --git a/fs/ioprio.c b/fs/ioprio.c
> index 748cfb9..666343d 100644
> --- a/fs/ioprio.c
> +++ b/fs/ioprio.c
> @@ -113,8 +113,11 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)

Interesting...

The task-list lock is read-held at this point, which should mean that
the PID mapping cannot change.  The lockdep_tasklist_lock_is_held()
function does lockdep_is_held(&tasklist_lock), which must therefore
only be checking for write-holding the lock.  The fix would be to
make lockdep_tasklist_lock_is_held() check for either read-holding or
write-holding tasklist lock.

Or is there some subtle reason that read-holding the tasklist lock is
not sufficient?

							Thanx, Paul

>  		case IOPRIO_WHO_PROCESS:
>  			if (!who)
>  				p = current;
> -			else
> +			else {
> +				rcu_read_lock();
>  				p = find_task_by_vpid(who);
> +				rcu_read_unlock();
> +			}
>  			if (p)
>  				ret = set_task_ioprio(p, ioprio);
>  			break;
> @@ -202,8 +205,11 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
>  		case IOPRIO_WHO_PROCESS:
>  			if (!who)
>  				p = current;
> -			else
> +			else {
> +				rcu_read_lock();
>  				p = find_task_by_vpid(who);
> +				rcu_read_unlock();
> +			}
>  			if (p)
>  				ret = get_task_ioprio(p);
>  			break;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]

On (10/29/10 13:16), Paul E. McKenney wrote:
> On Fri, Oct 29, 2010 at 03:55:50PM +0300, Sergey Senozhatsky wrote:
> > Commit 4221a9918e38b7494cee341dda7b7b4bb8c04bde "Add RCU check for 
> > find_task_by_vpid()" introduced rcu_lockdep_assert to find_task_by_pid_ns.
> > Assertion failed in sys_ioprio_get. The patch is fixing assertion
> > failure in ioprio_set as well. 
> > 
> >  ===================================================
> >  [ INFO: suspicious rcu_dereference_check() usage. ]
> >  ---------------------------------------------------
> >  kernel/pid.c:419 invoked rcu_dereference_check() without protection!
> > 
> >  rcu_scheduler_active = 1, debug_locks = 0
> >  1 lock held by iotop/4254:
> >  #0:  (tasklist_lock){.?.?..}, at: [<ffffffff811104b4>] sys_ioprio_get+0x22/0x2da
> > 
> >  stack backtrace:
> >  Pid: 4254, comm: iotop Not tainted
> >  Call Trace:
> >  [<ffffffff810656f2>] lockdep_rcu_dereference+0xaa/0xb2
> >  [<ffffffff81053c67>] find_task_by_pid_ns+0x4f/0x68
> >  [<ffffffff81053c9d>] find_task_by_vpid+0x1d/0x1f
> >  [<ffffffff811104e2>] sys_ioprio_get+0x50/0x2da
> >  [<ffffffff81002182>] system_call_fastpath+0x16/0x1b
> > 
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > 
> > ---
> > 
> > diff --git a/fs/ioprio.c b/fs/ioprio.c
> > index 748cfb9..666343d 100644
> > --- a/fs/ioprio.c
> > +++ b/fs/ioprio.c
> > @@ -113,8 +113,11 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
> 
> Interesting...
> 
> The task-list lock is read-held at this point, which should mean that
> the PID mapping cannot change.  The lockdep_tasklist_lock_is_held()
> function does lockdep_is_held(&tasklist_lock), which must therefore
> only be checking for write-holding the lock.  The fix would be to
> make lockdep_tasklist_lock_is_held() check for either read-holding or
> write-holding tasklist lock.
> 
> Or is there some subtle reason that read-holding the tasklist lock is
> not sufficient?
>

Hello,

On the kernel/pid.c side we have the requirement that 
find_task_by_vpid -> find_task_by_pid_ns

should be called with rcu_read_lock.

/*
 * Must be called under rcu_read_lock().
 */
struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)
{
	rcu_lockdep_assert(rcu_read_lock_held());
	return pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
}

 
Should it be changed to (let's say)

struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)
{
-	rcu_lockdep_assert(rcu_read_lock_held());
+	rcu_lockdep_assert(rcu_read_lock_held() || lockdep_tasklist_lock_is_held());
	return pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
}


	Sergey

> 							Thanx, Paul
> 
> >  		case IOPRIO_WHO_PROCESS:
> >  			if (!who)
> >  				p = current;
> > -			else
> > +			else {
> > +				rcu_read_lock();
> >  				p = find_task_by_vpid(who);
> > +				rcu_read_unlock();
> > +			}
> >  			if (p)
> >  				ret = set_task_ioprio(p, ioprio);
> >  			break;
> > @@ -202,8 +205,11 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
> >  		case IOPRIO_WHO_PROCESS:
> >  			if (!who)
> >  				p = current;
> > -			else
> > +			else {
> > +				rcu_read_lock();
> >  				p = find_task_by_vpid(who);
> > +				rcu_read_unlock();
> > +			}
> >  			if (p)
> >  				ret = get_task_ioprio(p);
> >  			break;
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Tetsuo Handa]

Sergey Senozhatsky wrote:
> On (10/29/10 13:16), Paul E. McKenney wrote:
> > Interesting...
> > 
> > The task-list lock is read-held at this point, which should mean that
> > the PID mapping cannot change.  The lockdep_tasklist_lock_is_held()
> > function does lockdep_is_held(&tasklist_lock), which must therefore
> > only be checking for write-holding the lock.  The fix would be to
> > make lockdep_tasklist_lock_is_held() check for either read-holding or
> > write-holding tasklist lock.
> > 
> > Or is there some subtle reason that read-holding the tasklist lock is
> > not sufficient?

This was discussed in the thread at http://kerneltrap.org/mailarchive/linux-kernel/2009/12/10/4517520 .
Quoting from one of posts in that thead http://kerneltrap.org/mailarchive/linux-kernel/2010/2/8/4536388

| Usually tasklist gives enough protection, but if copy_process() fails
| it calls free_pid() lockless and does call_rcu(delayed_put_pid().
| This means, without rcu lock find_pid_ns() can't scan the hash table
| safely.

And now the patch that adds

	rcu_lockdep_assert(rcu_read_lock_held());

was merged in accordance with that comment.
Therefore, I thing below change is not good.

> Should it be changed to (let's say)
> 
> struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)
> {
> -	rcu_lockdep_assert(rcu_read_lock_held());
> +	rcu_lockdep_assert(rcu_read_lock_held() || lockdep_tasklist_lock_is_held());
> 	return pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
> }

Regards.

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Paul E. McKenney]

On Sat, Oct 30, 2010 at 10:14:23PM +0900, Tetsuo Handa wrote:
> Sergey Senozhatsky wrote:
> > On (10/29/10 13:16), Paul E. McKenney wrote:
> > > Interesting...
> > > 
> > > The task-list lock is read-held at this point, which should mean that
> > > the PID mapping cannot change.  The lockdep_tasklist_lock_is_held()
> > > function does lockdep_is_held(&tasklist_lock), which must therefore
> > > only be checking for write-holding the lock.  The fix would be to
> > > make lockdep_tasklist_lock_is_held() check for either read-holding or
> > > write-holding tasklist lock.
> > > 
> > > Or is there some subtle reason that read-holding the tasklist lock is
> > > not sufficient?
> 
> This was discussed in the thread at http://kerneltrap.org/mailarchive/linux-kernel/2009/12/10/4517520 .
> Quoting from one of posts in that thead http://kerneltrap.org/mailarchive/linux-kernel/2010/2/8/4536388
> 
> | Usually tasklist gives enough protection, but if copy_process() fails
> | it calls free_pid() lockless and does call_rcu(delayed_put_pid().
> | This means, without rcu lock find_pid_ns() can't scan the hash table
> | safely.
> 
> And now the patch that adds
> 
> 	rcu_lockdep_assert(rcu_read_lock_held());
> 
> was merged in accordance with that comment.
> Therefore, I thing below change is not good.
> 
> > Should it be changed to (let's say)
> > 
> > struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace *ns)
> > {
> > -	rcu_lockdep_assert(rcu_read_lock_held());
> > +	rcu_lockdep_assert(rcu_read_lock_held() || lockdep_tasklist_lock_is_held());
> > 	return pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
> > }

So we should remove the lockdep_tasklist_lock_is_held() and then
apply Sergey's patch, correct?

							Thanx, Paul

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Tetsuo Handa]

Paul E. McKenney wrote:
> So we should remove the lockdep_tasklist_lock_is_held() and then
> apply Sergey's patch, correct?

Yes. rcu_lockdep_assert(rcu_read_lock_held()) in find_task_by_pid_ns()
is correct and callers need to use rcu_read_lock().

As of 2.6.32, there are 20+ users who missed rcu_read_lock().
So, similar reports will be posted like popcorn.
http://kerneltrap.org/mailarchive/linux-kernel/2009/12/11/4518016

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Paul E. McKenney]

On Sun, Oct 31, 2010 at 08:33:35AM +0900, Tetsuo Handa wrote:
> Paul E. McKenney wrote:
> > So we should remove the lockdep_tasklist_lock_is_held() and then
> > apply Sergey's patch, correct?
> 
> Yes. rcu_lockdep_assert(rcu_read_lock_held()) in find_task_by_pid_ns()
> is correct and callers need to use rcu_read_lock().
> 
> As of 2.6.32, there are 20+ users who missed rcu_read_lock().
> So, similar reports will be posted like popcorn.
> http://kerneltrap.org/mailarchive/linux-kernel/2009/12/11/4518016

OK, let's see how these are in 2.6.36...   The summary is that almost
all of the popcorn kernels have already popped.

> Users missing read_lock(&tasklist_lock) when calling find_task_by_vpid():
> 
>   get_net_ns_by_pid() in net/core/net_namespace.c

	Fixed with rcu_read_lock().

>   seq_print_userip_objs() in kernel/trace/trace_output.c

	Fixed with rcu_read_lock().

>   fill_pid() in kernel/taskstats.c

	This one seems to be named fill_stats_for_pid(), and it
	is fixed with rcu_read_lock().

>   fill_tgid() in kernel/taskstats.c

	This one seems to be named fill_stats_for_tgid(), and it
	is fixed with rcu_read_lock().

>   futex_find_get_task() in kernel/futex.c

	Fixed with rcu_read_lock().

>   SYSCALL_DEFINE3(get_robust_list) in kernel/futex.c

	Fixed with rcu_read_lock().

>   compat_sys_get_robust_list() in kernel/futex_compat.c

	Fixed with rcu_read_lock().

>   ptrace_get_task_struct() in kernel/ptrace.c

	Fixed with rcu_read_lock().

>   do_send_specific() in kernel/signal.c

	Fixed with rcu_read_lock().

>   find_get_context() in kernel/perf_event.c

	Fixed with rcu_read_lock() plus code motion.

>   posix_cpu_clock_get() in kernel/posix-cpu-timers.c

	Fixed with rcu_read_lock().

>   good_sigevent() in kernel/posix-timers.c

	Fixed with rcu_read_lock() in caller.

>   attach_task_by_pid() in kernel/cgroup.c

	Fixed with rcu_read_lock().

>   SYSCALL_DEFINE1(getpgid) in kernel/sys.c

	Fixed with rcu_read_lock().

>   SYSCALL_DEFINE1(getsid) in kernel/sys.c

	Fixed with rcu_read_lock().

>   do_sched_setscheduler() in kernel/sched.c

	Fixed with rcu_read_lock().

> 
> Users missing rcu_read_lock() when calling find_task_by_vpid():
> 
>   SYSCALL_DEFINE3(ioprio_set) in fs/ioprio.c

	Patches in flight.

>   SYSCALL_DEFINE2(ioprio_get) in fs/ioprio.c

	Patches in flight.

>   cap_get_target_pid() in kernel/capability.c

	Fixed with rcu_read_lock().

>   audit_prepare_user_tty() in kernel/audit.c

	Fixed with rcu_read_lock().

>   audit_receive_msg() in kernel/audit.c

	Fixed with rcu_read_lock(), two instances.

>   check_clock() in kernel/posix-cpu-timers.c

	This one has read_lock(&tasklist_lock).

>   posix_cpu_timer_create() in kernel/posix-cpu-timers.c

	This one has read_lock(&tasklist_lock).

>   SYSCALL_DEFINE3(setpriority) in kernel/sys.c

	This one has both read_lock(&tasklist_lock) and rcu_read_lock().
	Both belt -and- suspenders...

>   SYSCALL_DEFINE2(getpriority) in kernel/sys.c

	Ditto.

>   SYSCALL_DEFINE2(setpgid) in kernel/sys.c

	This one has both write_lock_irq(&tasklist_lock) and
	rcu_read_lock().  -Serious- belt along with the suspenders...

>   SYSCALL_DEFINE1(sched_getscheduler) in kernel/sched.c

	Fixed with rcu_read_lock().

>   SYSCALL_DEFINE2(sched_getparam) in kernel/sched.c

	Fixed with rcu_read_lock().

>   sched_setaffinity() in kernel/sched.c

	Fixed with rcu_read_lock().

>   sched_getaffinity() in kernel/sched.c

	Fixed with rcu_read_lock().

>   SYSCALL_DEFINE2(sched_rr_get_interval) in kernel/sched.c

	Fixed with rcu_read_lock().

>   tomoyo_is_select_one() in security/tomoyo/common.c

	This one has both read_lock(&tasklist_lock) and rcu_read_lock().
	Both belt -and- suspenders...  (Also renamed to tomoyo_select_one().)

>   tomoyo_read_pid() in security/tomoyo/common.c

	This one has both read_lock(&tasklist_lock) and rcu_read_lock().
	Both belt -and- suspenders...

>   SYSCALL_DEFINE6(move_pages) in mm/migrate.c

	This one does read_lock(&tasklist_lock).

>   SYSCALL_DEFINE4(migrate_pages) in mm/mempolicy.c

	No call to find_task_by_vpid() in migrate_pages() anymore,
	but there is one protected by read_lock(&tasklist_lock) in
	move_pages().

>   find_process_by_pid() in arch/mips/kernel/mips-mt-fpaff.c

	No change, so let's check the callers:

	mipsmt_sys_sched_setaffinity(): rcu_read_lock().
	mipsmt_sys_sched_getaffinity(): read_lock(&tasklist_lock).
	do_sched_setscheduler(): rcu_read_lock().
	sched_getscheduler(): rcu_read_lock().
	sched_getparam(): rcu_read_lock().
	sched_setaffinity(): rcu_read_lock().
	sched_getaffinity(); rcu_read_lock().
	sched_rr_get_interval(): rcu_read_lock().

	So all of these are protected.

>   pfm_get_task() in arch/ia64/kernel/perfmon.c

	This one does read_lock(&tasklist_lock).

>   cxn_pin_by_pid() in arch/frv/mm/mmu-context.c

	This one does read_lock(&tasklist_lock).

> 
> Users missing read_lock(&tasklist_lock) when calling find_task_by_pid_ns():
> 
>   rest_init() in init/main.c

	Fixed with rcu_read_lock().

>   proc_pid_lookup() in fs/proc/base.c

	Fixed with rcu_read_lock().

>   proc_task_lookup() in fs/proc/base.c

	Fixed with rcu_read_lock().

>   first_tid() in fs/proc/base.c

	Fixed with rcu_read_lock().

>   getthread() in kernel/kgdb.c

	This is now in kernel/debug/gdbstub.c...  It looks to need
	an rcu_read_lock().  It has a comment, but...

>   mconsole_stack() in arch/um/drivers/mconsole_kern.c

	This one still needs help.

> 
> Users missing rcu_read_lock() when calling find_task_by_pid_ns():
> 
>   rest_init() in init/main.c

	Fixed with rcu_read_lock().

>   getthread() in kernel/kgdb.c

	This one still needs help.

>   mconsole_stack() in arch/um/drivers/mconsole_kern.c

	This one still needs help.

So mostly there...

							Thanx, Paul

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Tetsuo Handa]

Hello.

Paul E. McKenney wrote:
> > Users missing rcu_read_lock() when calling find_task_by_vpid():
> > 
> >   check_clock() in kernel/posix-cpu-timers.c
> 
> 	This one has read_lock(&tasklist_lock).
> 
Excuse me. Holding tasklist_lock lock does not help.
We must call rcu_read_lock() explicitly.
That's why 9728e5d6 "kernel/pid.c: update comment on find_task_by_pid_ns" was made.

I think there are users who needlessly call read_lock(&tasklist_lock)
when they can use rcu_read_lock() instead.
But I don't know when to use read_lock(&tasklist_lock).

If read_lock(&tasklist_lock) is needed only when we want to access
the "struct task_struct" after rcu_read_unlock(), maybe it is cleaner to
use a helper like

struct task_struct *find_task_and_get(pid_t pid)
{
	struct task_struct *task;
	read_lock(&tasklist_lock);
	rcu_read_lock();
	task = find_task_by_vpid(pid);
	rcu_read_unlock();
	if (task)
		get_task_struct(task);
	read_unlock(&tasklist_lock);
	return task;
}

and hide tasklist_lock.

Regards.

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Paul E. McKenney]

On Mon, Nov 08, 2010 at 07:04:43AM +0900, Tetsuo Handa wrote:
> Hello.
> 
> Paul E. McKenney wrote:
> > > Users missing rcu_read_lock() when calling find_task_by_vpid():
> > > 
> > >   check_clock() in kernel/posix-cpu-timers.c
> > 
> > 	This one has read_lock(&tasklist_lock).
> > 
> Excuse me. Holding tasklist_lock lock does not help.
> We must call rcu_read_lock() explicitly.
> That's why 9728e5d6 "kernel/pid.c: update comment on find_task_by_pid_ns" was made.

OK, good point, there are a few more kernels of unpopped corn here.

> I think there are users who needlessly call read_lock(&tasklist_lock)
> when they can use rcu_read_lock() instead.
> But I don't know when to use read_lock(&tasklist_lock).
> 
> If read_lock(&tasklist_lock) is needed only when we want to access
> the "struct task_struct" after rcu_read_unlock(), maybe it is cleaner to
> use a helper like
> 
> struct task_struct *find_task_and_get(pid_t pid)
> {
> 	struct task_struct *task;
> 	read_lock(&tasklist_lock);
> 	rcu_read_lock();
> 	task = find_task_by_vpid(pid);
> 	rcu_read_unlock();
> 	if (task)
> 		get_task_struct(task);
> 	read_unlock(&tasklist_lock);
> 	return task;
> }
> 
> and hide tasklist_lock.

This makes a lot of sense to me!  That said, most of the current
open-coded variants of your find_task_and_get() seem to have the
rcu_read_unlock() after the get_task_struct() rather than before.  But I
don't claim to understand the locking design of this part of the kernel
well enough to say which is the best approach.

So, either way, will you be submitting the patches for this?

							Thanx, Paul

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

On (11/07/10 19:01), Paul E. McKenney wrote:
> On Mon, Nov 08, 2010 at 07:04:43AM +0900, Tetsuo Handa wrote:
> > Hello.
> > 
> > Paul E. McKenney wrote:
> > > > Users missing rcu_read_lock() when calling find_task_by_vpid():
> > > > 
> > > >   check_clock() in kernel/posix-cpu-timers.c
> > > 
> > > 	This one has read_lock(&tasklist_lock).
> > > 
> > Excuse me. Holding tasklist_lock lock does not help.
> > We must call rcu_read_lock() explicitly.
> > That's why 9728e5d6 "kernel/pid.c: update comment on find_task_by_pid_ns" was made.
> 
> OK, good point, there are a few more kernels of unpopped corn here.
>

Hello,
I prepared a patch for posix-cpu-timers. 

[PATCH] posix-cpu-timers: rcu_read_lock/unlock protect find_task_by_vpid call
Reviewed-by: Oleg Nesterov <oleg@redhat.com>

http://lkml.org/lkml/2010/11/3/257


	Sergey

 
> > I think there are users who needlessly call read_lock(&tasklist_lock)
> > when they can use rcu_read_lock() instead.
> > But I don't know when to use read_lock(&tasklist_lock).
> > 
> > If read_lock(&tasklist_lock) is needed only when we want to access
> > the "struct task_struct" after rcu_read_unlock(), maybe it is cleaner to
> > use a helper like
> > 
> > struct task_struct *find_task_and_get(pid_t pid)
> > {
> > 	struct task_struct *task;
> > 	read_lock(&tasklist_lock);
> > 	rcu_read_lock();
> > 	task = find_task_by_vpid(pid);
> > 	rcu_read_unlock();
> > 	if (task)
> > 		get_task_struct(task);
> > 	read_unlock(&tasklist_lock);
> > 	return task;
> > }
> > 
> > and hide tasklist_lock.
> 
> This makes a lot of sense to me!  That said, most of the current
> open-coded variants of your find_task_and_get() seem to have the
> rcu_read_unlock() after the get_task_struct() rather than before.  But I
> don't claim to understand the locking design of this part of the kernel
> well enough to say which is the best approach.
> 
> So, either way, will you be submitting the patches for this?
> 
> 							Thanx, Paul
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Paul E. McKenney]

On Mon, Nov 08, 2010 at 12:28:17PM +0200, Sergey Senozhatsky wrote:
> On (11/07/10 19:01), Paul E. McKenney wrote:
> > On Mon, Nov 08, 2010 at 07:04:43AM +0900, Tetsuo Handa wrote:
> > > Hello.
> > > 
> > > Paul E. McKenney wrote:
> > > > > Users missing rcu_read_lock() when calling find_task_by_vpid():
> > > > > 
> > > > >   check_clock() in kernel/posix-cpu-timers.c
> > > > 
> > > > 	This one has read_lock(&tasklist_lock).
> > > > 
> > > Excuse me. Holding tasklist_lock lock does not help.
> > > We must call rcu_read_lock() explicitly.
> > > That's why 9728e5d6 "kernel/pid.c: update comment on find_task_by_pid_ns" was made.
> > 
> > OK, good point, there are a few more kernels of unpopped corn here.
> >
> 
> Hello,
> I prepared a patch for posix-cpu-timers. 
> 
> [PATCH] posix-cpu-timers: rcu_read_lock/unlock protect find_task_by_vpid call
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> 
> http://lkml.org/lkml/2010/11/3/257

Ah, very good, thank you!!!

							Thanx, Paul

> 	Sergey
> 
>  
> > > I think there are users who needlessly call read_lock(&tasklist_lock)
> > > when they can use rcu_read_lock() instead.
> > > But I don't know when to use read_lock(&tasklist_lock).
> > > 
> > > If read_lock(&tasklist_lock) is needed only when we want to access
> > > the "struct task_struct" after rcu_read_unlock(), maybe it is cleaner to
> > > use a helper like
> > > 
> > > struct task_struct *find_task_and_get(pid_t pid)
> > > {
> > > 	struct task_struct *task;
> > > 	read_lock(&tasklist_lock);
> > > 	rcu_read_lock();
> > > 	task = find_task_by_vpid(pid);
> > > 	rcu_read_unlock();
> > > 	if (task)
> > > 		get_task_struct(task);
> > > 	read_unlock(&tasklist_lock);
> > > 	return task;
> > > }
> > > 
> > > and hide tasklist_lock.
> > 
> > This makes a lot of sense to me!  That said, most of the current
> > open-coded variants of your find_task_and_get() seem to have the
> > rcu_read_unlock() after the get_task_struct() rather than before.  But I
> > don't claim to understand the locking design of this part of the kernel
> > well enough to say which is the best approach.
> > 
> > So, either way, will you be submitting the patches for this?
> > 
> > 							Thanx, Paul
> > 

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Davidlohr Bueso]

On Fri, 2010-10-29 at 15:55 +0300, Sergey Senozhatsky wrote:
> Commit 4221a9918e38b7494cee341dda7b7b4bb8c04bde "Add RCU check for 
> find_task_by_vpid()" introduced rcu_lockdep_assert to find_task_by_pid_ns.
> Assertion failed in sys_ioprio_get. The patch is fixing assertion
> failure in ioprio_set as well. 
> 
>  ===================================================
>  [ INFO: suspicious rcu_dereference_check() usage. ]
>  ---------------------------------------------------
>  kernel/pid.c:419 invoked rcu_dereference_check() without protection!
>  
>  rcu_scheduler_active = 1, debug_locks = 0
>  1 lock held by iotop/4254:
>  #0:  (tasklist_lock){.?.?..}, at: [<ffffffff811104b4>] sys_ioprio_get+0x22/0x2da
>  
>  stack backtrace:
>  Pid: 4254, comm: iotop Not tainted
>  Call Trace:
>  [<ffffffff810656f2>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffff81053c67>] find_task_by_pid_ns+0x4f/0x68
>  [<ffffffff81053c9d>] find_task_by_vpid+0x1d/0x1f
>  [<ffffffff811104e2>] sys_ioprio_get+0x50/0x2da
>  [<ffffffff81002182>] system_call_fastpath+0x16/0x1b
>  
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> ---
> 
> diff --git a/fs/ioprio.c b/fs/ioprio.c
> index 748cfb9..666343d 100644
> --- a/fs/ioprio.c
> +++ b/fs/ioprio.c
> @@ -113,8 +113,11 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
>  		case IOPRIO_WHO_PROCESS:
>  			if (!who)
>  				p = current;
> -			else
> +			else {
> +				rcu_read_lock();
>  				p = find_task_by_vpid(who);
> +				rcu_read_unlock();
> +			}
>  			if (p)
>  				ret = set_task_ioprio(p, ioprio);
>  			break;
> @@ -202,8 +205,11 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
>  		case IOPRIO_WHO_PROCESS:
>  			if (!who)
>  				p = current;
> -			else
> +			else {
> +				rcu_read_lock();
>  				p = find_task_by_vpid(who);
> +				rcu_read_unlock();
> +			}
>  			if (p)
>  				ret = get_task_ioprio(p);
>  			break;

If you add the rcu_read_lock/unlock() sections, we would also need to
update the comment above accordingly.

Thanks,
Davidlohr

From: Davidlohr Bueso <dave@gnu.org>
Subject: [PATCH] ioprio: remove comment to not use RCU

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 fs/ioprio.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/fs/ioprio.c b/fs/ioprio.c
index 748cfb9..72d71de 100644
--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -103,11 +103,6 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who,
int, ioprio)
 	}
 
 	ret = -ESRCH;
-	/*
-	 * We want IOPRIO_WHO_PGRP/IOPRIO_WHO_USER to be "atomic",
-	 * so we can't use rcu_read_lock(). See re-copy of ->ioprio
-	 * in copy_process().
-	 */
 	read_lock(&tasklist_lock);
 	switch (which) {
 		case IOPRIO_WHO_PROCESS:
-- 
1.7.1

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

On (11/08/10 13:01), Davidlohr Bueso wrote:
> On Fri, 2010-10-29 at 15:55 +0300, Sergey Senozhatsky wrote:
> > Commit 4221a9918e38b7494cee341dda7b7b4bb8c04bde "Add RCU check for 
> > find_task_by_vpid()" introduced rcu_lockdep_assert to find_task_by_pid_ns.
> > Assertion failed in sys_ioprio_get. The patch is fixing assertion
> > failure in ioprio_set as well. 
> > 
> >  ===================================================
> >  [ INFO: suspicious rcu_dereference_check() usage. ]
> >  ---------------------------------------------------
> >  kernel/pid.c:419 invoked rcu_dereference_check() without protection!
> >  
> >  rcu_scheduler_active = 1, debug_locks = 0
> >  1 lock held by iotop/4254:
> >  #0:  (tasklist_lock){.?.?..}, at: [<ffffffff811104b4>] sys_ioprio_get+0x22/0x2da
> >  
> >  stack backtrace:
> >  Pid: 4254, comm: iotop Not tainted
> >  Call Trace:
> >  [<ffffffff810656f2>] lockdep_rcu_dereference+0xaa/0xb2
> >  [<ffffffff81053c67>] find_task_by_pid_ns+0x4f/0x68
> >  [<ffffffff81053c9d>] find_task_by_vpid+0x1d/0x1f
> >  [<ffffffff811104e2>] sys_ioprio_get+0x50/0x2da
> >  [<ffffffff81002182>] system_call_fastpath+0x16/0x1b
> >  
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > 
> > ---
> > 
> > diff --git a/fs/ioprio.c b/fs/ioprio.c
> > index 748cfb9..666343d 100644
> > --- a/fs/ioprio.c
> > +++ b/fs/ioprio.c
> > @@ -113,8 +113,11 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
> >  		case IOPRIO_WHO_PROCESS:
> >  			if (!who)
> >  				p = current;
> > -			else
> > +			else {
> > +				rcu_read_lock();
> >  				p = find_task_by_vpid(who);
> > +				rcu_read_unlock();
> > +			}
> >  			if (p)
> >  				ret = set_task_ioprio(p, ioprio);
> >  			break;
> > @@ -202,8 +205,11 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
> >  		case IOPRIO_WHO_PROCESS:
> >  			if (!who)
> >  				p = current;
> > -			else
> > +			else {
> > +				rcu_read_lock();
> >  				p = find_task_by_vpid(who);
> > +				rcu_read_unlock();
> > +			}
> >  			if (p)
> >  				ret = get_task_ioprio(p);
> >  			break;
> 
> If you add the rcu_read_lock/unlock() sections, we would also need to
> update the comment above accordingly.
>

Hello,
I think, this comment is relevant to IOPRIO_WHO_PGRP/IOPRIO_WHO_USER cases.
I only touched IOPRIO_WHO_PROCESS and IOPRIO_WHO_PROCESS.
So, imho, no need to remove it.


	Sergey
 
> 
> From: Davidlohr Bueso <dave@gnu.org>
> Subject: [PATCH] ioprio: remove comment to not use RCU
> 
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
>  fs/ioprio.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ioprio.c b/fs/ioprio.c
> index 748cfb9..72d71de 100644
> --- a/fs/ioprio.c
> +++ b/fs/ioprio.c
> @@ -103,11 +103,6 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who,
> int, ioprio)
>  	}
>  
>  	ret = -ESRCH;
> -	/*
> -	 * We want IOPRIO_WHO_PGRP/IOPRIO_WHO_USER to be "atomic",
> -	 * so we can't use rcu_read_lock(). See re-copy of ->ioprio
> -	 * in copy_process().
> -	 */
>  	read_lock(&tasklist_lock);
>  	switch (which) {
>  		case IOPRIO_WHO_PROCESS:

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]

Re: [PATCH] rcu_read_lock/unlock protect find_task_by_vpid call

[Davidlohr Bueso]

On Mon, 2010-11-08 at 18:18 +0200, Sergey Senozhatsky wrote:
> On (11/08/10 13:01), Davidlohr Bueso wrote:
> > On Fri, 2010-10-29 at 15:55 +0300, Sergey Senozhatsky wrote:
> > > Commit 4221a9918e38b7494cee341dda7b7b4bb8c04bde "Add RCU check for 
> > > find_task_by_vpid()" introduced rcu_lockdep_assert to find_task_by_pid_ns.
> > > Assertion failed in sys_ioprio_get. The patch is fixing assertion
> > > failure in ioprio_set as well. 
> > > 
> > >  ===================================================
> > >  [ INFO: suspicious rcu_dereference_check() usage. ]
> > >  ---------------------------------------------------
> > >  kernel/pid.c:419 invoked rcu_dereference_check() without protection!
> > >  
> > >  rcu_scheduler_active = 1, debug_locks = 0
> > >  1 lock held by iotop/4254:
> > >  #0:  (tasklist_lock){.?.?..}, at: [<ffffffff811104b4>] sys_ioprio_get+0x22/0x2da
> > >  
> > >  stack backtrace:
> > >  Pid: 4254, comm: iotop Not tainted
> > >  Call Trace:
> > >  [<ffffffff810656f2>] lockdep_rcu_dereference+0xaa/0xb2
> > >  [<ffffffff81053c67>] find_task_by_pid_ns+0x4f/0x68
> > >  [<ffffffff81053c9d>] find_task_by_vpid+0x1d/0x1f
> > >  [<ffffffff811104e2>] sys_ioprio_get+0x50/0x2da
> > >  [<ffffffff81002182>] system_call_fastpath+0x16/0x1b
> > >  
> > > 
> > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > 
> > > ---
> > > 
> > > diff --git a/fs/ioprio.c b/fs/ioprio.c
> > > index 748cfb9..666343d 100644
> > > --- a/fs/ioprio.c
> > > +++ b/fs/ioprio.c
> > > @@ -113,8 +113,11 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
> > >  		case IOPRIO_WHO_PROCESS:
> > >  			if (!who)
> > >  				p = current;
> > > -			else
> > > +			else {
> > > +				rcu_read_lock();
> > >  				p = find_task_by_vpid(who);
> > > +				rcu_read_unlock();
> > > +			}
> > >  			if (p)
> > >  				ret = set_task_ioprio(p, ioprio);
> > >  			break;
> > > @@ -202,8 +205,11 @@ SYSCALL_DEFINE2(ioprio_get, int, which, int, who)
> > >  		case IOPRIO_WHO_PROCESS:
> > >  			if (!who)
> > >  				p = current;
> > > -			else
> > > +			else {
> > > +				rcu_read_lock();
> > >  				p = find_task_by_vpid(who);
> > > +				rcu_read_unlock();
> > > +			}
> > >  			if (p)
> > >  				ret = get_task_ioprio(p);
> > >  			break;
> > 
> > If you add the rcu_read_lock/unlock() sections, we would also need to
> > update the comment above accordingly.
> >

Ah, yes indeed, misread the cases, sorry about the noise.

Thanks,
Davidlohr