* [Virtio-fs] Question on ACLs support in virtiofs @ 2021-02-12 10:30 Luis Henriques 2021-02-15 20:52 ` Vivek Goyal 0 siblings, 1 reply; 10+ messages in thread From: Luis Henriques @ 2021-02-12 10:30 UTC (permalink / raw) To: virtio-fs 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? Cheers, -- Luis ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs 2021-02-12 10:30 [Virtio-fs] Question on ACLs support in virtiofs Luis Henriques @ 2021-02-15 20:52 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-02-15 20:52 UTC (permalink / raw) To: Luis Henriques; +Cc: virtio-fs, Linux fsdevel mailing list, Miklos Szeredi 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs @ 2021-02-15 20:52 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-02-15 20:52 UTC (permalink / raw) To: Luis Henriques; +Cc: virtio-fs, Miklos Szeredi, Linux fsdevel mailing list 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs 2021-02-15 20:52 ` Vivek Goyal @ 2021-02-16 15:11 ` Miklos Szeredi -1 siblings, 0 replies; 10+ messages in thread From: Miklos Szeredi @ 2021-02-16 15:11 UTC (permalink / raw) To: Vivek Goyal; +Cc: virtio-fs-list, Linux fsdevel mailing list On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > 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. Sounds good. > 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. fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in xattr ops, which means that umask is now a per-thread propery in virtiofsd. So setting umask before create ops sounds like a good solution. Thanks, Miklos > > Vivek > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs @ 2021-02-16 15:11 ` Miklos Szeredi 0 siblings, 0 replies; 10+ messages in thread From: Miklos Szeredi @ 2021-02-16 15:11 UTC (permalink / raw) To: Vivek Goyal; +Cc: Luis Henriques, virtio-fs-list, Linux fsdevel mailing list On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > 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. Sounds good. > 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. fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in xattr ops, which means that umask is now a per-thread propery in virtiofsd. So setting umask before create ops sounds like a good solution. Thanks, Miklos > > Vivek > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs 2021-02-16 15:11 ` Miklos Szeredi (?) @ 2021-02-16 15:54 ` Vivek Goyal 2021-02-17 20:08 ` Dr. David Alan Gilbert -1 siblings, 1 reply; 10+ messages in thread From: Vivek Goyal @ 2021-02-16 15:54 UTC (permalink / raw) To: Miklos Szeredi; +Cc: virtio-fs-list, Linux fsdevel mailing list On Tue, Feb 16, 2021 at 04:11:20PM +0100, Miklos Szeredi wrote: > On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > 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. > > Sounds good. > > > 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. > > fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in > xattr ops, which means that umask is now a per-thread propery in > virtiofsd. Aha.. I forgot about that. Thanks. > > So setting umask before create ops sounds like a good solution. I will give it a try along with an option to enable/disable acl support in virtiofsd. Vivek ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs 2021-02-16 15:54 ` Vivek Goyal @ 2021-02-17 20:08 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 10+ messages in thread From: Dr. David Alan Gilbert @ 2021-02-17 20:08 UTC (permalink / raw) To: Vivek Goyal; +Cc: virtio-fs-list, Linux fsdevel mailing list, Miklos Szeredi * Vivek Goyal (vgoyal@redhat.com) wrote: > On Tue, Feb 16, 2021 at 04:11:20PM +0100, Miklos Szeredi wrote: > > On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > 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. > > > > Sounds good. > > > > > 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. > > > > fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in > > xattr ops, which means that umask is now a per-thread propery in > > virtiofsd. > > Aha.. I forgot about that. Thanks. Isn't that actually variable; in that we cna't do that unshare in some cases when we don't have the capability? Dave > > > > So setting umask before create ops sounds like a good solution. > > I will give it a try along with an option to enable/disable acl > support in virtiofsd. > > Vivek > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs @ 2021-02-17 20:08 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 10+ messages in thread From: Dr. David Alan Gilbert @ 2021-02-17 20:08 UTC (permalink / raw) To: Vivek Goyal; +Cc: Miklos Szeredi, virtio-fs-list, Linux fsdevel mailing list * Vivek Goyal (vgoyal@redhat.com) wrote: > On Tue, Feb 16, 2021 at 04:11:20PM +0100, Miklos Szeredi wrote: > > On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > 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. > > > > Sounds good. > > > > > 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. > > > > fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in > > xattr ops, which means that umask is now a per-thread propery in > > virtiofsd. > > Aha.. I forgot about that. Thanks. Isn't that actually variable; in that we cna't do that unshare in some cases when we don't have the capability? Dave > > > > So setting umask before create ops sounds like a good solution. > > I will give it a try along with an option to enable/disable acl > support in virtiofsd. > > Vivek > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs 2021-02-17 20:08 ` Dr. David Alan Gilbert @ 2021-02-17 20:52 ` Vivek Goyal -1 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-02-17 20:52 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: virtio-fs-list, Linux fsdevel mailing list, Miklos Szeredi On Wed, Feb 17, 2021 at 08:08:12PM +0000, Dr. David Alan Gilbert wrote: > * Vivek Goyal (vgoyal@redhat.com) wrote: > > On Tue, Feb 16, 2021 at 04:11:20PM +0100, Miklos Szeredi wrote: > > > On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > 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. > > > > > > Sounds good. > > > > > > > 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. > > > > > > fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in > > > xattr ops, which means that umask is now a per-thread propery in > > > virtiofsd. > > > > Aha.. I forgot about that. Thanks. > > Isn't that actually variable; in that we cna't do that unshare in some > cases when we don't have the capability? fv_queue_worker() always calls unshare(CLONE_FS). fchdir() is the optional part. We care about unshare(CLONE_FS) so that umask is not shared between threads. Vivek > > Dave > > > > > > So setting umask before create ops sounds like a good solution. > > > > I will give it a try along with an option to enable/disable acl > > support in virtiofsd. > > > > Vivek > > > > _______________________________________________ > > Virtio-fs mailing list > > Virtio-fs@redhat.com > > https://www.redhat.com/mailman/listinfo/virtio-fs > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Virtio-fs] Question on ACLs support in virtiofs @ 2021-02-17 20:52 ` Vivek Goyal 0 siblings, 0 replies; 10+ messages in thread From: Vivek Goyal @ 2021-02-17 20:52 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Miklos Szeredi, virtio-fs-list, Linux fsdevel mailing list On Wed, Feb 17, 2021 at 08:08:12PM +0000, Dr. David Alan Gilbert wrote: > * Vivek Goyal (vgoyal@redhat.com) wrote: > > On Tue, Feb 16, 2021 at 04:11:20PM +0100, Miklos Szeredi wrote: > > > On Mon, Feb 15, 2021 at 9:52 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > > > > > 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. > > > > > > Sounds good. > > > > > > > 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. > > > > > > fv_queue_worker() does unshare(CLONE_FS) for the fchdir() call in > > > xattr ops, which means that umask is now a per-thread propery in > > > virtiofsd. > > > > Aha.. I forgot about that. Thanks. > > Isn't that actually variable; in that we cna't do that unshare in some > cases when we don't have the capability? fv_queue_worker() always calls unshare(CLONE_FS). fchdir() is the optional part. We care about unshare(CLONE_FS) so that umask is not shared between threads. Vivek > > Dave > > > > > > So setting umask before create ops sounds like a good solution. > > > > I will give it a try along with an option to enable/disable acl > > support in virtiofsd. > > > > Vivek > > > > _______________________________________________ > > Virtio-fs mailing list > > Virtio-fs@redhat.com > > https://www.redhat.com/mailman/listinfo/virtio-fs > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-17 20:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-02-12 10:30 [Virtio-fs] Question on ACLs support in virtiofs Luis Henriques 2021-02-15 20:52 ` Vivek Goyal 2021-02-15 20:52 ` Vivek Goyal 2021-02-16 15:11 ` Miklos Szeredi 2021-02-16 15:11 ` Miklos Szeredi 2021-02-16 15:54 ` Vivek Goyal 2021-02-17 20:08 ` Dr. David Alan Gilbert 2021-02-17 20:08 ` Dr. David Alan Gilbert 2021-02-17 20:52 ` Vivek Goyal 2021-02-17 20:52 ` Vivek Goyal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.