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