Messages in this thread | | | Date | Thu, 24 Nov 2005 09:46:08 +0530 | From | Maneesh Soni <> | Subject | Re: What protection does sysfs_readdir have with SMP/Preemption? |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |