Date:	Thu, 9 Dec 2010 15:38:42 +1100
From:	Nick Piggin <npiggin@...nel.dk>
To:	Dave Chinner <david@...morbit.com>
Cc:	Nick Piggin <npiggin@...nel.dk>, Nick Piggin <npiggin@...e.de>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/46] Revert "fs: use RCU read side protection in
  d_validate"

On Thu, Dec 09, 2010 at 11:44:13AM +1100, Dave Chinner wrote:
> On Wed, Dec 08, 2010 at 08:38:24PM +1100, Nick Piggin wrote:
> > On Wed, Dec 08, 2010 at 12:16:56PM +1100, Dave Chinner wrote:
> > > On Sat, Nov 27, 2010 at 08:56:03PM +1100, Nick Piggin wrote:
> > > > This reverts commit 3825bdb7ed920845961f32f364454bee5f469abb.
> > > > 
> > > > Patch is broken, you can't dget() without holding any locks!
> > > 
> > > I believe you can - for the same reasons we can take a reference to
> > > an inode without holding the inode_lock. That is, as long as the
> > > caller already holds an active reference to the dentry,
> > > dget() can be used to take another reference without needing the
> > > dcache_lock.
> > > 
> > > Such usage appears to be described in the comment above dget() and
> > > there's a BUG_ON() in dget() to catch callers that don't already
> > > have an active reference. An example of a valid unlocked dget():
> > > d_alloc() does an unlocked dget() to take a reference to the parent
> > > dentry whichn we already are guaranteed to have a reference to.
> > 
> > Of course you can dget if you already have a reference :)
> 
> Right, so the commit message is wrong. Can you update it to tell us why
> dget() can't be used there - the commit message from the second
> patch explained it far better....

I suppose if you're not reading it in the context of d_validate,
then yes. And as an historical record, I'll clarify.

Obviously if we do have a reference, then we can take another,
and if we don't, then we need more than RCU because RCU only
provides persistence guarantee for the memory, not any persistence
or validity guarantee for the object.

 
> > > As to d_validate() - it depends on the caller behaviour as to
> > > whether the unlocked dget() is valid or not.  From a cursory check
> > > of the NCP and SMB readdir caches, both appear to hold an active
> > > reference to the dentry it is passing to d_validate().
> > 
> > I don't see where? Can you point to where the refcount is taken?
> > AFAIKS it drops the reference 3 lines after it puts the pointer
> > into cache.
> 
> Yeah, you're right, I missed that one - I spent more tiem checking
> the validation part of the code than the initial insertion. Hence
> my request:

Yes, I'm pretty sure it doesn't have any references.

 
> > > If that is
> > > the case then there is nothing wrong with the way d_validate uses
> > > dget(). Can someone with more SMB/NCP expertise than me validate the
> > > use of cached dentries?
> > 
> > Then why would it have to use d_validate if it has a reference?
> > That is supposed to be for an "untrusted" pointer (which is why
> > it had all the crazy checks that it's in kmem and in the right
> > slab etc).
> 
> Code changes. It may not be doing what it was originally
> needed/intended to be doing - I don't need to waste time on code
> archeology and second guessing when there are others around that can
> tell me this off the top oftheir head. ;)

Well the d_validate API is meant to provide that, so it's broken
whether or not its callers use it correctly. It's also exported
to external modules...

Yes we should remove smbfs and rip the cache out of ncpfs and
remove d_validate entirely when possible (or, provide a more
reasonable API and caching library entirely in the dcache code
that a filesystem might use). But this is the right first step.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists