[Virtio-fs] Question on ACLs support in virtiofs

[Vivek Goyal]

On Fri, Feb 12, 2021 at 10:30:13AM +0000, Luis Henriques wrote:
> Hi!
> 
> I've recently executed the generic fstests on virtiofs and decided to have
> a closer look at generic/099 failure.  In a nutshell, here's the sequence
> of commands that reproduce that failure:
> 
> # umask 0
> # mkdir acldir
> # chacl -b "u::rwx,g::rwx,o::rwx" "u::r-x,g::r--,o::---" acldir
> # touch acldir/file1
> # umask 722
> # touch acldir/file2
> # ls -l acldir
> total 0
> -r--r----- 1 root root 0 Feb 12 10:04 file1
> ----r----- 1 root root 0 Feb 12 10:05 file2
> 
> The failure is that setting umask to 722 shouldn't affect the new file2
> because acldir has a default ACL (from umask(2): "... if the parent
> directory has a default ACL (see acl(5)), the umask is ignored...").
> 
> So... I tried to have look at the code, and initially I thought that the
> problem was in (kernel) function fuse_create_open(), where we have this:
> 
> 	if (!fm->fc->dont_mask)
> 		mode &= ~current_umask();
> 
> but then I went down the rabbit hole, into the user-space code, and
> couldn't reach a conclusion.  Maybe the issue is that there's in fact no
> support for this POSIX ACLs in virtiofs/FUSE?  Any ideas?

Hi,

[ CC Miklos and linux-fsdevel ]

I debugged into this a little. There are many knobs and it is little
confusing that what are right set of fixes. 

So what's happening in this case is that fc->dont_mask is not set. That
means fuse client is modifying mode using umask. First time you
touch file, umask is 0, so there is no modification. But next time,
you set umask to 722, and fuse modifies mode before sending file
create request to server. virtiofs server is already running with
umask 0, so it does not touch the mode.

So that means, that in case of default acl, fuse client should not
be modifying mode using umask. But question is when should fuse
skip applying umask.

I see that fuse always sets SB_POSIXACL. That means VFS is not
going to apply umask and all the umask handling is with-in fuse.

sb->s_flags |= SB_POSIXACL;

Currently fuse sets fc->dont_mask in two conditions.

- If the caller mounted with flag MS_POSIXACL, then fc->dont_mask is set.
- If fuse server opted in for option FUSE_DONT_MASK, then fc->dont_mask
  is set. 

I see that for virtiofs, both the conditions are not true out of the
box. In fact looks like ACL support is not fully enabled, because
I don't see fuse server opting in for FUSE_POSIX_ACL.

I suspect that we probably should provide an option in virtiofsd to
enable/disable acl support.

Setting FUSE_DONT_MASK is tricky. If we leave it to fuse, that means
fuse will have to query acl to figure out if default acl is set or
not on parent dir. And that data could be stale and there could be
races w.r.t setting acls from other client.

If we do set FUSE_DONT_MASK, that means in file creation path virtiofsd
server will have to switch its umask to one provided in request. Given
its a per process property, we will have to have some locks to make
sure other create requests are not progressing in parallel. And that
hope host does the right thing. That is apply umask if parent dir does
not have default acl otherwise apply umask (as set by virtiofsd process).

Miklos, does above sound reasonable. You might have more thoughts on
how to handle this best in fuse/virtiofs.

Vivek