All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pedro Alves <pedro@codesourcery.com>
To: Denys Vlasenko <vda.linux@googlemail.com>
Cc: Tejun Heo <tj@kernel.org>, Oleg Nesterov <oleg@redhat.com>,
	jan.kratochvil@redhat.com, linux-kernel@vger.kernel.org,
	torvalds@linux-foundation.org, akpm@linux-foundation.org,
	indan@nul.nu, bdonlan@gmail.com
Subject: Re: [PATCHSET ptrace] ptrace: implement PTRACE_SEIZE/INTERRUPT and group stop notification, take#4
Date: Fri, 3 Jun 2011 16:24:08 +0100	[thread overview]
Message-ID: <201106031624.09371.pedro@codesourcery.com> (raw)
In-Reply-To: <BANLkTinx8enCJHNMm+LN0TBtSF0=7PGRNw@mail.gmail.com>

On Friday 03 June 2011 15:12:55, Denys Vlasenko wrote:
> On Fri, Jun 3, 2011 at 2:11 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> >> > > > * What to do about events which are reported by genuine SIGTRAP
> >> > > >  generation?
> >> > >
> >> > > I don't understand what you meant here. Example(s)?
> >> >
> >> > The syscall, breakpoint, single step SIGTRAPs which can't be
> >> > distinguished from userland generated SIGTRAPs and may be mixed and/or
> >> > lost.  Maybe it's best to leave them alone or maybe we can add some
> >> > way to distinguish them which is mostly backward compatible (which is
> >> > enabled w/ SEIZE and hopefully doesn't require noticeable userland
> >> > changes).
> >>
> >> Currently, all PTRACE_EVENTs are enabled with ptrace options.
> >>
> >> I propose using the same way instead of using something different.
> >> It works. It's not problematic. Just add more PTRACE_O_foo bits.
> >> Then user who really want it will use it, and users which
> >> would rather use their existing code with less changes aren't
> >> forced to change.
> >
> > I'm in principle against this.  What realistic good does it
> > bring over making exception/syscall SIGTRAPs distinguisheable
> > on the siginfo?
> 
> Requiring GETSIGINFO on every syscall trap is (1) a bit expensive
> and (2) was never needed before.

On (1) - a tracer can enable the sysgood bit today, no need to
GETSIGINFO in that case.  syscall traps are not generated if
a thread is not traced.  This leaves user-sent SIGTRAPs, breakpoints,
single-step/trace, data breakpoints/watchpoints.  The first
three are generatable without a tracer installed.  The latter,
not possible on x86 to tweak the debug registers from userspace
today (unless a ptracer tweaks the debug registers, and then
detaches, without clearing them), but possible to workaround with
a kernel module.  Likely possible to trigger on other archs though.

Does strace end up passing non-syscall SIGTRAPs down to
the tracee that it shouldn't today, due to bad ptrace API?  What
are those SIGTRAPs?

On (2) - Tracers who need to handle breakpoints and single-steps
need to do other ptrace calls on a SIGTRAP to handle the event.
Read registers (read current PC, unless you get that from ... siginfo),
read debug registers --- check whether you've hit a hardware
breakpoint or data breakpoint/watchpoint.  On PPC, for example, GDB
reads siginfo on every SIGTRAP to check for TRAP_HWBKPT on si_code,
and check its si_errno for which hwbreak triggered, so "never needed
before" is false.

And a flurry of user-sent SIGTRAPs is not something anything
sane would do.  :-)

> 
> >  Userspace should see these SIGTRAPs if a tracer
> > isn't there anyway, and, even if a tracer _is_ there, you
> > may want to forward breakpoint/step SIGTRAPs to the
> > tracee, just as if a ptracer wasn't there --- I do that often to
> > debug an in-process self debugger.
> 
> Ah, you are talking about int3 SIGTRAP? That one, IMHO,
> should be treated as "real" signal and passed down to the program
> by strace.

Of course.  int1's (single-step's, caused by TF (trace flag)
set on x86) should be treated the same.  These are real exceptions
that should be forwarded to a program as signals.  What are
the SIGTRAPs that shouldn't be passed down to the program
by strace (excluding the current PTRACE_EVENT_XXX SIGTRAPs)?

> For debuggers which use int3 as breakpoint insn,
> well, I don't know. 

They need to resort to heuristics based on their own state machine
logic (did I set the thread stepping?, is the PC the same as it was
before?, etc., is there an int3 at PC - 1?), to infer whether
a SIGTRAP corresponds to a known breakpoint hit or not,
and unwind the PC if so.  All that is heuristic, and can fail, e.g,
if you overwrite the int3 in memory, before waitpid'ing the SIGTRAP.
Failing to unwind the PC or unwinding it when you shouldn't is
disastrous.

> Perhaps they don't cope well with user-sent
> SIGTRAPs anyway?

Given that breakpoint/trace SIGTRAPs should be passed down to
the program, the fact that user-sent SIGTRAPs don't queue
with those, is not a ptrace problem --- it's a unix signals
problem that can happen without ptrace involved.

But who sends these anyway?  :-P

> >  Being able to distinguish the
> > cause of the SIGTRAP is useful for self debuggers as well,
> > which leads to putting the info in siginfo anyway.
> 
> I don't understand what use case you have in mind.

There are programs that implement a sort of self debugger.
A "ptracer" without ptrace.  We've written one recently for
x86/x86-64.  Install signal handlers for all signals, reserve
one thread for the "debugger", and a signal to interrupt other
threads.  Handle breakpoints and single-steps by handling SIGTRAP.
You need to take some trade offs, but it works.

If we have that info already available on siginfo (at least
some archs do, and I hope x86 gets it too), then it looks
like adding more special ptrace stuff wouldn't do a
significant good.

-- 
Pedro Alves

  reply	other threads:[~2011-06-03 15:24 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-29 23:12 [PATCHSET ptrace] ptrace: implement PTRACE_SEIZE/INTERRUPT and group stop notification, take#4 Tejun Heo
2011-05-29 23:12 ` [PATCH 01/17] ptrace: remove silly wait_trap variable from ptrace_attach() Tejun Heo
2011-06-01 18:47   ` Oleg Nesterov
2011-06-02  5:03     ` Tejun Heo
2011-06-02 11:39   ` [PATCH UPDATED " Tejun Heo
2011-05-29 23:12 ` [PATCH 02/17] job control: rename signal->group_stop and flags to jobctl and update them Tejun Heo
2011-05-29 23:12 ` [PATCH 03/17] ptrace: ptrace_check_attach(): rename @kill to @ignore_state and add comments Tejun Heo
2011-05-29 23:12 ` [PATCH 04/17] ptrace: relocate set_current_state(TASK_TRACED) in ptrace_stop() Tejun Heo
2011-05-29 23:12 ` [PATCH 05/17] job control: introduce JOBCTL_PENDING_MASK and task_clear_jobctl_pending() Tejun Heo
2011-05-29 23:12 ` [PATCH 06/17] job control: make task_clear_jobctl_pending() clear TRAPPING automatically Tejun Heo
2011-05-29 23:12 ` [PATCH 07/17] job control: introduce task_set_jobctl_pending() Tejun Heo
2011-05-29 23:12 ` [PATCH 08/17] ptrace: use bit_waitqueue for TRAPPING instead of wait_chldexit Tejun Heo
2011-06-02 11:41   ` [PATCH UPDATED " Tejun Heo
2011-05-29 23:12 ` [PATCH 09/17] signal: remove three noop tracehooks Tejun Heo
2011-05-29 23:12 ` [PATCH 10/17] job control: introduce JOBCTL_TRAP_STOP and use it for group stop trap Tejun Heo
2011-05-29 23:12 ` [PATCH 11/17] ptrace: implement PTRACE_SEIZE Tejun Heo
2011-06-01 19:01   ` Oleg Nesterov
2011-06-01 19:55     ` Oleg Nesterov
2011-06-02  5:13     ` Tejun Heo
2011-06-02 11:43   ` [PATCH UPDATED " Tejun Heo
2011-05-29 23:12 ` [PATCH 12/17] ptrace: implement PTRACE_INTERRUPT Tejun Heo
2011-05-29 23:12 ` [PATCH 13/17] ptrace: add siginfo.si_pt_flags Tejun Heo
2011-05-29 23:12 ` [PATCH 14/17] ptrace: make group stop state visible via PTRACE_GETSIGINFO Tejun Heo
2011-05-29 23:12 ` [PATCH 15/17] ptrace: don't let PTRACE_SETSIGINFO override __SI_TRAP siginfo Tejun Heo
2011-05-29 23:12 ` [PATCH 16/17] ptrace: implement TRAP_NOTIFY and use it for group stop events Tejun Heo
2011-05-29 23:12 ` [PATCH 17/17] ptrace: implement PTRACE_LISTEN Tejun Heo
2011-06-02 17:33   ` Oleg Nesterov
2011-06-13 14:10     ` Tejun Heo
2011-06-13 20:33       ` Oleg Nesterov
2011-06-14  6:45         ` Tejun Heo
2011-05-30 15:42 ` [PATCHSET ptrace] ptrace: implement PTRACE_SEIZE/INTERRUPT and group stop notification, take#4 Oleg Nesterov
2011-06-01  5:39   ` Tejun Heo
2011-06-02 12:31     ` Tejun Heo
2011-06-02 14:51       ` Denys Vlasenko
2011-06-03  1:24         ` Tejun Heo
2011-06-03 10:25           ` Pedro Alves
2011-06-16  8:38             ` Tejun Heo
2011-06-16  9:56               ` Pedro Alves
2011-06-17 19:08                 ` Oleg Nesterov
2011-06-03 11:57           ` Denys Vlasenko
2011-06-03 12:11             ` Pedro Alves
2011-06-03 14:12               ` Denys Vlasenko
2011-06-03 15:24                 ` Pedro Alves [this message]
2011-06-03 15:46             ` Oleg Nesterov
2011-06-02 18:27       ` Oleg Nesterov
2011-06-02 21:09         ` Denys Vlasenko
2011-06-03  1:34           ` Tejun Heo
2011-06-03 11:37             ` Denys Vlasenko
2011-06-03 11:58               ` Denys Vlasenko
2011-06-03 15:37             ` Oleg Nesterov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201106031624.09371.pedro@codesourcery.com \
    --to=pedro@codesourcery.com \
    --cc=akpm@linux-foundation.org \
    --cc=bdonlan@gmail.com \
    --cc=indan@nul.nu \
    --cc=jan.kratochvil@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=vda.linux@googlemail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.