This is a re-submission of the patch I sent about a month ago.
While working on my code I realized that d_drop() might race against
__d_lookup(). __d_drop() (which is called by d_drop() after acquiring
the dcache_lock) is accessing dentry->d_flags to set the DCACHE_UNHASHED
flag. This shouldn't be done without holding dentry->d_lock, like stated
in dcache.h:
struct dentry {
...
unsigned int d_flags; /* protected by d_lock */
...
};
Therefore d_drop() must acquire the dentry->d_lock. Likewise every use
of __d_drop() must acquire that lock.
This patch fixes d_drop() and every grep'able __d_drop() use. This patch
is against today's http://linux.bkbits.net/linux-2.5.
Comments please,
Jan
On Wed, 16 Feb 2005, Jan Blunck wrote:
> This is a re-submission of the patch I sent about a month ago.
>
> While working on my code I realized that d_drop() might race against
> __d_lookup(). __d_drop() (which is called by d_drop() after acquiring the
> dcache_lock) is accessing dentry->d_flags to set the DCACHE_UNHASHED flag.
> This shouldn't be done without holding dentry->d_lock, like stated in
> dcache.h:
>
> struct dentry {
> ...
> unsigned int d_flags; /* protected by d_lock */
> ...
> };
>
> Therefore d_drop() must acquire the dentry->d_lock. Likewise every use of
> __d_drop() must acquire that lock.
>
> This patch fixes d_drop() and every grep'able __d_drop() use. This patch is
> against today's http://linux.bkbits.net/linux-2.5.
>
For my part, in autofs4, I would prefer:
--- linux-2.6.9/fs/autofs4/root.c.d_lock 2005-02-16 23:15:18.000000000 +0800
+++ linux-2.6.9/fs/autofs4/root.c 2005-02-16 23:15:35.000000000 +0800
@@ -621,7 +621,7 @@
spin_unlock(&dcache_lock);
return -ENOTEMPTY;
}
- __d_drop(dentry);
+ d_drop(dentry);
spin_unlock(&dcache_lock);
dput(ino->dentry);
==> Regarding Re: [PATCH] dcache d_drop() bug fix / __d_drop() use fix; [email protected] adds:
raven> On Wed, 16 Feb 2005, Jan Blunck wrote:
>> This is a re-submission of the patch I sent about a month ago.
>>
>> While working on my code I realized that d_drop() might race against
>> __d_lookup(). __d_drop() (which is called by d_drop() after acquiring the
>> dcache_lock) is accessing dentry->d_flags to set the DCACHE_UNHASHED flag.
>> This shouldn't be done without holding dentry->d_lock, like stated in
>> dcache.h:
>>
>> struct dentry {
>> ...
>> unsigned int d_flags; /* protected by d_lock */
>> ...
>> };
>>
>> Therefore d_drop() must acquire the dentry->d_lock. Likewise every use of
>> __d_drop() must acquire that lock.
>>
>> This patch fixes d_drop() and every grep'able __d_drop() use. This patch is
>> against today's http://linux.bkbits.net/linux-2.5.
>>
raven> For my part, in autofs4, I would prefer:
> --- linux-2.6.9/fs/autofs4/root.c.d_lock 2005-02-16 23:15:18.000000000 +0800
> +++ linux-2.6.9/fs/autofs4/root.c 2005-02-16 23:15:35.000000000 +0800
> @@ -621,7 +621,7 @@
> spin_unlock(&dcache_lock);
> return -ENOTEMPTY;
> }
> - __d_drop(dentry);
> + d_drop(dentry);
> spin_unlock(&dcache_lock);
> dput(ino->dentry);
Ian, this would deadlock. You already hold the dcache lock here, and
d_drop takes it:
static inline void d_drop(struct dentry *dentry)
{
spin_lock(&dcache_lock);
__d_drop(dentry);
spin_unlock(&dcache_lock);
}
The proposed patch was to take the dentry->d_lock.
-Jeff
On Wed, 16 Feb 2005, Jeff Moyer wrote:
> ==> Regarding Re: [PATCH] dcache d_drop() bug fix / __d_drop() use fix; [email protected] adds:
>
> raven> On Wed, 16 Feb 2005, Jan Blunck wrote:
> >> This is a re-submission of the patch I sent about a month ago.
> >>
> >> While working on my code I realized that d_drop() might race against
> >> __d_lookup(). __d_drop() (which is called by d_drop() after acquiring the
> >> dcache_lock) is accessing dentry->d_flags to set the DCACHE_UNHASHED flag.
> >> This shouldn't be done without holding dentry->d_lock, like stated in
> >> dcache.h:
> >>
> >> struct dentry {
> >> ...
> >> unsigned int d_flags; /* protected by d_lock */
> >> ...
> >> };
> >>
> >> Therefore d_drop() must acquire the dentry->d_lock. Likewise every use of
> >> __d_drop() must acquire that lock.
> >>
> >> This patch fixes d_drop() and every grep'able __d_drop() use. This patch is
> >> against today's http://linux.bkbits.net/linux-2.5.
> >>
>
> raven> For my part, in autofs4, I would prefer:
>
> > --- linux-2.6.9/fs/autofs4/root.c.d_lock 2005-02-16 23:15:18.000000000 +0800
> > +++ linux-2.6.9/fs/autofs4/root.c 2005-02-16 23:15:35.000000000 +0800
> > @@ -621,7 +621,7 @@
> > spin_unlock(&dcache_lock);
> > return -ENOTEMPTY;
> > }
> > - __d_drop(dentry);
> > + d_drop(dentry);
> > spin_unlock(&dcache_lock);
>
> > dput(ino->dentry);
>
> Ian, this would deadlock. You already hold the dcache lock here, and
> d_drop takes it:
Ha!
Yes. There is a difference between d_lock and dcache_lock.
I need glasses!
Please ignore what I said.
Ian