linux-kernel - Re: Linux 2.6.26-rc4

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <1212509263.3025.66.camel@raven.themaw.net>
Date:	Wed, 04 Jun 2008 00:07:42 +0800
From:	Ian Kent <raven@...maw.net>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Al Viro <viro@...IV.linux.org.uk>,
	Miklos Szeredi <miklos@...redi.hu>, jesper@...gh.cc,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: Linux 2.6.26-rc4


On Tue, 2008-06-03 at 08:01 -0700, Linus Torvalds wrote:
> 
> On Tue, 3 Jun 2008, Ian Kent wrote:
> > > 
> > > > I think it must be autofs4 doing something weird.  Like this in
> > > > autofs4_lookup_unhashed():
> > > > 
> > > > 			/*
> > > > 			 * Make the rehashed dentry negative so the VFS
> > > > 			 * behaves as it should.
> > > > 			 */
> > > > 			if (inode) {
> > > > 				dentry->d_inode = NULL;
> 
> Uhhuh. Yeah, that's not allowed.
> 
> A dentry inode can start _out_ as NULL, but it can never later become NULL 
> again until it is totally unused.
> 
> > > Lovely.  If we ever step into that with somebody else (no matter who)
> > > holding a reference to that dentry, we are certainly well and truly
> > > buggered.  It's not just mount(2) - everything in the tree assumes that
> > > holding a reference to positive dentry guarantees that it remains
> > > positive.
> 
> Indeed. Things like regular file ops won't even test the inode, since they 
> know that "open()" will only open a dentry with a positive entry, so they 
> know that the dentry->inode is non-NULL.
> 
> [ Although some code-paths do test - but that is just because people are 
>   so used to testign that pointers are non-NULL. ]
> 
> > The intent here is that, the dentry above is unhashed at this point, and
> > if hasn't been reclaimed by the VFS, it is made negative and replaces
> > the unhashed negative dentry passed to ->lookup(). The reference count
> > is incremented to account for the reference held by the path walk.
> > 
> > What am I doing wrong here?
> 
> What's wrong is that you can't do that "dentry->d_inode = NULL". EVER.

OK.

> 
> Why would you want to? If the dentry is already unhashed, then no _new_ 
> lookups will ever find it anyway, so it's effectively unfindable anyway. 
> Except by people who *have* to find it, ie the people who already hold it 
> open (because, for example, they opened it earlier, or because they 
> chdir()'ed into a subdirectory).

The code we're talking about deals with a race between expiring and
mounting an autofs mount point at the same time. 

I'll have a closer look and see if I can make it work without turning
the dentry negative.

> 
> So why don't you just return a NULL dentry instead, for a unhashed dentry? 
> Or do the "goto next" thing?

That just won't work for the case this is meant to deal with.

Ian


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