Bug 245689

Comment 1 Kubilay Kocak 2020-04-17 10:38:08 UTC

Thank you for the report Mooses

Alan landed our most recent (base 350665) FuseFS infrastructure improvements and may have insight into the best way to address this

Comment 2 Alan Somers 2020-04-17 13:03:48 UTC

Please post full steps to reproduce.

Comment 3 Alan Somers 2020-04-17 13:06:42 UTC

Also, please show me the output of kldstat.

Comment 4 Conrad Meyer 2020-04-17 17:09:36 UTC

The bulk of the 'access'es are from VOP_LOOKUP checking that the directories of the path to the file has +x permission(s) for the current user.  FUSE does this a little weirdly (it basically has an inlined copy of vn_dir_check_exec() embedded, instead of calling the helper) but regardless, this is canonical for FreeBSD lookup.

If FUSE(5) can rely on filesystems to do their own checking as part of lookup, then that is probably an easy optimization.  (Apparently Linux does so.)

chmod => VOP_SETATTR => fuse_vnop_setattr.  At the bottom (success case) we send fuse_internal_access(), then fuse_internal_setattr().  UFS does something similar in ufs_chmod().

Basically: FreeBSD's VOP model has ACCESS as a separate vop, and this is the common pattern.

Probably we can bypass these ACCESS checks in the ~FSESS_DEFAULT_PERMISSIONS case.

Created attachment 213598 [details]
How to install a minimal test instance of MooseFS on FreeBSD

I've attached a how to for creating a test environment. After you've installed it as per instructions, do this:

1) On one console open the debug file (as root):
# cat /mnt/mfs/.oplog

2) Then on the second console, run the test commands (as a non-root user!!!):
# cd /mnt/mfs/
# touch test.txt ; chmod u-w test.txt ; chmod u+w test.txt

You will get the "chmod: test1.txt: Permission denied" error after the last chmod and you can observe the calls that were sent to MooseFS on the first console.

root@test:/ # kldstat
Id Refs Address                Size Name
 1   10 0xffffffff80200000  2448f20 kernel
 2    1 0xffffffff82819000     2668 intpm.ko
 3    1 0xffffffff8281c000      b50 smbus.ko
 4    1 0xffffffff8281d000      acf mac_ntpd.ko
 5    1 0xffffffff8281e000     fcf0 fusefs.ko

Comment 8 Alan Somers 2020-05-04 03:13:53 UTC

I've reproduced the issue on FreeBSD head (Thanks for the great instructions!).  Here is what I found.

Issue 1 is essentially a FUSE protocol bug.  The access mode the kernel wants is not W_OK, it's VADMIN.  But the FUSE protocol has no concept of that mode.  Instead, we ask the daemon for W_OK instead, assuming it's close enough.  It's not obvious what we should be doing instead.  Maybe we should always evaluate such requests in-kernel, as if default_permissions were set.

Issue 2 has a simple solution on MooseFS's side.  If a FUSE daemon ever returns ENOSYS to FUSE_ACCESS, then the client will never send FUSE_ACCESS again.  Instead it will assume that access will always be granted.  That would be appropriate for MooseFS if it checks permissions during FUSE_WRITE, FUSE_SETATTR, etc.  Is that an option for you?

The answer to both questions is - please, let the filesystem that does not send "default_permissions" worry itself about stuff. Now it looks like even when the fs does not send it, you still check. Is there a reason for it?

Of course, issue 1 should be solved in both cases: with "default_permissions" and without. With it yes, I think the solution might be evaluating in kernel. But without "default_permissions" the best solution is just not to check - if the operation is not permitted, the fs will not allow it. It's what is written explicitly in FUSE documentation.

The solution with always assuming access is granted is not great either, because there are two instances where access should be called - when you cd into a directory to check if the x permission is set (X_OK) and when a user explicitly uses access system call in a program.

Comment 10 Alan Somers 2020-05-08 03:15:14 UTC

(In reply to MooseFS FreeBSD Team from comment #9)
You've got it backwards.  We only send FUSE_ACCESS when the file system _doesn't_ set default_permissions.  When it does, we perform all access checks in the kernel.

I can certainly fix issue #1: it looks like a one-liner.

But issue #2 is much more complicated.  The root cause of the problem is simply that FreeBSD's and Linux's VFSes work differently.  FreeBSD uses VOP_ACCESS(9) copiously, delegating to file systems most aspects of authorization checking.  But Linux performs much more of that at a higher level.  The FUSE protocol was naively designed as a serialization of Linux's in-kernel file system API, and the result is several warts like this.  From the perspective of FreeBSD's FUSE driver, I can't tell which VOP_ACCESS()s are the result of access(2) and which are the result of any other operation.  So without some major hacks up-stack, there's little I can do to reduce the frequency of FUSE_ACCESS operations.  The only sure way to cut down the traffic is to mount a file system with default_permissions, or to return ENOSYS for FUSE_ACCESS.

It's great that you can fix issue nr 1. This IS the issue here, after all.

As for nr 2 - we were hoping that we could get rid of the accesses, but if we can't, we can't. We want MooseFS to be fast, but first of all we want it to be reliable. We can't use any of the proposed solutions. We can't use "default_permissions" because of ACLs and MooseFS specific permissions that we need to handle ourselves. And we can't return ENOSYS because we want those "real" accesses to work. So the answer has to stay "nothing can be done" for nr 2. We may consider adding the ENOSYS solution as a switchable option to the system (off by default) if we get user requests about that.

Comment 12 Alan Somers 2020-05-08 22:46:53 UTC

Created attachment 214294 [details]
Fix issues with FUSE_ACCESS when default_permissions is disabled

This patch fixes two issues relating to FUSE_ACCESS when the default_permissions mount option is disabled:

* VOP_ACCESS() calls with VADMIN set should never be sent to a fuse server in the form of FUSE_ACCESS operations.  The FUSE protocol has no equivalent of VADMIN, so we must evaluate such things kernel-side, regardless of the default_permissions setting.

* The FUSE protocol only requires FUSE_ACCESS to be sent for two purposes: for the access(2) syscall and to check directory permissions for searchability during lookup.  FreeBSD sends it much more frequently, due to differences between our VFS and Linux's, for which FUSE was designed.  But this patch does eliminate several cases not required by the FUSE protocol:
  - for any FUSE_*XATTR operation
  - when creating a new file
  - when deleting a file
  - when setting timestamps, such as by utimensat(2).

* Additionally, when default_permissions is disabled, this patch removes one FUSE_GETATTR operation when deleting a file.

Comment 13 Alan Somers 2020-05-08 22:48:01 UTC

Could you please run your tests again, with this patch applied?  Bonus points if you run the following command while the tests are ongoing.  If it prints anything at all, that indicates a kernel bug.

sudo dtrace -i 'fusefs:fusefs:internal:access_vadmin {stack();}'

Comment 14 Alan Somers 2020-05-08 23:04:50 UTC

A slightly updated patch is under review at
https://reviews.freebsd.org/D24777

I tried to test it, but when I dry run the patch, I get an error:

Hunk #2 failed at 247.
1 out of 6 hunks failed while patching sys/fs/fuse/fuse_vnops.c

I checked it and it looks like in "my" sources function "priv_check_cred", called in line 250 of sys/fs/fuse/fuse_vnops.c has 3 parameters and in the patch it has 2 (and it's not a changed line, so they should match).

I tried both on fresh 12.1 and on 12.1 updated to the last patch level (p5), both have the same problem.

What should I do to test it properly?

Comment 16 Alan Somers 2020-05-18 13:13:08 UTC

(In reply to MooseFS FreeBSD Team from comment #15)

The patch is intended for head, not stable/12.  Do you have a FreeBSD head VM?  If not, I can port the patch to stable/12.

Now we have a VM with head :)

I tested, it works. Thank you very much!

Question: how can we know which version of kernel will have this patch applied? Will it be available already in first release of 13.0? The reason I'm asking: we currently have a workaround for this problem in our code, but we would like to get rid of it sooner rather than later. We can check kernel version during startup (mounting) and not apply the workaround if kernel/system version is greater than X, we just need to know X.

Comment 18 Alan Somers 2020-05-22 14:25:57 UTC

(In reply to MooseFS FreeBSD Team from comment #17)
Glad it works.  I'll commit it shortly.  Regarding availability, yes it will be available in 13.0-RELEASE.  Barring complications, it will be available in 12.2-RELEASE, too.  For more fine-grained detection, here's what you should do:

* For 13.0-CURRENT, assume the fix is in place.  There is no need to support versions of head older than the latest.

* For 12-STABLE, you can look at the SVN revision number in the kern.version sysctl.  For compile-time detection, you can check __FreeBSD_version in /usr/include/sys/param.h .  I'm not going to bump that version as part of this commit, but something usually increases it once a week or so.

One more question: did you mother, Mrs. Team, name you "MooseFS FreeBSD", or is that an alias?  If so, shall I credit the alias in the commit message, or is there a different name you would like for me to use?

Comment 19 commit-hook 2020-05-22 18:12:12 UTC

A commit references this bug:

Author: asomers
Date: Fri May 22 18:11:20 UTC 2020
New revision: 361401
URL: https://svnweb.freebsd.org/changeset/base/361401

Log:
  Fix issues with FUSE_ACCESS when default_permissions is disabled

  This patch fixes two issues relating to FUSE_ACCESS when the
  default_permissions mount option is disabled:

  * VOP_ACCESS() calls with VADMIN set should never be sent to a fuse server
    in the form of FUSE_ACCESS operations. The FUSE protocol has no equivalent
    of VADMIN, so we must evaluate such things kernel-side, regardless of the
    default_permissions setting.

  * The FUSE protocol only requires FUSE_ACCESS to be sent for two purposes:
    for the access(2) syscall and to check directory permissions for
    searchability during lookup. FreeBSD sends it much more frequently, due to
    differences between our VFS and Linux's, for which FUSE was designed. But
    this patch does eliminate several cases not required by the FUSE protocol:

    * for any FUSE_*XATTR operation
    * when creating a new file
    * when deleting a file
    * when setting timestamps, such as by utimensat(2).

  * Additionally, when default_permissions is disabled, this patch removes one
    FUSE_GETATTR operation when deleting a file.

  PR:		245689
  Reported by:	MooseFS FreeBSD Team <freebsd@moosefs.pro>
  Reviewed by:	cem
  MFC after:	2 weeks
  Differential Revision:	https://reviews.freebsd.org/D24777

Changes:
  head/sys/fs/fuse/fuse_internal.c
  head/sys/fs/fuse/fuse_vnops.c
  head/tests/sys/fs/fusefs/access.cc
  head/tests/sys/fs/fusefs/rename.cc
  head/tests/sys/fs/fusefs/rmdir.cc
  head/tests/sys/fs/fusefs/unlink.cc
  head/tests/sys/fs/fusefs/utils.cc
  head/tests/sys/fs/fusefs/utils.hh
  head/tests/sys/fs/fusefs/xattr.cc

(In reply to Alan Somers from comment #18)

Alan,
Indeed, this is an alias (mainly for maintaining MooseFS ports for FreeBSD and freebsd@moosefs.pro is set as a maintainer). It also allows a few people at our team be notified about updates. Both this issue and 245688 was raised by Agata Kruszona-Zawadzka <chogata@moosefs.pro>.

We will discuss it internally and we may limit usage of freebsd@moosefs.pro account to raising bug related to MooseFS's ports updates. Other issues could be raised by specific people and freebsd@moosefs.pro alias could be CC'd in such case.

Thanks for pointing this out,
Piotr

Comment 21 commit-hook 2020-06-12 20:32:56 UTC

A commit references this bug:

Author: asomers
Date: Fri Jun 12 20:32:29 UTC 2020
New revision: 362116
URL: https://svnweb.freebsd.org/changeset/base/362116

Log:
  MFC r361401:

  Fix issues with FUSE_ACCESS when default_permissions is disabled

  This patch fixes two issues relating to FUSE_ACCESS when the
  default_permissions mount option is disabled:

  * VOP_ACCESS() calls with VADMIN set should never be sent to a fuse server
    in the form of FUSE_ACCESS operations. The FUSE protocol has no equivalent
    of VADMIN, so we must evaluate such things kernel-side, regardless of the
    default_permissions setting.

  * The FUSE protocol only requires FUSE_ACCESS to be sent for two purposes:
    for the access(2) syscall and to check directory permissions for
    searchability during lookup. FreeBSD sends it much more frequently, due to
    differences between our VFS and Linux's, for which FUSE was designed. But
    this patch does eliminate several cases not required by the FUSE protocol:

    * for any FUSE_*XATTR operation
    * when creating a new file
    * when deleting a file
    * when setting timestamps, such as by utimensat(2).

  * Additionally, when default_permissions is disabled, this patch removes one
    FUSE_GETATTR operation when deleting a file.

  PR:		245689
  Reported by:	MooseFS FreeBSD Team <freebsd@moosefs.pro>
  Reviewed by:	cem
  Differential Revision:	https://reviews.freebsd.org/D24777

Changes:
_U  stable/12/
  stable/12/sys/fs/fuse/fuse_internal.c
  stable/12/sys/fs/fuse/fuse_vnops.c
  stable/12/tests/sys/fs/fusefs/access.cc
  stable/12/tests/sys/fs/fusefs/rename.cc
  stable/12/tests/sys/fs/fusefs/rmdir.cc
  stable/12/tests/sys/fs/fusefs/unlink.cc
  stable/12/tests/sys/fs/fusefs/utils.cc
  stable/12/tests/sys/fs/fusefs/utils.hh
  stable/12/tests/sys/fs/fusefs/xattr.cc

Comment 22 Kubilay Kocak 2020-07-14 05:38:43 UTC

^Triage: Track merge