[Top] [All Lists]

Re: inode_permission NULL pointer dereference in 3.13-rc1

To: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: inode_permission NULL pointer dereference in 3.13-rc1
From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Date: Fri, 29 Nov 2013 23:55:37 +0000
Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <CA+55aFxgPQq_1n8Pv6gmMk+=yX9YkA10y6EpS1ECY8OB8-wFig@xxxxxxxxxxxxxx>
References: <20131128225102.GS10988@dastard> <20131128234441.GQ10323@xxxxxxxxxxxxxxxxxx> <CA+55aFxLZxy75fO4ZXO4Stiu1sMx1q=eJ7HSk-UTCX61jPrirA@xxxxxxxxxxxxxx> <20131129024121.GS10323@xxxxxxxxxxxxxxxxxx> <20131129035939.GT10323@xxxxxxxxxxxxxxxxxx> <20131129040658.GU10323@xxxxxxxxxxxxxxxxxx> <20131129041416.GV10323@xxxxxxxxxxxxxxxxxx> <20131129065941.GW10323@xxxxxxxxxxxxxxxxxx> <20131129194438.GA11052@xxxxxxxxx> <CA+55aFxgPQq_1n8Pv6gmMk+=yX9YkA10y6EpS1ECY8OB8-wFig@xxxxxxxxxxxxxx>
Sender: Al Viro <viro@xxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Nov 29, 2013 at 12:17:23PM -0800, Linus Torvalds wrote:

> Al - even in your scenario I don't see a NULL nd->inode, because when
> we do an rmdir we remove the dentry, we don't turn it into a negative
> one. Afaik, it would be a violation of all our dentry rules to change
> the dentry->d_inode field while the dentry is live. The only way to
> get a negative dentry (ie d_inode == NULL) should be from lookup (and
> from a rename that switches the dentries around, but even then the
> d_inode _stays_ NULL, it's just that we move the dentry itself
> around).

Look at the end of vfs_rmdir(); d_delete() in there will turn dentry
negative if nobody else hold references to it.  So yes, dentry of
directory *can* go negative under you, unless you've grabbed a reference.
Which we do not do in RCU mode, obviously.

What would be a violation of all rules is dentry held by somebody else
becoming negative.  And d_delete() avoids that, but the whole point of
RCU-mode pathwalk is to _not_ hold intermediates.  So nd->inode is
needed.  Sure, it'll get ->d_seq bumped, but that won't do you much good
when it comes to attempt to dereference nd->inode.  Sure, we can turn
all places that access nd->inode into
        struct dentry *dentry = nd->path.dentry;
        struct inode *inode = dentry->d_inode;
        if (read_seqcount_retry(&dentry->d_seq, nd->seq))
                /* too fucking bad, we'd lost the race */
                /* use inode */
but it will be just as messy as maintaining nd->inode _and_ quite a bit

<Prev in Thread] Current Thread [Next in Thread>