Re: What protection does sysfs_readdir have with SMP/Preemption?

[Maneesh Soni <maneesh@in.ibm.com>]

On Wed, Nov 23, 2005 at 09:15:44AM -0500, Steven Rostedt wrote:
> On Wed, 2005-11-23 at 19:28 +0530, Maneesh Soni wrote:
> 
> > 
> > hmm looks like we got some situation which is not desirable and could lead
> > to bogus sysfs_dirent in the parent list. It may not be the exact problem
> > in this case though, but needs fixing IMO.
> > 
> > After sysfs_make_dirent(), the ref count for sysfs dirent will be 2.
> > (one from allocation, and after linking the new dentry to it). On
> > error from sysfs_create(), we do sysfs_put() once, decrementing the
> > ref count to 1. And again when the new dentry for which we couldn't
> > allocate the d_inode, is d_drop()'ed. In sysfs_d_iput() we again
> > sysfs_put(), and decrement the sysfs dirent's ref count to 0, which will
> > be the final sysfs_put(), and it will free the sysfs_dirent but never
> > unlinks it from the parent list. So, parent list could still will having
> > links to the freed sysfs_dirent in its s_children list.
> > 
> > so basically list_del_init(&sd->s_sibling) should be done in error path
> > in create_dir().
> > 
> > Could you also put the appended patch in your trial runs..
> > 
> 
> I'm already playing around with this. You might want this patch instead.
> I noticed that if sysfs_make_dirent fails to allocate the sd, then a
> null will be passed to sysfs_put.
> 
> But this is not the end of the problems.  I'll follow up on that comment
> right after this.
> 
> -- Steve
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Index: linux-2.6.15-rc2-git2/fs/sysfs/dir.c
> ===================================================================
> --- linux-2.6.15-rc2-git2.orig/fs/sysfs/dir.c	2005-11-23 08:40:33.000000000 -0500
> +++ linux-2.6.15-rc2-git2/fs/sysfs/dir.c	2005-11-23 08:52:57.000000000 -0500
> @@ -112,7 +112,11 @@
>  			}
>  		}
>  		if (error && (error != -EEXIST)) {
> -			sysfs_put((*d)->d_fsdata);
> +			struct sysfs_dirent *sd = (*d)->d_fsdata;
> +			if (sd) {
> + 				list_del_init(&sd->s_sibling);
> +				sysfs_put(sd);
> +			}
>  			d_drop(*d);
>  		}
>  		dput(*d);
> 
> 

Agreed. This makes more sense.


Thanks
Maneesh


-- 
Maneesh Soni
Linux Technology Center, 
IBM India Software Labs,
Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-25044990