On Thursday 10 May 2007 07:20, Andrew Morton wrote:
> On Wed, 9 May 2007 14:13:27 +0530 Sripathi Kodi <[EMAIL PROTECTED]>
wrote:
> > Hi,
> >
> > This patch makes ptrace_attach use write_trylock_irqsave.
> >
> > Signed-off-by: Sripathi Kodi <[EMAIL PROTECTED]>
> >
> > ---
> > kernel/ptrace.c | 7 +++----
> > 1 files changed, 3 insertions(+), 4 deletions(-)
> >
> > Index: linux-2.6.21/kernel/ptrace.c
> > ===================================================================
> > --- linux-2.6.21.orig/kernel/ptrace.c
> > +++ linux-2.6.21/kernel/ptrace.c
> > @@ -160,6 +160,7 @@ int ptrace_may_attach(struct task_struct
> > int ptrace_attach(struct task_struct *task)
> > {
> > int retval;
> > + unsigned long flags = 0;
> >
> > retval = -EPERM;
> > if (task->pid <= 1)
> > @@ -178,9 +179,7 @@ repeat:
> > * cpu's that may have task_lock).
> > */
> > task_lock(task);
> > - local_irq_disable();
> > - if (!write_trylock(&tasklist_lock)) {
> > - local_irq_enable();
> > + if (!write_trylock_irqsave(&tasklist_lock, flags)) {
> > task_unlock(task);
> > do {
> > cpu_relax();
> > @@ -208,7 +207,7 @@ repeat:
> > force_sig_specific(SIGSTOP, task);
> >
> > bad:
> > - write_unlock_irq(&tasklist_lock);
> > + write_unlock_irqrestore(&tasklist_lock, flags);
> > task_unlock(task);
> > out:
> > return retval;
>
> Your changelogs aren't vey logical. The context for this change is off in
>
> a different patch. I reproduce it here:
> > I am trying to fix the BUG I mentioned here:
> > http://lkml.org/lkml/2007/04/20/41. I noticed that an elegant way to
> > solve this problem is to have a write_trylock_irqsave helper function.
> > Since we don't have this now, the code in ptrace_attach implements it
> > using local_irq_disable and write_trylock. I wish to add
> > write_trylock_irqsave to mainline kernel and then fix the -rt specific
> > problem using this.
>
> I can't imagine why -rt's write_unlock_irq() doesn't do local_irq_enable().
-rt's write_unlock_irq() does local_irq_enable() while dealing with 'raw'
rwlock_t. However tasklist_lock is a regular rwlock_t and hence -rt doesn't
save/restore irqs while dealing with it. The probelm in ptrace_attach arises
because we explicitely call local_irq_disable(), whereas write_unlock_irq()
doesn't restore them.
>
> I have no problem adding write_trylock_irqsave() - it fills a gap in the
> API.
>
> Once we have write_trylock_irqsave() it makes sense to use it here.
>
> One the downside, we added a few bytes to the SMP kernel, which I guess we
> can live with.
>
> Whether this change is desired in -rt I don't know. Ingo?
I will send a patch against -rt in a little while.
>
> I don't think the initialisation of `flags' there was needed?
Yes, it was not needed. I will send a patch to remove it.
Thanks,
Sripathi.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
