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 <[email protected]>
Reported-by: Chuck Lever <[email protected]>
---
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
> On Feb 9, 2017, at 9:22 AM, Christoph Hellwig <[email protected]> wrote:
>=20
> 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.
>=20
> Fixes: 41f53350 ("nfsd: special case truncates some more")
> Signed-off-by: Christoph Hellwig <[email protected]>
> Reported-by: Chuck Lever <[email protected]>
> ---
> fs/nfsd/vfs.c | 81 =
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 80 insertions(+), 1 deletion(-)
>=20
> 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)
> }
> }
>=20
> +/* 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 =3D 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 =3D 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 =3D inode_permission(inode, MAY_WRITE);
> + if (error)
> + goto mnt_drop_write_and_out;
> + }
> +
> + error =3D -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 =3D d_real(path->dentry, NULL, O_WRONLY);
> + error =3D PTR_ERR(upperdentry);
> + if (IS_ERR(upperdentry))
> + goto mnt_drop_write_and_out;
> +
> + error =3D 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 =3D break_lease(inode, O_WRONLY);
> + if (error)
> + goto put_write_and_out;
> +
> + error =3D locks_verify_truncate(inode, NULL, length);
> + if (!error)
> + error =3D security_path_truncate(path);
> + if (!error)
> + error =3D 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)) =3D=3D =
0))
> implicit_mtime =3D true;
>=20
> - host_err =3D vfs_truncate(&path, iap->ia_size);
> + host_err =3D nfsd_truncate(&path, iap->ia_size);
> if (host_err)
> goto out_host_err;
>=20
> --=20
> 2.11.0
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
[email protected]
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)
{
--b.
>
> Fixes: 41f53350 ("nfsd: special case truncates some more")
> Signed-off-by: Christoph Hellwig <[email protected]>
> Reported-by: Chuck Lever <[email protected]>
> ---
> 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
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.
> On Feb 9, 2017, at 11:01 AM, J. Bruce Fields <[email protected]> =
wrote:
>=20
> 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.
>=20
> This also needs:
>=20
>=20
> 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);
>=20
> 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 <[email protected]>
> --b.
>=20
>>=20
>> Fixes: 41f53350 ("nfsd: special case truncates some more")
>> Signed-off-by: Christoph Hellwig <[email protected]>
>> Reported-by: Chuck Lever <[email protected]>
>> ---
>> fs/nfsd/vfs.c | 81 =
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 80 insertions(+), 1 deletion(-)
>>=20
>> 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)
>> }
>> }
>>=20
>> +/* 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 =3D 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 =3D 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 =3D inode_permission(inode, MAY_WRITE);
>> + if (error)
>> + goto mnt_drop_write_and_out;
>> + }
>> +
>> + error =3D -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 =3D d_real(path->dentry, NULL, O_WRONLY);
>> + error =3D PTR_ERR(upperdentry);
>> + if (IS_ERR(upperdentry))
>> + goto mnt_drop_write_and_out;
>> +
>> + error =3D 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 =3D break_lease(inode, O_WRONLY);
>> + if (error)
>> + goto put_write_and_out;
>> +
>> + error =3D locks_verify_truncate(inode, NULL, length);
>> + if (!error)
>> + error =3D security_path_truncate(path);
>> + if (!error)
>> + error =3D 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)) =3D=3D =
0))
>> implicit_mtime =3D true;
>>=20
>> - host_err =3D vfs_truncate(&path, iap->ia_size);
>> + host_err =3D nfsd_truncate(&path, iap->ia_size);
>> if (host_err)
>> goto out_host_err;
>>=20
>> --=20
>> 2.11.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
[email protected]
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.
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