[PATCH] proc: calculate the correct /proc/<pid> link count

[PATCH] proc: calculate the correct /proc/<pid> link count

[Vegard Nossum]

From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Mon, 2 Jun 2008 08:57:45 +0200
Subject: [PATCH] proc: calculate the correct /proc/<pid> link count

commit e9720acd728a46cb40daa52c99a979f7c4ff195c introduced a
/proc/self/net directory without bumping the corresponding link count
for /proc/self.

This patch replaces the static link count initializations with a call
that counts the number of directory entries in the given pid_entry
table whenever it is instantiated, and thus relieves the burden of
manually keeping the two in sync.

Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Pavel Emelyanov <xemul@openvz.org>
Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 fs/proc/base.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index c447e07..334ce46 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -127,6 +127,23 @@ struct pid_entry {
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = &proc_##OTYPE } )
 
+/* Count the number of hardlinks for the pid_entry table, excluding the .
+ * and .. links. */
+static unsigned int pid_entry_count_dirs(const struct pid_entry *entries,
+	unsigned int n)
+{
+	unsigned int i;
+	unsigned int count;
+
+	count = 0;
+	for (i = 0; i < n; ++i) {
+		if (S_ISDIR(entries[i].mode))
+			++count;
+	}
+
+	return count;
+}
+
 int maps_protect;
 EXPORT_SYMBOL(maps_protect);
 
@@ -2585,10 +2602,9 @@ static struct dentry *proc_pid_instantiate(struct inode *dir,
 	inode->i_op = &proc_tgid_base_inode_operations;
 	inode->i_fop = &proc_tgid_base_operations;
 	inode->i_flags|=S_IMMUTABLE;
-	inode->i_nlink = 5;
-#ifdef CONFIG_SECURITY
-	inode->i_nlink += 1;
-#endif
+
+	inode->i_nlink = 2 + pid_entry_count_dirs(tgid_base_stuff,
+		ARRAY_SIZE(tgid_base_stuff));
 
 	dentry->d_op = &pid_dentry_operations;
 
@@ -2816,10 +2832,9 @@ static struct dentry *proc_task_instantiate(struct inode *dir,
 	inode->i_op = &proc_tid_base_inode_operations;
 	inode->i_fop = &proc_tid_base_operations;
 	inode->i_flags|=S_IMMUTABLE;
-	inode->i_nlink = 4;
-#ifdef CONFIG_SECURITY
-	inode->i_nlink += 1;
-#endif
+
+	inode->i_nlink = 2 + pid_entry_count_dirs(tid_base_stuff,
+		ARRAY_SIZE(tid_base_stuff));
 
 	dentry->d_op = &pid_dentry_operations;
 
-- 
1.5.4.1

Re: [PATCH] proc: calculate the correct /proc/<pid> link count

[Andrew Morton]

On Tue, 3 Jun 2008 15:16:59 +0200 Vegard Nossum <vegard.nossum@gmail.com> wrote:

> From: Vegard Nossum <vegard.nossum@gmail.com>
> Date: Mon, 2 Jun 2008 08:57:45 +0200
> Subject: [PATCH] proc: calculate the correct /proc/<pid> link count
> 
> commit e9720acd728a46cb40daa52c99a979f7c4ff195c

Linus gets upset when we refer to commits only by their hash.

I did this:

This patch:

  commit e9720acd728a46cb40daa52c99a979f7c4ff195c
  Author: Pavel Emelyanov <xemul@openvz.org>
  Date:   Fri Mar 7 11:08:40 2008 -0800

    [NET]: Make /proc/net a symlink on /proc/self/net (v3)

introduced a /proc/self/net directory without bumping the corresponding
link count for /proc/self.

> introduced a
> /proc/self/net directory without bumping the corresponding link count
> for /proc/self.
> 
> This patch replaces the static link count initializations with a call
> that counts the number of directory entries in the given pid_entry
> table whenever it is instantiated, and thus relieves the burden of
> manually keeping the two in sync.

Seems much saner.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c447e07..334ce46 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -127,6 +127,23 @@ struct pid_entry {
>  		NULL, &proc_single_file_operations,	\
>  		{ .proc_show = &proc_##OTYPE } )
>  
> +/* Count the number of hardlinks for the pid_entry table, excluding the .
> + * and .. links. */

Like this:

/*
 * Count the number of hardlinks for the pid_entry table, excluding the .
 * and .. links.
 */

please.

> +static unsigned int pid_entry_count_dirs(const struct pid_entry *entries,
> +	unsigned int n)
> +{
> +	unsigned int i;
> +	unsigned int count;
> +
> +	count = 0;
> +	for (i = 0; i < n; ++i) {
> +		if (S_ISDIR(entries[i].mode))
> +			++count;
> +	}
> +
> +	return count;
> +}

I'm unable to correlate the code with the comment.  There is nothing in
here which handles .  and ..?

 
> @@ -2585,10 +2602,9 @@ static struct dentry *proc_pid_instantiate(struct inode *dir,
>  	inode->i_op = &proc_tgid_base_inode_operations;
>  	inode->i_fop = &proc_tgid_base_operations;
>  	inode->i_flags|=S_IMMUTABLE;
> -	inode->i_nlink = 5;
> -#ifdef CONFIG_SECURITY
> -	inode->i_nlink += 1;
> -#endif
> +
> +	inode->i_nlink = 2 + pid_entry_count_dirs(tgid_base_stuff,
> +		ARRAY_SIZE(tgid_base_stuff));
>  

oh, can we do that?  Is it possible for some code somewhere to come
along and add a new entry to /proc/pid which doesn't appear in
these static tables?

I guess that doesn't happen.  In which case can we not calculate the
unmber of directories in these two tables just a single time, at
bootup?

I think I'm missing things here...

Re: [PATCH] proc: calculate the correct /proc/<pid> link count

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Tue, 3 Jun 2008 15:16:59 +0200 Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
>> From: Vegard Nossum <vegard.nossum@gmail.com>
>> Date: Mon, 2 Jun 2008 08:57:45 +0200
>> Subject: [PATCH] proc: calculate the correct /proc/<pid> link count
>> 
>> commit e9720acd728a46cb40daa52c99a979f7c4ff195c

I sent a message acking the patch but it seems to have gotten lost.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>> +static unsigned int pid_entry_count_dirs(const struct pid_entry *entries,
>> +	unsigned int n)
>> +{
>> +	unsigned int i;
>> +	unsigned int count;
>> +
>> +	count = 0;
>> +	for (i = 0; i < n; ++i) {
>> +		if (S_ISDIR(entries[i].mode))
>> +			++count;
>> +	}
>> +
>> +	return count;
>> +}
>
> I'm unable to correlate the code with the comment.  There is nothing in
> here which handles .  and ..?

Because they don't appear in the table.  It seems the comment was to make
that clear.

>> @@ -2585,10 +2602,9 @@ static struct dentry *proc_pid_instantiate(struct inode
> *dir,
>>  	inode->i_op = &proc_tgid_base_inode_operations;
>>  	inode->i_fop = &proc_tgid_base_operations;
>>  	inode->i_flags|=S_IMMUTABLE;
>> -	inode->i_nlink = 5;
>> -#ifdef CONFIG_SECURITY
>> -	inode->i_nlink += 1;
>> -#endif
>> +
>> +	inode->i_nlink = 2 + pid_entry_count_dirs(tgid_base_stuff,
>> +		ARRAY_SIZE(tgid_base_stuff));
>>  
>
> oh, can we do that?  Is it possible for some code somewhere to come
> along and add a new entry to /proc/pid which doesn't appear in
> these static tables?

Currently we do not dynamically modify the pid_entry tables.
On some days I think it would be a nice addition, to make it easier to
handle modular subsystems.  Given the number of #ifdefs we have
in those tables something more dynamic may ultimately be the way
to go.

However we still have in lookup:
	/*
	 * Yes, it does not scale. And it should not. Don't add
	 * new entries into /proc/<tgid>/ without very good reasons.
	 */
Which doesn't seem to have much impact as these directories are
slowly growing.


> I guess that doesn't happen.  In which case can we not calculate the
> unmber of directories in these two tables just a single time, at
> bootup?
>
> I think I'm missing things here...

Not much.  

Historically /proc used to be very bad with the link counts on
directories.   There was a switch statement that hard coded nlinks for
every directory, and only used the values 2 or 3.  Last time I was in
there I fixed it up so we actually returned the proper hard link
counts for the directories.  In this last conversation I realized we
could be more maintainable without a hard coded number.

Eric