[PATCH] sysfs: Take sysfs_mutex when fetching the root inode.

[PATCH] sysfs: Take sysfs_mutex when fetching the root inode.

[Eric W. Biederman]

sysfs_get_inode ultimately calls sysfs_count_nlink when the a
directory inode is fectched.  sysfs_count_nlink needs to be
called under the sysfs_mutex to guard against the unlikely
but possible scenario that the root directory is changing
as we are counting the number entries in it, and just in
general to be consistent.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 fs/sysfs/mount.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index ab343e3..f3e34fd 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -53,7 +53,9 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
 	sysfs_sb = sb;
 
 	/* get root inode, initialize and unlock it */
+	mutex_lock(&sysfs_mutex);
 	inode = sysfs_get_inode(&sysfs_root);
+	mutex_unlock(&sysfs_mutex);
 	if (!inode) {
 		pr_debug("sysfs: could not get root inode\n");
 		return -ENOMEM;
-- 
1.5.6.3

[PATCH] sysfs: Reference sysfs_dirent from sysfs inodes.

[Eric W. Biederman]

The sysfs_dirent serves as both an inode and a directory entry
for sysfs.  To prevent the sysfs inode numbers from being freed
prematurely hold a reference to sysfs_dirent from the sysfs inode.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 fs/sysfs/inode.c |   10 ++++++++++
 fs/sysfs/mount.c |    1 +
 fs/sysfs/sysfs.h |    1 +
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index dfa3d94..dd269df 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -147,6 +147,7 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
 {
 	struct bin_attribute *bin_attr;
 
+	inode->i_private = sysfs_get(sd);
 	inode->i_mapping->a_ops = &sysfs_aops;
 	inode->i_mapping->backing_dev_info = &sysfs_backing_dev_info;
 	inode->i_op = &sysfs_inode_operations;
@@ -214,6 +215,15 @@ struct inode * sysfs_get_inode(struct sysfs_dirent *sd)
 	return inode;
 }
 
+void sysfs_delete_inode(struct inode *inode)
+{
+	struct sysfs_dirent *sd  = inode->i_private;
+
+	truncate_inode_pages(&inode->i_data, 0);
+	clear_inode(inode);
+	sysfs_put(sd);
+}
+
 int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name)
 {
 	struct sysfs_addrm_cxt acxt;
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index f3e34fd..59fcd58 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -30,6 +30,7 @@ struct kmem_cache *sysfs_dir_cachep;
 static const struct super_operations sysfs_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= generic_delete_inode,
+	.delete_inode	= sysfs_delete_inode,
 };
 
 struct sysfs_dirent sysfs_root = {
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 93c6d6b..9055d04 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -145,6 +145,7 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)
  * inode.c
  */
 struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
+void sysfs_delete_inode(struct inode *inode);
 int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
 int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name);
 int sysfs_inode_init(void);
-- 
1.5.6.3

Re: [PATCH] sysfs: Reference sysfs_dirent from sysfs inodes.

[Tejun Heo]

Eric W. Biederman wrote:
> The sysfs_dirent serves as both an inode and a directory entry
> for sysfs.  To prevent the sysfs inode numbers from being freed
> prematurely hold a reference to sysfs_dirent from the sysfs inode.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Thanks for working on this.  Can you please add a comment explaining
it on top of sysfs_delete_inode()?

-- 
tejun

Re: [PATCH] sysfs: Reference sysfs_dirent from sysfs inodes.

[Andrew Morton]

On Fri, 23 Jan 2009 15:33:48 +0900
Tejun Heo <tj@kernel.org> wrote:

> Eric W. Biederman wrote:
> > The sysfs_dirent serves as both an inode and a directory entry
> > for sysfs.  To prevent the sysfs inode numbers from being freed
> > prematurely hold a reference to sysfs_dirent from the sysfs inode.
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> 
> Thanks for working on this.  Can you please add a comment explaining
> it on top of sysfs_delete_inode()?

Like this?

--- a/fs/sysfs/inode.c~sysfs-reference-sysfs_dirent-from-sysfs-inodes-fix
+++ a/fs/sysfs/inode.c
@@ -215,6 +215,13 @@ struct inode * sysfs_get_inode(struct sy
 	return inode;
 }
 
+/*
+ * The sysfs_dirent serves as both an inode and a directory entry for sysfs.
+ * To prevent the sysfs inode numbers from being freed prematurely we take a
+ * reference to sysfs_dirent from the sysfs inode.  A
+ * super_operations.delete_inode() implementation is needed to drop that
+ * reference upon inode destruction.
+ */
 void sysfs_delete_inode(struct inode *inode)
 {
 	struct sysfs_dirent *sd  = inode->i_private;
_

Re: [PATCH] sysfs: Reference sysfs_dirent from sysfs inodes.

[Tejun Heo]

Andrew Morton wrote:
> On Fri, 23 Jan 2009 15:33:48 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> Eric W. Biederman wrote:
>>> The sysfs_dirent serves as both an inode and a directory entry
>>> for sysfs.  To prevent the sysfs inode numbers from being freed
>>> prematurely hold a reference to sysfs_dirent from the sysfs inode.
>>>
>>> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>> Thanks for working on this.  Can you please add a comment explaining
>> it on top of sysfs_delete_inode()?
> 
> Like this?
> 
> --- a/fs/sysfs/inode.c~sysfs-reference-sysfs_dirent-from-sysfs-inodes-fix
> +++ a/fs/sysfs/inode.c
> @@ -215,6 +215,13 @@ struct inode * sysfs_get_inode(struct sy
>  	return inode;
>  }
>  
> +/*
> + * The sysfs_dirent serves as both an inode and a directory entry for sysfs.
> + * To prevent the sysfs inode numbers from being freed prematurely we take a
> + * reference to sysfs_dirent from the sysfs inode.  A
> + * super_operations.delete_inode() implementation is needed to drop that
> + * reference upon inode destruction.
> + */
>  void sysfs_delete_inode(struct inode *inode)
>  {
>  	struct sysfs_dirent *sd  = inode->i_private;

Yeap, looks good to me.  :-)

Thanks.

-- 
tejun

Re: [PATCH] sysfs: Reference sysfs_dirent from sysfs inodes.

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Andrew Morton wrote:
>> On Fri, 23 Jan 2009 15:33:48 +0900
>> Tejun Heo <tj@kernel.org> wrote:
>> 
>>> Eric W. Biederman wrote:
>>>> The sysfs_dirent serves as both an inode and a directory entry
>>>> for sysfs.  To prevent the sysfs inode numbers from being freed
>>>> prematurely hold a reference to sysfs_dirent from the sysfs inode.
>>>>
>>>> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>>> Thanks for working on this.  Can you please add a comment explaining
>>> it on top of sysfs_delete_inode()?
>> 
>> Like this?
>> 
>> --- a/fs/sysfs/inode.c~sysfs-reference-sysfs_dirent-from-sysfs-inodes-fix
>> +++ a/fs/sysfs/inode.c
>> @@ -215,6 +215,13 @@ struct inode * sysfs_get_inode(struct sy
>>  	return inode;
>>  }
>>  
>> +/*
>> + * The sysfs_dirent serves as both an inode and a directory entry for sysfs.
>> + * To prevent the sysfs inode numbers from being freed prematurely we take a
>> + * reference to sysfs_dirent from the sysfs inode.  A
>> + * super_operations.delete_inode() implementation is needed to drop that
>> + * reference upon inode destruction.
>> + */
>>  void sysfs_delete_inode(struct inode *inode)
>>  {
>>  	struct sysfs_dirent *sd  = inode->i_private;
>
> Yeap, looks good to me.  :-)

Thanks guys.

Eric

Re: [PATCH] sysfs: Take sysfs_mutex when fetching the root inode.

[Tejun Heo]

Eric W. Biederman wrote:
> sysfs_get_inode ultimately calls sysfs_count_nlink when the a
> directory inode is fectched.  sysfs_count_nlink needs to be
> called under the sysfs_mutex to guard against the unlikely
> but possible scenario that the root directory is changing
> as we are counting the number entries in it, and just in
> general to be consistent.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun