linux-kernel - Re: [PATCH 2/2] CRED: Fix __task_cred()'s lockdep check and banner comment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 4 Aug 2010 15:17:49 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	David Howells <dhowells@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	paulmck@...ux.vnet.ibm.com, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	Jiri Olsa <jolsa@...hat.com>,
	Roland McGrath <roland@...hat.com>
Subject: Re: [PATCH 2/2] CRED: Fix __task_cred()'s lockdep check and banner
	comment

On 08/03, Linus Torvalds wrote:
>
> On Tue, Aug 3, 2010 at 2:34 AM, David Howells <dhowells@...hat.com> wrote:
> >
> > A previous patch:
> >
> >        commit 8f92054e7ca1d3a3ae50fb42d2253ac8730d9b2a
> >        Author: David Howells <dhowells@...hat.com>
> >        Date:   Thu Jul 29 12:45:55 2010 +0100
> >        Subject: CRED: Fix __task_cred()'s lockdep check and banner comment

I am not sure I understand this patch.

__task_cred() checks rcu_read_lock_held() || task_is_dead(), and
task_is_dead(task) is ((task)->exit_state != 0).

OK, task_is_dead() is valid for, say, wait_task_zombie(). But
wait_task_stopped() calls __task_cred(p) without rcu lock and p is alive.
The code is correct, this thread can do nothing until we drop ->siglock.

> > fixed the lockdep checks on __task_cred().  This has shown up a place in the
> > signalling code where a lock should be held - namely that
> > check_kill_permission() requires its callers to hold the RCU lock.
>
> It's not just check_kill_permission(), is it? I thought we could do
> the "for_each_process()" loops with just RCU, rather than holding the
> whole tasklist_lock?

Yes, for_each_process() is rcu-safe by itself.

> So I _think_ that getting the RCU read-lock would
> make it possible to get rid of the tasklist_lock in there too? At
> least in kill_something_info().

As for kill_something_info(), I think yes. I even sent (iirc) the
protoptype patch a long ago. We can't just remove tasklist, we should
avoid the races fork/exit/exec in the kill(-1, SIG) case.

The same for kill_pgrp/__kill_pgrp_info(). We need tasklist to ensure
that nobody in this group can escape the signal. This seems solveable
too, it was even discussed a bit.

> > It's may be that it would be better to add RCU read lock calls in
> > group_send_sig_info() only, around the call to check_kill_permission().

I must admit, at first glance changing check_kill_permission() to take
rcu lock looks better to me.

> On the
> > other hand, some of the callers are either holding the RCU read lock already,
> > or have disabled interrupts,

Hmm. So, local_irq_disable() "officially" blocks rcu? It does in practice
(unless I missed the new version of RCU), but, say,  posix_timer_event()
takes rcu_read_lock() exactly because I thought we shouldn't assume that
irqs_disabled() acts as rcu_read_lock() ?

There are other examples of rcu_read_lock() under local_irq_disable().

> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -773,6 +773,7 @@ static void forget_original_parent(struct task_struct *father)
> >
> >        exit_ptrace(father);
> >
> > +       rcu_read_lock();
> >        write_lock_irq(&tasklist_lock);
> >        reaper = find_new_reaper(father);

No, this doesn't look right. find_new_reaper() can drop tasklist and sleep.

Besides, this patch conflicts with the change in -mm tree. And imho this
looks a bit as "action at a distance".

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/