nfsd: restore owner override for truncate
diff mbox

Message ID 20170209160131.GE17622@fieldses.org
State New
Headers show

Commit Message

J . Bruce Fields Feb. 9, 2017, 4:01 p.m. UTC
On Thu, Feb 09, 2017 at 03:22:38PM +0100, Christoph Hellwig 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.

This also needs:



--b.

> 
> 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);
> +
> +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

Comments

Christoph Hellwig Feb. 9, 2017, 4:01 p.m. UTC | #1
On Thu, Feb 09, 2017 at 11:01:31AM -0500, J. Bruce Fields wrote:
> On Thu, Feb 09, 2017 at 03:22:38PM +0100, Christoph Hellwig 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.
> 
> This also needs:

Yes, indeed.
--
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 Feb. 9, 2017, 6:04 p.m. UTC | #2
> On Feb 9, 2017, at 11:01 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Thu, Feb 09, 2017 at 03:22:38PM +0100, Christoph Hellwig 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.
> 
> This also needs:
> 
> 
> diff --git a/fs/open.c b/fs/open.c
> index 9921f70bc5ca..5d8126b230d9 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -64,6 +64,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> 	inode_unlock(dentry->d_inode);
> 	return ret;
> }
> +EXPORT_SYMBOL_GPL(do_truncate);
> 
> long vfs_truncate(const struct path *path, loff_t length)
> {

After adding this change, I confirmed that t1050-large.sh is working
as expected.

Tested-by: Chuck Lever <chuck.lever@oracle.com>


> --b.
> 
>> 
>> 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);
>> +
>> +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
J . Bruce Fields Feb. 9, 2017, 7:32 p.m. UTC | #3
On Thu, Feb 09, 2017 at 11:01:31AM -0500, J. Bruce Fields wrote:
> On Thu, Feb 09, 2017 at 03:22:38PM +0100, Christoph Hellwig 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.
> 
> This also needs:
> 
> 
> diff --git a/fs/open.c b/fs/open.c
> index 9921f70bc5ca..5d8126b230d9 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -64,6 +64,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  	inode_unlock(dentry->d_inode);
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(do_truncate);
>  
>  long vfs_truncate(const struct path *path, loff_t length)
>  {

Also there's still a warning about a nested mnt_want_write when called
from nfsd4_setattr.

Probably not hard to fix.

But at this point I'd like to revert the original 4af53350 "nfsd:
special case truncates some more".  It fixed a real bug (incorrect
handling of setattrs with both mode and file size), but a bug we've had
for a long time.  It can wait another week or two for us to get this all
right.

--b.
--
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

Patch
diff mbox

diff --git a/fs/open.c b/fs/open.c
index 9921f70bc5ca..5d8126b230d9 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -64,6 +64,7 @@  int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	inode_unlock(dentry->d_inode);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(do_truncate);
 
 long vfs_truncate(const struct path *path, loff_t length)
 {