[PATCH 8/8] fix handling of st_nlink on procfs root

[PATCH 8/8] fix handling of st_nlink on procfs root

[Al Viro]

Date: 1139427460 -0500

1) it should use nr_processes(), not nr_threads; otherwise we are getting
very confused find(1) and friends, among other things.
2) better do that at stat() time than at every damn lookup in procfs root.

Patch had been sitting in FC4 kernels for many months now...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

---

 fs/proc/inode.c |    4 ----
 fs/proc/root.c  |   17 +++++++++--------
 2 files changed, 9 insertions(+), 12 deletions(-)

ec5a7567ad58f55067f527e700723480cfbdbee5
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 6573f31..075d3e9 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -204,10 +204,6 @@ int proc_fill_super(struct super_block *
 	root_inode = proc_get_inode(s, PROC_ROOT_INO, &proc_root);
 	if (!root_inode)
 		goto out_no_root;
-	/*
-	 * Fixup the root inode's nlink value
-	 */
-	root_inode->i_nlink += nr_processes();
 	root_inode->i_uid = 0;
 	root_inode->i_gid = 0;
 	s->s_root = d_alloc_root(root_inode);
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 6889628..c3fd361 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -80,16 +80,16 @@ void __init proc_root_init(void)
 	proc_bus = proc_mkdir("bus", NULL);
 }
 
-static struct dentry *proc_root_lookup(struct inode * dir, struct dentry * dentry, struct nameidata *nd)
+static int proc_root_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat
+)
 {
-	/*
-	 * nr_threads is actually protected by the tasklist_lock;
-	 * however, it's conventional to do reads, especially for
-	 * reporting, without any locking whatsoever.
-	 */
-	if (dir->i_ino == PROC_ROOT_INO) /* check for safety... */
-		dir->i_nlink = proc_root.nlink + nr_threads;
+	generic_fillattr(dentry->d_inode, stat);
+	stat->nlink = proc_root.nlink + nr_processes();
+	return 0;
+}
 
+static struct dentry *proc_root_lookup(struct inode * dir, struct dentry * dentry, struct nameidata *nd)
+{
 	if (!proc_lookup(dir, dentry, nd)) {
 		return NULL;
 	}
@@ -134,6 +134,7 @@ static struct file_operations proc_root_
  */
 static struct inode_operations proc_root_inode_operations = {
 	.lookup		= proc_root_lookup,
+	.getattr	= proc_root_getattr,
 };
 
 /*
-- 
0.99.9.GIT

Re: [PATCH 8/8] fix handling of st_nlink on procfs root

[Eric W. Biederman]

Al Viro <viro@ftp.linux.org.uk> writes:

> Date: 1139427460 -0500
>
> 1) it should use nr_processes(), not nr_threads; otherwise we are getting
> very confused find(1) and friends, among other things.
> 2) better do that at stat() time than at every damn lookup in procfs root.
>
> Patch had been sitting in FC4 kernels for many months now...

Ack.

There are some other similar problems still in /proc.

In my pid namespace work I have some managed to clean most of
this up, and finally split proc into two filesystems.

The only was I was able to get the union to work was
to let lookup return files in an internal mount.

The only problem was that /proc/irq/..  != /proc/

I will finish all of this up shortly but do you know a good
way to do a union mount when we mount proc?

Eric

Re: [PATCH 8/8] fix handling of st_nlink on procfs root

[Al Viro]

On Wed, Feb 08, 2006 at 06:04:36PM -0700, Eric W. Biederman wrote:
> There are some other similar problems still in /proc.
> 
> In my pid namespace work I have some managed to clean most of
> this up, and finally split proc into two filesystems.
> 
> The only was I was able to get the union to work was
> to let lookup return files in an internal mount.
> 
> The only problem was that /proc/irq/..  != /proc/

That's not the only problem here, unfortunately.

> I will finish all of this up shortly but do you know a good
> way to do a union mount when we mount proc?

Not transparently; mount(2) should _not_ mount two filesystems at once.
Note that you'll run into serious problems as soon as you try to mount/umount/
mount --move the stuff there.  And doing unionfs <spit> approach will cause
fsckloads of fun issues with lifetimes.

Re: [PATCH 8/8] fix handling of st_nlink on procfs root

[Eric W. Biederman]

Al Viro <viro@ftp.linux.org.uk> writes:

> On Wed, Feb 08, 2006 at 06:04:36PM -0700, Eric W. Biederman wrote:
>> There are some other similar problems still in /proc.
>> 
>> In my pid namespace work I have some managed to clean most of
>> this up, and finally split proc into two filesystems.
>> 
>> The only was I was able to get the union to work was
>> to let lookup return files in an internal mount.
>> 
>> The only problem was that /proc/irq/..  != /proc/
>
> That's not the only problem here, unfortunately.

Well at the moment it seems to be.  Basically a case of everything
seems to work but the semantics are weird and ugly, and not worth
doing if the legacy semantics are not maintained.

>> I will finish all of this up shortly but do you know a good
>> way to do a union mount when we mount proc?
>
> Not transparently; mount(2) should _not_ mount two filesystems at once.
> Note that you'll run into serious problems as soon as you try to mount/umount/
> mount --move the stuff there.  And doing unionfs <spit> approach will cause
> fsckloads of fun issues with lifetimes.

:)

Do you know if there is anything in what autofs does for mounts that
could be reused.

To a certain extent it would work find if I had a mount point and
all of the legacy directories were symlinks to it.

Anyway there are lots of possibilities and I will work something
out before it makes into the stable kernel.

I keep having the feeling that I might just wind up with everything
making sense under proc as I create more namespaces :)

Eric

Re: [PATCH 8/8] fix handling of st_nlink on procfs root

[Eric W. Biederman]

Al Viro <viro@ftp.linux.org.uk> writes:

> Date: 1139427460 -0500
>
> 1) it should use nr_processes(), not nr_threads; otherwise we are getting
> very confused find(1) and friends, among other things.
> 2) better do that at stat() time than at every damn lookup in procfs root.
>
> Patch had been sitting in FC4 kernels for many months now...

And it is actually wrong.  It fails to take into account the static
/proc entries.

> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 6889628..c3fd361 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -80,16 +80,16 @@ void __init proc_root_init(void)
>  	proc_bus = proc_mkdir("bus", NULL);
>  }
>  
> -static struct dentry *proc_root_lookup(struct inode * dir, struct dentry *
> dentry, struct nameidata *nd)
> +static int proc_root_getattr(struct vfsmount *mnt, struct dentry *dentry,
> struct kstat *stat
> +)
>  {
> -	/*
> -	 * nr_threads is actually protected by the tasklist_lock;
> -	 * however, it's conventional to do reads, especially for
> -	 * reporting, without any locking whatsoever.
> -	 */
> -	if (dir->i_ino == PROC_ROOT_INO) /* check for safety... */
> -		dir->i_nlink = proc_root.nlink + nr_threads;
> +	generic_fillattr(dentry->d_inode, stat);
> +	stat->nlink = proc_root.nlink + nr_processes();
> +	return 0;
> +}


proc_root_getattr should look more like below.
Notice the addition of de->nlink, which accounts for the
static entries as well.  

static int proc_root_getattr(struct vfsmount *mnt, struct dentry *dentry,
                               struct kstat *stat)
{
       struct inode *inode = dentry->d_inode;
       struct proc_dir_entry *de = PDE(inode);
       generic_fillattr(inode, stat);

       if (de && de->nlink)
               inode->i_nlink = de->nlink;

       /* Get the proper hardlink count */
       stat->nlink += nr_processes();
       return 0;

}

Re: [PATCH 8/8] fix handling of st_nlink on procfs root

[Al Viro]

On Wed, Feb 15, 2006 at 02:20:17AM -0700, Eric W. Biederman wrote:
> And it is actually wrong.  It fails to take into account the static
> /proc entries.
> > +	stat->nlink = proc_root.nlink + nr_processes();

The hell it does. See ^^^^^^^^^^^^^^ this.

Re: [PATCH 8/8] fix handling of st_nlink on procfs root

[Eric W. Biederman]

Al Viro <viro@ftp.linux.org.uk> writes:

> On Wed, Feb 15, 2006 at 02:20:17AM -0700, Eric W. Biederman wrote:
>> And it is actually wrong.  It fails to take into account the static
>> /proc entries.
>> > +	stat->nlink = proc_root.nlink + nr_processes();
>
> The hell it does. See ^^^^^^^^^^^^^^ this.

Right.  Sorry.  I had a similar issue and when I check your patch for the
same problem somehow I thought proc_root was a struct inode and not a
struct proc_dir_entry. 

Eric