On Thu, Dec 01, 2011 at 12:31:58AM -0600, Tyler Hicks wrote:
> On 2011-12-01 11:47:09, Chris Dunlop wrote:
>> On Wed, Nov 30, 2011 at 08:54:43AM +0000, David Howells wrote:
>>> Chris Dunlop <chris@onth...> wrote:
>>>> To avoid other people further wasting their and your time on
>>>> exactly the same thing future, how something like the following
>>>> patch, based on your comment in:
>>>> http://article.gmane.org/gmane.linux.nfs/40370 >>>>
>>>> ...and, if that's acceptable, is it worthwhile doing for the
>>>> other file systems which are likewise currently vulnerable when
>>>> abused by broken layered file systems?
>>> Also, this may get fixed by Al's atomic open patches - but obviously it hasn't
>>> been yet...
>>>> Don't oops when abused by broken layered file systems
>>>> Signed-off-by: Chris Dunlop <chris@onth...>
>>> Acked-by: David Howells <dhowells@redh...>
>>> It's also worth printing a message - this *is* a kernel bug of some description
>>> if it happens.
>> Like the below? This covers the d_revalidate for 9p, afs, coda,
>> hfs, ncpfs, proc, sysfs.
> I don't like the looks of this patch. It makes sense for NFS to error
> out of d_revalidate() when passed a NULL nameidata pointer because NFS
> actually uses the nameidata to do something useful. That can't be said
> about the other filesystems in this patch.
I can see nd is used in nfs_open_revalidate(), but is it
necessarily used in nfs_lookup_revalidate()? I'm way out of my
depth here, but everywhere it's used in nfs_lookup_revalidate()
nfs_lookup_verify_inode()) there are also checks for nd != NULL.
> Why not handle the other filesystems like the previous fixes you
> referenced in your original email by checking for a non-NULL nd like
> if (nd && nd->flags & LOOKUP_RCU)
> return -ECHILD;
'Cos Trond scared me into it! ;-)
But mostly because I don't really know what I'm doing. The
original patch came about because I was tracking down the Oops
in the NFS code and it seemed such an obvious fix that
lookup_one_len() passes down a hard-coded NULL and that NULL
isn't checked in all the d_revalidate routines. I thought I'd do
the right thing and make sure it was checked everywhere. Little
did I know there's "history" behind it! I'm afraid I don't know
anywhere near enough to argue about the right way to deal with it.
> I'm also not sure about the printk in the NFS case. Instead of littering
> the logs, we should probably just disallow the stacked filesystem (are
> we talking about eCryptfs here?) from mounting on top of NFS in the
> first place.
See other reply: it wasn't a stacked file system.
But it seems useful to have the d_revalidate routines indicate
via the log that they're being abused.