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