| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090419205154.GA18110@fieldses.org>
Date: Sun, 19 Apr 2009 16:51:54 -0400
From: "J. Bruce Fields" <bfields@...ldses.org>
To: David Woodhouse <dwmw2@...radead.org>
Cc: Al Viro <viro@...IV.linux.org.uk>, hooanon05@...oo.co.jp,
hch@...radead.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH v2] Fix i_mutex handling in nfsd readdir
On Sun, Apr 19, 2009 at 01:27:49PM +0100, David Woodhouse wrote:
> Commit 14f7dd63 ("Copy XFS readdir hack into nfsd code") introduced a
> bug to generic code which had been extant for a long time in the XFS
> version -- it started to call through into lookup_one_len() and hence
> into the file systems' ->lookup() methods without i_mutex held on the
> directory.
>
> This patch fixes it by locking the directory's i_mutex again before
> calling the filldir functions. The original deadlocks which commit
> 14f7dd63 was designed to avoid are still avoided, because they were due
> to fs-internal locking, not i_mutex.
>
> Commit 05f4f678 ("nfsd4: don't do lookup within readdir in recovery
> code") introduced a similar problem there, which this also addresses.
>
> While we're at it, fix the return type of nfsd_buffered_readdir() which
> should be a __be32 not an int -- it's an NFS errno, not a Linux errno.
> And return nfserrno(-ENOMEM) when allocation fails, not just -ENOMEM.
> Sparse would have caught both of those if it wasn't so busy bitching
> about __cold__.
>
> Commit 05f4f678 ("nfsd4: don't do lookup within readdir in recovery
> code") introduced a similar problem with calling lookup_one_len()
> without i_mutex, which this patch also addresses.
>
> Reported-by: J. R. Okajima <hooanon05@...oo.co.jp>
> Signed-off-by: David Woodhouse <David.Woodhouse@...el.com>
> Umm-I-can-live-with-that-by: Al Viro <viro@...iv.linux.org.uk>
> ---
> Still haven't tested the NFSv4 bit -- Bruce?
Thanks, there's an iterator in there that calls a passed-in function,
some examples of which were taking the i_mutex--so some fixing up is
needed. I'll follow up with a patch once I've got one tested.
--b.
>
> This time I remembered to remove the no-longer-used 'done:' label from
> nfsd_buffered_readdir() too.
>
> diff --git a/fs/namei.c b/fs/namei.c
> index b8433eb..78f253c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1248,6 +1248,8 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
> int err;
> struct qstr this;
>
> + WARN_ON_ONCE(!mutex_is_locked(&base->d_inode->i_mutex));
> +
> err = __lookup_one_len(name, &this, base, len);
> if (err)
> return ERR_PTR(err);
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 3444c00..210709c 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -229,21 +229,23 @@ nfsd4_list_rec_dir(struct dentry *dir, recdir_func *f)
> goto out;
> status = vfs_readdir(filp, nfsd4_build_namelist, &names);
> fput(filp);
> + mutex_lock(&dir->d_inode->i_mutex);
> while (!list_empty(&names)) {
> entry = list_entry(names.next, struct name_list, list);
>
> dentry = lookup_one_len(entry->name, dir, HEXDIR_LEN-1);
> if (IS_ERR(dentry)) {
> status = PTR_ERR(dentry);
> - goto out;
> + break;
> }
> status = f(dir, dentry);
> dput(dentry);
> if (status)
> - goto out;
> + break;
> list_del(&entry->list);
> kfree(entry);
> }
> + mutex_unlock(&dir->d_inode->i_mutex);
> out:
> while (!list_empty(&names)) {
> entry = list_entry(names.next, struct name_list, list);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index ab93fcf..0874299 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1885,8 +1885,8 @@ static int nfsd_buffered_filldir(void *__buf, const char *name, int namlen,
> return 0;
> }
>
> -static int nfsd_buffered_readdir(struct file *file, filldir_t func,
> - struct readdir_cd *cdp, loff_t *offsetp)
> +static __be32 nfsd_buffered_readdir(struct file *file, filldir_t func,
> + struct readdir_cd *cdp, loff_t *offsetp)
> {
> struct readdir_data buf;
> struct buffered_dirent *de;
> @@ -1896,11 +1896,12 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
>
> buf.dirent = (void *)__get_free_page(GFP_KERNEL);
> if (!buf.dirent)
> - return -ENOMEM;
> + return nfserrno(-ENOMEM);
>
> offset = *offsetp;
>
> while (1) {
> + struct inode *dir_inode = file->f_path.dentry->d_inode;
> unsigned int reclen;
>
> cdp->err = nfserr_eof; /* will be cleared on successful read */
> @@ -1919,26 +1920,38 @@ static int nfsd_buffered_readdir(struct file *file, filldir_t func,
> if (!size)
> break;
>
> + /*
> + * Various filldir functions may end up calling back into
> + * lookup_one_len() and the file system's ->lookup() method.
> + * These expect i_mutex to be held, as it would within readdir.
> + */
> + host_err = mutex_lock_killable(&dir_inode->i_mutex);
> + if (host_err)
> + break;
> +
> de = (struct buffered_dirent *)buf.dirent;
> while (size > 0) {
> offset = de->offset;
>
> if (func(cdp, de->name, de->namlen, de->offset,
> de->ino, de->d_type))
> - goto done;
> + break;
>
> if (cdp->err != nfs_ok)
> - goto done;
> + break;
>
> reclen = ALIGN(sizeof(*de) + de->namlen,
> sizeof(u64));
> size -= reclen;
> de = (struct buffered_dirent *)((char *)de + reclen);
> }
> + mutex_unlock(&dir_inode->i_mutex);
> + if (size > 0) /* We bailed out early */
> + break;
> +
> offset = vfs_llseek(file, 0, SEEK_CUR);
> }
>
> - done:
> free_page((unsigned long)(buf.dirent));
>
> if (host_err)
>
> --
> dwmw2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists