[confused] can orangefs ACLs be removed at all?

[confused] can orangefs ACLs be removed at all?

[Al Viro]

	Prior to 4bef69000d93 (orangefs: react properly to
posix_acl_update_mode's aftermath.) it used to be possible
to do orangefs_set_acl(inode, NULL, ACL_TYPE_ACCESS) -
it would've removed the corresponding xattr and that would
be it.  Now it fails with -EINVAL without having done
anything.  How is one supposed to remove ACLs there?

	Moreover, if you change an existing ACL to something
that is expressible by pure mode, you end up calling
__orangefs_setattr(), which will call posix_acl_chmod().
And AFAICS that will happen with *old* ACL still cached,
so you'll get ACL_MASK/ACL_OTHER updated in the old ACL.

	How can that possibly work?  Sure, you want to
propagate the updated mode to server - after you've
done the actual update (possibly removal) of ACL-encoding
xattr there...

Re: [confused] can orangefs ACLs be removed at all?

[Mike Marshall]

Hi Al...

I've been out of the loop for over a week, I only saw
your questions yesterday... I have one small debugfs
patch on linux-next I will submit for the merge window
today, and will have to go back out of the loop for a
few more days (temps will drop, I'm insulating the plumbing
on my house).

When I was writing and testing 4bef69000d93, as I remember,
I used getfacl and setfacl to see that things worked as
I expected them to.

I looked at my code while thinking about your questions, and
they seem like good ones. I have a couple of questions that will
help me when I return to this in a few days:

>> it used to be possible to do
>> orangefs_set_acl(inode, NULL, ACL_TYPE_ACCESS)

The way I tested (which maybe misses important stuff?) usually
caused posix_acl_xattr_set -> set_posix_acl -> orangefs_set_acl ...
Is there a simple userspace command that would send a NULL? When
would there be a NULL?

>> How is one supposed to remove ACLs there?

setfacl -m and setfacl -x both seem to work. I also have a userspace
test program I wrote that uses the internal orangefs api (not through
the kernel) to manipulate xattrs on orangefs files. Going through the
kernel with setfacl and looking at the results with my test program
seems as expected (I can make acls come and go).

>> Moreover, if you change an existing ACL to something
>> that is expressible by pure mode...

I don't remember having trouble before, but now when I try to set
an acl (on orangefs or ext4) that I think is expressible in pure mode,
the mode doesn't change, rather the acl is still set... can you
suggest a simple setfacl (or other) example I can use to test?

I will get back to this in a few days and work to get the code
into a condition that you think is reasonable.

Thanks!

-Mike

On Fri, Jan 31, 2020 at 7:56 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         Prior to 4bef69000d93 (orangefs: react properly to
> posix_acl_update_mode's aftermath.) it used to be possible
> to do orangefs_set_acl(inode, NULL, ACL_TYPE_ACCESS) -
> it would've removed the corresponding xattr and that would
> be it.  Now it fails with -EINVAL without having done
> anything.  How is one supposed to remove ACLs there?
>
>         Moreover, if you change an existing ACL to something
> that is expressible by pure mode, you end up calling
> __orangefs_setattr(), which will call posix_acl_chmod().
> And AFAICS that will happen with *old* ACL still cached,
> so you'll get ACL_MASK/ACL_OTHER updated in the old ACL.
>
>         How can that possibly work?  Sure, you want to
> propagate the updated mode to server - after you've
> done the actual update (possibly removal) of ACL-encoding
> xattr there...

Re: [confused] can orangefs ACLs be removed at all?

[Al Viro]

On Thu, Feb 06, 2020 at 10:35:12AM -0500, Mike Marshall wrote:

> I looked at my code while thinking about your questions, and
> they seem like good ones. I have a couple of questions that will
> help me when I return to this in a few days:
> 
> >> it used to be possible to do
> >> orangefs_set_acl(inode, NULL, ACL_TYPE_ACCESS)
> 
> The way I tested (which maybe misses important stuff?) usually
> caused posix_acl_xattr_set -> set_posix_acl -> orangefs_set_acl ...
> Is there a simple userspace command that would send a NULL? When
> would there be a NULL?

setfattr -x system.posix_acl_access <filename>

works on ext4 and I don't see any way for it to work with current
orangefs_set_acl().

> I don't remember having trouble before, but now when I try to set
> an acl (on orangefs or ext4) that I think is expressible in pure mode,
> the mode doesn't change, rather the acl is still set... can you
> suggest a simple setfacl (or other) example I can use to test?

setfacl -b <filename>

works on ext4, goes by setxattr() with non-NULL acl that gets folded
into NULL by  posix_acl_update_mode().  Sure, you call __orangefs_setattr()
there, so mode does get updated.  What you don't do is telling the
server to get rid of xattr on that file.  And I don't see where the
cached acl is dropped, but I might be missing something.

Note that e.g. ext4 does this:
        if ((type == ACL_TYPE_ACCESS) && acl) {
                error = posix_acl_update_mode(inode, &mode, &acl);
                if (error)
                        goto out_stop;
                if (mode != inode->i_mode)
                        update_mode = 1;
        }

        error = __ext4_set_acl(handle, inode, type, acl, 0 /* xattr_flags */);
        if (!error && update_mode) {
                inode->i_mode = mode;
                inode->i_ctime = current_time(inode);
                ext4_mark_inode_dirty(handle, inode);
        }
the first part is more or less what your commit tries to do, but
note that __ext4_set_acl() is called in all cases; changing i_mode
is done after it, not instead of it.  And __ext4_set_acl() does
set_cached_acl() in the end (on success, that is).  So does
__orangefs_set_acl(), but you don't can it in that case; _maybe_
something else deals with that, but I don't see any plausible
candidates in there.

Sorry for the lack of direct orangefs testcases - I don't have
orangefs testbed set up right now, and IIRC setting it up had been
an interesting exercise...

Re: [confused] can orangefs ACLs be removed at all?

[Mike Marshall]

I've been away from this for a while, but have been working on it
now for several days... by working on it, I mean I've been reading
the code back into fs and forward into the userspace part of orangefs,
and comparing what orangefs does with what ext4 and tmpfs do when I
set and unset acls...

I can observe that these acls are foldable into mode,
no acls are set, this asdf file is on ext4:

hubcap@vm1 ~]$ ls -l asdf
-rw-rw----. 1 hubcap hubcap 0 Mar  6 15:34 asdf
[hubcap@vm1 ~]$ setfacl -m u::rwx asdf
[hubcap@vm1 ~]$ ls -l asdf
-rwxrw----. 1 hubcap hubcap 0 Mar  6 15:34 asdf
[hubcap@vm1 ~]$ setfacl -m g::rwx asdf
[hubcap@vm1 ~]$ ls -l asdf
-rwxrwx---. 1 hubcap hubcap 0 Mar  6 15:34 asdf
[hubcap@vm1 ~]$ setfacl -m o::rwx asdf
[hubcap@vm1 ~]$ ls -l asdf
-rwxrwxrwx. 1 hubcap hubcap 0 Mar  6 15:34 asdf

There must be more, perhaps from the perspective of
root setting the acl, or...? What are some other
examples of acls that get folded into mode that I
could test with?

Al>> Moreover, if you change an existing ACL to something
Al>> that is expressible by pure mode,

Can you suggest an example here, too?

Finally (for today :-) ) what happened here? Orangefs
reacts differently than ext4... in both cases the acl
was set, but on ext4 the mode was also
changed...

hubcap@vm1 ~]$ touch /pvfsmnt/asdf /home/hubcap/asdf
[hubcap@vm1 ~]$ ls -l /pvfsmnt/asdf /home/hubcap/asdf
-rw-rw-r--. 1 hubcap hubcap 0 Mar 13 11:50 /home/hubcap/asdf
-rw-rw-r--. 1 hubcap hubcap 0 Mar 13 11:50 /pvfsmnt/asdf

root@vm1 hubcap]# chown root /home/hubcap/asdf /pvfsmnt/asdf
[root@vm1 hubcap]# ls -l /home/hubcap/asdf /pvfsmnt/asdf
-rw-rw-r--. 1 root hubcap 0 Mar 13 11:50 /home/hubcap/asdf
-rw-rw-r--. 1 root hubcap 0 Mar 13 11:50 /pvfsmnt/asdf
[root@vm1 hubcap]# setfacl -m u:hubcap:rwx /home/hubcap/asdf /pvfsmnt/asdf
[root@vm1 hubcap]# ls -l /home/hubcap/asdf /pvfsmnt/asdf
-rw-rwxr--+ 1 root hubcap 0 Mar 13 11:50 /home/hubcap/asdf
-rw-rw-r--+ 1 root hubcap 0 Mar 13 11:50 /pvfsmnt/asdf

-Mike

On Fri, Jan 31, 2020 at 7:56 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         Prior to 4bef69000d93 (orangefs: react properly to
> posix_acl_update_mode's aftermath.) it used to be possible
> to do orangefs_set_acl(inode, NULL, ACL_TYPE_ACCESS) -
> it would've removed the corresponding xattr and that would
> be it.  Now it fails with -EINVAL without having done
> anything.  How is one supposed to remove ACLs there?
>
>         Moreover, if you change an existing ACL to something
> that is expressible by pure mode, you end up calling
> __orangefs_setattr(), which will call posix_acl_chmod().
> And AFAICS that will happen with *old* ACL still cached,
> so you'll get ACL_MASK/ACL_OTHER updated in the old ACL.
>
>         How can that possibly work?  Sure, you want to
> propagate the updated mode to server - after you've
> done the actual update (possibly removal) of ACL-encoding
> xattr there...