lkml.org 
[lkml]   [2011]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 02/11] ptrace: implement PTRACE_SEIZE
    On 05/08, Tejun Heo wrote:
    >
    > After PTRACE_SEIZE, tracee will trap. Which trap will happen isn't
    > fixed. If other trap conditions exist (signal delivery or group
    > stop), they might be taken; otherwise, a trap with exit_code SIGTRAP |
    > (PTRACE_EVENT_INTERRUPT << 8) is taken.
    > guaranteed.

    Personally, I think the new behaviour is fine. But, as usual, I'd like
    to know what Jan/Denys think.


    As for the implementation,

    > -static int ptrace_attach(struct task_struct *task)
    > +static int ptrace_attach(struct task_struct *task, long request,
    > + unsigned long flags)
    > {
    > + bool seize = request == PTRACE_SEIZE;

    Cough. I really hate the cosmetic nits but can't resist...

    bool seize = (request == PTRACE_SEIZE);

    looks more parseable, but feel free to ignore.

    > @@ -247,6 +272,14 @@ static int ptrace_attach(struct task_struct *task)
    > if (task_is_stopped(task)) {
    > task->jobctl |= JOBCTL_STOP_PENDING | JOBCTL_TRAPPING;
    > signal_wake_up(task, 1);
    > + } else if (seize) {
    > + /*
    > + * Otherwise, SEIZE uses jobctl trap to put tracee into
    > + * TASK_TRACED, which doesn't have the nasty side effects
    > + * of sending SIGSTOP.
    > + */
    > + task->jobctl |= JOBCTL_TRAP_SEIZE;
    > + signal_wake_up(task, 0);

    OK... I am a bit worried we can set JOBCTL_TRAP_SEIZE even if the tracee
    was already killed, and if it is killed later JOBCTL_TRAP_SEIZE won't be
    cleared. Probably this is fine, ptrace_stop()->schedule() won't sleep in
    this case.

    Hmm. but see below.

    > @@ -1752,12 +1752,13 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
    > set_current_state(TASK_TRACED);
    >
    > /*
    > - * We're committing to trapping. Clearing JOBCTL_TRAPPING and
    > - * transition to TASK_TRACED should be atomic with respect to
    > - * siglock. This should be done after the arch hook as siglock is
    > - * released and regrabbed across it.
    > + * We're committing to trapping. Adjust ->jobctl. Updates to
    > + * these flags and transition to TASK_TRACED should be atomic with
    > + * respect to siglock. This should be done after the arch hook as
    > + * siglock may be released and regrabbed across it.
    > */
    > task_clear_jobctl_trapping(current);
    > + current->jobctl &= ~JOBCTL_TRAP_SEIZE;

    Yes. But, it seems, this is too late.

    Suppose that the JOBCTL_TRAP_SEIZE tracee was SIGKILLED before it reports
    PTRACE_EVENT_INTERRUPT. Now, if arch_ptrace_stop_needed() == T, ptrace_stop()
    returns without clearing JOBCTL_TRAP_SEIZE/TIF_SIGPENDING. This means
    get_signal_to_deliver() will loop forever.

    I never understood why ptrace_stop()->sigkill_pending() logic, I think
    we should check fatal_signal_pending() unconditionally. Oh, and we have
    other subtle issues here.

    > for (;;) {
    > struct k_sigaction *ka;
    > +
    > + /*
    > + * Check for ptrace trap conditions. Jobctl traps are used
    > + * to trap ptracee while staying transparent regarding
    > + * signal and job control.
    > + */
    > + if (unlikely(current->jobctl & JOBCTL_TRAP_MASK)) {
    > + ptrace_notify_locked(SIGTRAP |
    > + (PTRACE_EVENT_INTERRUPT << 8));
    > + continue;

    Shouldn't we recheck SIGNAL_CLD_MASK after ptrace_notify_locked() returns?
    Probably not, but I am not sure...

    In any case. This doesn't really matter, but can't we check JOBCTL_TRAP_MASK
    outside of the main loop? Unless we drop ->siglock this bit can't be changed,
    and every time we drop ->siglock we go to "relock".

    Oleg.



    \
     
     \ /
      Last update: 2011-05-09 18:23    [W:0.028 / U:59.604 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site