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 5:31 PM J. Bruce Fields wrote:
>
> 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?
That's a good question. The security_path_{mkdir,mknod} hooks would
then probably be passed the original create mode before applying the
umask, but at that point it's not clear what the new inode's final
mode will be, anyway.
> 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.
The path based security hooks are only used by apparmor and tomoyo.
Those hooks basically control who (which process) can do what where in
the filesystem, but nfsd isn't aware of the "who", and overlayfs is a
layer below the "where".
Andreas
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, &mode);
> if (likely(got_write))
> create_error = may_o_create(&nd->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, &mode);
> error = security_path_mknod(&path, 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, &mode);
> error = security_path_mkdir(&path, 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, &iap->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, &iap->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, &attr->mode);
>
> inode_lock_nested(udir, I_MUTEX_PARENT);
> newdentry = ovl_create_real(udir,
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 95882b3f5f62..7ee647b74bc2 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -578,6 +578,21 @@ posix_acl_chmod(struct inode *inode, umode_t mode)
> }
> EXPORT_SYMBOL(posix_acl_chmod);
>
> +/*
> + * On inode creation, filesystems with ACL support are expected to apply the
> + * umask when no ACL is in
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)
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?
Also I'm not sure if ecryptfs gets this right, so taking the ecryptfs
list into the CC.
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, &mode);
if (likely(got_write))
create_error = may_o_create(&nd->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, &mode);
error = security_path_mknod(&path, 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, &mode);
error = security_path_mkdir(&path, 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, &iap->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, &iap->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, &attr->mode);
inode_lock_nested(udir, I_MUTEX_PARENT);
newdentry = ovl_create_real(udir,
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 95882b3f5f62..7ee647b74bc2 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -578,6 +578,21 @@ posix_acl_chmod(struct inode *inode, umode_t mode)
}
EXPORT_SYMBOL(posix_acl_chmod);
+/*
+ * On inode creation, filesystems with ACL support are expected to apply the
+ * umask when no ACL is inherited from the parent directory; this is usually
+ * done by posix_acl_create. Filesystems like nfsd that call vfs_create,
+ * vfs_mknod, or vfs_mkdir directly are expected to call posix_acl_apply_umask
+ * to apply the umask first when necessary.
+ */
+void
+posix_acl_apply_umask(struct inode *inode, umode_t *mode)
+{
+ if (!IS_POSIXACL(inode))
+ *mode &= ~current_umask();
+}
+EXPORT_SYMBOL(posix_acl_apply_umask);
+
int
posix_acl_create(struct inode *dir, umode_t *mode,
struct posix_acl **default_acl, struct posix_acl **acl)
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 90797f1b421d..76e402ff4f92 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -73,6 +73,7 @@ extern int set_posix_acl(struct inode *, int, struct
posix_acl *);
#ifdef CONFIG_FS_POSIX_ACL
extern int posix_acl_chmod(struct inode *, umode_t);
+extern void posix_acl_apply_umask(struct inode *, umode_t *);
extern int posix_acl_create(struct inode
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
Hi,
On Tue, Jun 16, 2020 at 08:58:49PM -0400, J. Bruce Fields wrote:
> 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);
Thank you, could test this on my test setup and seem to work properly.
Should it also be CC'ed to sta...@vger.kernel.org so it is picked up
by the current supported stable series?
Regards,
Salvatore
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)
Hi Bruce,
On Mon, Jun 15, 2020 at 10:42:12PM -0400, J. Bruce Fields wrote:
> 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(&path, 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.
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.
Regards,
Salvatore
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:42:12PM -0400, J. Bruce Fields wrote: > 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(&path, 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. Yes agreed and can confirm: The other cases in fs/nfsd/vfs.c:nfsd_create_locked() seem to have the problem as well. Regards, Salvatore
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)
Hi Bruce,
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(&path, 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;
Thank you!
Tested your patch on top, and it would solve the directory case, but
the underlying problem is more general (and as you said proably needs
further checking in other places):
root@nfs-test:~# mount -t nfs 192.168.122.150:/srv/data /mnt
root@nfs-test:~# mkdir /mnt/foo && ls -ld /mnt/foo && rmdir /mnt/foo
drwxr-xr-x 2 root root 4096 Jun 16 07:24 /mnt/foo
root@nfs-test:~# touch /mnt/foo && ls -ld /mnt/foo && rm /mnt/foo
-rw-rw-rw- 1 root root 0 Jun 16 07:25 /mnt/foo
root@nfs-test:~# umount /mnt
root@nfs-test:~#
So when creating files the umask is still ignored in the noacl mounted
case.
Regards,
Salvatore
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(&path, 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(&path, 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 Filesystem with noacl (was: Re: Bug#962254: NFS(v4) broken at 4.19.118-2)
Hi Bruce, On Mon, Jun 15, 2020 at 10:50:35AM -0400, J. Bruce Fields wrote: > 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. I think the following is reproducible on a ext4 exported share (with underlying filesystem mounted with noacl to mimic the suspect from the reporter). I tested the same with an older kernel from Debian stretch (running 4.9.210-1+deb9u1) but this does not show the same behaviour. The current test system is running 5.6.14-2 Debian kernel (so 5.6.14). 1/ Create an ext4 filesystem: # mkfs.ext4 /dev/vdb1 2/ Mount the filesystem with noacl (to mimic the situation): /dev/vdb1 /srv/data ext4 defaults,noacl 0 0 root@nfs-test:~# mount | grep vdb1 /dev/vdb1 on /srv/data type ext4 (rw,relatime,noacl) 3/ Export with /srv/data 192.168.122.1/24(rw,sync,no_subtree_check,no_root_squash) 4/ Reproduce the issue root@nfs-test:~# mount -t nfs 192.168.122.150:/srv/data /mnt root@nfs-test:~# mkdir /mnt/foo && ls -ld /mnt/foo && rmdir /mnt/foo drwxrwxrwx 2 root root 4096 Jun 15 20:24 /mnt/foo root@nfs-test:~# mount | grep data /dev/vdb1 on /srv/data type ext4 (rw,relatime,noacl) 192.168.122.150:/srv/data on /mnt type nfs4 (rw,relatime,vers=4.2,rsize=524288,wsize=524288,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,clientaddr=192.168.122.150,local_lock=none,addr=192.168.122.150) root@nfs-test:~# umount /mnt Looking at a wireshark captured sniff, the situation was the same as in the previous ZFS case, in the gettattr from the client and the server does indicate support for the new mode_umask. Then later in the CREATE operation, the client sets the mode_umask attribute, with mode part set to '0777' and umask to '022'. The mode replied is then as well '0777'. Notabene: if not mounting the filesystem with noacl, then there is no observed behaviour change here. Regards, Salvatore
