[resending without a text/html subpart]
This is https://bugzilla.kernel.org/show_bug.cgi?id=114751
Since some time around kernel 3.16, NFSv3 mounts can return EPERM for
POSIX extended ACL calls if the current user does not match the owner,
instead of EOPNOTSUPP.
James Brown, CC'd, has confirmed the bug is still present in mainline 4.4.
Using ftrace, I've observed[0] that the call fails inside the VFS
layer before even hitting the NFS code, which makes me suspect
013cdf1088d7235da9477a2375654921d9b9ba9f (“nfs: use generic posix ACL
infrastructure for v3 Posix ACLs”). Unfortunately I haven't had a
chance to confirm that theory by building the before/after revisions.
CC'ing Christoph, who authored that series.
- Nelson
[0] http://nelhagedebugsshit.tumblr.com/post/116688651723/broken-posix-acls-on-nfs-on-kernel-316
Hi Nelson,
this was indeed most likely caused by my patch. Just to narrow things
down can your clarify that the scenarious is that you have CONFIG_NFS_V3
set on your client, you're talking to a server not supporting ACLs
at all, and a tool trying to set an ACL is getting the wrong return
value? If so I should be able to reproduce this locally.
That's correct. The other detail that seems to be important is that
the user making the call must be different from the user owning the
file. We've also been using user remapping on the server, so that
non-xattr calls succeed in that configuration.
The reproducer James added in the bugzilla is:
(on machine with IP address 10.1.1.1)
sudo mkdir /nfs_test
sudo useradd -u 10000 test_user
sudo chown test_user /nfs_test
echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
/etc/exports
sudo exportfs -a
(on machine with IP address 10.1.1.2)
sudo mkdir /nfs_test
sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
touch /nfs_test/foo
install -m 755 /nfs_test/foo /nfs_test/bar
- Nelson
On Mon, Mar 21, 2016 at 7:43 AM Christoph Hellwig <[email protected]> wrote:
>
> Hi Nelson,
>
> this was indeed most likely caused by my patch. Just to narrow things
> down can your clarify that the scenarious is that you have CONFIG_NFS_V3
> set on your client, you're talking to a server not supporting ACLs
> at all, and a tool trying to set an ACL is getting the wrong return
> value? If so I should be able to reproduce this locally.
On Mon, Mar 21 2016, Nelson Elhage wrote:
> That's correct. The other detail that seems to be important is that
> the user making the call must be different from the user owning the
> file. We've also been using user remapping on the server, so that
> non-xattr calls succeed in that configuration.
>
> The reproducer James added in the bugzilla is:
>
> (on machine with IP address 10.1.1.1)
> sudo mkdir /nfs_test
> sudo useradd -u 10000 test_user
> sudo chown test_user /nfs_test
> echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
> /etc/exports
> sudo exportfs -a
>
> (on machine with IP address 10.1.1.2)
> sudo mkdir /nfs_test
> sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
> touch /nfs_test/foo
> install -m 755 /nfs_test/foo /nfs_test/bar
Did anything ever happen about this?
I have a customer with a similar problem (in 4.4) but I cannot see any
evidence of fixes landing in mainline.
Problem happens with you have uid mapping on the server
(e.g. anonuid=10000 as above) and a user with a different uid on the
client attempts setacl on a file with that user.
As anon is mapped to the owner of the file, setacl should be allowed.
However set_posix_acl() calls inode_owner_or_capable() which checks if
the client-side uid matches the visible inode->i_uid - they don't.
Testing i_uid on the client is always incorrect for permission checking
with NFS - the client should always ask the server, either with ACCESS
or, in this case, by simply attempting the operation.
Any suggestions how best to fix this?
- We could move the responsibility for permission checking into
i_op->set_acl, but that would be a large change and might make it too
easy for other filesystems to get it wrong.
- we could have some sort of flag asking set_posix_acl(), but that's
rather clumsy.... maybe if i_op->set_acl_check_perm use that without
testing ownership first??
- we could copy
posic_acl_xattr_{get,set,list} into nfs together with functions
they call, modify set_posix_acl() to not test ownership,
and provide a local 'struct xattr_handler' structure for NFS.
I don't really like any of those suggestions. Can someone else do any
better?
Thanks,
NeilBrown
>
> - Nelson
>
> On Mon, Mar 21, 2016 at 7:43 AM Christoph Hellwig <[email protected]> wrote:
>>
>> Hi Nelson,
>>
>> this was indeed most likely caused by my patch. Just to narrow things
>> down can your clarify that the scenarious is that you have CONFIG_NFS_V3
>> set on your client, you're talking to a server not supporting ACLs
>> at all, and a tool trying to set an ACL is getting the wrong return
>> value? If so I should be able to reproduce this locally.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 10, 2018 at 11:29:33AM +1000, NeilBrown wrote:
> On Mon, Mar 21 2016, Nelson Elhage wrote:
>
> > That's correct. The other detail that seems to be important is that
> > the user making the call must be different from the user owning the
> > file. We've also been using user remapping on the server, so that
> > non-xattr calls succeed in that configuration.
> >
> > The reproducer James added in the bugzilla is:
> >
> > (on machine with IP address 10.1.1.1)
> > sudo mkdir /nfs_test
> > sudo useradd -u 10000 test_user
> > sudo chown test_user /nfs_test
> > echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
> > /etc/exports
> > sudo exportfs -a
> >
> > (on machine with IP address 10.1.1.2)
> > sudo mkdir /nfs_test
> > sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
> > touch /nfs_test/foo
> > install -m 755 /nfs_test/foo /nfs_test/bar
>
> Did anything ever happen about this?
> I have a customer with a similar problem (in 4.4) but I cannot see any
> evidence of fixes landing in mainline.
>
> Problem happens with you have uid mapping on the server
> (e.g. anonuid=10000 as above) and a user with a different uid on the
> client attempts setacl on a file with that user.
> As anon is mapped to the owner of the file, setacl should be allowed.
> However set_posix_acl() calls inode_owner_or_capable() which checks if
> the client-side uid matches the visible inode->i_uid - they don't.
>
> Testing i_uid on the client is always incorrect for permission checking
> with NFS - the client should always ask the server, either with ACCESS
> or, in this case, by simply attempting the operation.
>
> Any suggestions how best to fix this?
> - We could move the responsibility for permission checking into
> i_op->set_acl, but that would be a large change and might make it too
> easy for other filesystems to get it wrong.
> - we could have some sort of flag asking set_posix_acl(), but that's
> rather clumsy.... maybe if i_op->set_acl_check_perm use that without
> testing ownership first??
> - we could copy
> posic_acl_xattr_{get,set,list} into nfs together with functions
> they call, modify set_posix_acl() to not test ownership,
> and provide a local 'struct xattr_handler' structure for NFS.
>
> I don't really like any of those suggestions. Can someone else do any
> better?
Do we have important callers of inode_owner_or_capable() in the vfs (as
opposed to in individual filesystems), and do any of them pose a similar
problem for network filesystems?
--b.
On Fri, Aug 10, 2018 at 01:00:27PM -0400, Bruce Fields wrote:
> On Fri, Aug 10, 2018 at 11:29:33AM +1000, NeilBrown wrote:
> > On Mon, Mar 21 2016, Nelson Elhage wrote:
> >
> > > That's correct. The other detail that seems to be important is that
> > > the user making the call must be different from the user owning the
> > > file. We've also been using user remapping on the server, so that
> > > non-xattr calls succeed in that configuration.
> > >
> > > The reproducer James added in the bugzilla is:
> > >
> > > (on machine with IP address 10.1.1.1)
> > > sudo mkdir /nfs_test
> > > sudo useradd -u 10000 test_user
> > > sudo chown test_user /nfs_test
> > > echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
> > > /etc/exports
> > > sudo exportfs -a
> > >
> > > (on machine with IP address 10.1.1.2)
> > > sudo mkdir /nfs_test
> > > sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
> > > touch /nfs_test/foo
> > > install -m 755 /nfs_test/foo /nfs_test/bar
> >
> > Did anything ever happen about this?
> > I have a customer with a similar problem (in 4.4) but I cannot see any
> > evidence of fixes landing in mainline.
> >
> > Problem happens with you have uid mapping on the server
> > (e.g. anonuid=10000 as above) and a user with a different uid on the
> > client attempts setacl on a file with that user.
> > As anon is mapped to the owner of the file, setacl should be allowed.
> > However set_posix_acl() calls inode_owner_or_capable() which checks if
> > the client-side uid matches the visible inode->i_uid - they don't.
> >
> > Testing i_uid on the client is always incorrect for permission checking
> > with NFS - the client should always ask the server, either with ACCESS
> > or, in this case, by simply attempting the operation.
> >
> > Any suggestions how best to fix this?
> > - We could move the responsibility for permission checking into
> > i_op->set_acl, but that would be a large change and might make it too
> > easy for other filesystems to get it wrong.
> > - we could have some sort of flag asking set_posix_acl(), but that's
> > rather clumsy.... maybe if i_op->set_acl_check_perm use that without
> > testing ownership first??
> > - we could copy
> > posic_acl_xattr_{get,set,list} into nfs together with functions
> > they call, modify set_posix_acl() to not test ownership,
> > and provide a local 'struct xattr_handler' structure for NFS.
> >
> > I don't really like any of those suggestions. Can someone else do any
> > better?
>
> Do we have important callers of inode_owner_or_capable() in the vfs (as
> opposed to in individual filesystems), and do any of them pose a similar
> problem for network filesystems?
do_linkat()->may_linkat() looks kinda suspicious to me. Or what about
the O_NOATIME check in map_open()? Just engaging in dumb grepping
here....
--b.
On Fri, Aug 10 2018, Bruce Fields wrote:
> On Fri, Aug 10, 2018 at 01:00:27PM -0400, Bruce Fields wrote:
>> On Fri, Aug 10, 2018 at 11:29:33AM +1000, NeilBrown wrote:
>> > On Mon, Mar 21 2016, Nelson Elhage wrote:
>> >
>> > > That's correct. The other detail that seems to be important is that
>> > > the user making the call must be different from the user owning the
>> > > file. We've also been using user remapping on the server, so that
>> > > non-xattr calls succeed in that configuration.
>> > >
>> > > The reproducer James added in the bugzilla is:
>> > >
>> > > (on machine with IP address 10.1.1.1)
>> > > sudo mkdir /nfs_test
>> > > sudo useradd -u 10000 test_user
>> > > sudo chown test_user /nfs_test
>> > > echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
>> > > /etc/exports
>> > > sudo exportfs -a
>> > >
>> > > (on machine with IP address 10.1.1.2)
>> > > sudo mkdir /nfs_test
>> > > sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
>> > > touch /nfs_test/foo
>> > > install -m 755 /nfs_test/foo /nfs_test/bar
>> >
>> > Did anything ever happen about this?
>> > I have a customer with a similar problem (in 4.4) but I cannot see any
>> > evidence of fixes landing in mainline.
>> >
>> > Problem happens with you have uid mapping on the server
>> > (e.g. anonuid=10000 as above) and a user with a different uid on the
>> > client attempts setacl on a file with that user.
>> > As anon is mapped to the owner of the file, setacl should be allowed.
>> > However set_posix_acl() calls inode_owner_or_capable() which checks if
>> > the client-side uid matches the visible inode->i_uid - they don't.
>> >
>> > Testing i_uid on the client is always incorrect for permission checking
>> > with NFS - the client should always ask the server, either with ACCESS
>> > or, in this case, by simply attempting the operation.
>> >
>> > Any suggestions how best to fix this?
>> > - We could move the responsibility for permission checking into
>> > i_op->set_acl, but that would be a large change and might make it too
>> > easy for other filesystems to get it wrong.
>> > - we could have some sort of flag asking set_posix_acl(), but that's
>> > rather clumsy.... maybe if i_op->set_acl_check_perm use that without
>> > testing ownership first??
>> > - we could copy
>> > posic_acl_xattr_{get,set,list} into nfs together with functions
>> > they call, modify set_posix_acl() to not test ownership,
>> > and provide a local 'struct xattr_handler' structure for NFS.
>> >
>> > I don't really like any of those suggestions. Can someone else do any
>> > better?
>>
>> Do we have important callers of inode_owner_or_capable() in the vfs (as
>> opposed to in individual filesystems), and do any of them pose a similar
>> problem for network filesystems?
>
> do_linkat()->may_linkat() looks kinda suspicious to me. Or what about
> the O_NOATIME check in map_open()? Just engaging in dumb grepping
> here....
>
> --b.
NOATIME, both in open and fcntl, is rejected on NFS. This seems valid
as there is no way in the protocol to ask the server to no update the
atime.
Others I found we just short-cuts to avoid calling i_op->permission() if
the caller was an owner. I don't *think* that would affect NFS much
... though if an owner didn't have write permission, some things might
be incorrectly forbidden. Maybe.
Thanks,
NeilBrown
On Sun, Aug 12, 2018 at 08:28:00AM +1000, NeilBrown wrote:
> On Fri, Aug 10 2018, Bruce Fields wrote:
>
> > On Fri, Aug 10, 2018 at 01:00:27PM -0400, Bruce Fields wrote:
> >> On Fri, Aug 10, 2018 at 11:29:33AM +1000, NeilBrown wrote:
> >> > On Mon, Mar 21 2016, Nelson Elhage wrote:
> >> >
> >> > > That's correct. The other detail that seems to be important is that
> >> > > the user making the call must be different from the user owning the
> >> > > file. We've also been using user remapping on the server, so that
> >> > > non-xattr calls succeed in that configuration.
> >> > >
> >> > > The reproducer James added in the bugzilla is:
> >> > >
> >> > > (on machine with IP address 10.1.1.1)
> >> > > sudo mkdir /nfs_test
> >> > > sudo useradd -u 10000 test_user
> >> > > sudo chown test_user /nfs_test
> >> > > echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
> >> > > /etc/exports
> >> > > sudo exportfs -a
> >> > >
> >> > > (on machine with IP address 10.1.1.2)
> >> > > sudo mkdir /nfs_test
> >> > > sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
> >> > > touch /nfs_test/foo
> >> > > install -m 755 /nfs_test/foo /nfs_test/bar
> >> >
> >> > Did anything ever happen about this?
> >> > I have a customer with a similar problem (in 4.4) but I cannot see any
> >> > evidence of fixes landing in mainline.
> >> >
> >> > Problem happens with you have uid mapping on the server
> >> > (e.g. anonuid=10000 as above) and a user with a different uid on the
> >> > client attempts setacl on a file with that user.
> >> > As anon is mapped to the owner of the file, setacl should be allowed.
> >> > However set_posix_acl() calls inode_owner_or_capable() which checks if
> >> > the client-side uid matches the visible inode->i_uid - they don't.
> >> >
> >> > Testing i_uid on the client is always incorrect for permission checking
> >> > with NFS - the client should always ask the server, either with ACCESS
> >> > or, in this case, by simply attempting the operation.
> >> >
> >> > Any suggestions how best to fix this?
> >> > - We could move the responsibility for permission checking into
> >> > i_op->set_acl, but that would be a large change and might make it too
> >> > easy for other filesystems to get it wrong.
> >> > - we could have some sort of flag asking set_posix_acl(), but that's
> >> > rather clumsy.... maybe if i_op->set_acl_check_perm use that without
> >> > testing ownership first??
> >> > - we could copy
> >> > posic_acl_xattr_{get,set,list} into nfs together with functions
> >> > they call, modify set_posix_acl() to not test ownership,
> >> > and provide a local 'struct xattr_handler' structure for NFS.
> >> >
> >> > I don't really like any of those suggestions. Can someone else do any
> >> > better?
> >>
> >> Do we have important callers of inode_owner_or_capable() in the vfs (as
> >> opposed to in individual filesystems), and do any of them pose a similar
> >> problem for network filesystems?
> >
> > do_linkat()->may_linkat() looks kinda suspicious to me. Or what about
> > the O_NOATIME check in map_open()? Just engaging in dumb grepping
> > here....
> >
> > --b.
>
> NOATIME, both in open and fcntl, is rejected on NFS. This seems valid
> as there is no way in the protocol to ask the server to no update the
> atime.
>
> Others I found we just short-cuts to avoid calling i_op->permission() if
> the caller was an owner. I don't *think* that would affect NFS much
> ... though if an owner didn't have write permission, some things might
> be incorrectly forbidden. Maybe.
OK, so not too important. Still, it sounds like
inode_owner_or_capable() is something people expect to work for any
filesystem, so I wonder if there's a way to do that. Or at least
disable it.
--b.
On Sun, Aug 12 2018, Bruce Fields wrote:
> On Sun, Aug 12, 2018 at 08:28:00AM +1000, NeilBrown wrote:
>> On Fri, Aug 10 2018, Bruce Fields wrote:
>>
>> > On Fri, Aug 10, 2018 at 01:00:27PM -0400, Bruce Fields wrote:
>> >> On Fri, Aug 10, 2018 at 11:29:33AM +1000, NeilBrown wrote:
>> >> > On Mon, Mar 21 2016, Nelson Elhage wrote:
>> >> >
>> >> > > That's correct. The other detail that seems to be important is that
>> >> > > the user making the call must be different from the user owning the
>> >> > > file. We've also been using user remapping on the server, so that
>> >> > > non-xattr calls succeed in that configuration.
>> >> > >
>> >> > > The reproducer James added in the bugzilla is:
>> >> > >
>> >> > > (on machine with IP address 10.1.1.1)
>> >> > > sudo mkdir /nfs_test
>> >> > > sudo useradd -u 10000 test_user
>> >> > > sudo chown test_user /nfs_test
>> >> > > echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
>> >> > > /etc/exports
>> >> > > sudo exportfs -a
>> >> > >
>> >> > > (on machine with IP address 10.1.1.2)
>> >> > > sudo mkdir /nfs_test
>> >> > > sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
>> >> > > touch /nfs_test/foo
>> >> > > install -m 755 /nfs_test/foo /nfs_test/bar
>> >> >
>> >> > Did anything ever happen about this?
>> >> > I have a customer with a similar problem (in 4.4) but I cannot see any
>> >> > evidence of fixes landing in mainline.
>> >> >
>> >> > Problem happens with you have uid mapping on the server
>> >> > (e.g. anonuid=10000 as above) and a user with a different uid on the
>> >> > client attempts setacl on a file with that user.
>> >> > As anon is mapped to the owner of the file, setacl should be allowed.
>> >> > However set_posix_acl() calls inode_owner_or_capable() which checks if
>> >> > the client-side uid matches the visible inode->i_uid - they don't.
>> >> >
>> >> > Testing i_uid on the client is always incorrect for permission checking
>> >> > with NFS - the client should always ask the server, either with ACCESS
>> >> > or, in this case, by simply attempting the operation.
>> >> >
>> >> > Any suggestions how best to fix this?
>> >> > - We could move the responsibility for permission checking into
>> >> > i_op->set_acl, but that would be a large change and might make it too
>> >> > easy for other filesystems to get it wrong.
>> >> > - we could have some sort of flag asking set_posix_acl(), but that's
>> >> > rather clumsy.... maybe if i_op->set_acl_check_perm use that without
>> >> > testing ownership first??
>> >> > - we could copy
>> >> > posic_acl_xattr_{get,set,list} into nfs together with functions
>> >> > they call, modify set_posix_acl() to not test ownership,
>> >> > and provide a local 'struct xattr_handler' structure for NFS.
>> >> >
>> >> > I don't really like any of those suggestions. Can someone else do any
>> >> > better?
>> >>
>> >> Do we have important callers of inode_owner_or_capable() in the vfs (as
>> >> opposed to in individual filesystems), and do any of them pose a similar
>> >> problem for network filesystems?
>> >
>> > do_linkat()->may_linkat() looks kinda suspicious to me. Or what about
>> > the O_NOATIME check in map_open()? Just engaging in dumb grepping
>> > here....
>> >
>> > --b.
>>
>> NOATIME, both in open and fcntl, is rejected on NFS. This seems valid
>> as there is no way in the protocol to ask the server to no update the
>> atime.
>>
>> Others I found we just short-cuts to avoid calling i_op->permission() if
>> the caller was an owner. I don't *think* that would affect NFS much
>> ... though if an owner didn't have write permission, some things might
>> be incorrectly forbidden. Maybe.
>
> OK, so not too important. Still, it sounds like
> inode_owner_or_capable() is something people expect to work for any
> filesystem, so I wonder if there's a way to do that. Or at least
> disable it.
We could add a new flag - MAY_OWN (or something) - to the flags
recognised by inode_permission() and i_op->permission().
If ->permission isn't set, inode_permission() uses
inode_owner_or_capable().
If it is, it gets to call that, or do whatever is appropriate.
Is this flag the same as NFS_MAY_OWNER_OVERRIDE or not....??
NeilBrown
On Mon, Aug 13 2018, NeilBrown wrote:
> On Sun, Aug 12 2018, Bruce Fields wrote:
>> OK, so not too important. Still, it sounds like
>> inode_owner_or_capable() is something people expect to work for any
>> filesystem, so I wonder if there's a way to do that. Or at least
>> disable it.
>
> We could add a new flag - MAY_OWN (or something) - to the flags
> recognised by inode_permission() and i_op->permission().
>
> If ->permission isn't set, inode_permission() uses
> inode_owner_or_capable().
> If it is, it gets to call that, or do whatever is appropriate.
>
> Is this flag the same as NFS_MAY_OWNER_OVERRIDE or not....??
>
Pursuing this thought...
NFSD_MAY_OWNER_OVERRIDE means "an operation is requested which
may always be performed by the owner of the file, even if they
don't have explicit permission via DAC setting."
I think this is a reasonable description of how inode_owner_or_capable()
is used. It is sometimes used on its own, where there is no permission
but that is relevant such as O_NOATIME or set_posix_acl(), or is used
as a precursor to and inode_permission() check, as in notify_change().
The biggest difference is that NFSD_MAY_OWNER_OVERRIDE does have the
"or_capable".
As nfsd drops CAP_FOWNER, and the extra test won't hurt it.
So I now think that a good solution to this problem would be to hoist
NFSD_MAY_OWNER_OVERRIDE into the VFS and change inode_permission() and
various i_op->permission functions to handle it.
All we need is a good name....
MAY_BY_OWNER ???
MAY_IF_OWNER
MAY_BE_OWNER ???
MAY_READ means "may I please read this file". The flag needs to say
"may I act as the owner of this file", so
MAY_ACT_AS_OWNER ????
Thought?
NeilBrown
On Tue, Aug 14, 2018 at 07:03:14PM +1000, NeilBrown wrote:
> On Mon, Aug 13 2018, NeilBrown wrote:
>
> > On Sun, Aug 12 2018, Bruce Fields wrote:
> >> OK, so not too important. Still, it sounds like
> >> inode_owner_or_capable() is something people expect to work for any
> >> filesystem, so I wonder if there's a way to do that. Or at least
> >> disable it.
> >
> > We could add a new flag - MAY_OWN (or something) - to the flags
> > recognised by inode_permission() and i_op->permission().
> >
> > If ->permission isn't set, inode_permission() uses
> > inode_owner_or_capable().
> > If it is, it gets to call that, or do whatever is appropriate.
> >
> > Is this flag the same as NFS_MAY_OWNER_OVERRIDE or not....??
> >
>
> Pursuing this thought...
> NFSD_MAY_OWNER_OVERRIDE means "an operation is requested which
> may always be performed by the owner of the file, even if they
> don't have explicit permission via DAC setting."
>
> I think this is a reasonable description of how inode_owner_or_capable()
> is used. It is sometimes used on its own, where there is no permission
> but that is relevant such as O_NOATIME or set_posix_acl(), or is used
> as a precursor to and inode_permission() check, as in notify_change().
>
> The biggest difference is that NFSD_MAY_OWNER_OVERRIDE does have the
> "or_capable".
> As nfsd drops CAP_FOWNER, and the extra test won't hurt it.
>
> So I now think that a good solution to this problem would be to hoist
> NFSD_MAY_OWNER_OVERRIDE into the VFS and change inode_permission() and
> various i_op->permission functions to handle it.
>
> All we need is a good name....
> MAY_BY_OWNER ???
> MAY_IF_OWNER
> MAY_BE_OWNER ???
>
> MAY_READ means "may I please read this file". The flag needs to say
> "may I act as the owner of this file", so
> MAY_ACT_AS_OWNER ????
It's still a little different from the other permission bits in that I
believe
permission(., READ|WRITE)
== permission(., READ) && permission(., WRITE)
but
permission(., READ|OWNER_OVERRIDE)
== permission(., READ) || permission(., OWNER_OVERRIDE)
?
Anyway, naming aside.... I don't know, sounds like it might work?
Honestly I'm not completely sure I understand the proposal.
--b.
On Tue, Aug 14 2018, Bruce Fields wrote:
> On Tue, Aug 14, 2018 at 07:03:14PM +1000, NeilBrown wrote:
>> On Mon, Aug 13 2018, NeilBrown wrote:
>>
>> > On Sun, Aug 12 2018, Bruce Fields wrote:
>> >> OK, so not too important. Still, it sounds like
>> >> inode_owner_or_capable() is something people expect to work for any
>> >> filesystem, so I wonder if there's a way to do that. Or at least
>> >> disable it.
>> >
>> > We could add a new flag - MAY_OWN (or something) - to the flags
>> > recognised by inode_permission() and i_op->permission().
>> >
>> > If ->permission isn't set, inode_permission() uses
>> > inode_owner_or_capable().
>> > If it is, it gets to call that, or do whatever is appropriate.
>> >
>> > Is this flag the same as NFS_MAY_OWNER_OVERRIDE or not....??
>> >
>>
>> Pursuing this thought...
>> NFSD_MAY_OWNER_OVERRIDE means "an operation is requested which
>> may always be performed by the owner of the file, even if they
>> don't have explicit permission via DAC setting."
>>
>> I think this is a reasonable description of how inode_owner_or_capable()
>> is used. It is sometimes used on its own, where there is no permission
>> but that is relevant such as O_NOATIME or set_posix_acl(), or is used
>> as a precursor to and inode_permission() check, as in notify_change().
>>
>> The biggest difference is that NFSD_MAY_OWNER_OVERRIDE does have the
>> "or_capable".
>> As nfsd drops CAP_FOWNER, and the extra test won't hurt it.
>>
>> So I now think that a good solution to this problem would be to hoist
>> NFSD_MAY_OWNER_OVERRIDE into the VFS and change inode_permission() and
>> various i_op->permission functions to handle it.
>>
>> All we need is a good name....
>> MAY_BY_OWNER ???
>> MAY_IF_OWNER
>> MAY_BE_OWNER ???
>>
>> MAY_READ means "may I please read this file". The flag needs to say
>> "may I act as the owner of this file", so
>> MAY_ACT_AS_OWNER ????
>
> It's still a little different from the other permission bits in that I
> believe
>
> permission(., READ|WRITE)
> == permission(., READ) && permission(., WRITE)
>
> but
>
> permission(., READ|OWNER_OVERRIDE)
> == permission(., READ) || permission(., OWNER_OVERRIDE)
>
A little different from some other permission bits.
We have
#define MAY_EXEC 0x00000001
#define MAY_WRITE 0x00000002
#define MAY_READ 0x00000004
#define MAY_APPEND 0x00000008
#define MAY_ACCESS 0x00000010
#define MAY_OPEN 0x00000020
#define MAY_CHDIR 0x00000040
/* called from RCU mode, don't block */
#define MAY_NOT_BLOCK 0x00000080
MAY_CHDIR says something like "test the other bits, but first make sure
your cache is up-to-date".
MAY_NOT_BLOCK says "test the other bits, but not if you would need to
block.
MAY_OWNER would be "test the other bits, but only if not the owner".
So: not much more ad-hoc than other bits.
> ?
>
> Anyway, naming aside.... I don't know, sounds like it might work?
> Honestly I'm not completely sure I understand the proposal.
I guess I should supply a patch...
NeilBrown
On Tue, Aug 14 2018, Bruce Fields wrote:
> Honestly I'm not completely sure I understand the proposal.
Ok, here is a concrete RFC proposal which should make it easier to
understand.
I've tested that this fixes the specific problem in that a user with a
uid that doesn't match the file, but which the server will give
ownership rights to, can now setacl a file.
Thanks,
NeilBrown
From 34f8b23b224e575d5f1fa30834b247e82a854546 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Thu, 16 Aug 2018 10:37:21 +1000
Subject: [PATCH] VFS: introduce MAY_ACT_AS_OWNER
A few places in VFS, particularly set_posix_acl(), use
inode_owner_or_capable() to check if the caller has "owner"
access to the inode.
This assumes that it is valid to test inode->i_uid, which is not
always the case. Particularly in the case of NFS it is not valid to
us i_uid (or i_mode) for permission tests - the server needs to make
the decision.
As a result if the server is remaping uids
(e.g. all-squash,anon_uid=1000),
then all users should have ownership access, but most users will not
be able to set acls.
This patch moves the ownership test to inode_permission and
i_op->permission.
A new flag for this functions, MAY_ACT_AS_OWNER is introduced.
generic_permission() now handles this correctly and many
i_op->permission functions call this function() and don't need any
changes. A few are changed to handle MAY_ACT_AS_OWNER exactly
as generic_permission() does, using inode_owner_or_capable().
For these filesystems, no behavioural change should be noticed.
For NFS, nfs_permission is changed to always return 0 (success) if
MAY_ACT_AS_OWNER. For NFS, and operations which use this flag should
be sent to the server, and the server will succeed or fail as
appropriate.
nfsd has a similar flag - NFSD_MAY_OWNER_OVERRIDE. This is currently
implemented by a test on inode->i_uid. It is changed to map this
flag to MAY_ACT_AS_OWNER.
Fixes: 013cdf1088d7 ("nfs: use generic posix ACL infrastructure for v3 Posix ACLs")
Signed-off-by: NeilBrown <[email protected]>
---
fs/afs/security.c | 10 ++++++++++
fs/attr.c | 12 +++++-------
fs/coda/dir.c | 10 ++++++++++
fs/fcntl.c | 2 +-
fs/fuse/dir.c | 10 ++++++++++
fs/namei.c | 9 +++++++++
fs/nfs/dir.c | 8 ++++++++
fs/nfsd/vfs.c | 9 +++++----
fs/posix_acl.c | 2 +-
fs/xattr.c | 2 +-
include/linux/fs.h | 8 ++++++++
11 files changed, 68 insertions(+), 14 deletions(-)
diff --git a/fs/afs/security.c b/fs/afs/security.c
index 81dfedb7879f..ac2e39de8bff 100644
--- a/fs/afs/security.c
+++ b/fs/afs/security.c
@@ -349,6 +349,16 @@ int afs_permission(struct inode *inode, int mask)
if (mask & MAY_NOT_BLOCK)
return -ECHILD;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
+
_enter("{{%x:%u},%lx},%x,",
vnode->fid.vid, vnode->fid.vnode, vnode->flags, mask);
diff --git a/fs/attr.c b/fs/attr.c
index d22e8187477f..c1160bd9416b 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -87,7 +87,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
/* Make sure a caller can chmod. */
if (ia_valid & ATTR_MODE) {
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
/* Also check the setgid bit! */
if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
@@ -98,7 +98,7 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
/* Check for setting the inode time. */
if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
}
@@ -246,11 +246,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
if (IS_IMMUTABLE(inode))
return -EPERM;
- if (!inode_owner_or_capable(inode)) {
- error = inode_permission(inode, MAY_WRITE);
- if (error)
- return error;
- }
+ error = inode_permission(inode, MAY_ACT_AS_OWNER | MAY_WRITE);
+ if (error)
+ return error;
}
if ((ia_valid & ATTR_MODE)) {
diff --git a/fs/coda/dir.c b/fs/coda/dir.c
index 00876ddadb43..7e31f68d4973 100644
--- a/fs/coda/dir.c
+++ b/fs/coda/dir.c
@@ -80,6 +80,16 @@ int coda_permission(struct inode *inode, int mask)
if (mask & MAY_NOT_BLOCK)
return -ECHILD;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
+
mask &= MAY_READ | MAY_WRITE | MAY_EXEC;
if (!mask)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 12273b6ea56d..dbf2531dc7fa 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -46,7 +46,7 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
/* O_NOATIME can only be set by the owner or superuser */
if ((arg & O_NOATIME) && !(filp->f_flags & O_NOATIME))
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
/* required for strict SunOS emulation */
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d80aab0d5982..46deb2652dca 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1117,6 +1117,16 @@ static int fuse_permission(struct inode *inode, int mask)
if (!fuse_allow_current_process(fc))
return -EACCES;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
+
/*
* If attributes are needed, refresh them before proceeding
*/
diff --git a/fs/namei.c b/fs/namei.c
index 3cd396277cd3..364bfafa5b01 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -335,6 +335,15 @@ int generic_permission(struct inode *inode, int mask)
{
int ret;
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER) {
+ if (inode_owner_or_capable(inode))
+ return 0;
+ mask &= ~MAY_ACT_AS_OWNER;
+ if (!mask)
+ /* No other permission will suffice */
+ return -EACCES;
+ }
/*
* Do the basic permission checks.
*/
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index d7f158c3efc8..e487429318d8 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2516,6 +2516,14 @@ int nfs_permission(struct inode *inode, int mask)
nfs_inc_stats(inode, NFSIOS_VFSACCESS);
+ /* Short-circuit for owner */
+ if (mask & MAY_ACT_AS_OWNER)
+ /*
+ * Ownership will be tested by server when we
+ * actually try operation.
+ */
+ return 0;
+
if ((mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
goto out;
/* Is this sys_access() ? */
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 55a099e47ba2..467b3418af20 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2038,12 +2038,13 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
* We must trust the client to do permission checking - using "ACCESS"
* with NFSv3.
*/
- if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
- uid_eq(inode->i_uid, current_fsuid()))
- return 0;
/* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
- err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
+ if (acc & NFSD_MAY_OWNER_OVERRIDE)
+ err = inode_permission(inode, ((acc & (MAY_READ|MAY_WRITE|MAY_EXEC))
+ | MAY_ACT_AS_OWNER));
+ else
+ err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
/* Allow read access to binaries even when mode 111 */
if (err == -EACCES && S_ISREG(inode->i_mode) &&
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 2fd0fde16fe1..a90c7dca892a 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -863,7 +863,7 @@ set_posix_acl(struct inode *inode, int type, struct posix_acl *acl)
if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
return acl ? -EACCES : 0;
- if (!inode_owner_or_capable(inode))
+ if (inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
if (acl) {
diff --git a/fs/xattr.c b/fs/xattr.c
index f9cb1db187b7..9ce0a0994abd 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -127,7 +127,7 @@ xattr_permission(struct inode *inode, const char *name, int mask)
if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
return (mask & MAY_WRITE) ? -EPERM : -ENODATA;
if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
- (mask & MAY_WRITE) && !inode_owner_or_capable(inode))
+ (mask & MAY_WRITE) && inode_permission(inode, MAY_ACT_AS_OWNER) < 0)
return -EPERM;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1ec33fd0423f..2641fb50bed0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -92,6 +92,14 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define MAY_CHDIR 0x00000040
/* called from RCU mode, don't block */
#define MAY_NOT_BLOCK 0x00000080
+/*
+ * File Owner is always allowed to perform pending
+ * operation. If current user is an owner, or if
+ * filesystem performs permission check at time-of-operation,
+ * then succeed, else require some other permission
+ * if listed.
+ */
+#define MAY_ACT_AS_OWNER 0x00000100
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
--
2.14.0.rc0.dirty
On Thu, Aug 16, 2018 at 10:39:35AM +1000, NeilBrown wrote:
> On Tue, Aug 14 2018, Bruce Fields wrote:
> > Honestly I'm not completely sure I understand the proposal.
>
> Ok, here is a concrete RFC proposal which should make it easier to
> understand.
> I've tested that this fixes the specific problem in that a user with a
> uid that doesn't match the file, but which the server will give
> ownership rights to, can now setacl a file.
Thanks, this makes sense to me.
I might try to split this change into a couple steps, but I'm not sure
exactly how.
Minor nits:
> From 34f8b23b224e575d5f1fa30834b247e82a854546 Mon Sep 17 00:00:00 2001
> From: NeilBrown <[email protected]>
> Date: Thu, 16 Aug 2018 10:37:21 +1000
> Subject: [PATCH] VFS: introduce MAY_ACT_AS_OWNER
>
> A few places in VFS, particularly set_posix_acl(), use
> inode_owner_or_capable() to check if the caller has "owner"
> access to the inode.
> This assumes that it is valid to test inode->i_uid, which is not
> always the case. Particularly in the case of NFS it is not valid to
> us i_uid (or i_mode) for permission tests - the server needs to make
> the decision.
>
> As a result if the server is remaping uids
remapping
> (e.g. all-squash,anon_uid=1000),
> then all users should have ownership access, but most users will not
> be able to set acls.
>
> This patch moves the ownership test to inode_permission and
> i_op->permission.
> A new flag for this functions, MAY_ACT_AS_OWNER is introduced.
these functions?
> generic_permission() now handles this correctly and many
> i_op->permission functions call this function() and don't need any
> changes. A few are changed to handle MAY_ACT_AS_OWNER exactly
> as generic_permission() does, using inode_owner_or_capable().
> For these filesystems, no behavioural change should be noticed.
>
> For NFS, nfs_permission is changed to always return 0 (success) if
> MAY_ACT_AS_OWNER. For NFS, and operations which use this flag should
any operations
> @@ -2038,12 +2038,13 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
> * We must trust the client to do permission checking - using "ACCESS"
> * with NFSv3.
> */
> - if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
> - uid_eq(inode->i_uid, current_fsuid()))
> - return 0;
>
> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
Can we do the same for NFSD_MAY_OWNER_OVERRIDE and drop the extra "if"
statement?
> - err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
> + if (acc & NFSD_MAY_OWNER_OVERRIDE)
> + err = inode_permission(inode, ((acc & (MAY_READ|MAY_WRITE|MAY_EXEC))
> + | MAY_ACT_AS_OWNER));
> + else
> + err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC));
>
> /* Allow read access to binaries even when mode 111 */
> if (err == -EACCES && S_ISREG(inode->i_mode) &&
--b.
On Thu, Aug 16 2018, Bruce Fields wrote:
> On Thu, Aug 16, 2018 at 10:39:35AM +1000, NeilBrown wrote:
>> On Tue, Aug 14 2018, Bruce Fields wrote:
>> > Honestly I'm not completely sure I understand the proposal.
>>
>> Ok, here is a concrete RFC proposal which should make it easier to
>> understand.
>> I've tested that this fixes the specific problem in that a user with a
>> uid that doesn't match the file, but which the server will give
>> ownership rights to, can now setacl a file.
>
> Thanks, this makes sense to me.
>
> I might try to split this change into a couple steps, but I'm not sure
> exactly how.
I was originally thinking for keeping the nfsd change in a separate
patch, but it was so tiny...
>
> Minor nits:
All nits addressed - thanks.
>> @@ -2038,12 +2038,13 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
>> * We must trust the client to do permission checking - using "ACCESS"
>> * with NFSv3.
>> */
>> - if ((acc & NFSD_MAY_OWNER_OVERRIDE) &&
>> - uid_eq(inode->i_uid, current_fsuid()))
>> - return 0;
>>
>> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */
>
> Can we do the same for NFSD_MAY_OWNER_OVERRIDE and drop the extra "if"
> statement?
Probably. If we apply this change first, then it should be trivial.
It would be a bit nicer if we could use enum for bits (a bit like "go"
allows), but I don't think this is too bad. Thoughts?
(Do we really need NFSD_MAY_MASK ???)
Thanks,
NeilBrown
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index a7e107309f76..6ca707511f65 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -13,13 +13,14 @@
* Flags for nfsd_permission
*/
#define NFSD_MAY_NOP 0
-#define NFSD_MAY_EXEC 0x001 /* == MAY_EXEC */
-#define NFSD_MAY_WRITE 0x002 /* == MAY_WRITE */
-#define NFSD_MAY_READ 0x004 /* == MAY_READ */
-#define NFSD_MAY_SATTR 0x008
-#define NFSD_MAY_TRUNC 0x010
-#define NFSD_MAY_LOCK 0x020
-#define NFSD_MAY_MASK 0x03f
+#define NFSD_MAY_EXEC MAY_EXEC
+#define NFSD_MAY_WRITE MAY_WRITE
+#define NFSD_MAY_READ MAY_READ
+#define NFSD_MAY_SATTR (__MAY_UNUSED << 0)
+#define NFSD_MAY_TRUNC (__MAY_UNUSED << 1)
+#define NFSD_MAY_LOCK (__MAY_UNUSED << 2)
+#define __NFSD_MAY_UNUSED (__MAY_UNUSED << 3)
+#define NFSD_MAY_MASK (__NFSD_MAY_UNUSED - 1)
/* extra hints to permission and open routines: */
#define NFSD_MAY_OWNER_OVERRIDE 0x040
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1ec33fd0423f..aed6a65f06b8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -93,6 +93,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
/* called from RCU mode, don't block */
#define MAY_NOT_BLOCK 0x00000080
+#define __MAY_UNUSED 0x00000100
+
/*
* flags in file.f_mode. Note that FMODE_READ and FMODE_WRITE must correspond
* to O_WRONLY and O_RDWR via the strange trick in do_dentry_open()