/proc visibility patch breaks GDB, etc.

/proc visibility patch breaks GDB, etc.

[Peter Chubb]

In fs/proc/base.c:proc_pid_lookup(), the patch

        read_unlock(&tasklist_lock); 
        if (!task) 
                goto out; 
+       if (!thread_group_leader(task)) 
+               goto out_drop_task; 
  
        inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO); 

means that threads other than the thread group leader don't appear in
the /proc top-level directory.  Programs that are informed via pid of
events can no longer find the appropriate process -- for example,
using gdb on a multi-threaded process, or profiling using perfmon.

The immediate symptom is GDB saying:
    Could not open /proc/757/status
when 757 is a TID not a PID.

Re: /proc visibility patch breaks GDB, etc.

[Andrew Morton]

Peter Chubb <peter@chubb.wattle.id.au> wrote:
>
> 
> In fs/proc/base.c:proc_pid_lookup(), the patch
> 
>         read_unlock(&tasklist_lock); 
>         if (!task) 
>                 goto out; 
> +       if (!thread_group_leader(task)) 
> +               goto out_drop_task; 
>   
>         inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO); 
> 
> means that threads other than the thread group leader don't appear in
> the /proc top-level directory.  Programs that are informed via pid of
> events can no longer find the appropriate process -- for example,
> using gdb on a multi-threaded process, or profiling using perfmon.
> 
> The immediate symptom is GDB saying:
>     Could not open /proc/757/status
> when 757 is a TID not a PID.

What does `ls /proc/757' say?  Presumably no such file or directory?
It's fairly bizare behaviour to be able to open files which don't exist
according to readdir, which is why we made that change.

The node to tid 757 should appear under its thread group leader's
directory: /proc/NNN/757.  That node should be visible to readdir, and that
is the node which gdb and perfmon should be looking for.

Of course, we may have burnt our boats with the initial silliness.  It's a
shame that gdb and perfmon went and used it.

Re: /proc visibility patch breaks GDB, etc.

[Daniel Jacobowitz]

On Wed, Feb 25, 2004 at 10:44:10PM -0800, Andrew Morton wrote:
> Peter Chubb <peter@chubb.wattle.id.au> wrote:
> >
> > 
> > In fs/proc/base.c:proc_pid_lookup(), the patch
> > 
> >         read_unlock(&tasklist_lock); 
> >         if (!task) 
> >                 goto out; 
> > +       if (!thread_group_leader(task)) 
> > +               goto out_drop_task; 
> >   
> >         inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO); 
> > 
> > means that threads other than the thread group leader don't appear in
> > the /proc top-level directory.  Programs that are informed via pid of
> > events can no longer find the appropriate process -- for example,
> > using gdb on a multi-threaded process, or profiling using perfmon.
> > 
> > The immediate symptom is GDB saying:
> >     Could not open /proc/757/status
> > when 757 is a TID not a PID.
> 
> What does `ls /proc/757' say?  Presumably no such file or directory?
> It's fairly bizare behaviour to be able to open files which don't exist
> according to readdir, which is why we made that change.
> 
> The node to tid 757 should appear under its thread group leader's
> directory: /proc/NNN/757.  That node should be visible to readdir, and that
> is the node which gdb and perfmon should be looking for.
> 
> Of course, we may have burnt our boats with the initial silliness.  It's a
> shame that gdb and perfmon went and used it.

GDB has used it since long before any of this happened.  LinuxThreads
didn't use thread groups, remember?

There is no way from GDB to figure out which thread is the thread group
leader, either, so I don't know how you propose it find the proc
directory now.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: /proc visibility patch breaks GDB, etc.

[David Mosberger]

>>>>> On Wed, 25 Feb 2004 22:44:10 -0800, Andrew Morton <akpm@osdl.org> said:

  Andrew> Peter Chubb <peter@chubb.wattle.id.au> wrote:
  >> 
  >> 
  >> In fs/proc/base.c:proc_pid_lookup(), the patch
  >> 
  >> read_unlock(&tasklist_lock); if (!task) goto out; + if
  >> (!thread_group_leader(task)) + goto out_drop_task;
  >> 
  >> inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO);
  >> 
  >> means that threads other than the thread group leader don't
  >> appear in the /proc top-level directory.  Programs that are
  >> informed via pid of events can no longer find the appropriate
  >> process -- for example, using gdb on a multi-threaded process, or
  >> profiling using perfmon.
  >> 
  >> The immediate symptom is GDB saying: Could not open
  >> /proc/757/status when 757 is a TID not a PID.

  Andrew> What does `ls /proc/757' say?  Presumably no such file or
  Andrew> directory?  It's fairly bizare behaviour to be able to open
  Andrew> files which don't exist according to readdir, which is why
  Andrew> we made that change.

Excuse, but this seems seriously FOOBAR.  I understand that it's
interesting to see the thread-leader/thread relationship, but surely
that's no reason to break backwards compatibility and the ability to
look up _any_ task's info via /proc/PID/.  A program that only wants
to show "processes" (thread-group leaders) can simply read
/proc/PID/status and ignore the entries for which Tgid != PPid.

Perhaps you could put relative symlinks in task/?  Something like
this:

 $ ls -l /proc/self/task
 dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13494 -> ..
 dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13495 -> ../../13495

perhaps?

	--david

Re: /proc visibility patch breaks GDB, etc.

[Andrew Morton]

David Mosberger <davidm@napali.hpl.hp.com> wrote:
>
> >>>>> On Wed, 25 Feb 2004 22:44:10 -0800, Andrew Morton <akpm@osdl.org> said:
> 
>   Andrew> Peter Chubb <peter@chubb.wattle.id.au> wrote:
>   >> 
>   >> 
>   >> In fs/proc/base.c:proc_pid_lookup(), the patch
>   >> 
>   >> read_unlock(&tasklist_lock); if (!task) goto out; + if
>   >> (!thread_group_leader(task)) + goto out_drop_task;
>   >> 
>   >> inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO);
>   >> 
>   >> means that threads other than the thread group leader don't
>   >> appear in the /proc top-level directory.  Programs that are
>   >> informed via pid of events can no longer find the appropriate
>   >> process -- for example, using gdb on a multi-threaded process, or
>   >> profiling using perfmon.
>   >> 
>   >> The immediate symptom is GDB saying: Could not open
>   >> /proc/757/status when 757 is a TID not a PID.
> 
>   Andrew> What does `ls /proc/757' say?  Presumably no such file or
>   Andrew> directory?  It's fairly bizare behaviour to be able to open
>   Andrew> files which don't exist according to readdir, which is why
>   Andrew> we made that change.
> 
> Excuse, but this seems seriously FOOBAR.  I understand that it's
> interesting to see the thread-leader/thread relationship, but surely
> that's no reason to break backwards compatibility and the ability to
> look up _any_ task's info via /proc/PID/.

Well you can't look them up - you can only open them.  But I take your
point.  In another life, these things would appear under a special
/proc/magical_directory_which_has_dopey_semantics.

> A program that only wants
> to show "processes" (thread-group leaders) can simply read
> /proc/PID/status and ignore the entries for which Tgid != PPid.
> 
> Perhaps you could put relative symlinks in task/?  Something like
> this:
> 
>  $ ls -l /proc/self/task
>  dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13494 -> ..
>  dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13495 -> ../../13495
> 
> perhaps?

Well the contents of /proc/pid/task are OK at present.

I guess we should revert that change.

Re: /proc visibility patch breaks GDB, etc.

[Peter Chubb]

>>>>> "David" == David Mosberger <davidm@napali.hpl.hp.com> writes:

>>>>> On Wed, 25 Feb 2004 22:44:10 -0800, Andrew Morton <akpm@osdl.org> said:
Andrew> Peter Chubb <peter@chubb.wattle.id.au> wrote:
  >> 
  >> The immediate symptom is GDB saying: Could not open
  >> /proc/757/status when 757 is a TID not a PID.

Andrew> What does `ls /proc/757' say?  Presumably no such file or
Andrew> directory?  It's fairly bizare behaviour to be able to open
Andrew> files which don't exist according to readdir, which is why
Andrew> we made that change.

David> Excuse, but this seems seriously FOOBAR.  I understand that
David> it's interesting to see the thread-leader/thread relationship,
David> but surely that's no reason to break backwards compatibility
David> and the ability to look up _any_ task's info via /proc/PID/.  A
David> program that only wants to show "processes" (thread-group
David> leaders) can simply read /proc/PID/status and ignore the
David> entries for which Tgid != PPid.

Think of scanning /proc on a large SMP machine (e.g., Altix with >512
processors) running lots and lots of multithreaded processes to find
all processes --- like ps(1) does --- and you can see why not
cluttering up /proc with threads is a good idea.


--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

Re: /proc visibility patch breaks GDB, etc.

[Kingsley Cheung]

On Thu, Feb 26, 2004 at 12:09:59PM -0800, Andrew Morton wrote:
> David Mosberger <davidm@napali.hpl.hp.com> wrote:
> >
> > >>>>> On Wed, 25 Feb 2004 22:44:10 -0800, Andrew Morton <akpm@osdl.org> said:
> > 
> >   Andrew> Peter Chubb <peter@chubb.wattle.id.au> wrote:
> >   >> 
> >   >> 
> >   >> In fs/proc/base.c:proc_pid_lookup(), the patch
> >   >> 
> >   >> read_unlock(&tasklist_lock); if (!task) goto out; + if
> >   >> (!thread_group_leader(task)) + goto out_drop_task;
> >   >> 
> >   >> inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO);
> >   >> 
> >   >> means that threads other than the thread group leader don't
> >   >> appear in the /proc top-level directory.  Programs that are
> >   >> informed via pid of events can no longer find the appropriate
> >   >> process -- for example, using gdb on a multi-threaded process, or
> >   >> profiling using perfmon.
> >   >> 
> >   >> The immediate symptom is GDB saying: Could not open
> >   >> /proc/757/status when 757 is a TID not a PID.
> > 
> >   Andrew> What does `ls /proc/757' say?  Presumably no such file or
> >   Andrew> directory?  It's fairly bizare behaviour to be able to open
> >   Andrew> files which don't exist according to readdir, which is why
> >   Andrew> we made that change.
> > 
> > Excuse, but this seems seriously FOOBAR.  I understand that it's
> > interesting to see the thread-leader/thread relationship, but surely
> > that's no reason to break backwards compatibility and the ability to
> > look up _any_ task's info via /proc/PID/.
> 
> Well you can't look them up - you can only open them.  But I take your
> point.  In another life, these things would appear under a special
> /proc/magical_directory_which_has_dopey_semantics.
> 
> > A program that only wants
> > to show "processes" (thread-group leaders) can simply read
> > /proc/PID/status and ignore the entries for which Tgid != PPid.
> > 
> > Perhaps you could put relative symlinks in task/?  Something like
> > this:
> > 
> >  $ ls -l /proc/self/task
> >  dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13494 -> ..
> >  dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13495 -> ../../13495
> > 
> > perhaps?
> 
> Well the contents of /proc/pid/task are OK at present.
> 
> I guess we should revert that change.

Ah, so there was some fundamental reason behind that behaviour!
Perhaps then a comment or two in the code to explain that such
behaviour (prior to change) is intended in proc_pid_lookup()?  That
way it will be clear that is intended behaviour.

Am I correct to assume though that the corresponding change in
proc_task_lookup() should stay?  The existing behaviour there was that
one could do say,

cat /proc/<pid>/task/<tid>/stat, where tid could be any thread and not
a part of the thread group pid.  

The patch that broke backwards compatibility for GDB likewise changed
that.  It enforces that tid must be a part of the pid thread group.

--
		Kingsley

Re: /proc visibility patch breaks GDB, etc.

[Peter Chubb]

>>>>> "Kingsley" == Kingsley Cheung <kingsley@aurema.com> writes:

Kingsley> On Thu, Feb 26, 2004 at 12:09:59PM -0800, Andrew Morton
Kingsley> wrote:

Kingsley> The patch that broke backwards compatibility for GDB
Kingsley> likewise changed that.  It enforces that tid must be a part
Kingsley> of the pid thread group.

For what it's worth, the patch also breaks JVM 1.4re2.

Peter C

Re: /proc visibility patch breaks GDB, etc.

[Andrew Morton]

Kingsley Cheung <kingsley@aurema.com> wrote:
>
> Am I correct to assume though that the corresponding change in
> proc_task_lookup() should stay?  The existing behaviour there was that
> one could do say,
> 
> cat /proc/<pid>/task/<tid>/stat, where tid could be any thread and not
> a part of the thread group pid.  

That sounds especially broken - let's hope that nobody has started using it
(but how did you even discover this?  Code audit?)

How's this?

diff -puN fs/proc/base.c~proc-thread-visibility-revert fs/proc/base.c
--- 25/fs/proc/base.c~proc-thread-visibility-revert	Thu Feb 26 15:17:48 2004
+++ 25-akpm/fs/proc/base.c	Thu Feb 26 15:17:48 2004
@@ -1582,13 +1582,14 @@ struct dentry *proc_pid_lookup(struct in
 	read_unlock(&tasklist_lock);
 	if (!task)
 		goto out;
-	if (!thread_group_leader(task))
-		goto out_drop_task;
 
 	inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO);
 
-	if (!inode)
-		goto out_drop_task;
+
+	if (!inode) {
+		put_task_struct(task);
+		goto out;
+	}
 	inode->i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
 	inode->i_op = &proc_tgid_base_inode_operations;
 	inode->i_fop = &proc_tgid_base_operations;
@@ -1613,8 +1614,6 @@ struct dentry *proc_pid_lookup(struct in
 		goto out;
 	}
 	return NULL;
-out_drop_task:
-	put_task_struct(task);
 out:
 	return ERR_PTR(-ENOENT);
 }

_

Re: /proc visibility patch breaks GDB, etc.

[Kingsley Cheung]

On Thu, Feb 26, 2004 at 03:19:17PM -0800, Andrew Morton wrote:
> Kingsley Cheung <kingsley@aurema.com> wrote:
> >
> > Am I correct to assume though that the corresponding change in
> > proc_task_lookup() should stay?  The existing behaviour there was that
> > one could do say,
> > 
> > cat /proc/<pid>/task/<tid>/stat, where tid could be any thread and not
> > a part of the thread group pid.  
> 
> That sounds especially broken - let's hope that nobody has started using it
> (but how did you even discover this?  Code audit?)

Completely an accident on my part.  While writing code to traverse
threads in a group and obtain their data usage, I was comparing what I
could see from the output with the shell and I just happened to do a
typo on the tid value... To my suprise it worked.

> 
> How's this?

Looks like proc_pid_lookup() was never changed at all :)

> 
> diff -puN fs/proc/base.c~proc-thread-visibility-revert fs/proc/base.c
> --- 25/fs/proc/base.c~proc-thread-visibility-revert	Thu Feb 26 15:17:48 2004
> +++ 25-akpm/fs/proc/base.c	Thu Feb 26 15:17:48 2004
> @@ -1582,13 +1582,14 @@ struct dentry *proc_pid_lookup(struct in
>  	read_unlock(&tasklist_lock);
>  	if (!task)
>  		goto out;
> -	if (!thread_group_leader(task))
> -		goto out_drop_task;
>  
>  	inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO);
>  
> -	if (!inode)
> -		goto out_drop_task;
> +
> +	if (!inode) {
> +		put_task_struct(task);
> +		goto out;
> +	}
>  	inode->i_mode = S_IFDIR|S_IRUGO|S_IXUGO;
>  	inode->i_op = &proc_tgid_base_inode_operations;
>  	inode->i_fop = &proc_tgid_base_operations;
> @@ -1613,8 +1614,6 @@ struct dentry *proc_pid_lookup(struct in
>  		goto out;
>  	}
>  	return NULL;
> -out_drop_task:
> -	put_task_struct(task);
>  out:
>  	return ERR_PTR(-ENOENT);
>  }
> 
> _

-- 
		Kingsley

Re: /proc visibility patch breaks GDB, etc.

[Peter Chubb]

>>>>> "Andrew" == Andrew Morton <akpm@osdl.org> writes:

Andrew> Kingsley Cheung <kingsley@aurema.com> wrote:
>>  Am I correct to assume though that the corresponding change in
>> proc_task_lookup() should stay?  The existing behaviour there was
>> that one could do say,
>> 
>> cat /proc/<pid>/task/<tid>/stat, where tid could be any thread and
>> not a part of the thread group pid.

Andrew> That sounds especially broken - let's hope that nobody has
Andrew> started using it (but how did you even discover this?  Code
Andrew> audit?)

Andrew> How's this?

Looks fine to me --- should fix my immediate problems.  But maybe we
*want* (in 2.7?) to deprecate /proc/tid where tid is a thread ID not a
process ID.

--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*