Page MenuHomeFreeBSD

fusefs: don't panic if FUSE_GETATTR fails durint VOP_GETPAGES
ClosedPublic

Authored by asomers on Sep 16 2021, 8:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 4:51 PM
Unknown Object (File)
Sun, Nov 24, 7:24 AM
Unknown Object (File)
Fri, Nov 22, 1:13 PM
Unknown Object (File)
Fri, Nov 15, 5:49 AM
Unknown Object (File)
Oct 23 2024, 9:24 AM
Unknown Object (File)
Oct 1 2024, 3:45 AM
Unknown Object (File)
Sep 23 2024, 10:33 PM
Unknown Object (File)
Sep 23 2024, 3:53 PM
Subscribers

Details

Summary

During VOP_GETPAGES, fusefs needs to determine the file's length, which
could require a FUSE_GETATTR operation. If that fails, it's better to
SIGBUS than panic.

MFC after: 2 weeks
Sponsored by: Axcient

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41582
Build 38471: arc lint + arc unit

Event Timeline

Looks reasonable to me but let me tag @kib or @markj for changes to vfs_bio.c

markj added inline comments.
tests/sys/fs/fusefs/read.cc
656

Did you mean to leave this in?

This revision is now accepted and ready to land.Sep 16 2021, 9:40 PM
tests/sys/fs/fusefs/read.cc
656

Did you mean to leave this in?

Oops.

This revision now requires review to proceed.Sep 16 2021, 9:59 PM
sys/kern/vfs_bio.c
5284

error = 1 is rude. Why not set it to some reasonable errno, e.g. EIO?

sys/kern/vfs_bio.c
5284

Because it isn't an errno. It's really just used as a boolean. See line 5358.

sys/kern/vfs_bio.c
5284

It is errno, it contains errno value (right now from bread() return).

In fact, if get_blksize() needs to fail, it probably should have a different interface, returning error and taking bsize as pointer. I never supposed that such use might appear.

sys/kern/vfs_bio.c
5284
tests/sys/fs/fusefs/read.cc
778

You may install signal handler for limited scope with sigaction(2), which returns previous handler. Then at the end of the scope, another sigaction can restore the signal disposition.

Relying on mlock(2) there is IMO too fragile.

tests/sys/fs/fusefs/read.cc
778

You may install signal handler for limited scope with sigaction(2), which returns previous handler. Then at the end of the scope, another sigaction can restore the signal disposition.

Relying on mlock(2) there is IMO too fragile.

That's what I tried at first. It caught the signal fine. The problem is that there isn't a way to abort whatever operation caused the page fault. Instead, the page fault handler would just be invoked over and over. mlock is useful here because it actually returns when there is an error.

tests/sys/fs/fusefs/read.cc
778

You may install signal handler for limited scope with sigaction(2), which returns previous handler. Then at the end of the scope, another sigaction can restore the signal disposition.

Relying on mlock(2) there is IMO too fragile.

That's what I tried at first. It caught the signal fine. The problem is that there isn't a way to abort whatever operation caused the page fault. Instead, the page fault handler would just be invoked over and over. mlock is useful here because it actually returns when there is an error.

This is usually done with longjmp from the signal handler.

In fact signal handler with SA_SIGINFO would provide you more info about event, so for instance you can verify the faulting address, type of fault, and code location where the issue occurred. Unfortunately Mach VM pager interface is somewhat shallow, so we cannot add actual error that occured in the read attempt (you already see that with VM_PAGER_ERROR hiding any detailed errno).

  • Use setjmp/longjmp to handle SIGBUS instead of mlock

Don't you want to obtain siginfo_t and check that the SIGBUS signal was generated for access to p?

Use sighandler(2) with SA_SIGINFO for that.

  • Verify the address of the faulting data
tests/sys/fs/fusefs/read.cc
654

I would keep old SIGBUS disposition and restore it in teardown. I believe you said that GoogleTest is confused in cleanup if SIGBUS handler is redefined? If not, then this is not strictly needed but improves modularity of the code.

656

I believe to be formally correct, this code needs atomic_signal_fence(memory_order::seq_cst) before p access.

asomers added inline comments.
tests/sys/fs/fusefs/read.cc
654

No, I just meant that an unhandled SIGBUS confuses GoogleTest. By itself, GoogleTest does not alter the SIGBUS handler.

  • Use a memory fence after the faulting instruction
tests/sys/fs/fusefs/read.cc
657

The fence should be before faulting access, otherwise the handler is not guaranteed to see e.g. jmpbuf update

  • Correct memory fence placement
asomers added inline comments.
tests/sys/fs/fusefs/read.cc
657

Ahh, I thought the purpose of the fence was to ensure that the faulting access and the FAIL did not get reordered. Fixed.

I am fine with the test code, but other part of the patch needs update.

tests/sys/fs/fusefs/read.cc
657

Yes, signal fence is basically same as compiler memory barrier, it prevents compiler from reordering things past fence, which would otherwise be acceptable for normal control flow.

asomers marked an inline comment as done.
  • Delete unintended code
  • Use setjmp/longjmp to handle SIGBUS instead of mlock
  • Verify the address of the faulting data
  • Use a memory fence after the faulting instruction
  • Correct memory fence placement
kib added inline comments.
sys/fs/fuse/fuse_vnops.c
2215

Why not err?

This revision is now accepted and ready to land.Sep 21 2021, 6:49 PM
sys/fs/fuse/fuse_vnops.c
2215

Just because to the user's perspective anything that means their pages won't be ready looks like an I/O error. err, in this case, comes from the FUSE server, which could set it to anything it wants: EACCES, EBADF, or even something totally inappropriate like ENOTEMPTY. On the other hand, you could argue that the FUSE server knows better than we do. It's all pretty academic though, since vfs_bio_getpages ignores the precise value of the error anyway. What do you think?

sys/fs/fuse/fuse_vnops.c
2215

Yes, it is purely theoretical. I suggested err on principle of not loosing the information, at least not there.

Anyway, I accepted the review, do whatever you think is the best.