Re: [PATCH 1/1] fix d_revalidate oopsen on NFS exports

[Tyler Hicks]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > 
> > Acked-by: David Howells <dhowells@xxxxxxxxxx>
> > 
> > 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.

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;

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.

Tyler

> 
> Note:  jfs isn't susceptible to this problem, but the resolution
> doesn't look like the other file systems, and from the comment
> I'm not sure if the problem was really understood and if it's
> doing the right thing:
> 
> static int jfs_ci_revalidate(struct dentry *dentry, struct nameidata *nd)
> {
>         ...
>         /*
>          * This may be nfsd (or something), anyway, we can't see the
>          * intent of this. So, since this can be for creation, drop it.
>          */
>         if (!nd)
>                 return 0;
> 
>         /*
>          * Drop the negative dentry, in order to make sure to use the
>          * case sensitive name which is specified by user if this is
>          * for creation.
>          */
>         if (nd->flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET))
>                 return 0;
>         ...
> }
> 
> Chris.
> 
> ----------------------------------------------------------------------
> Don't oops when abused by broken layered file systems
> 
> Signed-off-by: Chris Dunlop <chris@xxxxxxxxxxxx>
> ---
>  fs/9p/vfs_dentry.c    |    6 ++++++
>  fs/afs/dir.c          |    6 ++++++
>  fs/coda/dir.c         |    6 ++++++
>  fs/hfs/sysdep.c       |    6 ++++++
>  fs/ncpfs/dir.c        |    6 ++++++
>  fs/nfs/dir.c          |   12 ++++++++++++
>  fs/proc/proc_sysctl.c |    5 +++++
>  fs/sysfs/dir.c        |    6 ++++++
>  8 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c
> index e022890..3b082dc 100644
> --- a/fs/9p/vfs_dentry.c
> +++ b/fs/9p/vfs_dentry.c
> @@ -106,6 +106,12 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
>  	struct inode *inode;
>  	struct v9fs_inode *v9inode;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "v9fs_lookup_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 1b0b195..4003d76 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -607,6 +607,12 @@ static int afs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
>  	void *dir_version;
>  	int ret;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "afs_d_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> diff --git a/fs/coda/dir.c b/fs/coda/dir.c
> index 0239433..ede8e77 100644
> --- a/fs/coda/dir.c
> +++ b/fs/coda/dir.c
> @@ -544,6 +544,12 @@ static int coda_dentry_revalidate(struct dentry *de, struct nameidata *nd)
>  	struct inode *inode;
>  	struct coda_inode_info *cii;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "coda_dentry_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> diff --git a/fs/hfs/sysdep.c b/fs/hfs/sysdep.c
> index 19cf291..b130d91 100644
> --- a/fs/hfs/sysdep.c
> +++ b/fs/hfs/sysdep.c
> @@ -18,6 +18,12 @@ static int hfs_revalidate_dentry(struct dentry *dentry, struct nameidata *nd)
>  	struct inode *inode;
>  	int diff;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "hfs_revalidate_dentry:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
> index 9c51f62..6580d1d 100644
> --- a/fs/ncpfs/dir.c
> +++ b/fs/ncpfs/dir.c
> @@ -302,6 +302,12 @@ ncp_lookup_validate(struct dentry *dentry, struct nameidata *nd)
>  	if (dentry == dentry->d_sb->s_root)
>  		return 1;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "ncp_lookup_validate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index b238d95..51b3d54 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1103,6 +1103,12 @@ static int nfs_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
>  	struct nfs_fattr *fattr = NULL;
>  	int error;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "nfs_lookup_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> @@ -1508,6 +1514,12 @@ static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd)
>  	struct nfs_open_context *ctx;
>  	int openflags, ret = 0;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "nfs_open_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 1a77dbe..20ef3ab 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -389,6 +389,11 @@ static const struct inode_operations proc_sys_dir_operations = {
>  
>  static int proc_sys_revalidate(struct dentry *dentry, struct nameidata *nd)
>  {
> +	if (!nd) {
> +		printk(KERN_ERR "proc_sys_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  	return !PROC_I(dentry->d_inode)->sysctl->unregistering;
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index ea9120a..6373450 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -242,6 +242,12 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)
>  	struct sysfs_dirent *sd;
>  	int is_dir;
>  
> +	if (!nd) {
> +		printk(KERN_ERR "sysfs_dentry_revalidate:"
> +		       " called from layered filesystem without intents\n");
> +		return -EIO;
> +	}
> +
>  	if (nd->flags & LOOKUP_RCU)
>  		return -ECHILD;
>  
> -- 
> 1.7.0.4
> 
> ----------------------------------------------------------------------
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Attachment: signature.asc
Description: Digital signature