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 18:38:16 +0200
From:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	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/10 17:10), Oleg Nesterov wrote:
> 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?
>

Oleg, not a problem at all. Which one should I resend?

#  desc.
1) added rcu_read_lock/unlock
2) removed tasklist_lock
3) added has_group_leader_pid


	Sergey

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

Content of type "application/pgp-signature" skipped