Messages in this thread | | | Date | Wed, 3 Nov 2010 18:38:16 +0200 | From | Sergey Senozhatsky <> | 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@redhat.com> > [unhandled content-type:application/pgp-signature] | |