fusefs: don't panic if FUSE_GETATTR fails durint VOP_GETPAGES

[asomers]

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

[emaste]

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

[markj]

tests/sys/fs/fusefs/read.cc

656

Did you mean to leave this in?

[asomers]

tests/sys/fs/fusefs/read.cc

656

Did you mean to leave this in?

Oops.

[asomers]

• Delete unintended code

[kib]

sys/kern/vfs_bio.c

5284 ↗

(On Diff #95267)

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

[asomers]

sys/kern/vfs_bio.c

5284 ↗

(On Diff #95267)

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

[kib]

sys/kern/vfs_bio.c

5284 ↗

(On Diff #95267)

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.

[kib]

sys/kern/vfs_bio.c

5284 ↗

(On Diff #95267)

D31998

[kib]

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.

[asomers]

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.

[kib]

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).

[asomers]

• Use setjmp/longjmp to handle SIGBUS instead of mlock

[kib]

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.

[asomers]

• Verify the address of the faulting data

[kib]

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]

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.

[asomers]

• Use a memory fence after the faulting instruction

[kib]

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

[asomers]

• Correct memory fence placement

[asomers]

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.

[kib]

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]

• 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

[asomers]

@kib I rebased the changes.

[kib]

sys/fs/fuse/fuse_vnops.c

2211

Why not err?

[asomers]

sys/fs/fuse/fuse_vnops.c

2211

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?

[kib]

sys/fs/fuse/fuse_vnops.c

2211

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.