nfsd: restore owner override for truncate
diff mbox

Message ID 20170209142238.5322-1-hch@lst.de
State New
Headers show

Commit Message

Christoph Hellwig Feb. 9, 2017, 2:22 p.m. UTC
The switch to vfs_truncate in nfsd_setattr dropped the owner override
used for NFS permissions.  Add a copy of vfs_truncate with it restored
to the nfsd code for now as it's very late in the cycle, but there
should be a way to consolidate it back in the future.

Fixes: 41f53350 ("nfsd: special case truncates some more")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Chuck Lever <chucklever@gmail.com>
---
 fs/nfsd/vfs.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

Comments

Chuck Lever Feb. 9, 2017, 3:56 p.m. UTC | #1
> On Feb 9, 2017, at 9:22 AM, Christoph Hellwig <hch@lst.de> wrote:
> 
> The switch to vfs_truncate in nfsd_setattr dropped the owner override
> used for NFS permissions.  Add a copy of vfs_truncate with it restored
> to the nfsd code for now as it's very late in the cycle, but there
> should be a way to consolidate it back in the future.
> 
> Fixes: 41f53350 ("nfsd: special case truncates some more")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Chuck Lever <chucklever@gmail.com>
> ---
> fs/nfsd/vfs.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index a974368026a1..fd4a32e0b0b4 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -332,6 +332,85 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
> 	}
> }
> 
> +/* copy of vfs_truncate with NFS owner override hacked in, sigh.. */
> +static long nfsd_truncate(const struct path *path, loff_t length)
> +{
> +	struct inode *inode;
> +	struct dentry *upperdentry;
> +	long error;
> +
> +	inode = path->dentry->d_inode;
> +
> +	/* For directories it's -EISDIR, for other non-regulars - -EINVAL */
> +	if (S_ISDIR(inode->i_mode))
> +		return -EISDIR;
> +	if (!S_ISREG(inode->i_mode))
> +		return -EINVAL;
> +
> +	error = mnt_want_write(path->mnt);
> +	if (error)
> +		goto out;
> +
> +	/*
> +	 * The file owner always gets access permission for accesses that
> +	 * would normally be checked at open time. This is to make
> +	 * file access work even when the client has done a fchmod(fd, 0).
> +	 *
> +	 * However, `cp foo bar' should fail nevertheless when bar is
> +	 * readonly. A sensible way to do this might be to reject all
> +	 * attempts to truncate a read-only file, because a creat() call
> +	 * always implies file truncation.
> +	 * ... but this isn't really fair.  A process may reasonably call
> +	 * ftruncate on an open file descriptor on a file with perm 000.
> +	 * We must trust the client to do permission checking - using "ACCESS"
> +	 * with NFSv3.
> +	 */
> +	if (!uid_eq(inode->i_uid, current_fsuid())) {
> +		error = inode_permission(inode, MAY_WRITE);
> +		if (error)
> +			goto mnt_drop_write_and_out;
> +	}
> +
> +	error = -EPERM;
> +	if (IS_APPEND(inode))
> +		goto mnt_drop_write_and_out;
> +
> +	/*
> +	 * If this is an overlayfs then do as if opening the file so we get
> +	 * write access on the upper inode, not on the overlay inode.  For
> +	 * non-overlay filesystems d_real() is an identity function.
> +	 */
> +	upperdentry = d_real(path->dentry, NULL, O_WRONLY);
> +	error = PTR_ERR(upperdentry);
> +	if (IS_ERR(upperdentry))
> +		goto mnt_drop_write_and_out;
> +
> +	error = get_write_access(upperdentry->d_inode);
> +	if (error)
> +		goto mnt_drop_write_and_out;
> +
> +	/*
> +	 * Make sure that there are no leases.  get_write_access() protects
> +	 * against the truncate racing with a lease-granting setlease().
> +	 */
> +	error = break_lease(inode, O_WRONLY);
> +	if (error)
> +		goto put_write_and_out;
> +
> +	error = locks_verify_truncate(inode, NULL, length);
> +	if (!error)
> +		error = security_path_truncate(path);
> +	if (!error)
> +		error = do_truncate(path->dentry, length, 0, NULL);

do_truncate does not seem to be available in the EXPORT_SYMBOL pool.


> +
> +put_write_and_out:
> +	put_write_access(upperdentry->d_inode);
> +mnt_drop_write_and_out:
> +	mnt_drop_write(path->mnt);
> +out:
> +	return error;
> +}
> +
> /*
>  * Set various file attributes.  After this call fhp needs an fh_put.
>  */
> @@ -398,7 +477,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> 		    ((iap->ia_valid & ~(ATTR_SIZE | ATTR_MTIME)) == 0))
> 			implicit_mtime = true;
> 
> -		host_err = vfs_truncate(&path, iap->ia_size);
> +		host_err = nfsd_truncate(&path, iap->ia_size);
> 		if (host_err)
> 			goto out_host_err;
> 
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chucklever@gmail.com



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kbuild test robot Feb. 9, 2017, 11:56 p.m. UTC | #2
Hi Christoph,

[auto build test ERROR on nfsd/nfsd-next]
[also build test ERROR on v4.10-rc7 next-20170209]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christoph-Hellwig/nfsd-restore-owner-override-for-truncate/20170210-035117
base:   git://linux-nfs.org/~bfields/linux.git nfsd-next
config: powerpc-mvme5100_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

>> ERROR: "do_truncate" [fs/nfsd/nfsd.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Patch
diff mbox

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index a974368026a1..fd4a32e0b0b4 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -332,6 +332,85 @@  nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
 	}
 }
 
+/* copy of vfs_truncate with NFS owner override hacked in, sigh.. */
+static long nfsd_truncate(const struct path *path, loff_t length)
+{
+	struct inode *inode;
+	struct dentry *upperdentry;
+	long error;
+
+	inode = path->dentry->d_inode;
+
+	/* For directories it's -EISDIR, for other non-regulars - -EINVAL */
+	if (S_ISDIR(inode->i_mode))
+		return -EISDIR;
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;
+
+	error = mnt_want_write(path->mnt);
+	if (error)
+		goto out;
+
+	/*
+	 * The file owner always gets access permission for accesses that
+	 * would normally be checked at open time. This is to make
+	 * file access work even when the client has done a fchmod(fd, 0).
+	 *
+	 * However, `cp foo bar' should fail nevertheless when bar is
+	 * readonly. A sensible way to do this might be to reject all
+	 * attempts to truncate a read-only file, because a creat() call
+	 * always implies file truncation.
+	 * ... but this isn't really fair.  A process may reasonably call
+	 * ftruncate on an open file descriptor on a file with perm 000.
+	 * We must trust the client to do permission checking - using "ACCESS"
+	 * with NFSv3.
+	 */
+	if (!uid_eq(inode->i_uid, current_fsuid())) {
+		error = inode_permission(inode, MAY_WRITE);
+		if (error)
+			goto mnt_drop_write_and_out;
+	}
+
+	error = -EPERM;
+	if (IS_APPEND(inode))
+		goto mnt_drop_write_and_out;
+
+	/*
+	 * If this is an overlayfs then do as if opening the file so we get
+	 * write access on the upper inode, not on the overlay inode.  For
+	 * non-overlay filesystems d_real() is an identity function.
+	 */
+	upperdentry = d_real(path->dentry, NULL, O_WRONLY);
+	error = PTR_ERR(upperdentry);
+	if (IS_ERR(upperdentry))
+		goto mnt_drop_write_and_out;
+
+	error = get_write_access(upperdentry->d_inode);
+	if (error)
+		goto mnt_drop_write_and_out;
+
+	/*
+	 * Make sure that there are no leases.  get_write_access() protects
+	 * against the truncate racing with a lease-granting setlease().
+	 */
+	error = break_lease(inode, O_WRONLY);
+	if (error)
+		goto put_write_and_out;
+
+	error = locks_verify_truncate(inode, NULL, length);
+	if (!error)
+		error = security_path_truncate(path);
+	if (!error)
+		error = do_truncate(path->dentry, length, 0, NULL);
+
+put_write_and_out:
+	put_write_access(upperdentry->d_inode);
+mnt_drop_write_and_out:
+	mnt_drop_write(path->mnt);
+out:
+	return error;
+}
+
 /*
  * Set various file attributes.  After this call fhp needs an fh_put.
  */
@@ -398,7 +477,7 @@  nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 		    ((iap->ia_valid & ~(ATTR_SIZE | ATTR_MTIME)) == 0))
 			implicit_mtime = true;
 
-		host_err = vfs_truncate(&path, iap->ia_size);
+		host_err = nfsd_truncate(&path, iap->ia_size);
 		if (host_err)
 			goto out_host_err;