lkml.org 
[lkml]   [2011]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 7/7] ptrace: clean transitions between TASK_STOPPED and TRACED
    To me, the whole series is fine.

    As for the user-visible changes, I believe they are carefully documented,
    hopefully Roland and Jan can take a look.


    This patch looks good too, a couple of minor nits below.

    On 12/24, Tejun Heo wrote:
    >
    > + * task_clear_group_stop_trapping - clear group stop trapping bit
    > + * @task: target task
    > + *
    > + * If GROUP_STOP_TRAPPING is set, a ptracer is waiting for us. Clear it
    > + * and wake up the ptracer. Note that we don't need any further locking.
    > + * @task->siglock guarantees that @task->parent points to the ptracer.
    > + *
    > + * CONTEXT:
    > + * Must be called with @task->sighand->siglock held.
    > + */
    > +static void task_clear_group_stop_trapping(struct task_struct *task)
    > +{
    > + if (unlikely(task->group_stop & GROUP_STOP_TRAPPING)) {
    > + task->group_stop &= ~GROUP_STOP_TRAPPING;
    > + __wake_up_sync(&task->parent->signal->wait_chldexit,
    > + TASK_UNINTERRUPTIBLE, 1);

    OK... we are doing __wake_up_sync_key(key => NULL), this looks unfriendly
    to child_wait_callback(). But TASK_UNINTERRUPTIBLE means we can't abuse
    the tracer's subthreads doing do_wait().

    > void task_clear_group_stop(struct task_struct *task)
    > {
    > task->group_stop &= ~(GROUP_STOP_PENDING | GROUP_STOP_CONSUME);
    > + task_clear_group_stop_trapping(task);
    > }

    Not a comment, but the question. I am not sure task_clear_group_stop()
    needs task_clear_group_stop_trapping(), please see below...

    > @@ -1694,6 +1716,14 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
    > }
    >
    > /*
    > + * We're committing to trapping. Clearing GROUP_STOP_TRAPPING and
    > + * transition to TASK_TRACED should be atomic with respect to
    > + * siglock. Do it after the arch hook as siglock is released and
    > + * regrabbed across it.
    > + */
    > + task_clear_group_stop_trapping(current);

    This wakes up the tracer. It can return from sys_ptrace(), call do_wait(),
    and take tasklist_lock before us.

    Of course, this is only theoretical problem, but perhaps it makes sense
    to do this after __set_current_state(TASK_TRACED), otherwise
    task_stopped_code() can fail.

    > @@ -1839,13 +1875,25 @@ static int do_signal_stop(int signr)
    > schedule();
    >
    > spin_lock_irq(&current->sighand->siglock);
    > - } else
    > - ptrace_stop(current->exit_code, CLD_STOPPED, 0, NULL);
    > + } else {
    > + ptrace_stop(current->group_stop & GROUP_STOP_SIGMASK,
    > + CLD_STOPPED, 0, NULL);
    > + current->exit_code = 0;
    > + }
    > +
    > + /*
    > + * GROUP_STOP_PENDING could be set if another group stop has
    > + * started since being woken up or ptrace wants us to transit
    > + * between TASK_STOPPED and TRACED. Retry group stop.
    > + */
    > + if (current->group_stop & GROUP_STOP_PENDING) {
    > + WARN_ON_ONCE(!(current->group_stop & GROUP_STOP_SIGMASK));
    > + goto retry;
    > + }
    >
    > spin_unlock_irq(&current->sighand->siglock);

    Can't we add task_clear_group_stop_trapping() right before we drop
    ->siglock ? This way we can remove it from task_clear_group_stop(),
    afaics. Once again, this is up to you. Looks more clean to me, but
    this is of course subjective.

    If GROUP_STOP_PENDING is not set, but GROUP_STOP_TRAPPING is set,
    then this task was SIGKILL'ed or SIGCONT'ed, we can notify the
    tracer.

    Otherwise (ignoring ptrace_stop), there is no reasons to check
    GROUP_STOP_TRAPPING. It was set under ->siglock when the tracee
    was in TASK_STOPPED state few lines above.

    Oleg.



    \
     
     \ /
      Last update: 2011-01-05 17:45    [W:0.025 / U:183.816 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site