linux-kernel - Re: [PATCH 06/11] ptrace: make group stop state visible via 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: <20110512164745.GA20215@redhat.com>
Date:	Thu, 12 May 2011 18:47:45 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	jan.kratochvil@...hat.com, vda.linux@...glemail.com,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org, indan@....nu,
	Tony Luck <tony.luck@...el.com>,
	Fenghua Yu <fenghua.yu@...el.com>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Kyle McMartin <kyle@...artin.ca>, Helge Deller <deller@....de>,
	"James E.J. Bottomley" <jejb@...isc-linux.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Paul Mackerras <paulus@...ba.org>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	"David S. Miller" <davem@...emloft.net>,
	Chris Metcalf <cmetcalf@...era.com>, x86@...nel.org
Subject: Re: [PATCH 06/11] ptrace: make group stop state visible via
	PTRACE_GETSIGINFO

On 05/11, Tejun Heo wrote:
>
> On Tue, May 10, 2011 at 06:55:45PM +0200, Oleg Nesterov wrote:
> > IOW, if the tracee reports via ptrace_notify*, the tracee can look at
> > si_pt_flags == stop-in-effect. If the tracer reports a signal, the
> > tracer obviously lacks this info, hmm.
>
> Which indicates tracee is in group stop trap.

What do you mean?

si_pt_flags doesn't "exist" when the tracee reports the signal or
CLD_STOPPED. This doesn't look clean.

> > Probably I need more time to get used to this... But at first glance
> > this looks a bit unnatural. Say, can't we simply implement
> > PTRACE_GET_GROUP_STOP_STATUS request which returns this (and probably
> > more) info?
>
> I don't know.  PTRACE_GETSIGINFO seemed to already fit the bill and I
> want to avoid introducing a new request if at all possible.  It sure
> is a bit quirky but doesn't compromisea functionality.

I am not sure too, but the new request is much simpler to use, and it
is more extensible. We can report more info. Say, the state of
JOBCTL_STOP_CONSUME or something else.

> > > 		  if (!ptrace(PTRACE_GETSIGINFO, tracee, NULL, &si)) {
> > > 			  if (si.si_code) {
> > > 				  stopped = !!si.si_status;
> >
> > In this case this "si_code != 0" check is correct, but how can the
> > tracer detect this case in general?
>
> This was quick hack.  Proper test would look like,
>
> 	si.si_code && (si.si_pt_flags & PTRACE_SI_STOPPED)

This doesn't look right too? How can we know we can trust si_pt_flags?
This needs some YES_YOU_CAN_CHECK_si_pt_flags(si_code), but I can't
understand what it should do right now...

> > > +		/* report whether group stop is in effect w/ SI_STOPPED */
> > > +		if (sig->group_stop_count || (sig->flags & SIGNAL_STOP_STOPPED))
> >
> > We have more and more "group_stop_count || SIGNAL_STOP_STOPPED" checks,
> > perhaps we should make a helper. Or at least invent the short name to
> > denote the group-stopped-or-in-progress to simplify the discussions ;)
>
> Yeah, how about group_stop_in_effect()?

Or may me signal_stop_stopped(struct signal_struct *sig), like
signal_group_exit/SIGNAL_GROUP_EXIT. But I am fine with
group_stop_in_effect, probably it is more explanatorily.

> > Still, this is strange. With this change ptrace_getsiginfo() reports
> > the extra "volatile" info which wasn't reported by the tracee itself.
> > If the tracer does PTRACE_SETSIGINFO twice in a row, it can see the
> > different si_pt_flags's.
>
> (answering to both get/setsiginfo concerns)
>
> * I think we better block PTRACE_SETSIGINFO for non signal delivery
>   traps.  It doesn't make any sense.  Let's just fail that with
>   -EINVAL if PT_SEIZED.

Oh I agree, it does not make any sense. Should we change the current
behaviour for PT_SEIZED? I don't really care, this looks minor.

> * I don't think PTRACE_GETSIGINFO returning volatile information to be
>   problematic.  The information is generated on the fly on trap
>   anyway.

Yes. And I'd understand if si_pt_flags was filled by the tracee
during the trap (although I do not think this makes sense) to record
the state at the time of this trap.

But PTRACE_GETSIGINFO returns the dynamic info which reflects the
process-wide state at the time of syscall.

Oleg.

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