xfs
[Top] [All Lists]

Re: inode_permission NULL pointer dereference in 3.13-rc1

To: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
Subject: Re: inode_permission NULL pointer dereference in 3.13-rc1
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Fri, 29 Nov 2013 12:17:23 -0800
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, Christoph Hellwig <hch@xxxxxxxxxxxxx>, linux-fsdevel <linux-fsdevel@xxxxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=h2W8bGfkppD/QY+RZ/wnAY3q9+mlh5WeHicmuzL+ev0=; b=miuJsFPJ/u0++h7QPfIJv68JoBuWxu+rlpxj/u8CD/rVo4n3sc1jvZSR7aFMYm9RTU qo7YHcv9RVOEkrSU3L/4m5nW8GLIHeb3XMZ3Nc7gNCbXrbTXrQTXMxUO54dcRwt9608Q QoVWlWUzp/ZtWVAHnxD0/Lt8Vs/ObTP8qzDv953LY1gIji0SWtxFm+QFi6EBRFlDSIjK vOj/Efvet89BdQknPI+V8z7jlYaSf2DuOMGoxJAb+xHsK9zGb0tcfbrrih+9d8y+CHno Ua+3tNwUDijlZb27MsEE75yJyx3Zg2VEOSAOv7/chPtiMWgmmbRWgUfc9IelQaT4TmZp QwvQ==
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=h2W8bGfkppD/QY+RZ/wnAY3q9+mlh5WeHicmuzL+ev0=; b=TkeaiBuPDRbu1nfh001A4erKj7w8xyR4d8Blm30X+mlk3TZKTAjbUnuDTscv3QCtG2 Dz0ezRNCvEw/QAAY9SbUC7zZVqgFL1rYtmIXI8jM+DKqlzOFbzd4S5fDB7rTQ+9kgvBx smIq/Vc27Lvclc9FBq2aqo4HYxMmxUzYiMGNg=
In-reply-to: <20131129194438.GA11052@xxxxxxxxx>
References: <20131128162618.GO10323@xxxxxxxxxxxxxxxxxx> <20131128212301.GP10323@xxxxxxxxxxxxxxxxxx> <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>
Sender: linus971@xxxxxxxxx
On Fri, Nov 29, 2013 at 11:44 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> So, should d870b4a191a3 be queued up for
> -stable releases to resolve this issue there as well, or am I
> misunderstanding your post above?

No, the bug d870b4a191a3 fixes is new to the 3.13 merge window (it's
due to the newly RCU'd vfsmounts).

But Al seems to have found another race that has been around for a
while. But that's a separate issue, and there's no patch for that yet
(and nobody has ever hit it, because we'd scream bloody murder if they
did).

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

That said, I do agree that the games we play with "nd->inode" are not
at all obvious. And I'm not actually convinced we need "nd->inode" AT
ALL these days. We used to have it for other reasons, but afaik these
days the only actual users are

 - some sanity tests in the lookup code

 - a few "inode_permission()" calls that I suspect could equally well
just use path.dentry->inode. Because either the dentry is stable, or
we're in RCU lookup and we'll revalidate it anyway.

Maybe I'm wrong. But iirc, the big reason for 'nd->inode' originally
was that it was the inode that was verified against the dentry
sequence number that we then passed on as a stable inode to the
low-level filesysystems. But that whole calling convention got cleaned
up with commits 12f8ad4b0533 and da53be12bbb4, and I really think most
of the historical reasons for having that nd->inode have gone away.

I dunno. I didn't check the three remaining "inode_permission()"
users, and maybe I missed some other use too. So maybe it's still
required, but my initial reaction when looking at it was "why do we
even have it any more"?

            Linus

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