Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported Filesystem with noacl (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
On Wed, Jun 17, 2020 at 04:42:56PM +0200, Andreas Gruenbacher wrote:
> Hi Bruce,
>
> On Wed, Jun 17, 2020 at 2:58 AM J. Bruce Fields wrote:
> > I think I'll send the following upstream.
>
> looking good, but how about using a little helper for this?
I like it. And the new comment's helpful too.
>
> Also I'm not sure if ecryptfs gets this right, so taking the ecryptfs
> list into the CC.
Yes, questions I had while doing this:
- cachefiles, ecrypfs, devtmpfs, and unix_mknod skip the check,
is that OK for all of them? (Overlayfs too, I think?--that
code's harder to follow.
- why don't vfs_{create,mknod,mkdir} do the IS_POSIXACL check
themselves? Even if it's unnecessary for some callers, surely
it wouldn't be wrong?
I also wondered why both vfs_{create,mknod,mkdir} and the callers were
calling security hooks, but now I see that the callers are calling
security_path_* hooks and the vfs_ functions are calling
security_inode_* hooks, so I guess they're not redundant.
Though now I wonder why some of the callers (nfsd, overlayfs) are
skipping the security_path_* hooks.
--b.
>
> Thanks,
> Andreas
>
> --
>
> Add a posix_acl_apply_umask helper for filesystems like nfsd to apply
> the umask before calling into vfs_create, vfs_mkdir, and vfs_mknod when
> necessary.
>
> Signed-off-by: Andreas Gruenbacher
> ---
> fs/namei.c| 9 +++--
> fs/nfsd/vfs.c | 6 ++
> fs/overlayfs/dir.c| 4 ++--
> fs/posix_acl.c| 15 +++
> include/linux/posix_acl.h | 6 ++
> 5 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 72d4219c93ac..a68887d3a446 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3054,8 +3054,7 @@ static struct dentry *lookup_open(struct nameidata *nd,
> struct file *file,
> if (open_flag & O_CREAT) {
> if (open_flag & O_EXCL)
> open_flag &= ~O_TRUNC;
> - if (!IS_POSIXACL(dir->d_inode))
> - mode &= ~current_umask();
> + posix_acl_apply_umask(dir->d_inode, );
> if (likely(got_write))
> create_error = may_o_create(>path, dentry, mode);
> else
> @@ -3580,8 +3579,7 @@ long do_mknodat(int dfd, const char __user *filename,
> umode_t mode,
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
>
> - if (!IS_POSIXACL(path.dentry->d_inode))
> - mode &= ~current_umask();
> + posix_acl_apply_umask(path.dentry->d_inode, );
> error = security_path_mknod(, dentry, mode, dev);
> if (error)
> goto out;
> @@ -3657,8 +3655,7 @@ long do_mkdirat(int dfd, const char __user *pathname,
> umode_t mode)
> if (IS_ERR(dentry))
> return PTR_ERR(dentry);
>
> - if (!IS_POSIXACL(path.dentry->d_inode))
> - mode &= ~current_umask();
> + posix_acl_apply_umask(path.dentry->d_inode, );
> error = security_path_mkdir(, dentry, mode);
> if (!error)
> error = vfs_mkdir(path.dentry->d_inode, dentry, mode);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index d22a056da477..0c625b004b0c 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1226,8 +1226,7 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct
> svc_fh *fhp,
> iap->ia_mode = 0;
> iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
>
> - if (!IS_POSIXACL(dirp))
> - iap->ia_mode &= ~current_umask();
> + posix_acl_apply_umask(dirp, >ia_mode);
>
> err = 0;
> host_err = 0;
> @@ -1461,8 +1460,7 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh
> *fhp,
> goto out;
> }
>
> - if (!IS_POSIXACL(dirp))
> - iap->ia_mode &= ~current_umask();
> + posix_acl_apply_umask(dirp, >ia_mode);
>
> host_err = vfs_create(dirp, dchild, iap->ia_mode, true);
> if (host_err < 0) {
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 1bba4813f9cb..4d98db2a0208 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -325,8 +325,8 @@ static int ovl_create_upper(struct dentry *dentry, struct
> inode *inode,
> struct dentry *newdentry;
> int err;
>
> - if (!attr->hardlink && !IS_POSIXACL(udir))
> - attr->mode &= ~current_umask();
> + if (!attr->hardlink)
> +posix_acl_apply_umask(udir, >mode);
>
> inode_lock_nested(udir, I_MUTEX_PARENT);
>
Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported Filesystem with noacl
On Wed, Jun 17, 2020 at 06:58:29AM +0200, Salvatore Bonaccorso wrote: > On Tue, Jun 16, 2020 at 08:58:49PM -0400, J. Bruce Fields wrote: > Thank you, could test this on my test setup and seem to work properly. Great, thanks. > Should it also be CC'ed to sta...@vger.kernel.org so it is picked up > by the current supported stable series? Will do.--b.
Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported Filesystem with noacl (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
On Tue, Jun 16, 2020 at 06:16:58PM +0200, Salvatore Bonaccorso wrote:
> This might be unneeded to test but as additional datapoint which
> confirms the suspect: I tried check the commit around 47057abde515
> ("nfsd: add support for the umask attribute") in 4.10-rc1
>
> A kernel built with 47057abde515~1, and mounting from an enough recent
> client which has at least dff25ddb4808 ("nfs: add support for the
> umask attribute") does not show the observed behaviour, the server
> built with 47057abde515 does.
Thanks for the confirmation!
I think I'll send the following upstream.
--b.
commit 595ccdca9321
Author: J. Bruce Fields
Date: Tue Jun 16 16:43:18 2020 -0400
nfsd: apply umask on fs without ACL support
The server is failing to apply the umask when creating new objects on
filesystems without ACL support.
To reproduce this, you need to use NFSv4.2 and a client and server
recent enough to support umask, and you need to export a filesystem that
lacks ACL support (for example, ext4 with the "noacl" mount option).
Filesystems with ACL support are expected to take care of the umask
themselves (usually by calling posix_acl_create).
For filesystems without ACL support, this is up to the caller of
vfs_create(), vfs_mknod(), or vfs_mkdir().
Reported-by: Elliott Mitchell
Reported-by: Salvatore Bonaccorso
Fixes: 47057abde515 ("nfsd: add support for the umask attribute")
Signed-off-by: J. Bruce Fields
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 0aa02eb18bd3..8fa3e0ff3671 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1225,6 +1225,9 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh
*fhp,
iap->ia_mode = 0;
iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type;
+ if (!IS_POSIXACL(dirp))
+ iap->ia_mode &= ~current_umask();
+
err = 0;
host_err = 0;
switch (type) {
@@ -1457,6 +1460,9 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
goto out;
}
+ if (!IS_POSIXACL(dirp))
+ iap->ia_mode &= ~current_umask();
+
host_err = vfs_create(dirp, dchild, iap->ia_mode, true);
if (host_err < 0) {
fh_drop_write(fhp);
Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported Filesystem with noacl (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
On Mon, Jun 15, 2020 at 10:38:20PM -0400, J. Bruce Fields wrote: > Thanks for the detailed reproducer. > > It's weird, as the server is basically just setting the transmitted > umask and then calling into the vfs to handle the rest, so it's not much > different from any other user. But the same reproducer run just on the > ext4 filesystem does give the right permissions > > Oh, but looking at the system call, fs_namei.c:do_mkdirat(), it does: > > if (!IS_POSIXACL(path.dentry->d_inode)) > mode &= ~current_umask(); > error = security_path_mkdir(, dentry, mode); > if (!error) > error = vfs_mkdir(path.dentry->d_inode, dentry, mode); > > whereas nfsd just calls into vfs_mkdir(). > > And that IS_POSIXACL() check is exactly a check whether the filesystem > supports ACLs. So I guess it's the responsibility of the caller of > vfs_mkdir() to handle that case. But, that's unsatisfying: why isn't vfs_mkdir() taking care of this itself? And what about that security_path_mkdir() call? And are the other cases of that switch in fs/nfsd/vfs.c:nfsd_create_locked() correct? I think there may be some more cleanup here called for, I'll poke around tomorrow. --b.
Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported Filesystem with noacl (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
Thanks for the detailed reproducer.
It's weird, as the server is basically just setting the transmitted
umask and then calling into the vfs to handle the rest, so it's not much
different from any other user. But the same reproducer run just on the
ext4 filesystem does give the right permissions
Oh, but looking at the system call, fs_namei.c:do_mkdirat(), it does:
if (!IS_POSIXACL(path.dentry->d_inode))
mode &= ~current_umask();
error = security_path_mkdir(, dentry, mode);
if (!error)
error = vfs_mkdir(path.dentry->d_inode, dentry, mode);
whereas nfsd just calls into vfs_mkdir().
And that IS_POSIXACL() check is exactly a check whether the filesystem
supports ACLs. So I guess it's the responsibility of the caller of
vfs_mkdir() to handle that case.
So the obvious fix is something like (untested!)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 0aa02eb18bd3..dabdcca58969 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1234,6 +1234,8 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh
*fhp,
nfsd_check_ignore_resizing(iap);
break;
case S_IFDIR:
+ if (!IS_POSIXACL(dirp))
+ iap->ia_mode &= ~current_umask();
host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
if (!host_err && unlikely(d_unhashed(dchild))) {
struct dentry *d;
--b.
Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported ZFS (with acltype=off) (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
On Sat, Jun 13, 2020 at 11:45:27AM -0700, Elliott Mitchell wrote: > I disagree with this assessment. All of the reporters have been using > ZFS, but this could indicate an absence of testers using other > filesystems. We need someone with a NFS server which has a 4.15+ kernel > and uses a different filesystem which supports ACLs. Honestly I don't think I currently have a regression test for this so it's possible I could have missed something upstream. I haven't seen any reports, though ZFS's ACL implementation is very different from any in-tree filesystem's, and given limited time, a filesystem with no prospect of going upstream isn't going to get much attention, so, yes, I'd need to see a reproducer on xfs or ext4 or something. --b.
Bug#702448: nfs-common: UID's do not get properly mapped
I've created users test1 and test2 in /etc/passwd, but with swapped UID's There's a common (and understable) mistake here: many people think that NFSv4 never requires uid's and gid's to agree between hosts. This is not true. NFSv4, when used with traditional auth_sys authentication, requires names and ID's to agree between client and server. The behavior you describe is as expected. The issue is that while the NFSv4 protocol itself uses names, it's the RPC protocol (which NFSv4 runs on top of) which carries the information about who is performing a given RPC. How exactly it identifies the user depends on the security flavor. In the auth_sys case, it uses numeric ID's. For example, in this case, when you create a file, the owner of the new file will the one who performed the create. The server determines this from the uid sent in the rpc header. However, when you ls -l the file, the information about who owns the file is returned as a name in the body of the NFSv4 reply. The clearest way to see what's going on is to rerun your test cases while watching the traffic in wireshark, and look at both the rpc headers and the bodies of the requests. If you wish to get away from numeric id's on the wire, you need to switch to using NFSv4 with kerberos instead of auth_sys. -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
