linux-kernel - Re: [PATCH 01/46] Revert "fs: use RCU read side protection in d

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Dec 2010 11:44:13 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Nick Piggin <npiggin@...nel.dk>
Cc:	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 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....

> > 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:

> > 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. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
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/