linux-kernel - Re: [PATCH] posix-cpu-timers: rcu_read_lock/unlock protect find_task_by

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, 3 Nov 2010 17:10:59 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Stanislaw Gruszka <sgruszka@...hat.com>
Subject: Re: [PATCH] posix-cpu-timers: rcu_read_lock/unlock protect
	find_task_by_vpid call

On 11/03, Oleg Nesterov wrote:
>
> Sergey, Thomas, please wait a bit.
>
> Yes, I believe this patch is fine by itself. But looking into
> posix-cpu-timers.c again, I suspect that those "other problems
> with de_thread" I already mentioned are much more serious and
> need the urgent fix.

Yes. I'll send the patch tomorrow.


However, my initial thinking was wrong, that bug is orthogonal
to this patch.

Sergey, how much will you hate me if I ask you to re-send it again?


> > On (11/02/10 19:33), Oleg Nesterov wrote:
> > > > On Tue, 2 Nov 2010, Sergey Senozhatsky wrote:
> > > >
> > > > > > We can remove the tasklist_lock while at it. rcu_read_lock is enough.
> > > > > >
> > >
> > > Yes, I believe posix-cpu-timers.c shouldn't use tasklist at all,
> > > but it is not trivial to change this code.
> > >
> > >[..]
> > >
> > > I think this change is fine, but please note that thread_group_leader()
> > > check is not relaible without tasklist. If we race with de_thread()
> > > find_task_by_vpid() can find the new leader before it updates its
> > > ->group_leader. IOW, posix_cpu_timer_create() can fail when it shouldn't.
> > >
> > > Not that I think this really matters, posix_cpu_timer_create() has
> > > other problems with de_thread(). But perhaps it makes sense to
> > > change posix_cpu_timer_create() to use has_group_leader_pid() instead,
> > > just to make this code not look racy and avoid adding new problems.
> > >
> > > The real fix, I think, should change cpu_timer_list to use
> > > struct pid* instead of task_struct.
> > >
> >
> > Hello,
> > Using has_group_leader_pid instead of thread_group_leader, when tasklist_lock
> > is not aquired (check_clock and posix_cpu_timer_create).

This doesn't look like a valid changelog ;) I'd suggest you to write
the new one without quoting old emails. It should explain that a)
tasklist_lock is not enough for find_vpid() and b) it is not needed.

Also, you forgot to add your signed-of-by.

Otherwise,

Reviewed-by: Oleg Nesterov <oleg@...hat.com>

--
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/