Hi Elliott,
[I'm adding linux-nfs upstream hopefully J. Bruce Fields or others can
help clarifying]
On Thu, Jun 11, 2020 at 03:37:11PM -0700, Elliott Mitchell wrote:
> Bit more experimentation on this issue.
>
> I tried a very small C program meant to create files with fewer
> permissions bits set. This succeeded which strengthens the theory of
> the umask getting ignored.
>
> I haven't seen anything hinting whether this is more a client or server
> issue.
>
> I can speculate perhaps somewhere between 4.9 and 4.15 the NFS client
> code stepped closer to proper the "proper" 4.2 protocol. If a
> corresponding NFS server was slow at getting merged, what we're seeing
> could happen.
>
> Alternatively someone was trying to get a Linux NFS v4.2 client to work
> better with a different NFS v4.2 server, so they fixed Linux's NFS v4.2
> client. Yet they failed to test with Linux's v4.2 server.
>
>
> This though is speculation. All I can say is sometime between kernels
> 4.9 and 4.15, NFS v4.2 got broken. There are hints this is related to
> handling of umask.
I was initially confused because of the mentioning of only appearing
with the update to 4.19.118-2 but this is now cleared up, so it shows
up when changing from 4.9.x from stretch to 4.19.x.
Now I'm quite unsure if this should and is to be considered a Linux
kernel issue. What follows is just what I found with respect of the
mentioned behaviour. There is a specific aspect of the NFSv4.2
implementation:
In upstream, with [nfsv4.2-umask-support], [47057abde515] NFSv4.2
support was added. The repsective RFC describing it is [RFC8275].
[nfsv4.2-umask-support]: <https://lore.kernel.org/linux-nfs/[email protected]/>
[47057abde515]: <https://git.kernel.org/linus/47057abde515155a4fee53038e7772d6b387e0aa>
[RFC8275]: <https://tools.ietf.org/html/rfc8275>
Since, they allow the umask to be ignored in the presence of
inheritable NFSv4 ACLs.
Now what is or will be confusing is that the behaviour is reproducible
with ZFS default of acltype=off (aclinherit=restricted, sharenfs=off).
Reproducing the issue is easy as follows (all done on Debian unstable
to verify the behaviours can be triggered there as well with more
current 5.6.14-2, zfs-linux on 0.8.4-1):
# zpool create zfs_test /dev/vdb
and exporting /zfs_test in /etc/exports as
/zfs_test 192.168.122.1/24(rw,sync,no_subtree_check,no_root_squash)
The properties of zfs_test would be:
# zfs get acltype,aclinherit,sharenfs zfs_test
NAME PROPERTY VALUE SOURCE
zfs_test acltype off local
zfs_test aclinherit restricted local
zfs_test sharenfs off default
And reproducing then with
# mount -t nfs 192.168.122.150:/zfs_test /mnt
# mkdir /mnt/foo && ls -ld /mnt/foo && rmdir /mnt/foo
drwxrwxrwx 2 root root 2 Jun 13 14:25 /mnt/fo
# umount /mnt
The comment from J. Bruce Fields, in
https://bugzilla.redhat.com/show_bug.cgi?id=1667761#c1 can help debug
it further:
> To start debugging this, I'd recommend looking running wireshark to
> sniff traffic while running your reproducer (mount, mkdir) and
> compare to what's expected from the umask RFC. Somewhere there
> should be a getattr from the client for the supported_attrs
> attribute, and the reply from the server will probably indicate
> support for the new mode_umask attribute. If you find the CREATE
> operation that creates the new directory, you should see the client
> set the mode_umask attribute, with the mode part set to the open
> mode and the umask to the process umask. If those values look
> right, then the problem is likely on the server side.
In fact in sniffing the traffic, there, 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'.
If further needed to debug we should try to distill a sniff with
wireshark providing the repsective pcap.
https://bugzilla.redhat.com/show_bug.cgi?id=1667761
did not further contain specific information on followups.
https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1779736
indicated this was specifically observed on ZFS on Linux only. Seth
Arnold's answer seem to be inline with that that the issue is more on
the ZFS on Linux side and the issue keeps biting people a bit
unexpectedly. Why does this break with ACL off settings?
But there was at least one other (but again without further
detail/followups) that it was observed on an export from OpenWRT, but
no specific details here:
https://bugs.openwrt.org/index.php?do=details&task_id=2581
Both Debian bugs itself were as well with underlying ZFS filesystem exported:
https://bugs.debian.org/934160
https://bugs.debian.org/962254
Any hint on were to pin-point the issue? Both on Linux anf ZFS on
Linux side or only on one of the components?
Regards,
Salvatore
On Sat, Jun 13, 2020 at 02:54:31PM +0200, Salvatore Bonaccorso wrote:
> indicated this was specifically observed on ZFS on Linux only. Seth
> Arnold's answer seem to be inline with that that the issue is more on
> the ZFS on Linux side and the issue keeps biting people a bit
> unexpectedly. Why does this break with ACL off settings?
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.
I'm though doubtful ACLs are related to the actual problem. My
impression of what I've read is they're a useful tool to work around the
problem, but not related to the actual cause.
> But there was at least one other (but again without further
> detail/followups) that it was observed on an export from OpenWRT, but
> no specific details here:
>
> https://bugs.openwrt.org/index.php?do=details&task_id=2581
This appears to be the same reporter as the RedHat bug report (comment 3
on the RedHat report). This is a report for the server portion of the
reporter's setup.
Analyzing the setup, I disagree with one of the prior assessment of this
report. This is OpenWRT on x86_64 hardware which would suggest a
high-end router or embedded device. Such might well have ECC memory and
a processor fast enough to handle ZFS.
Let me add one more data point. I had been thinking I might need the
additional features in Linux-ZFS 0.7.12. As such my NFS server had been
running a 4.9 kernel with Debian's ZFS 0.7.12-2+debg10u1~bpo9+1 packages.
Now with the problem manifesting my NFS server is running a 4.19 kernel
with Debian's ZFS 0.7.12-2+deb10u2 packages.
I could well believe the actual root cause is a problem with the
Linux-ZFS implementation. What manifested the problem though seems to be
in Linux's NFS implementation between 4.9 and 4.15. ie Linux-ZFS
implemented /something/ which worked when implemented, but may not have
properly implemented the intended API and was broken by Linux-NFS.
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | [email protected] PGP 87145445 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
If you are violating our license please also don't spam our list when
using your crappy combination.
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.
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
On Mon, Jun 15, 2020 at 10:50:35AM -0400, J. Bruce Fields wrote:
> 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.
Salvatore managing to reproduce it with ext4 yet all prior reports with
the filesystem used being known was ZFS seems to suggest one of two
things.
First, could be enabling POSIX ACLs has been very strongly pushed by
other filesystems, while ZFS hasn't pushed them as strongly.
Second, could be a substantial majority of users of NFS are using ZFS.
If the former, this simply means an additional test case is needed. If
the latter, then any testing of NFS which excludes ZFS is going to have
underwhelming coverage.
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | [email protected] PGP 87145445 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
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.
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.
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
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
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
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 <[email protected]>
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 <[email protected]>
Reported-by: Salvatore Bonaccorso <[email protected]>
Fixes: 47057abde515 ("nfsd: add support for the umask attribute")
Signed-off-by: J. Bruce Fields <[email protected]>
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);
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 <[email protected]>
> 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 <[email protected]>
> Reported-by: Salvatore Bonaccorso <[email protected]>
> Fixes: 47057abde515 ("nfsd: add support for the umask attribute")
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> 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 [email protected] so it is picked up
by the current supported stable series?
Regards,
Salvatore
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 [email protected] so it is picked up
> by the current supported stable series?
Will do.--b.
Hi Bruce,
On Wed, Jun 17, 2020 at 2:58 AM J. Bruce Fields <[email protected]> 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 <[email protected]>
---
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 *, umode_t *, struct posix_acl **,
struct posix_acl **);
extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
@@ -99,6 +100,11 @@ static inline int posix_acl_chmod(struct inode *inode, umode_t mode)
#define simple_set_acl NULL
+static inline void posix_acl_apply_umask(struct inode *inode, umode_t *mode)
+{
+ *mode &= ~current_umask();
+}
+
static inline int simple_acl_create(struct inode *dir, struct inode *inode)
{
return 0;
base-commit: 69119673bd50b176ded34032fadd41530fb5af21
prerequisite-patch-id: a8319d40da9f70f478892d0bd8e63f540364b089
--
2.26.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 <[email protected]> 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 <[email protected]>
> ---
> 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 *, umode_t *, struct posix_acl **,
> struct posix_acl **);
> extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **);
> @@ -99,6 +100,11 @@ static inline int posix_acl_chmod(struct inode *inode, umode_t mode)
>
> #define simple_set_acl NULL
>
> +static inline void posix_acl_apply_umask(struct inode *inode, umode_t *mode)
> +{
> + *mode &= ~current_umask();
> +}
> +
> static inline int simple_acl_create(struct inode *dir, struct inode *inode)
> {
> return 0;
>
> base-commit: 69119673bd50b176ded34032fadd41530fb5af21
> prerequisite-patch-id: a8319d40da9f70f478892d0bd8e63f540364b089
> --
> 2.26.2
>
On Wed, Jun 17, 2020 at 5:31 PM J. Bruce Fields <[email protected]> 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 <[email protected]> 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