linux-kernel - Re: [PATCH 1/1] ptrace: make sure do_wait() won't hang after PTRACE

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: <20110218170212.GS21209@htj.dyndns.org>
Date:	Fri, 18 Feb 2011 18:02:12 +0100
From:	Tejun Heo <tj@...nel.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Roland McGrath <roland@...hat.com>,
	Denys Vlasenko <vda.linux@...glemail.com>,
	jan.kratochvil@...hat.com, linux-kernel@...r.kernel.org,
	torvalds@...ux-foundation.org, akpm@...ux-foundation.org
Subject: Re: [PATCH 1/1] ptrace: make sure do_wait() won't hang after
 PTRACE_ATTACH

Hello, Oleg.

Still trying to follow the new discussion.

On Tue, Feb 15, 2011 at 09:27:47PM +0100, Oleg Nesterov wrote:
> > The reason for the transition to TASK_TRACED is to prevent a race with
> > SIGCONT waking the task.  There is always a race with SIGKILL waking it,
> > but the circumstances where that can really matter are far fewer.
> > You need to make sure that the work PTRACE_GETSIGINFO does to access
> > last_siginfo cannot race with that pointer disappearing or the stack
> > space it points to becoming invalid.  I think the use of siglock ensures
> > that, but Oleg should verify it.
> 
> Yes, I think this is safe.
> 
> I do not really like this idea because it looks a bit strange to treat
> PTRACE_GETSIGINFO specially, and this doesn't solve all problems. And,
> once again, I still hope we can change ptrace_resume() so that it doesn't
> wakeup the stopped (I mean, SIGNAL_STOP_STOPPED) tracee, in this case this
> hack is not needed.
> 
> And. We are going to add the new requests which doesn't need the stopped
> tracee anyway. So we can just add PTRACE_HAS_SIGINFO which returns
> child->last_siginfo != NULL. This looks simpler, and this is compatible.
> Of course this check is racy, but this doesn't matter. PTRACE_GETSIGINFO
> is equally racy if it doesn't change the state to TASK_TRACED.

This is probably where we disagree the most but I think the weird part
isn't making PTRACE_GETSIGINFO exempt from TASK_TRACE transition.  The
weirdness starts when the tracee is put into TASK_STOPPED while being
ptraced.  I think such dual modes of operation inherently lead to
strange problems.

Instead of having simple "a ptracer stops in TASK_TRACED and its
execution is under the control of ptrace", we end up with the tracee
stopping here or there depending on why it stops and the involved
behavioral subtleties like consumption of wait state and the mentioned
GETSIGINFO problem.  The patch which puts the tracee into TASK_TRACED
on ATTACH already fix two problems discussed in this thread without
doing anything wonky.  I think it says a lot.

As it currently stands, SIGSTOP/CONT while ptraced doesn't work and
even if we bend the rules subtly and provide sneaky ways like the
above, userland needs to be modified to make use of it anyway.  I
think it would be far cleaner to simply make ptracee always stop in
TASK_TRACED and give the ptracer a way to notice what's happening to
the tracee w.r.t. group stop, so that it can comply if it wants to.
What do you think?

Thank you.

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