Hi NFS folks,
During our xfstests, we found generic/633 fails like:
============================================
FSTYP -- nfs
PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP PREEMPT Sat Feb 19 15:09:03 CST 2022
MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
generic/633 0s ... [failed, exit status 1]- output mismatch (see /root/xfstests-dev/results//generic/633.out.bad)
--- tests/generic/633.out 2021-05-23 14:03:08.879999997 +0800
+++ /root/xfstests-dev/results//generic/633.out.bad 2022-02-19 16:31:28.660000013 +0800
@@ -1,2 +1,4 @@
QA output created by 633
Silence is golden
+idmapped-mounts.c: 7906: setgid_create - Success - failure: is_setgid
+idmapped-mounts.c: 13907: run_test - Success - failure: create operations in directories with setgid bit set
...
(Run 'diff -u /root/xfstests-dev/tests/generic/633.out /root/xfstests-dev/results//generic/633.out.bad' to see the entire diff)
Ran: generic/633
Failures: generic/633
Failed 1 of 1 tests
============================================
The failed test is about setgid inheritance.
When a file is created with S_ISGID in the directory with S_ISGID,
NFS doesn't strip the setgid bit of the new created file but others
(ext4/xfs/btrfs) do. They call inode_init_owner() which does
the strip after new_inode().
However, NFS has its own logical to handle inode capacities.
As the test says the behavior can be filesystem type specific,
I'd report to you NFS guys and ask whether it's a bug or not?
Thanks.
--
Su
On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner wrote:
> On Sat, Feb 19, 2022 at 08:34:30AM +0000, [email protected] wrote:
> > Hi NFS folks,
> > During our xfstests, we found generic/633 fails like:
> > ============================================
> > FSTYP -- nfs
> > PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP
> > PREEMPT Sat Feb 19 15:09:03 CST 2022
> > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
> >
> > generic/633 0s ... [failed, exit status 1]- output mismatch (see
> > /root/xfstests-dev/results//generic/633.out.bad)
> > --- tests/generic/633.out 2021-05-23 14:03:08.879999997 +0800
> > +++ /root/xfstests-dev/results//generic/633.out.bad 2022-02-19
> > 16:31:28.660000013 +0800
> > @@ -1,2 +1,4 @@
> > QA output created by 633
> > Silence is golden
> > +idmapped-mounts.c: 7906: setgid_create - Success - failure:
> > is_setgid
> > +idmapped-mounts.c: 13907: run_test - Success - failure: create
> > operations in directories with setgid bit set
> > ...
> > (Run 'diff -u /root/xfstests-dev/tests/generic/633.out
> > /root/xfstests-dev/results//generic/633.out.bad' to see the entire
> > diff)
> > Ran: generic/633
> > Failures: generic/633
> > Failed 1 of 1 tests
> > ============================================
> >
> > The failed test is about setgid inheritance.
> > When a file is created with S_ISGID in the directory with S_ISGID,
> > NFS doesn't strip the setgid bit of the new created file but
> > others
> > (ext4/xfs/btrfs) do. They call inode_init_owner() which does
> > the strip after new_inode().
> > However, NFS has its own logical to handle inode capacities.
> > As the test says the behavior can be filesystem type specific,
> > I'd report to you NFS guys and ask whether it's a bug or not?
>
> Thanks for the report. I'm not sure why NFS would have different
> rules
> for setgid inheritance. So I'm inclined to think that this is simply
> a
> bug similar to what we saw in xfs and ceph. But I'll let the NFS
> folks
> determine that.
>
> XFS is only special in so far as it has a sysctl that lets it alter
> sgid
> inheritance behavior. And it only has that to allow for legacy irix
> semantics iiuc.
OK, so how do you expect this 'idmapped-mounts' functionality to work
on NFS? I'm not asking about this bug in particular. I'm asking about
what this is supposed to do in general.
At a quick glance, it looks to me as if these idmapped mount helpers
are just hacking different values into the inode cache representation
of files, and then somehow expecting that to result in different
behaviour.
That's not going to work with NFS, for two reasons:
1. Security is enforced by the server and not the client. If you
want these values you're poking into the inode cache to change
the behaviour of the server, then they have to be propagated by
the client to that server.
2. The NFS security model is authentication based. In particular,
when strong authentication is being used, then my identity is
established by user+password that I've logged in as to the
kerberos server (or whatever). So while the idmapped mount stuff
may be poking values into my credential or the inode cache, the
server is going to ignore all that and tell me that any file I
create is owned by user trond@domain. It will not allow me to
change file ownership or to override access permissions unless
trond@domain happens to be a privileged user such as root.
I'm pretty sure the cifs/smb client will have the same problem.
--
Trond Myklebust Linux NFS client maintainer, Hammerspace
[email protected]
On Sat, Feb 19, 2022 at 08:34:30AM +0000, [email protected] wrote:
> Hi NFS folks,
> During our xfstests, we found generic/633 fails like:
> ============================================
> FSTYP -- nfs
> PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP PREEMPT Sat Feb 19 15:09:03 CST 2022
> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
>
> generic/633 0s ... [failed, exit status 1]- output mismatch (see /root/xfstests-dev/results//generic/633.out.bad)
> --- tests/generic/633.out 2021-05-23 14:03:08.879999997 +0800
> +++ /root/xfstests-dev/results//generic/633.out.bad 2022-02-19 16:31:28.660000013 +0800
> @@ -1,2 +1,4 @@
> QA output created by 633
> Silence is golden
> +idmapped-mounts.c: 7906: setgid_create - Success - failure: is_setgid
> +idmapped-mounts.c: 13907: run_test - Success - failure: create operations in directories with setgid bit set
> ...
> (Run 'diff -u /root/xfstests-dev/tests/generic/633.out /root/xfstests-dev/results//generic/633.out.bad' to see the entire diff)
> Ran: generic/633
> Failures: generic/633
> Failed 1 of 1 tests
> ============================================
>
> The failed test is about setgid inheritance.
> When a file is created with S_ISGID in the directory with S_ISGID,
> NFS doesn't strip the setgid bit of the new created file but others
> (ext4/xfs/btrfs) do. They call inode_init_owner() which does
> the strip after new_inode().
> However, NFS has its own logical to handle inode capacities.
> As the test says the behavior can be filesystem type specific,
> I'd report to you NFS guys and ask whether it's a bug or not?
Thanks for the report. I'm not sure why NFS would have different rules
for setgid inheritance. So I'm inclined to think that this is simply a
bug similar to what we saw in xfs and ceph. But I'll let the NFS folks
determine that.
XFS is only special in so far as it has a sysctl that lets it alter sgid
inheritance behavior. And it only has that to allow for legacy irix
semantics iiuc.
Christian
On Sat, Feb 19, 2022 at 05:00:18PM +0000, Trond Myklebust wrote:
> On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner wrote:
> > On Sat, Feb 19, 2022 at 08:34:30AM +0000, [email protected] wrote:
> > > Hi NFS folks,
> > > During our xfstests, we found generic/633 fails like:
> > > ============================================
> > > FSTYP -- nfs
> > > PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP
> > > PREEMPT Sat Feb 19 15:09:03 CST 2022
> > > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> > > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
> > >
> > > generic/633 0s ... [failed, exit status 1]- output mismatch (see
> > > /root/xfstests-dev/results//generic/633.out.bad)
> > > --- tests/generic/633.out 2021-05-23 14:03:08.879999997 +0800
> > > +++ /root/xfstests-dev/results//generic/633.out.bad 2022-02-19
> > > 16:31:28.660000013 +0800
> > > @@ -1,2 +1,4 @@
> > > QA output created by 633
> > > Silence is golden
> > > +idmapped-mounts.c: 7906: setgid_create - Success - failure:
> > > is_setgid
> > > +idmapped-mounts.c: 13907: run_test - Success - failure: create
> > > operations in directories with setgid bit set
> > > ...
> > > (Run 'diff -u /root/xfstests-dev/tests/generic/633.out
> > > /root/xfstests-dev/results//generic/633.out.bad' to see the entire
> > > diff)
> > > Ran: generic/633
> > > Failures: generic/633
> > > Failed 1 of 1 tests
> > > ============================================
> > >
> > > The failed test is about setgid inheritance.
> > > When a file is created with S_ISGID in the directory with S_ISGID,
> > > NFS doesn't strip the setgid bit of the new created file but
> > > others
> > > (ext4/xfs/btrfs) do. They call inode_init_owner() which does
> > > the strip after new_inode().
> > > However, NFS has its own logical to handle inode capacities.
> > > As the test says the behavior can be filesystem type specific,
> > > I'd report to you NFS guys and ask whether it's a bug or not?
> >
> > Thanks for the report. I'm not sure why NFS would have different
> > rules
> > for setgid inheritance. So I'm inclined to think that this is simply
> > a
> > bug similar to what we saw in xfs and ceph. But I'll let the NFS
> > folks
> > determine that.
> >
> > XFS is only special in so far as it has a sysctl that lets it alter
> > sgid
> > inheritance behavior. And it only has that to allow for legacy irix
> > semantics iiuc.
>
> OK, so how do you expect this 'idmapped-mounts' functionality to work
> on NFS? I'm not asking about this bug in particular. I'm asking about
> what this is supposed to do in general.
Just to clarify, the bug has nothing to do with idmapped mounts. The
idmapped mount testsuite always had a vfs generic part. That vfs generic
part has been made available to all filesystems supporting xfstests a
few weeks ago. (So far this setgid inheritance bug here has been present
in 3 filesystems.)
>
> At a quick glance, it looks to me as if these idmapped mount helpers
> are just hacking different values into the inode cache representation
> of files, and then somehow expecting that to result in different
(Just to clarify, the inode cache is never changed by an idmapped mount
unless a new filesystem object is created/permantently changed. The
inode cache is left alone otherwise and ownership is only transiently
mapped when checking permissions.)
> behaviour.
> That's not going to work with NFS, for two reasons:
> 1. Security is enforced by the server and not the client. If you
> want these values you're poking into the inode cache to change
> the behaviour of the server, then they have to be propagated by
> the client to that server.
> 2. The NFS security model is authentication based. In particular,
> when strong authentication is being used, then my identity is
> established by user+password that I've logged in as to the
> kerberos server (or whatever). So while the idmapped mount stuff
> may be poking values into my credential or the inode cache, the
> server is going to ignore all that and tell me that any file I
> create is owned by user trond@domain. It will not allow me to
> change file ownership or to override access permissions unless
> trond@domain happens to be a privileged user such as root.
>
> I'm pretty sure the cifs/smb client will have the same problem.
I did a POC for ceph a while back and I did also have POC patches for
cifs/smb. For ceph e.g. the {g,u}id with which a file is to be created
with or will have its ownership changed to is sent from the client to
the server with server being responsible for deciding whether that
creation is allowed or not.
NFS already has a notion of idmapping via upcalls afair. Whether or not
support for idmapped mounts make sense for NFS depends on how the
current idmapping feature is implemented and what use-cases aren't
supported by it that idmapped mounts can.
On Wed, 2022-02-23 at 09:44 +0100, Christian Brauner wrote:
> On Sat, Feb 19, 2022 at 05:00:18PM +0000, Trond Myklebust wrote:
> > On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner wrote:
> > > On Sat, Feb 19, 2022 at 08:34:30AM +0000,
> > > [email protected] wrote:
> > > > Hi NFS folks,
> > > > During our xfstests, we found generic/633 fails like:
> > > > ============================================
> > > > FSTYP -- nfs
> > > > PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP
> > > > PREEMPT Sat Feb 19 15:09:03 CST 2022
> > > > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> > > > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
> > > >
> > > > generic/633 0s ... [failed, exit status 1]- output mismatch
> > > > (see
> > > > /root/xfstests-dev/results//generic/633.out.bad)
> > > > --- tests/generic/633.out 2021-05-23 14:03:08.879999997
> > > > +0800
> > > > +++ /root/xfstests-dev/results//generic/633.out.bad 2022-
> > > > 02-19
> > > > 16:31:28.660000013 +0800
> > > > @@ -1,2 +1,4 @@
> > > > QA output created by 633
> > > > Silence is golden
> > > > +idmapped-mounts.c: 7906: setgid_create - Success -
> > > > failure:
> > > > is_setgid
> > > > +idmapped-mounts.c: 13907: run_test - Success - failure:
> > > > create
> > > > operations in directories with setgid bit set
> > > > ...
> > > > (Run 'diff -u /root/xfstests-dev/tests/generic/633.out
> > > > /root/xfstests-dev/results//generic/633.out.bad' to see the
> > > > entire
> > > > diff)
> > > > Ran: generic/633
> > > > Failures: generic/633
> > > > Failed 1 of 1 tests
> > > > ============================================
> > > >
> > > > The failed test is about setgid inheritance.
> > > > When a file is created with S_ISGID in the directory with
> > > > S_ISGID,
> > > > NFS doesn't strip the setgid bit of the new created file but
> > > > others
> > > > (ext4/xfs/btrfs) do. They call inode_init_owner() which does
> > > > the strip after new_inode().
> > > > However, NFS has its own logical to handle inode capacities.
> > > > As the test says the behavior can be filesystem type specific,
> > > > I'd report to you NFS guys and ask whether it's a bug or not?
> > >
> > > Thanks for the report. I'm not sure why NFS would have different
> > > rules
> > > for setgid inheritance. So I'm inclined to think that this is
> > > simply
> > > a
> > > bug similar to what we saw in xfs and ceph. But I'll let the NFS
> > > folks
> > > determine that.
> > >
> > > XFS is only special in so far as it has a sysctl that lets it
> > > alter
> > > sgid
> > > inheritance behavior. And it only has that to allow for legacy
> > > irix
> > > semantics iiuc.
> >
> > OK, so how do you expect this 'idmapped-mounts' functionality to
> > work
> > on NFS? I'm not asking about this bug in particular. I'm asking
> > about
> > what this is supposed to do in general.
>
> Just to clarify, the bug has nothing to do with idmapped mounts. The
> idmapped mount testsuite always had a vfs generic part. That vfs
> generic
> part has been made available to all filesystems supporting xfstests a
> few weeks ago. (So far this setgid inheritance bug here has been
> present
> in 3 filesystems.)
The setgid stuff works just fine with regular use, when the server is
able to determine when to clear the bit. It only breaks with this kind
of test where the server is being lied to by the client's upper layers.
>
> >
> > At a quick glance, it looks to me as if these idmapped mount
> > helpers
> > are just hacking different values into the inode cache
> > representation
> > of files, and then somehow expecting that to result in different
>
> (Just to clarify, the inode cache is never changed by an idmapped
> mount
> unless a new filesystem object is created/permantently changed. The
> inode cache is left alone otherwise and ownership is only transiently
> mapped when checking permissions.)
>
> > behaviour.
> > That's not going to work with NFS, for two reasons:
> > 1. Security is enforced by the server and not the client. If you
> > want these values you're poking into the inode cache to
> > change
> > the behaviour of the server, then they have to be propagated
> > by
> > the client to that server.
> > 2. The NFS security model is authentication based. In
> > particular,
> > when strong authentication is being used, then my identity is
> > established by user+password that I've logged in as to the
> > kerberos server (or whatever). So while the idmapped mount
> > stuff
> > may be poking values into my credential or the inode cache,
> > the
> > server is going to ignore all that and tell me that any file
> > I
> > create is owned by user trond@domain. It will not allow me to
> > change file ownership or to override access permissions
> > unless
> > trond@domain happens to be a privileged user such as root.
> >
> > I'm pretty sure the cifs/smb client will have the same problem.
>
> I did a POC for ceph a while back and I did also have POC patches for
> cifs/smb. For ceph e.g. the {g,u}id with which a file is to be
> created
> with or will have its ownership changed to is sent from the client to
> the server with server being responsible for deciding whether that
> creation is allowed or not.
> NFS already has a notion of idmapping via upcalls afair. Whether or
> not
> support for idmapped mounts make sense for NFS depends on how the
> current idmapping feature is implemented and what use-cases aren't
> supported by it that idmapped mounts can.
The NFS idmapping by upcalls solves a very different issue. It exists
to make sure that the client and server are both on the same page
w.r.t. account to uid/gid mapping.
i.e. it solves a problem when they are using kerberos, and are not in
agreement over uid/gid assignment to the principal account names.
It will actually cause further conflicts with idmapped mounts.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Wed, Feb 23, 2022 at 12:24:02PM +0000, Trond Myklebust wrote:
> On Wed, 2022-02-23 at 09:44 +0100, Christian Brauner wrote:
> > On Sat, Feb 19, 2022 at 05:00:18PM +0000, Trond Myklebust wrote:
> > > On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner wrote:
> > > > On Sat, Feb 19, 2022 at 08:34:30AM +0000,
> > > > [email protected] wrote:
> > > > > Hi NFS folks,
> > > > > During our xfstests, we found generic/633 fails like:
> > > > > ============================================
> > > > > FSTYP -- nfs
> > > > > PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP
> > > > > PREEMPT Sat Feb 19 15:09:03 CST 2022
> > > > > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> > > > > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
> > > > >
> > > > > generic/633 0s ... [failed, exit status 1]- output mismatch
> > > > > (see
> > > > > /root/xfstests-dev/results//generic/633.out.bad)
> > > > > --- tests/generic/633.out 2021-05-23 14:03:08.879999997
> > > > > +0800
> > > > > +++ /root/xfstests-dev/results//generic/633.out.bad 2022-
> > > > > 02-19
> > > > > 16:31:28.660000013 +0800
> > > > > @@ -1,2 +1,4 @@
> > > > > QA output created by 633
> > > > > Silence is golden
> > > > > +idmapped-mounts.c: 7906: setgid_create - Success -
> > > > > failure:
> > > > > is_setgid
> > > > > +idmapped-mounts.c: 13907: run_test - Success - failure:
> > > > > create
> > > > > operations in directories with setgid bit set
> > > > > ...
> > > > > (Run 'diff -u /root/xfstests-dev/tests/generic/633.out
> > > > > /root/xfstests-dev/results//generic/633.out.bad' to see the
> > > > > entire
> > > > > diff)
> > > > > Ran: generic/633
> > > > > Failures: generic/633
> > > > > Failed 1 of 1 tests
> > > > > ============================================
> > > > >
> > > > > The failed test is about setgid inheritance.
> > > > > When a file is created with S_ISGID in the directory with
> > > > > S_ISGID,
> > > > > NFS doesn't strip the setgid bit of the new created file but
> > > > > others
> > > > > (ext4/xfs/btrfs) do. They call inode_init_owner() which does
> > > > > the strip after new_inode().
> > > > > However, NFS has its own logical to handle inode capacities.
> > > > > As the test says the behavior can be filesystem type specific,
> > > > > I'd report to you NFS guys and ask whether it's a bug or not?
> > > >
> > > > Thanks for the report. I'm not sure why NFS would have different
> > > > rules
> > > > for setgid inheritance. So I'm inclined to think that this is
> > > > simply
> > > > a
> > > > bug similar to what we saw in xfs and ceph. But I'll let the NFS
> > > > folks
> > > > determine that.
> > > >
> > > > XFS is only special in so far as it has a sysctl that lets it
> > > > alter
> > > > sgid
> > > > inheritance behavior. And it only has that to allow for legacy
> > > > irix
> > > > semantics iiuc.
> > >
> > > OK, so how do you expect this 'idmapped-mounts' functionality to
> > > work
> > > on NFS? I'm not asking about this bug in particular. I'm asking
> > > about
> > > what this is supposed to do in general.
> >
> > Just to clarify, the bug has nothing to do with idmapped mounts. The
> > idmapped mount testsuite always had a vfs generic part. That vfs
> > generic
> > part has been made available to all filesystems supporting xfstests a
> > few weeks ago. (So far this setgid inheritance bug here has been
> > present
> > in 3 filesystems.)
>
> The setgid stuff works just fine with regular use, when the server is
> able to determine when to clear the bit. It only breaks with this kind
> of test where the server is being lied to by the client's upper layers.
I think you misunderstand: it is not possible to create idmapped mounts
for a mounted NFS client. In order for a filesystem to support idmapped
mounts it must set FS_ALLOW_IDMAP which currently only btrfs, ext4, fat,
and xfs do. The failing test does not use idmapped mounts in any way.
On Wed, Feb 23, 2022 at 05:09:26PM +0100, Christian Brauner wrote:
> On Wed, Feb 23, 2022 at 12:24:02PM +0000, Trond Myklebust wrote:
> > On Wed, 2022-02-23 at 09:44 +0100, Christian Brauner wrote:
> > > On Sat, Feb 19, 2022 at 05:00:18PM +0000, Trond Myklebust wrote:
> > > > On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner wrote:
> > > > > On Sat, Feb 19, 2022 at 08:34:30AM +0000,
> > > > > [email protected] wrote:
> > > > > > Hi NFS folks,
> > > > > > During our xfstests, we found generic/633 fails like:
> > > > > > ============================================
> > > > > > FSTYP -- nfs
> > > > > > PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom #236 SMP
> > > > > > PREEMPT Sat Feb 19 15:09:03 CST 2022
> > > > > > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> > > > > > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch /mnt/scratch
> > > > > >
> > > > > > generic/633 0s ... [failed, exit status 1]- output mismatch
> > > > > > (see
> > > > > > /root/xfstests-dev/results//generic/633.out.bad)
> > > > > > --- tests/generic/633.out 2021-05-23 14:03:08.879999997
> > > > > > +0800
> > > > > > +++ /root/xfstests-dev/results//generic/633.out.bad 2022-
> > > > > > 02-19
> > > > > > 16:31:28.660000013 +0800
> > > > > > @@ -1,2 +1,4 @@
> > > > > > QA output created by 633
> > > > > > Silence is golden
> > > > > > +idmapped-mounts.c: 7906: setgid_create - Success -
> > > > > > failure:
> > > > > > is_setgid
> > > > > > +idmapped-mounts.c: 13907: run_test - Success - failure:
> > > > > > create
> > > > > > operations in directories with setgid bit set
> > > > > > ...
> > > > > > (Run 'diff -u /root/xfstests-dev/tests/generic/633.out
> > > > > > /root/xfstests-dev/results//generic/633.out.bad' to see the
> > > > > > entire
> > > > > > diff)
> > > > > > Ran: generic/633
> > > > > > Failures: generic/633
> > > > > > Failed 1 of 1 tests
> > > > > > ============================================
> > > > > >
> > > > > > The failed test is about setgid inheritance.
> > > > > > When a file is created with S_ISGID in the directory with
> > > > > > S_ISGID,
> > > > > > NFS doesn't strip the setgid bit of the new created file but
> > > > > > others
> > > > > > (ext4/xfs/btrfs) do. They call inode_init_owner() which does
> > > > > > the strip after new_inode().
> > > > > > However, NFS has its own logical to handle inode capacities.
> > > > > > As the test says the behavior can be filesystem type specific,
> > > > > > I'd report to you NFS guys and ask whether it's a bug or not?
> > > > >
> > > > > Thanks for the report. I'm not sure why NFS would have different
> > > > > rules
> > > > > for setgid inheritance. So I'm inclined to think that this is
> > > > > simply
> > > > > a
> > > > > bug similar to what we saw in xfs and ceph. But I'll let the NFS
> > > > > folks
> > > > > determine that.
> > > > >
> > > > > XFS is only special in so far as it has a sysctl that lets it
> > > > > alter
> > > > > sgid
> > > > > inheritance behavior. And it only has that to allow for legacy
> > > > > irix
> > > > > semantics iiuc.
> > > >
> > > > OK, so how do you expect this 'idmapped-mounts' functionality to
> > > > work
> > > > on NFS? I'm not asking about this bug in particular. I'm asking
> > > > about
> > > > what this is supposed to do in general.
> > >
> > > Just to clarify, the bug has nothing to do with idmapped mounts. The
> > > idmapped mount testsuite always had a vfs generic part. That vfs
> > > generic
> > > part has been made available to all filesystems supporting xfstests a
> > > few weeks ago. (So far this setgid inheritance bug here has been
> > > present
> > > in 3 filesystems.)
> >
> > The setgid stuff works just fine with regular use, when the server is
> > able to determine when to clear the bit. It only breaks with this kind
> > of test where the server is being lied to by the client's upper layers.
>
> I think you misunderstand: it is not possible to create idmapped mounts
> for a mounted NFS client. In order for a filesystem to support idmapped
> mounts it must set FS_ALLOW_IDMAP which currently only btrfs, ext4, fat,
> and xfs do. The failing test does not use idmapped mounts in any way.
Ok, I dug into this yesterday.
If a new file or directory is created in a setgid directory then regular
files and directories will inherit their group ID from the parent
directory.
But NFS makes any files created by the root user be owned nobody and
nogroup. The test expects the newly created file and directory in the
setgid directory to be owned gid 0 but since NFS squashes that to
nogroup the test fails.
If I add no_root_squash solely for the sake of the test to the exported
directory the test passes.
So similar to xfs irix mode this is filesystem specific behavior. I'll
make sure to send a patch to xfstests and we'll skip the tests on nfs
for now.
I'm appending a trimmed down version of the test if anyone wants to
verify (see [1]).
Thanks!
[1]: (Requires libcap-dev to be installed. Compile with gcc <file> -o <binary> -lcap and run as root.)
#define _GNU_SOURCE
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <getopt.h>
#include <grp.h>
#include <limits.h>
#include <linux/limits.h>
#include <linux/types.h>
#include <pthread.h>
#include <pwd.h>
#include <sched.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/capability.h>
#include <sys/eventfd.h>
#include <sys/fsuid.h>
#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/xattr.h>
#include <unistd.h>
#define FILE1 "file1"
#define DIR1 "dir1"
#define log_stderr(format, ...) \
fprintf(stderr, "%s: %d: %s - %m - " format "\n", __FILE__, __LINE__, __func__, \
##__VA_ARGS__)
#define log_error_errno(__ret__, __errno__, format, ...) \
({ \
typeof(__ret__) __internal_ret__ = (__ret__); \
errno = (__errno__); \
log_stderr(format, ##__VA_ARGS__); \
__internal_ret__; \
})
#define log_errno(__ret__, format, ...) log_error_errno(__ret__, errno, format, ##__VA_ARGS__)
#define die_errno(__errno__, format, ...) \
({ \
errno = (__errno__); \
log_stderr(format, ##__VA_ARGS__); \
exit(EXIT_FAILURE); \
})
#define die(format, ...) die_errno(errno, format, ##__VA_ARGS__)
#define syserror(format, ...) \
({ \
fprintf(stderr, "%m - " format "\n", ##__VA_ARGS__); \
(-errno); \
})
#define syserror_set(__ret__, format, ...) \
({ \
typeof(__ret__) __internal_ret__ = (__ret__); \
errno = labs(__ret__); \
fprintf(stderr, "%m - " format "\n", ##__VA_ARGS__); \
__internal_ret__; \
})
bool switch_ids(uid_t uid, gid_t gid)
{
if (setgroups(0, NULL))
return syserror_set(-1, "failure: setgroups");
if (setresgid(gid, gid, gid))
return syserror_set(-1, "failure: setresgid");
if (setresuid(uid, uid, uid))
return syserror_set(-1, "failure: setresuid");
return true;
}
/* is_setgid - check whether file or directory is S_ISGID */
static bool is_setgid(int dfd, const char *path, int flags)
{
int ret;
struct stat st;
ret = fstatat(dfd, path, &st, flags);
if (ret < 0)
return false;
errno = 0; /* Don't report misleading errno. */
return (st.st_mode & S_ISGID);
}
/* __expected_uid_gid - check whether file is owned by the provided uid and gid */
static bool __expected_uid_gid(int dfd, const char *path, int flags,
uid_t expected_uid, gid_t expected_gid, bool log)
{
int ret;
struct stat st;
ret = fstatat(dfd, path, &st, flags);
if (ret < 0)
return log_errno(false, "failure: fstatat");
if (log && st.st_uid != expected_uid)
log_stderr("failure: uid(%d) != expected_uid(%d)", st.st_uid, expected_uid);
if (log && st.st_gid != expected_gid)
log_stderr("failure: gid(%d) != expected_gid(%d)", st.st_gid, expected_gid);
errno = 0; /* Don't report misleading errno. */
return st.st_uid == expected_uid && st.st_gid == expected_gid;
}
static bool expected_uid_gid(int dfd, const char *path, int flags,
uid_t expected_uid, gid_t expected_gid)
{
return __expected_uid_gid(dfd, path, flags,
expected_uid, expected_gid, true);
}
int wait_for_pid(pid_t pid)
{
int status, ret;
again:
ret = waitpid(pid, &status, 0);
if (ret == -1) {
if (errno == EINTR)
goto again;
return -1;
}
if (!WIFEXITED(status))
return -1;
return WEXITSTATUS(status);
}
static int caps_down(void)
{
bool fret = false;
cap_t caps = NULL;
int ret = -1;
caps = cap_get_proc();
if (!caps)
goto out;
ret = cap_clear_flag(caps, CAP_EFFECTIVE);
if (ret)
goto out;
ret = cap_set_proc(caps);
if (ret)
goto out;
fret = true;
out:
cap_free(caps);
return fret;
}
int main(int argc, char *argv[])
{
int file1_fd = -EBADF, t_dir1_fd = -EBADF;
pid_t pid;
if (argc < 1)
exit(1);
t_dir1_fd = open(argv[1], O_CLOEXEC | O_DIRECTORY);
if (t_dir1_fd < 0)
die("open");
if (fchmod(t_dir1_fd, S_IRUSR |
S_IWUSR |
S_IRGRP |
S_IWGRP |
S_IROTH |
S_IWOTH |
S_IXUSR |
S_IXGRP |
S_IXOTH |
S_ISGID), 0) {
die("failure: fchmod");
}
/* Verify that the setgid bit got raised. */
if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
die("failure: is_setgid");
}
pid = fork();
if (pid < 0) {
exit(1);
}
#define UID 1000
#define GID 0 /* Will fail on NFS without no_root_squash set. */
if (pid == 0) {
if (!switch_ids(UID, 10000))
die("failure: switch_ids");
if (!caps_down())
die("failure: caps_down");
/* create regular file via open() */
file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
if (file1_fd < 0)
die("failure: create");
/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
* bit needs to be stripped.
*/
if (is_setgid(t_dir1_fd, FILE1, 0))
die("failure: is_setgid");
/* create directory */
if (mkdirat(t_dir1_fd, DIR1, 0000))
die("failure: create");
/* Directories always inherit the setgid bit. */
if (!is_setgid(t_dir1_fd, DIR1, 0))
die("failure: is_setgid");
/*
* In setgid directories newly created files always inherit the
* gid from the parent directory. Verify that the file is owned
* by GID, not by gid 10000.
*/
if (!expected_uid_gid(t_dir1_fd, FILE1, 0, UID, GID))
die("failure: check ownership");
/*
* In setgid directories newly created directories always
* inherit the gid from the parent directory. Verify that the
* directory is owned by GID, not by gid 10000.
*/
if (!expected_uid_gid(t_dir1_fd, DIR1, 0, UID, GID))
die("failure: check ownership");
exit(EXIT_SUCCESS);
}
if (wait_for_pid(pid))
die("FAIL");
exit(0);
}
On Thu 24 Feb 2022 at 09:52, Christian Brauner
<[email protected]> wrote:
> On Wed, Feb 23, 2022 at 05:09:26PM +0100, Christian Brauner
> wrote:
>> On Wed, Feb 23, 2022 at 12:24:02PM +0000, Trond Myklebust
>> wrote:
>> > On Wed, 2022-02-23 at 09:44 +0100, Christian Brauner wrote:
>> > > On Sat, Feb 19, 2022 at 05:00:18PM +0000, Trond Myklebust
>> > > wrote:
>> > > > On Sat, 2022-02-19 at 12:34 +0100, Christian Brauner
>> > > > wrote:
>> > > > > On Sat, Feb 19, 2022 at 08:34:30AM +0000,
>> > > > > [email protected] wrote:
>> > > > > > Hi NFS folks,
>> > > > > > During our xfstests, we found generic/633 fails
>> > > > > > like:
>> > > > > > ============================================
>> > > > > > FSTYP -- nfs
>> > > > > > PLATFORM -- Linux/x86_64 btrfs 5.17.0-rc4-custom
>> > > > > > #236 SMP
>> > > > > > PREEMPT Sat Feb 19 15:09:03 CST 2022
>> > > > > > MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
>> > > > > > MOUNT_OPTIONS -- -o vers=4 127.0.0.1:/nfsscratch
>> > > > > > /mnt/scratch
>> > > > > >
>> > > > > > generic/633 0s ... [failed, exit status 1]- output
>> > > > > > mismatch
>> > > > > > (see
>> > > > > > /root/xfstests-dev/results//generic/633.out.bad)
>> > > > > > --- tests/generic/633.out 2021-05-23
>> > > > > > 14:03:08.879999997
>> > > > > > +0800
>> > > > > > +++
>> > > > > > /root/xfstests-dev/results//generic/633.out.bad 2022-
>> > > > > > 02-19
>> > > > > > 16:31:28.660000013 +0800
>> > > > > > @@ -1,2 +1,4 @@
>> > > > > > QA output created by 633
>> > > > > > Silence is golden
>> > > > > > +idmapped-mounts.c: 7906: setgid_create - Success
>> > > > > > -
>> > > > > > failure:
>> > > > > > is_setgid
>> > > > > > +idmapped-mounts.c: 13907: run_test - Success -
>> > > > > > failure:
>> > > > > > create
>> > > > > > operations in directories with setgid bit set
>> > > > > > ...
>> > > > > > (Run 'diff -u
>> > > > > > /root/xfstests-dev/tests/generic/633.out
>> > > > > > /root/xfstests-dev/results//generic/633.out.bad' to
>> > > > > > see the
>> > > > > > entire
>> > > > > > diff)
>> > > > > > Ran: generic/633
>> > > > > > Failures: generic/633
>> > > > > > Failed 1 of 1 tests
>> > > > > > ============================================
>> > > > > >
>> > > > > > The failed test is about setgid inheritance.
>> > > > > > When a file is created with S_ISGID in the directory
>> > > > > > with
>> > > > > > S_ISGID,
>> > > > > > NFS doesn't strip the setgid bit of the new created
>> > > > > > file but
>> > > > > > others
>> > > > > > (ext4/xfs/btrfs) do. They call inode_init_owner()
>> > > > > > which does
>> > > > > > the strip after new_inode().
>> > > > > > However, NFS has its own logical to handle inode
>> > > > > > capacities.
>> > > > > > As the test says the behavior can be filesystem type
>> > > > > > specific,
>> > > > > > I'd report to you NFS guys and ask whether it's a bug
>> > > > > > or not?
>> > > > >
>> > > > > Thanks for the report. I'm not sure why NFS would have
>> > > > > different
>> > > > > rules
>> > > > > for setgid inheritance. So I'm inclined to think that
>> > > > > this is
>> > > > > simply
>> > > > > a
>> > > > > bug similar to what we saw in xfs and ceph. But I'll
>> > > > > let the NFS
>> > > > > folks
>> > > > > determine that.
>> > > > >
>> > > > > XFS is only special in so far as it has a sysctl that
>> > > > > lets it
>> > > > > alter
>> > > > > sgid
>> > > > > inheritance behavior. And it only has that to allow for
>> > > > > legacy
>> > > > > irix
>> > > > > semantics iiuc.
>> > > >
>> > > > OK, so how do you expect this 'idmapped-mounts'
>> > > > functionality to
>> > > > work
>> > > > on NFS? I'm not asking about this bug in particular. I'm
>> > > > asking
>> > > > about
>> > > > what this is supposed to do in general.
>> > >
>> > > Just to clarify, the bug has nothing to do with idmapped
>> > > mounts. The
>> > > idmapped mount testsuite always had a vfs generic part.
>> > > That vfs
>> > > generic
>> > > part has been made available to all filesystems supporting
>> > > xfstests a
>> > > few weeks ago. (So far this setgid inheritance bug here has
>> > > been
>> > > present
>> > > in 3 filesystems.)
>> >
>> > The setgid stuff works just fine with regular use, when the
>> > server is
>> > able to determine when to clear the bit. It only breaks with
>> > this kind
>> > of test where the server is being lied to by the client's
>> > upper layers.
>>
>> I think you misunderstand: it is not possible to create
>> idmapped mounts
>> for a mounted NFS client. In order for a filesystem to support
>> idmapped
>> mounts it must set FS_ALLOW_IDMAP which currently only btrfs,
>> ext4, fat,
>> and xfs do. The failing test does not use idmapped mounts in
>> any way.
>
> Ok, I dug into this yesterday.
> If a new file or directory is created in a setgid directory then
> regular
> files and directories will inherit their group ID from the
> parent
> directory.
> But NFS makes any files created by the root user be owned nobody
> and
> nogroup. The test expects the newly created file and directory
> in the
> setgid directory to be owned gid 0 but since NFS squashes that
> to
> nogroup the test fails.
> If I add no_root_squash solely for the sake of the test to the
> exported
> directory the test passes.
>
No wonder! I hadn't noticed the behavior before and thought it's
a NFS bug.
> So similar to xfs irix mode this is filesystem specific
> behavior. I'll
> make sure to send a patch to xfstests and we'll skip the tests
> on nfs
> for now.
>
Thanks a lot! Being embarrassed for pushing my fix duty to you.
I should be more patience in digging deeper before report :-)
--
Su
> I'm appending a trimmed down version of the test if anyone wants
> to
> verify (see [1]).
>
> Thanks!
>
> [1]: (Requires libcap-dev to be installed. Compile with gcc
> <file> -o <binary> -lcap and run as root.)
>
> #define _GNU_SOURCE
> #include <dirent.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <getopt.h>
> #include <grp.h>
> #include <limits.h>
> #include <linux/limits.h>
> #include <linux/types.h>
> #include <pthread.h>
> #include <pwd.h>
> #include <sched.h>
> #include <stdbool.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/capability.h>
> #include <sys/eventfd.h>
> #include <sys/fsuid.h>
> #include <sys/mount.h>
> #include <sys/prctl.h>
> #include <sys/socket.h>
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <sys/xattr.h>
> #include <unistd.h>
>
> #define FILE1 "file1"
> #define DIR1 "dir1"
>
> #define log_stderr(format, ...)
> \
> fprintf(stderr, "%s: %d: %s - %m - " format "\n",
> __FILE__, __LINE__, __func__, \
> ##__VA_ARGS__)
>
> #define log_error_errno(__ret__, __errno__, format, ...) \
> ({ \
> typeof(__ret__) __internal_ret__ = (__ret__); \
> errno = (__errno__); \
> log_stderr(format, ##__VA_ARGS__); \
> __internal_ret__; \
> })
>
> #define log_errno(__ret__, format, ...) log_error_errno(__ret__,
> errno, format, ##__VA_ARGS__)
>
> #define die_errno(__errno__, format, ...) \
> ({ \
> errno = (__errno__); \
> log_stderr(format, ##__VA_ARGS__); \
> exit(EXIT_FAILURE); \
> })
>
> #define die(format, ...) die_errno(errno, format, ##__VA_ARGS__)
>
> #define syserror(format, ...) \
> ({ \
> fprintf(stderr, "%m - " format "\n",
> ##__VA_ARGS__); \
> (-errno); \
> })
>
> #define syserror_set(__ret__, format, ...) \
> ({ \
> typeof(__ret__) __internal_ret__ = (__ret__); \
> errno = labs(__ret__); \
> fprintf(stderr, "%m - " format "\n",
> ##__VA_ARGS__); \
> __internal_ret__; \
> })
>
> bool switch_ids(uid_t uid, gid_t gid)
> {
> if (setgroups(0, NULL))
> return syserror_set(-1, "failure: setgroups");
>
> if (setresgid(gid, gid, gid))
> return syserror_set(-1, "failure: setresgid");
>
> if (setresuid(uid, uid, uid))
> return syserror_set(-1, "failure: setresuid");
>
> return true;
> }
>
> /* is_setgid - check whether file or directory is S_ISGID */
> static bool is_setgid(int dfd, const char *path, int flags)
> {
> int ret;
> struct stat st;
>
> ret = fstatat(dfd, path, &st, flags);
> if (ret < 0)
> return false;
>
> errno = 0; /* Don't report misleading errno. */
> return (st.st_mode & S_ISGID);
> }
>
> /* __expected_uid_gid - check whether file is owned by the
> provided uid and gid */
> static bool __expected_uid_gid(int dfd, const char *path, int
> flags,
> uid_t expected_uid, gid_t
> expected_gid, bool log)
> {
> int ret;
> struct stat st;
>
> ret = fstatat(dfd, path, &st, flags);
> if (ret < 0)
> return log_errno(false, "failure: fstatat");
>
> if (log && st.st_uid != expected_uid)
> log_stderr("failure: uid(%d) !=
> expected_uid(%d)", st.st_uid, expected_uid);
>
> if (log && st.st_gid != expected_gid)
> log_stderr("failure: gid(%d) !=
> expected_gid(%d)", st.st_gid, expected_gid);
>
> errno = 0; /* Don't report misleading errno. */
> return st.st_uid == expected_uid && st.st_gid ==
> expected_gid;
> }
>
> static bool expected_uid_gid(int dfd, const char *path, int
> flags,
> uid_t expected_uid, gid_t
> expected_gid)
> {
> return __expected_uid_gid(dfd, path, flags,
> expected_uid, expected_gid,
> true);
> }
>
> int wait_for_pid(pid_t pid)
> {
> int status, ret;
>
> again:
> ret = waitpid(pid, &status, 0);
> if (ret == -1) {
> if (errno == EINTR)
> goto again;
>
> return -1;
> }
>
> if (!WIFEXITED(status))
> return -1;
>
> return WEXITSTATUS(status);
> }
>
> static int caps_down(void)
> {
> bool fret = false;
> cap_t caps = NULL;
> int ret = -1;
>
> caps = cap_get_proc();
> if (!caps)
> goto out;
>
> ret = cap_clear_flag(caps, CAP_EFFECTIVE);
> if (ret)
> goto out;
>
> ret = cap_set_proc(caps);
> if (ret)
> goto out;
>
> fret = true;
>
> out:
> cap_free(caps);
> return fret;
> }
>
> int main(int argc, char *argv[])
> {
> int file1_fd = -EBADF, t_dir1_fd = -EBADF;
> pid_t pid;
>
> if (argc < 1)
> exit(1);
>
> t_dir1_fd = open(argv[1], O_CLOEXEC | O_DIRECTORY);
> if (t_dir1_fd < 0)
> die("open");
>
> if (fchmod(t_dir1_fd, S_IRUSR |
> S_IWUSR |
> S_IRGRP |
> S_IWGRP |
> S_IROTH |
> S_IWOTH |
> S_IXUSR |
> S_IXGRP |
> S_IXOTH |
> S_ISGID), 0) {
> die("failure: fchmod");
> }
>
> /* Verify that the setgid bit got raised. */
> if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
> die("failure: is_setgid");
> }
>
> pid = fork();
> if (pid < 0) {
> exit(1);
> }
>
> #define UID 1000
> #define GID 0 /* Will fail on NFS without no_root_squash set. */
> if (pid == 0) {
> if (!switch_ids(UID, 10000))
> die("failure: switch_ids");
>
> if (!caps_down())
> die("failure: caps_down");
>
> /* create regular file via open() */
> file1_fd = openat(t_dir1_fd, FILE1, O_CREAT |
> O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> if (file1_fd < 0)
> die("failure: create");
>
> /* Neither in_group_p() nor
> capable_wrt_inode_uidgid() so setgid
> * bit needs to be stripped.
> */
> if (is_setgid(t_dir1_fd, FILE1, 0))
> die("failure: is_setgid");
>
> /* create directory */
> if (mkdirat(t_dir1_fd, DIR1, 0000))
> die("failure: create");
>
> /* Directories always inherit the setgid bit. */
> if (!is_setgid(t_dir1_fd, DIR1, 0))
> die("failure: is_setgid");
>
> /*
> * In setgid directories newly created files
> always inherit the
> * gid from the parent directory. Verify that
> the file is owned
> * by GID, not by gid 10000.
> */
> if (!expected_uid_gid(t_dir1_fd, FILE1, 0, UID,
> GID))
> die("failure: check ownership");
>
> /*
> * In setgid directories newly created
> directories always
> * inherit the gid from the parent directory.
> Verify that the
> * directory is owned by GID, not by gid 10000.
> */
> if (!expected_uid_gid(t_dir1_fd, DIR1, 0, UID,
> GID))
> die("failure: check ownership");
>
> exit(EXIT_SUCCESS);
> }
>
> if (wait_for_pid(pid))
> die("FAIL");
>
> exit(0);
> }