Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.kernel > #67232 > unrolled thread

Bug#962254: NFS(v4) broken at 4.19.118-2

Back to article view | Back to linux.debian.kernel

Contents

  Bug#962254: NFS(v4) broken at 4.19.118-2 Elliott Mitchell <ehem+debian@m5p.com> - 2020-06-05 07:50 +0200
    Processed: Re: Bug#962254: NFS(v4) broken at 4.19.118-2 "Debian Bug Tracking System" <owner@bugs.debian.org> - 2020-06-05 08:50 +0200
    Bug#962254: NFS(v4) broken at 4.19.118-2 Salvatore Bonaccorso <carnil@debian.org> - 2020-06-05 08:50 +0200
      Bug#962254: NFS(v4) broken at 4.19.118-2 Elliott Mitchell <ehem+debian@m5p.com> - 2020-06-05 20:20 +0200
        Bug#962254: NFS(v4) broken at 4.19.118-2 Salvatore Bonaccorso <carnil@debian.org> - 2020-06-05 20:40 +0200
          Bug#962254: NFS(v4) broken at 4.19.118-2 Elliott Mitchell <ehem+debian@m5p.com> - 2020-06-06 00:40 +0200
          Bug#934160: Bug#962254: NFS(v4) broken at 4.19.118-2 Elliott Mitchell <ehem+debian@m5p.com> - 2020-06-09 04:00 +0200
    Processed: Re: Bug#962254: NFS(v4) broken at 4.19.118-2 "Debian Bug Tracking System" <owner@bugs.debian.org> - 2020-06-09 04:00 +0200
    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) Salvatore Bonaccorso <carnil@debian.org> - 2020-06-13 15:00 +0200
    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) "J. Bruce Fields" <bfields@redhat.com> - 2020-06-15 17:10 +0200
      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) Salvatore Bonaccorso <carnil@debian.org> - 2020-06-15 21:00 +0200
    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) "J. Bruce Fields" <bfields@redhat.com> - 2020-06-16 04:50 +0200
      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) Salvatore Bonaccorso <carnil@debian.org> - 2020-06-16 07:40 +0200
      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) Salvatore Bonaccorso <carnil@debian.org> - 2020-06-16 18:30 +0200
        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) "J. Bruce Fields" <bfields@redhat.com> - 2020-06-17 03:10 +0200
          Bug#962254: Umask ignored when mounting NFSv4.2 share of an exported Filesystem with noacl Salvatore Bonaccorso <carnil@debian.org> - 2020-06-17 07:10 +0200
    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) Salvatore Bonaccorso <carnil@debian.org> - 2020-06-16 07:40 +0200
    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) "J. Bruce Fields" <bfields@redhat.com> - 2020-06-17 17:40 +0200
      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) Andreas Gruenbacher <agruenba@redhat.com> - 2020-06-17 19:00 +0200

#67232 — Bug#962254: NFS(v4) broken at 4.19.118-2

Package: src:linux
Version: 4.19.118+2
Severity: important

Somewhere between linux-image-4.19.0-8-amd64/4.19.98+1+deb10u1 and
linux-image-4.19.0-9-amd64/4.19.118+2 NFS, in particular v4 got broken.
Mounting an appropriate filesystem became unreliable, and once mounted
behavior is unpredictable.

In particular in the problematic case `umask 022 ; touch foo ; ls -l foo`
yields a -rw-rw-rw- file.

This occurs if *both* the server *and* client are on 4.19.118+2.  I have
confirmed this does NOT occur if the server is on a 4.9 kernel.  I have
also confirmed this does NOT occur if the client is on a 4.9 or
4.19.98+1+deb10u1 kernel.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[toc] | [next] | [standalone]

#67233 — Processed: Re: Bug#962254: NFS(v4) broken at 4.19.118-2

Processing control commands:

> tags -1 + moreinfo unreproducible
Bug #962254 [src:linux] NFS(v4) broken at 4.19.118-2
Added tag(s) unreproducible and moreinfo.

-- 
962254: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=962254
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems

[toc] | [prev] | [next] | [standalone]

#67234

Control: tags -1 + moreinfo unreproducible

Control: notfound -1 4.19.118+2
Control: found -1 4.19.118-2

Hi Elliot,

On Thu, Jun 04, 2020 at 10:16:07PM -0700, Elliott Mitchell wrote:
> Package: src:linux
> Version: 4.19.118+2
> Severity: important
> 
> Somewhere between linux-image-4.19.0-8-amd64/4.19.98+1+deb10u1 and
> linux-image-4.19.0-9-amd64/4.19.118+2 NFS, in particular v4 got broken.
> Mounting an appropriate filesystem became unreliable, and once mounted
> behavior is unpredictable.
> 
> In particular in the problematic case `umask 022 ; touch foo ; ls -l foo`
> yields a -rw-rw-rw- file.
> 
> This occurs if *both* the server *and* client are on 4.19.118+2.  I have
> confirmed this does NOT occur if the server is on a 4.9 kernel.  I have
> also confirmed this does NOT occur if the client is on a 4.9 or
> 4.19.98+1+deb10u1 kernel.

I cannot reproducde the described behaviour. Can you give more details
on your setup?

How do you export the filesystem?
What is the underlying filesystem exported?
How and whith which options do clients mount the NFS share?

Regards,
Salvatore

[toc] | [prev] | [next] | [standalone]

#67245

On Fri, Jun 05, 2020 at 08:44:26AM +0200, Salvatore Bonaccorso wrote:
> 
> On Thu, Jun 04, 2020 at 10:16:07PM -0700, Elliott Mitchell wrote:
> > Somewhere between linux-image-4.19.0-8-amd64/4.19.98+1+deb10u1 and
> > linux-image-4.19.0-9-amd64/4.19.118+2 NFS, in particular v4 got broken.
> > Mounting an appropriate filesystem became unreliable, and once mounted
> > behavior is unpredictable.
> > 
> > In particular in the problematic case `umask 022 ; touch foo ; ls -l foo`
> > yields a -rw-rw-rw- file.
> > 
> > This occurs if *both* the server *and* client are on 4.19.118+2.  I have
> > confirmed this does NOT occur if the server is on a 4.9 kernel.  I have
> > also confirmed this does NOT occur if the client is on a 4.9 or
> > 4.19.98+1+deb10u1 kernel.
> 
> I cannot reproducde the described behaviour. Can you give more details
> on your setup?
> 
> How do you export the filesystem?
> What is the underlying filesystem exported?
> How and whith which options do clients mount the NFS share?

Presently it is a whole directories being exported to hosts.  The
filesystem on the server is ZFS.

Client is mounting hard,intr.  Client is using cachefilesd, but that
appears unrelated to the issue.

As this is NFSv4 (v2 and v3 are thoroughly disabled on the server), TCP
is being used.  The port is non-standard.

I'm uncertain I properly tried server on 4.9, client on 4.19.118+2 (could
be this is strictly 4.19.118+2 NFSv4 client code).


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[toc] | [prev] | [next] | [standalone]

#67246

Hi Elliott,

Thanks for the additional information.

On Fri, Jun 05, 2020 at 10:43:49AM -0700, Elliott Mitchell wrote:
> On Fri, Jun 05, 2020 at 08:44:26AM +0200, Salvatore Bonaccorso wrote:
> > 
> > On Thu, Jun 04, 2020 at 10:16:07PM -0700, Elliott Mitchell wrote:
> > > Somewhere between linux-image-4.19.0-8-amd64/4.19.98+1+deb10u1 and
> > > linux-image-4.19.0-9-amd64/4.19.118+2 NFS, in particular v4 got broken.
> > > Mounting an appropriate filesystem became unreliable, and once mounted
> > > behavior is unpredictable.
> > > 
> > > In particular in the problematic case `umask 022 ; touch foo ; ls -l foo`
> > > yields a -rw-rw-rw- file.
> > > 
> > > This occurs if *both* the server *and* client are on 4.19.118+2.  I have
> > > confirmed this does NOT occur if the server is on a 4.9 kernel.  I have
> > > also confirmed this does NOT occur if the client is on a 4.9 or
> > > 4.19.98+1+deb10u1 kernel.
> > 
> > I cannot reproducde the described behaviour. Can you give more details
> > on your setup?
> > 
> > How do you export the filesystem?
> > What is the underlying filesystem exported?
> > How and whith which options do clients mount the NFS share?
> 
> Presently it is a whole directories being exported to hosts.  The
> filesystem on the server is ZFS.
> 
> Client is mounting hard,intr.  Client is using cachefilesd, but that
> appears unrelated to the issue.
> 
> As this is NFSv4 (v2 and v3 are thoroughly disabled on the server), TCP
> is being used.  The port is non-standard.
> 
> I'm uncertain I properly tried server on 4.9, client on 4.19.118+2 (could
> be this is strictly 4.19.118+2 NFSv4 client code).

This now let some rings bell, the described scenario is very similar
to what was reported in https://bugs.debian.org/934160

Respectively
https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1779736 and
https://bugzilla.redhat.com/show_bug.cgi?id=1667761 .

Salvatore

[toc] | [prev] | [next] | [standalone]

#67247

On Fri, Jun 05, 2020 at 08:36:31PM +0200, Salvatore Bonaccorso wrote:
> This now let some rings bell, the described scenario is very similar
> to what was reported in https://bugs.debian.org/934160
> 
> Respectively
> https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1779736 and
> https://bugzilla.redhat.com/show_bug.cgi?id=1667761 .

Those do indeed seem similar and could be the same bug, but attributing
the bug to a distinct package.  Alternatively this is several bugs and
*all* of them need to be present for the issue to occur.

Seems I'll need to do some checking of the VM with the earlier kernel
and see which updates cause it to break...


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[toc] | [prev] | [next] | [standalone]

#67258 — Bug#934160: Bug#962254: NFS(v4) broken at 4.19.118-2

Control: tags 962254 +security -unreproducible
Control: severity 962254 grave

On Fri, Jun 05, 2020 at 08:36:31PM +0200, Salvatore Bonaccorso wrote:
> This now let some rings bell, the described scenario is very similar
> to what was reported in https://bugs.debian.org/934160
> 
> Respectively
> https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1779736 and
> https://bugzilla.redhat.com/show_bug.cgi?id=1667761 .

Upon more experimentation I continue to favor this being a kernel bug
(src:linux, bug #962254) and not a bug with nfs-common.

Setting vers=4.1 works around the issue, so this is *strictly* NFSv4.2.

I was able to reproduce this issue on a system with nfs-common
1:1.3.4-2.1 and a 4.19.118-2 kernel.

Based upon what I've observed I believe this requires a recent kernel on
*both* NFS client and NFS server.  A NFS client with 4.9 connecting to a
NFS server with 4.19 does NOT experience this issue.

I suspect my earlier assessment of this appearing between 4.19.98-1 and
4.19.118-2 was erroneous.  I think I was mislead by the order of
computers being updated, and a NFS client with 4.19 connecting to a NFS
server with 4.9 also does not experience this issue.

From https://bugs.launchpad.net/ubuntu/+source/nfs-utils/+bug/1779736
this bug appeared somewhere between Linux kernels 4.9 and 4.15.

I concur with John Goerzen's assessment of this qualifying as grave due
to its security implications.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[toc] | [prev] | [next] | [standalone]

#67259 — Processed: Re: Bug#962254: NFS(v4) broken at 4.19.118-2

Processing control commands:

> tags 962254 +security -unreproducible
Bug #962254 [src:linux] NFS(v4) broken at 4.19.118-2
Ignoring request to alter tags of bug #962254 to the same tags previously set
Bug #962254 [src:linux] NFS(v4) broken at 4.19.118-2
Ignoring request to alter tags of bug #962254 to the same tags previously set
> severity 962254 grave
Bug #962254 [src:linux] NFS(v4) broken at 4.19.118-2
Ignoring request to change severity of Bug 962254 to the same value.

-- 
962254: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=962254
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems

[toc] | [prev] | [next] | [standalone]

#67303 — 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)

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/1477686228-12158-1-git-send-email-bfields@redhat.com/>
[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

[toc] | [prev] | [next] | [standalone]

#67309 — 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.

[toc] | [prev] | [next] | [standalone]

#67310 — 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

[toc] | [prev] | [next] | [standalone]

#67312 — 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.

[toc] | [prev] | [next] | [standalone]

#67314 — 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

[toc] | [prev] | [next] | [standalone]

#67316 — 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

[toc] | [prev] | [next] | [standalone]

#67321 — 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 <bfields@redhat.com>
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 <ehem+debian@m5p.com>
    Reported-by: Salvatore Bonaccorso <carnil@debian.org>
    Fixes: 47057abde515 ("nfsd: add support for the umask attribute")
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

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

[toc] | [prev] | [next] | [standalone]

#67322 — 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 <bfields@redhat.com>
> 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 <ehem+debian@m5p.com>
>     Reported-by: Salvatore Bonaccorso <carnil@debian.org>
>     Fixes: 47057abde515 ("nfsd: add support for the umask attribute")
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> 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 stable@vger.kernel.org so it is picked up
by the current supported stable series?

Regards,
Salvatore

[toc] | [prev] | [next] | [standalone]

#67313 — 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

[toc] | [prev] | [next] | [standalone]

#67323 — 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 <bfields@redhat.com> 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 <agruenba@redhat.com>
> ---
>  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
> 

[toc] | [prev] | [next] | [standalone]

#67324 — 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 <bfields@redhat.com> 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 <bfields@redhat.com> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.kernel

csiph-web