linux-kernel - Re: [PATCH 02/16] signal: fix CLD

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]

Message-ID: <20101221163105.GB13285@htj.dyndns.org>
Date:	Tue, 21 Dec 2010 17:31:07 +0100
From:	Tejun Heo <tj@...nel.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	roland@...hat.com, linux-kernel@...r.kernel.org,
	torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
	rjw@...k.pl, jan.kratochvil@...hat.com
Subject: Re: [PATCH 02/16] signal: fix CLD_CONTINUED notification target

Hello, Oleg.

On Mon, Dec 20, 2010 at 03:58:15PM +0100, Oleg Nesterov wrote:
> On 12/06, Tejun Heo wrote:
> >
> > CLD_CONTINUED notification code calls do_notify_parent_cldstop() with
> > its group_leader; however, do_notify_parent_cldstop() already uses the
> > group_leader for non-ptraced notifications.
> 
> Yes,
> 
> > The duplicate
> > ->group_leader dereferencing is unnecessary and leads to incorrectly
> > notifying the group_leader's ptracer of CLD_CONTINUED from a different
> > task in the group.  Fix it.
> 
> I do not really agree this is wrong, group_leader was used intentionally
> for ptrace case.
> 
> There is no "correct" thread who should report CLD_CONTINUED, a random
> thread wins and notifies its ->real_parent or debugger. If we always
> choose ->group_leader, we always knows what happens. With this patch
> we can't predict where does this notification go.
> 
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1867,7 +1867,7 @@ relock:
> >
> >  		if (why) {
> >  			read_lock(&tasklist_lock);
> > -			do_notify_parent_cldstop(current->group_leader, why);
> > +			do_notify_parent_cldstop(current, why);
> 
> OTOH, I see nothing really wrong with this change, and this all will
> be reworked by the next patches anyway.

Ah, okay, I thought it was an unintentional bug.  The reason why the
change is necessary is because of later changes which re-instates the
notification on ptrace detach.  The code is modified such that
notification pending is cleared iff the notification is delivered to
an actual parent.  The double dereference makes it difficult to tell
whether the notification is beling delivered to an actual parent or
not.

If this wasn't a bug, I think the proper thing to do is to move this
later where the change is necessary, after the code learns how to
buffer the notification until ptrace detach and needs to determine
whether it's being eaten by the debugger or not.

Thanks.

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