Date:	Sun, 9 Jan 2011 17:05:04 -0500
From:	Tejun Heo <tj@...nel.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	roland@...hat.com, jan.kratochvil@...hat.com,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org
Subject: Re: [PATCH 7/7] ptrace: clean transitions between TASK_STOPPED and
 TRACED

Hello, Oleg.  Sorry about the delay.  I've been and still am
travelling and won't be very responsive until mid next week.

On Wed, Jan 05, 2011 at 05:35:42PM +0100, Oleg Nesterov wrote:
> To me, the whole series is fine.

Awesome.

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

I think it would be a good idea to document the defined and probably
more importantly undefined aspects of the ptrace behavior somewhere
along with rational and implementation peculiarities.  Probably we
should create a file under Documentation and also make sure the ptrace
man page is kept synchronized with and point to it.

> 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().

Yeah, given the complexities around wait_chldexit, I'm not entirely
sure whether multiplexing it actually is a good idea.  It definitely
fits the purpose but I still feel dirty adding more subtleties to it.

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

Hmm... I wanted to make sure that task_clear_group_stop() clears all
group stop related status.  As the function may be called from kill
path too, I wanted to make sure it is guaranteed to be cleared
together.  That was the rationale but maybe there's a better place for
it.

> > @@ -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.

Right.  That's a slim possibility but definitely possible.

> > @@ -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.

Hmm... I don't really mind one way or the other.  I like the idea that
clear_group_stop() clears everything but at the same time the
suggested placing makes it more explicit which is a plus.  I'll think
a bit more about it but if it doesnt break anything let's move it.

Thanks.

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

Powered by blists - more mailing lists