[patch] fs: fix d_validate

[patch] fs: fix d_validate

[Nick Piggin]

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.

Signed-off-by: Nick Piggin <npiggin@kernel.dk>

---
 fs/dcache.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

Index: linux-2.6/fs/dcache.c
===================================================================
--- linux-2.6.orig/fs/dcache.c	2010-11-16 17:20:40.000000000 +1100
+++ linux-2.6/fs/dcache.c	2010-11-16 17:20:40.000000000 +1100
@@ -1483,41 +1483,30 @@ struct dentry *d_hash_and_lookup(struct
 }
 
 /**
- * d_validate - verify dentry provided from insecure source
+ * d_validate - verify dentry provided from insecure source (deprecated)
  * @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.
+ *
+ * This function is slow for big directories, and deprecated, do not use it.
  */
- 
 int d_validate(struct dentry *dentry, struct dentry *dparent)
 {
-	struct hlist_head *base;
-	struct hlist_node *lhp;
-
-	/* Check whether the ptr might be valid at all.. */
-	if (!kmem_ptr_validate(dentry_cache, dentry))
-		goto out;
-
-	if (dentry->d_parent != dparent)
-		goto out;
+	struct dentry *child;
 
 	spin_lock(&dcache_lock);
-	base = d_hash(dparent, dentry->d_name.hash);
-	hlist_for_each(lhp,base) { 
-		/* hlist_for_each_entry_rcu() not required for d_hash list
-		 * as it is parsed under dcache_lock
-		 */
-		if (dentry == hlist_entry(lhp, struct dentry, d_hash)) {
+	list_for_each_entry(child, &dparent->d_subdirs, d_u.d_child) {
+		if (dentry == child) {
 			__dget_locked(dentry);
 			spin_unlock(&dcache_lock);
 			return 1;
 		}
 	}
 	spin_unlock(&dcache_lock);
-out:
+
 	return 0;
 }
 EXPORT_SYMBOL(d_validate);

[patch] kernel: get rid of *_ptr_validate

[Nick Piggin]

This is a nasty ugly and error prone API. It's sole user, dcache, could not
get it right so there is roughly zero chance that anything else will.

Signed-off-by: Nick Piggin <npiggin@kernel.dk>

---
 include/linux/slab.h |    2 --
 mm/slab.c            |   32 +-------------------------------
 mm/slob.c            |    5 -----
 mm/slub.c            |   29 -----------------------------
 mm/util.c            |   21 ---------------------
 5 files changed, 1 insertion(+), 88 deletions(-)

Index: linux-2.6/include/linux/slab.h
===================================================================
--- linux-2.6.orig/include/linux/slab.h	2010-11-16 17:20:37.000000000 +1100
+++ linux-2.6/include/linux/slab.h	2010-11-16 17:20:43.000000000 +1100
@@ -106,8 +106,6 @@ int kmem_cache_shrink(struct kmem_cache
 void kmem_cache_free(struct kmem_cache *, void *);
 unsigned int kmem_cache_size(struct kmem_cache *);
 const char *kmem_cache_name(struct kmem_cache *);
-int kern_ptr_validate(const void *ptr, unsigned long size);
-int kmem_ptr_validate(struct kmem_cache *cachep, const void *ptr);
 
 /*
  * Please use this macro to create slab caches. Simply specify the
Index: linux-2.6/mm/slab.c
===================================================================
--- linux-2.6.orig/mm/slab.c	2010-11-16 17:20:37.000000000 +1100
+++ linux-2.6/mm/slab.c	2010-11-16 17:20:43.000000000 +1100
@@ -2781,7 +2781,7 @@ static void slab_put_obj(struct kmem_cac
 /*
  * Map pages beginning at addr to the given cache and slab. This is required
  * for the slab allocator to be able to lookup the cache and slab of a
- * virtual address for kfree, ksize, kmem_ptr_validate, and slab debugging.
+ * virtual address for kfree, ksize, and slab debugging.
  */
 static void slab_map_pages(struct kmem_cache *cache, struct slab *slab,
 			   void *addr)
@@ -3660,36 +3660,6 @@ void *kmem_cache_alloc_notrace(struct km
 EXPORT_SYMBOL(kmem_cache_alloc_notrace);
 #endif
 
-/**
- * kmem_ptr_validate - check if an untrusted pointer might be a slab entry.
- * @cachep: the cache we're checking against
- * @ptr: pointer to validate
- *
- * This verifies that the untrusted pointer looks sane;
- * it is _not_ a guarantee that the pointer is actually
- * part of the slab cache in question, but it at least
- * validates that the pointer can be dereferenced and
- * looks half-way sane.
- *
- * Currently only used for dentry validation.
- */
-int kmem_ptr_validate(struct kmem_cache *cachep, const void *ptr)
-{
-	unsigned long size = cachep->buffer_size;
-	struct page *page;
-
-	if (unlikely(!kern_ptr_validate(ptr, size)))
-		goto out;
-	page = virt_to_page(ptr);
-	if (unlikely(!PageSlab(page)))
-		goto out;
-	if (unlikely(page_get_cache(page) != cachep))
-		goto out;
-	return 1;
-out:
-	return 0;
-}
-
 #ifdef CONFIG_NUMA
 void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 {
Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2010-11-16 17:20:37.000000000 +1100
+++ linux-2.6/mm/slub.c	2010-11-16 17:20:43.000000000 +1100
@@ -2386,35 +2386,6 @@ static int kmem_cache_open(struct kmem_c
 }
 
 /*
- * Check if a given pointer is valid
- */
-int kmem_ptr_validate(struct kmem_cache *s, const void *object)
-{
-	struct page *page;
-
-	if (!kern_ptr_validate(object, s->size))
-		return 0;
-
-	page = get_object_page(object);
-
-	if (!page || s != page->slab)
-		/* No slab or wrong slab */
-		return 0;
-
-	if (!check_valid_pointer(s, page, object))
-		return 0;
-
-	/*
-	 * We could also check if the object is on the slabs freelist.
-	 * But this would be too expensive and it seems that the main
-	 * purpose of kmem_ptr_valid() is to check if the object belongs
-	 * to a certain slab.
-	 */
-	return 1;
-}
-EXPORT_SYMBOL(kmem_ptr_validate);
-
-/*
  * Determine the size of a slab object
  */
 unsigned int kmem_cache_size(struct kmem_cache *s)
Index: linux-2.6/mm/util.c
===================================================================
--- linux-2.6.orig/mm/util.c	2010-11-16 17:20:37.000000000 +1100
+++ linux-2.6/mm/util.c	2010-11-16 17:20:43.000000000 +1100
@@ -186,27 +186,6 @@ void kzfree(const void *p)
 }
 EXPORT_SYMBOL(kzfree);
 
-int kern_ptr_validate(const void *ptr, unsigned long size)
-{
-	unsigned long addr = (unsigned long)ptr;
-	unsigned long min_addr = PAGE_OFFSET;
-	unsigned long align_mask = sizeof(void *) - 1;
-
-	if (unlikely(addr < min_addr))
-		goto out;
-	if (unlikely(addr > (unsigned long)high_memory - size))
-		goto out;
-	if (unlikely(addr & align_mask))
-		goto out;
-	if (unlikely(!kern_addr_valid(addr)))
-		goto out;
-	if (unlikely(!kern_addr_valid(addr + size - 1)))
-		goto out;
-	return 1;
-out:
-	return 0;
-}
-
 /*
  * strndup_user - duplicate an existing string from user space
  * @s: The string to duplicate
Index: linux-2.6/mm/slob.c
===================================================================
--- linux-2.6.orig/mm/slob.c	2010-11-16 17:20:37.000000000 +1100
+++ linux-2.6/mm/slob.c	2010-11-16 17:20:43.000000000 +1100
@@ -678,11 +678,6 @@ int kmem_cache_shrink(struct kmem_cache
 }
 EXPORT_SYMBOL(kmem_cache_shrink);
 
-int kmem_ptr_validate(struct kmem_cache *a, const void *b)
-{
-	return 0;
-}
-
 static unsigned int slob_ready __read_mostly;
 
 int slab_is_available(void)

Re: [patch] fs: fix d_validate

[Christoph Hellwig]

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.


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
  */

Re: [patch] kernel: get rid of *_ptr_validate

[Christoph Hellwig]

On Tue, Nov 16, 2010 at 05:24:36PM +1100, Nick Piggin wrote:
> This is a nasty ugly and error prone API. It's sole user, dcache, could not
> get it right so there is roughly zero chance that anything else will.

Yes, please get rid of this junk..

Re: [patch] fs: fix d_validate

[Nick Piggin]

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
>   */

Re: [patch] fs: fix d_validate

[Linus Torvalds]

On Mon, Nov 15, 2010 at 10:23 PM, Nick Piggin <npiggin@kernel.dk> 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.

Hmm. Iirc (and it's possible that I don't) the original reason for the
whole kmem_ptr_validate() was for nfsd sending dentry pointers back
and forth as part of the FH.

So we really _did_ need to do basic validation, and no, we didn't care
about things like DEBUG_PAGEALLOC, because that's totally
inappropriate for a nfs server anyway. So it was "broken", but it
worked.

Now, I hope that kind of "we really fundamentally cannot even trust
the pointer" doesn't exist any more, in which case I agree with you.
But historically, it really did matter at some point, and when the
code says "insecure source", it really means it - it could
historically have come from user space or over the network.

                          Linus

Re: [patch] fs: fix d_validate

[Christoph Hellwig]

On Tue, Nov 16, 2010 at 08:20:52AM -0800, Linus Torvalds wrote:
> Hmm. Iirc (and it's possible that I don't) the original reason for the
> whole kmem_ptr_validate() was for nfsd sending dentry pointers back
> and forth as part of the FH.

If it did that it's been a long time ago, pre-2.4 at least.  In addition
to that since the introduction of export operations we can't NFS-export
either ncpfs or smbfs anyway.

Re: [patch] fs: fix d_validate

[Christoph Hellwig]

On Tue, Nov 16, 2010 at 09:25:33PM +1100, Nick Piggin wrote:
> Right, I depreated it, making it an easy backport. Your next patch
> and then removing it completely would be the next step.

I don't see much of a point in keeping it.  What your new version of
d_validate does is exactly what the two only callers do as a fallback.
Keeping it like that just means we search through d_subdirs another
time when the lookup fails.  No need to keep this beats around longer
than nessecary.

Re: [patch] fs: fix d_validate

[Nick Piggin]

On Tue, Nov 16, 2010 at 08:20:52AM -0800, Linus Torvalds wrote:
> On Mon, Nov 15, 2010 at 10:23 PM, Nick Piggin <npiggin@kernel.dk> 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.
> 
> Hmm. Iirc (and it's possible that I don't) the original reason for the
> whole kmem_ptr_validate() was for nfsd sending dentry pointers back
> and forth as part of the FH.

Yeah that looked to be the case, when I checked old kernels.

 
> So we really _did_ need to do basic validation, and no, we didn't care
> about things like DEBUG_PAGEALLOC, because that's totally
> inappropriate for a nfs server anyway. So it was "broken", but it
> worked.
> 
> Now, I hope that kind of "we really fundamentally cannot even trust
> the pointer" doesn't exist any more, in which case I agree with you.
> But historically, it really did matter at some point, and when the
> code says "insecure source", it really means it - it could
> historically have come from user space or over the network.

Well there is nothing in the tree that uses it after my patch to
fix d_validate (that was the only user). d_validate itself was only
used for a funny dir cache in ncpfs and smbfs which has a cache of
pointers to dentries but no references on them, which is pretty hard
to sanely do anything with.

I imagine that if such a kind of cache turns out to be needed in any
relevant filesystem, it could be properly implemented in the dcache
code and not have to play those games. If any user really comes up
for an untrusted pointer, we can always resurrect this -- the kmem
API is not broken as such, but it's just really easy to use wrongly.

Re: [patch] fs: fix d_validate

[Nick Piggin]

On Tue, Nov 16, 2010 at 05:28:17PM +0100, Christoph Hellwig wrote:
> On Tue, Nov 16, 2010 at 09:25:33PM +1100, Nick Piggin wrote:
> > Right, I depreated it, making it an easy backport. Your next patch
> > and then removing it completely would be the next step.
> 
> I don't see much of a point in keeping it.  What your new version of
> d_validate does is exactly what the two only callers do as a fallback.
> Keeping it like that just means we search through d_subdirs another
> time when the lookup fails.  No need to keep this beats around longer
> than nessecary.

It's really no problem to get my bug fixes merged, and keep the exported
symbol around on a deprecation schedule. It doesn't need to be held up by
Patches to rip the dir cache out of the filesystems.