Re: [patch] fs: fix d_validate

[Nick Piggin <npiggin@kernel.dk>]

On Tue, Nov 16, 2010 at 11:20:45AM +0100, Christoph Hellwig wrote:
> On Tue, Nov 16, 2010 at 05:23:19PM +1100, Nick Piggin wrote:
> > 
> > fs: d_validate fixes
> > 
> > d_validate has been broken for a long time.
> > 
> > kmem_ptr_validate does not guarantee that a pointer can be dereferenced
> > if it can go away at any time. Even rcu_read_lock doesn't help, because
> > the pointer might be queued in RCU callbacks but not executed yet.
> > 
> > So the parent cannot be checked, nor the name hashed. The dentry pointer
> > can not be touched until it can be verified under lock. Hashing simply
> > cannot be used.
> > 
> > Instead, verify the parent/child relationship by traversing parent's
> > d_child list. It's slow, but only ncpfs and the destaged smbfs care
> > about it, at this point.
> 
> That's what both callers already do when d_validate fails, so if we want
> to go down that route I'd suggest to just remove it, like in the patch
> below.  Note that there's probably a lot more caching infrastrucuture
> in ncpfs/smbfs that becomes dead by this as well.

Right, I depreated it, making it an easy backport. Your next patch
and then removing it completely would be the next step.

> 
> 
> Index: linux-2.6/drivers/staging/smbfs/cache.c
> ===================================================================
> --- linux-2.6.orig/drivers/staging/smbfs/cache.c	2010-11-16 10:49:55.915263307 +0100
> +++ linux-2.6/drivers/staging/smbfs/cache.c	2010-11-16 11:12:17.091018582 +0100
> @@ -73,33 +73,13 @@ smb_invalidate_dircache_entries(struct d
>  	spin_unlock(&dcache_lock);
>  }
>  
> -/*
> - * dget, but require that fpos and parent matches what the dentry contains.
> - * dentry is not known to be a valid pointer at entry.
> - */
>  struct dentry *
> -smb_dget_fpos(struct dentry *dentry, struct dentry *parent, unsigned long fpos)
> +smb_dget_fpos(struct dentry *parent, unsigned long fpos)
>  {
> -	struct dentry *dent = dentry;
> -	struct list_head *next;
> +	struct dentry *dent;
>  
> -	if (d_validate(dent, parent)) {
> -		if (dent->d_name.len <= SMB_MAXNAMELEN &&
> -		    (unsigned long)dent->d_fsdata == fpos) {
> -			if (!dent->d_inode) {
> -				dput(dent);
> -				dent = NULL;
> -			}
> -			return dent;
> -		}
> -		dput(dent);
> -	}
> -
> -	/* If a pointer is invalid, we search the dentry. */
>  	spin_lock(&dcache_lock);
> -	next = parent->d_subdirs.next;
> -	while (next != &parent->d_subdirs) {
> -		dent = list_entry(next, struct dentry, d_u.d_child);
> +	list_for_each_entry(dent, &parent->d_subdirs, d_u.d_child) {
>  		if ((unsigned long)dent->d_fsdata == fpos) {
>  			if (dent->d_inode)
>  				dget_locked(dent);
> @@ -107,7 +87,6 @@ smb_dget_fpos(struct dentry *dentry, str
>  				dent = NULL;
>  			goto out_unlock;
>  		}
> -		next = next->next;
>  	}
>  	dent = NULL;
>  out_unlock:
> Index: linux-2.6/drivers/staging/smbfs/dir.c
> ===================================================================
> --- linux-2.6.orig/drivers/staging/smbfs/dir.c	2010-11-16 10:52:14.577003425 +0100
> +++ linux-2.6/drivers/staging/smbfs/dir.c	2010-11-16 11:11:29.420254992 +0100
> @@ -166,8 +166,7 @@ smb_readdir(struct file *filp, void *dir
>  			struct dentry *dent;
>  			int res;
>  
> -			dent = smb_dget_fpos(ctl.cache->dentry[ctl.idx],
> -					     dentry, filp->f_pos);
> +			dent = smb_dget_fpos(dentry, filp->f_pos);
>  			if (!dent)
>  				goto invalid_cache;
>  
> Index: linux-2.6/drivers/staging/smbfs/proto.h
> ===================================================================
> --- linux-2.6.orig/drivers/staging/smbfs/proto.h	2010-11-16 10:52:00.746253110 +0100
> +++ linux-2.6/drivers/staging/smbfs/proto.h	2010-11-16 10:52:06.439266241 +0100
> @@ -43,7 +43,7 @@ extern void smb_renew_times(struct dentr
>  /* cache.c */
>  extern void smb_invalid_dir_cache(struct inode *dir);
>  extern void smb_invalidate_dircache_entries(struct dentry *parent);
> -extern struct dentry *smb_dget_fpos(struct dentry *dentry, struct dentry *parent, unsigned long fpos);
> +extern struct dentry *smb_dget_fpos(struct dentry *parent, unsigned long fpos);
>  extern int smb_fill_cache(struct file *filp, void *dirent, filldir_t filldir, struct smb_cache_control *ctrl, struct qstr *qname, struct smb_fattr *entry);
>  /* sock.c */
>  extern void smb_data_ready(struct sock *sk, int len);
> Index: linux-2.6/fs/ncpfs/dir.c
> ===================================================================
> --- linux-2.6.orig/fs/ncpfs/dir.c	2010-11-16 10:49:04.988254089 +0100
> +++ linux-2.6/fs/ncpfs/dir.c	2010-11-16 11:03:55.184018020 +0100
> @@ -367,28 +367,12 @@ finished:
>  }
>  
>  static struct dentry *
> -ncp_dget_fpos(struct dentry *dentry, struct dentry *parent, unsigned long fpos)
> +ncp_dget_fpos(struct dentry *parent, unsigned long fpos)
>  {
> -	struct dentry *dent = dentry;
> -	struct list_head *next;
> +	struct dentry *dent;
>  
> -	if (d_validate(dent, parent)) {
> -		if (dent->d_name.len <= NCP_MAXPATHLEN &&
> -		    (unsigned long)dent->d_fsdata == fpos) {
> -			if (!dent->d_inode) {
> -				dput(dent);
> -				dent = NULL;
> -			}
> -			return dent;
> -		}
> -		dput(dent);
> -	}
> -
> -	/* If a pointer is invalid, we search the dentry. */
>  	spin_lock(&dcache_lock);
> -	next = parent->d_subdirs.next;
> -	while (next != &parent->d_subdirs) {
> -		dent = list_entry(next, struct dentry, d_u.d_child);
> +	list_for_each_entry(dent, &parent->d_subdirs, d_u.d_child) {
>  		if ((unsigned long)dent->d_fsdata == fpos) {
>  			if (dent->d_inode)
>  				dget_locked(dent);
> @@ -397,11 +381,9 @@ ncp_dget_fpos(struct dentry *dentry, str
>  			spin_unlock(&dcache_lock);
>  			goto out;
>  		}
> -		next = next->next;
>  	}
>  	spin_unlock(&dcache_lock);
>  	return NULL;
> -
>  out:
>  	return dent;
>  }
> @@ -496,8 +478,7 @@ static int ncp_readdir(struct file *filp
>  			struct dentry *dent;
>  			int res;
>  
> -			dent = ncp_dget_fpos(ctl.cache->dentry[ctl.idx],
> -						dentry, filp->f_pos);
> +			dent = ncp_dget_fpos(dentry, filp->f_pos);
>  			if (!dent)
>  				goto invalid_cache;
>  			res = filldir(dirent, dent->d_name.name,
> Index: linux-2.6/fs/dcache.c
> ===================================================================
> --- linux-2.6.orig/fs/dcache.c	2010-11-16 10:57:20.960004264 +0100
> +++ linux-2.6/fs/dcache.c	2010-11-16 10:57:25.790005239 +0100
> @@ -1482,39 +1482,6 @@ out:
>  	return dentry;
>  }
>  
> -/**
> - * d_validate - verify dentry provided from insecure source
> - * @dentry: The dentry alleged to be valid child of @dparent
> - * @dparent: The parent dentry (known to be valid)
> - *
> - * An insecure source has sent us a dentry, here we verify it and dget() it.
> - * This is used by ncpfs in its readdir implementation.
> - * Zero is returned in the dentry is invalid.
> - */
> -int d_validate(struct dentry *dentry, struct dentry *parent)
> -{
> -	struct hlist_head *head = d_hash(parent, dentry->d_name.hash);
> -	struct hlist_node *node;
> -	struct dentry *d;
> -
> -	/* Check whether the ptr might be valid at all.. */
> -	if (!kmem_ptr_validate(dentry_cache, dentry))
> -		return 0;
> -	if (dentry->d_parent != parent)
> -		return 0;
> -
> -	rcu_read_lock();
> -	hlist_for_each_entry_rcu(d, node, head, d_hash) {
> -		if (d == dentry) {
> -			dget(dentry);
> -			return 1;
> -		}
> -	}
> -	rcu_read_unlock();
> -	return 0;
> -}
> -EXPORT_SYMBOL(d_validate);
> -
>  /*
>   * When a file is deleted, we have two options:
>   * - turn this dentry into a negative dentry
> Index: linux-2.6/include/linux/dcache.h
> ===================================================================
> --- linux-2.6.orig/include/linux/dcache.h	2010-11-16 10:57:14.736253110 +0100
> +++ linux-2.6/include/linux/dcache.h	2010-11-16 10:57:18.474254784 +0100
> @@ -305,9 +305,6 @@ extern struct dentry * d_lookup(struct d
>  extern struct dentry * __d_lookup(struct dentry *, struct qstr *);
>  extern struct dentry * d_hash_and_lookup(struct dentry *, struct qstr *);
>  
> -/* validate "insecure" dentry pointer */
> -extern int d_validate(struct dentry *, struct dentry *);
> -
>  /*
>   * helper function for dentry_operations.d_dname() members
>   */