Page MenuHomeFreeBSD

Fix issues with FUSE_ACCESS when default_permissions is disabled
ClosedPublic

Authored by asomers on May 8 2020, 10:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 13 2024, 8:49 PM
Unknown Object (File)
Dec 22 2023, 11:00 PM
Unknown Object (File)
Dec 13 2023, 2:36 PM
Unknown Object (File)
Oct 10 2023, 10:49 PM
Unknown Object (File)
Sep 24 2023, 5:06 PM
Unknown Object (File)
Sep 24 2023, 5:06 PM
Unknown Object (File)
Sep 24 2023, 5:06 PM
Unknown Object (File)
Sep 24 2023, 12:47 PM
Subscribers

Details

Summary

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.

Reported-by: freebsd@MooseFS.pro

Test Plan

New test cases added. No regressions spotted by pjdfstest

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fix some comments, and remove files that shouldn't have been part of the review

There was a PR associated with this, right? Is it 245689? Edit: found the link back here in the last comment, so, yep.

In D24777#545536, @cem wrote:

There was a PR associated with this, right? Is it 245689? Edit: found the link back here in the last comment, so, yep.

Yes, that's correct.

The functional changes mostly look good. There's one logic case that I think is slightly wrong but the other comments are stylistic. I did not look at the C++ tests.

sys/fs/fuse/fuse_internal.c
218–219 ↗(On Diff #71576)

style nit: C++/C99 comments are discouraged by style(9) and do not seem to be the prevailing style in this file, either.

sys/fs/fuse/fuse_vnops.c
255–260 ↗(On Diff #71576)

This is now a weird fallthrough. I'd suggest just replacing /* FALLTHROUGH */ with return (0);.

1842 ↗(On Diff #71576)

This expression is spelled checkperm elsewhere in this function.

1842–1844 ↗(On Diff #71576)

I think you need a else err = 0; case here. There are paths that set err but do not return immediately.

(Or you could move the following if (err) return err; inside the bracketed condition and unconditionally tail-call fuse_internal_setattr.)

tests/sys/fs/fusefs/lookup.cc
42 ↗(On Diff #71576)

Oops, this wasn't supposed to be part of the review. I'll revert it.

Respond to cem's comments.

Also, remove the changes to lookup.cc, and move the access_vadmin dtrace probe
earlier.

This revision is now accepted and ready to land.May 10 2020, 7:09 PM