On Thu, Apr 01, 2004 at 09:56:16AM -0500, Alan Stern wrote:
> On Thu, 1 Apr 2004, Maneesh Soni wrote:
>
> > On Wed, Mar 31, 2004 at 10:11:37AM -0500, Alan Stern wrote:
> > >
> > > Here's a suggestion. At the start of check_perm() grab the dentry
> > > semaphore, then check whether d_fsdata is NULL, if it isn't then do the
> > > kobject_get(), then unlock the semaphore.
> > >
> >
> > I have tried this with no luck. I still get
> > (Badness in kobject_get at lib/kobject.c:42) which means it is not correct fix.
>
> Did you remember to set dentry->d_fsdata to NULL? This has to be done in
> sysfs_remove_dir() sometime during the period when dentry->d_inode->i_sem
> is held. Right now the pointer to the kobject never gets erased. That
> Badness message is an indication that you are using a valid pointer to a
> kobject whose refcount is 0.
>
Yes, see the patch below. Probably the race window has become smaller but
Badness message is also an indication that somewhere kobject_cleanup has
started. I have not yet seen why race is still there.
There is one more bigger problem in this approach. It may miss kobject_put() in
sysfs_release() call and result will be memory leak, ->release() is not called,
rmmod hang, etc etc. So using ->d_fsdata for this purpose is not the proper
solution, IMO.
cpu 0 kobj->refcount cpu 1
kobject_unregister() (1) sysfs_open_file()
kobject_del() check_perm()
sysfs_remove_dir() (2) kobject_get(->d_fsdata)
(->d_fsdata = NULL)
kobject_put() (1)
sysfs_release()
kobj == NULL
diff -urN linux-2.6.5-rc2/fs/sysfs/dir.c linux-2.6.5-rc2-race-fix/fs/sysfs/dir.c
--- linux-2.6.5-rc2/fs/sysfs/dir.c 2004-03-20 05:41:05.000000000 +0530
+++ linux-2.6.5-rc2-race-fix/fs/sysfs/dir.c 2004-04-02 09:36:53.174323584 +0530
@@ -132,6 +132,11 @@
pr_debug("sysfs %s: removing dir\n",dentry->d_name.name);
down(&dentry->d_inode->i_sem);
+ /* try to avoid further references to kobject even if dentry
+ * is alive after remove dir.
+ */
+ dentry->d_fsdata = NULL;
+
spin_lock(&dcache_lock);
restart:
node = dentry->d_subdirs.next;
diff -urN linux-2.6.5-rc2/fs/sysfs/file.c linux-2.6.5-rc2-race-fix/fs/sysfs/file.c
--- linux-2.6.5-rc2/fs/sysfs/file.c 2004-03-20 05:41:01.000000000 +0530
+++ linux-2.6.5-rc2-race-fix/fs/sysfs/file.c 2004-04-02 09:51:37.409899344 +0530
@@ -78,11 +78,22 @@
static int fill_read_buffer(struct file * file, struct sysfs_buffer * buffer)
{
struct attribute * attr = file->f_dentry->d_fsdata;
- struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
+ struct dentry * kobj_dir = file->f_dentry->d_parent;
+ struct kobject * kobj = NULL;
struct sysfs_ops * ops = buffer->ops;
int ret = 0;
ssize_t count;
+ down(&kobj_dir->d_inode->i_sem);
+ /* No need to take a ref. on kobject, as it has been taken
+ * during ->open(), but ->d_fsdata can become NULL
+ */
+ if (kobj_dir->d_fsdata)
+ kobj = kobj_dir->d_fsdata;
+ up(&kobj_dir->d_inode->i_sem);
+
+ if (!kobj)
+ return -EINVAL;
if (!buffer->page)
buffer->page = (char *) get_zeroed_page(GFP_KERNEL);
if (!buffer->page)
@@ -199,9 +210,21 @@
flush_write_buffer(struct file * file, struct sysfs_buffer * buffer, size_t count)
{
struct attribute * attr = file->f_dentry->d_fsdata;
- struct kobject * kobj = file->f_dentry->d_parent->d_fsdata;
+ struct dentry * kobj_dir = file->f_dentry->d_parent;
+ struct kobject * kobj = NULL;
struct sysfs_ops * ops = buffer->ops;
+ down(&kobj_dir->d_inode->i_sem);
+ /* No need to take a ref. on kobject, as it has been taken
+ * during ->open(), but ->d_fsdata can become NULL
+ */
+ if (kobj_dir->d_fsdata)
+ kobj = kobj_dir->d_fsdata;
+ up(&kobj_dir->d_inode->i_sem);
+
+ if (!kobj)
+ return -EINVAL;
+
return ops->store(kobj,attr,buffer->page,count);
}
@@ -238,12 +261,18 @@
static int check_perm(struct inode * inode, struct file * file)
{
- struct kobject * kobj = kobject_get(file->f_dentry->d_parent->d_fsdata);
+ struct dentry * kobj_dir = file->f_dentry->d_parent;
+ struct kobject * kobj = NULL;
struct attribute * attr = file->f_dentry->d_fsdata;
struct sysfs_buffer * buffer;
struct sysfs_ops * ops = NULL;
int error = 0;
+ down(&kobj_dir->d_inode->i_sem);
+ if (kobj_dir->d_fsdata)
+ kobj = kobject_get(kobj_dir->d_fsdata);
+ up(&kobj_dir->d_inode->i_sem);
+
if (!kobj || !attr)
goto Einval;
@@ -320,10 +349,19 @@
static int sysfs_release(struct inode * inode, struct file * filp)
{
- struct kobject * kobj = filp->f_dentry->d_parent->d_fsdata;
+ struct dentry * kobj_dir = file->f_dentry->d_parent;
+ struct kobject * kobj = NULL;
struct attribute * attr = filp->f_dentry->d_fsdata;
struct sysfs_buffer * buffer = filp->private_data;
+ down(&kobj_dir->d_inode->i_sem);
+ /* No need to take a ref. on kobject, as it has been taken
+ * during ->open(), but ->d_fsdata can become NULL
+ */
+ if (kobj_dir->d_fsdata)
+ kobj = kobj_dir->d_fsdata;
+ up(&kobj_dir->d_inode->i_sem);
+
if (kobj)
kobject_put(kobj);
module_put(attr->owner);
--
Maneesh Soni
Linux Technology Center,
IBM Software Lab, Bangalore, India
email: [EMAIL PROTECTED]
Phone: 91-80-25044999 Fax: 91-80-25268553
T/L : 9243696
-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
