lkml.org 
[lkml]   [2011]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/1] fix d_revalidate oopsen on NFS exports
    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@onthe.net.au> 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@onthe.net.au>
    >>>
    >>> Acked-by: David Howells <dhowells@redhat.com>
    >>>
    >>> 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_neg_need_reval(), nfs_is_exclusive_create(),
    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
    > this:
    >
    > 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.

    Chris


    \
     
     \ /
      Last update: 2011-12-01 08:33    [W:7.062 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site