Message-ID: <20110128105957.GJ24925@htj.dyndns.org>
Date:	Fri, 28 Jan 2011 11:59:57 +0100
From:	Tejun Heo <tj@...nel.org>
To:	Roland McGrath <roland@...hat.com>
Cc:	oleg@...hat.com, linux-kernel@...r.kernel.org, rjw@...k.pl,
	jan.kratochvil@...hat.com
Subject: Re: [PATCH 2/4] ptrace: remove the extra wake_up_process() from
 ptrace_detach()

Hello, Roland.

On Thu, Jan 27, 2011 at 11:02:52PM -0800, Roland McGrath wrote:
> > On Mon, Jan 17, 2011 at 02:13:40PM -0800, Roland McGrath wrote:
> > > Of course I agree that the current code is wrong here.  But I'm still not
> > > at all clear on what practical compatibility problems it introduces.  This
> > > change is OK if and only if we are really making the only area clean and
> > > well-defined in the same release cycle.
> > 
> > I don't follow what you meant by the above sentence.  Can you please
> > clarify a bit?
> 
> I think I meant to write "the whole area" rather than "the only area".
> The point I meant to make is that removing this (wrong) wake-up entirely
> should only be done when we are also thoroughly cleaning up and verifying
> everything about the behavior of stopping/resuming in ptrace.  When we get
> to a clear statement of what all the rules are and we really believe that
> the new code matches those rules, then fine.  When we are still in the
> quagmire, no change is better than an incremental change whose subtle
> ramifications we can't clearly articulate.

Hmm... I'm not sure I quite agree.  We sure should proceed with the
big picture in mind but procedding gradually is usually the better and
sometimes the only way to get there.  Unconditional wake up doesn't
belong anywhere no matter which direction we end up taking.  It's a
natural step to take.

> > It's slightly more serious than that.  Calling wake_up_process()
> > unconditionally can cause a quite serious failure.  It wakes up a
> > process in any state and there are code paths which aren't prepared to
> > be woken up unless certain conditions are met.  
> 
> I understand that.  But I don't recall having seen anyone cite an actual
> scenario in the real world where it does something problematic.

I don't think it will be easily seen during regular use.  Debugging is
often pretty slow paced and tasks are usually stopped in well known
places.  But it does make a pretty decent attack vector for malicious
users.  I don't think it will take too much effort to come up with a
proof-of-concept DOS attack exploiting this.  It's plain dangerous to
do this no matter how unlikely regular users might hit it.

> > A safer choice can be changing it to wake up only tasks in interruptible
> > sleep.
> 
> For ptrace-related purposes, wake_up_state(child, TASK_TRACED|TASK_STOPPED)
> ought to cover anything that it is doing now that anything could reasonably
> be relying on.

That probably is something we need to do for -stable, I agree.

> > I have very difficult time imagining what real user visible
> > implication it would have in negative and noticeable manner, so while
> > I do agree that we should proceed carefully, I think it would be
> > better to take the chance and remove the erratic behavior.  We have
> > release cycles and testing at multiple layers after all.  If we find
> > out that it breaks something in serious manner, we can always revert.
> 
> "Difficult time imagining" and ptrace subtleties unfortunately often go
> together.

But that's the only way forward.  Ptrace sure is complicated, some
part due to its inherent nature but more because things were
continuously piled on top of it without proper clean up, but other
parts of kernel are complicated too and we make changes all the time.
We have the whole release cycle structure and testing hierarchies
throughout the ecosystem to enable changes like this.

> I want to be sure we are doing something right, rather than just
> being thoroughly unsure that we aren't doing something wrong.

But this is something which is apparently wrong and highly unlikely to
cause any problem especially because with the patchset an attached
task is always in traced state on detach and thus always gets
signal_wake_up().  The only difference is removal of an extra spurious
unconditional wakeup which may or may not disturb the group stop
depending on timing.  If we can't change this, I don't know what we
can.

Thank you.

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