Page MenuHomeFreeBSD

fusefs: fix a kernel panic regarding SCM_RIGHTS
ClosedPublic

Authored by asomers on Fri, Sep 19, 5:04 PM.
Tags
None
Referenced Files
F130386725: D52625.id162428.diff
Sun, Sep 28, 10:18 PM
F130376687: D52625.id162446.diff
Sun, Sep 28, 8:12 PM
Unknown Object (File)
Sun, Sep 28, 6:31 PM
Unknown Object (File)
Sun, Sep 28, 1:28 PM
Unknown Object (File)
Thu, Sep 25, 10:16 PM
Unknown Object (File)
Thu, Sep 25, 5:50 PM
Unknown Object (File)
Sat, Sep 20, 12:30 PM
Unknown Object (File)
Sat, Sep 20, 11:48 AM
Subscribers

Details

Summary

If the last copy of an open file resides within the socket buffer of a
unix-domain socket, then VOP_CLOSE will be called with no thread
information. Fix fusefs to handle that case, and add a regression test.

PR: 289686
Reported by: iron.udjin@gmail.com
MFC after: 1 week
Sponsored by: ConnectWise

Test Plan

Test case added

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 67176
Build 64059: arc lint + arc unit

Event Timeline

  • Also add a test case for writes to a file that lies in a sockbuf

Although triggered by unix(4), but really requires VFS(9) knowledge. So my review isn't worth much. Definitely is going to fix the panic, though.

This revision is now accepted and ready to land.Fri, Sep 19, 7:06 PM

I'm sure this will fix the panic, but I can't easily see if it's correct. For instance, if the unix domain socket code does asynchronous fd reclamation using unp_gc_task, the current thread will be some taskqueue thread. Its PID will probably be zero (but this is an implementation detail). I see some special handling for PID 0 e.g. in fuse_filehandle_get_anyflags(). Will this work the way you want in that case?

I'm sure this will fix the panic, but I can't easily see if it's correct. For instance, if the unix domain socket code does asynchronous fd reclamation using unp_gc_task, the current thread will be some taskqueue thread. Its PID will probably be zero (but this is an implementation detail). I see some special handling for PID 0 e.g. in fuse_filehandle_get_anyflags(). Will this work the way you want in that case?

The FUSE protocol says that every request sent to the server must include the pid, gid, and uid responsible. That's so the server can do access control itself, if it wants to. The entire concept of a fuse_filehandle exists solely to support that requirement. However, it's bullshit. It's a terrible antifeature, because there are many situations in which the kernel can't possibly set those accurately. Most obviously, if a write is coming from the writeback cache. So FUSE daemons can never rely on those fields. Yet, we try to set them anyway, for compliance's sake.

Regarding this particular bug, I'm happy as long as it does not panic. This is another one of those cases where we can't possibly set the uid, gid, or pid to their "correct" values.

There's another stupid detail in the protocol too: fhid. When the kernel does FUSE_OPEN, the server may return an fhid. It's an opaque 64-bit number. But we're supposed to set it in any future request that involves the same open file. It's easily possible for two processes (or even the same process) to open a file multiple times, and we're supposed to keep track of which fhid is needed by each. That's very difficult to due given the design of our VHS. It's easy in Linux, which is probably why it was added to the fuse protocol in the first place. The main purpose, again, is server-side access control. But we have a little optimization. If two different file descriptors have the same uid, pid, gid, and access mode, then we only store one file handle for both. That's because the server almost certainly would treat them the same, for access control decisions. IIRC, that "pid == 0" expression is there precisely because of situations where we don't know the true pid. In such a case, we don't want fuse_filehandle_get_anyflags to match on pid.

So to reiterate, I'm happy as long as it doesn't panic. I'm not upset that the pid field won't be "correct".

I'm sure this will fix the panic, but I can't easily see if it's correct. For instance, if the unix domain socket code does asynchronous fd reclamation using unp_gc_task, the current thread will be some taskqueue thread. Its PID will probably be zero (but this is an implementation detail). I see some special handling for PID 0 e.g. in fuse_filehandle_get_anyflags(). Will this work the way you want in that case?

The FUSE protocol says that every request sent to the server must include the pid, gid, and uid responsible. That's so the server can do access control itself, if it wants to. The entire concept of a fuse_filehandle exists solely to support that requirement. However, it's bullshit. It's a terrible antifeature, because there are many situations in which the kernel can't possibly set those accurately. Most obviously, if a write is coming from the writeback cache. So FUSE daemons can never rely on those fields. Yet, we try to set them anyway, for compliance's sake.

Regarding this particular bug, I'm happy as long as it does not panic. This is another one of those cases where we can't possibly set the uid, gid, or pid to their "correct" values.

There's another stupid detail in the protocol too: fhid. When the kernel does FUSE_OPEN, the server may return an fhid. It's an opaque 64-bit number. But we're supposed to set it in any future request that involves the same open file. It's easily possible for two processes (or even the same process) to open a file multiple times, and we're supposed to keep track of which fhid is needed by each. That's very difficult to due given the design of our VHS. It's easy in Linux, which is probably why it was added to the fuse protocol in the first place. The main purpose, again, is server-side access control. But we have a little optimization. If two different file descriptors have the same uid, pid, gid, and access mode, then we only store one file handle for both. That's because the server almost certainly would treat them the same, for access control decisions. IIRC, that "pid == 0" expression is there precisely because of situations where we don't know the true pid. In such a case, we don't want fuse_filehandle_get_anyflags to match on pid.

So to reiterate, I'm happy as long as it doesn't panic. I'm not upset that the pid field won't be "correct".

Ok! Thank you for the detailed explanation.

sys/fs/fuse/fuse_vnops.c
798

I'd add a comment explaining why we do this and noting it might be some async kernel context rather than a user thread.

Thanks for explanation, Alan!

Apologies for dropping this on the review after-the-fact, but I was looking over the unionfs version of the bug Alan filed (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=289700), and a couple of questions jumped out at me:

  1. @glebius Is there a reason we don't just pass curthread from unp_discard()? I could imagine there might be a good reason we want to pass NULL instead of curthread, but if so that probably should be documented in VOP_OPENCLOSE.9 to reduce the likelihood of foot-shooting from FS implementations.
  1. Would it be at all useful for FUSE if we were to change VOP_CLOSE (and other vnops that typically operate against file objects, eg VOP_READ/VOP_WRITE/VOP_IOCTL etc.) to also take the struct file* (if one exists, NULL otherwise) as is already done for VOP_OPEN? I realize that would be quite a lot of churn, so perhaps not worth the effort, but I also suspect something like that could be useful in cleaning up some of the especially-wrong parts of unionfs (@olce may have different thoughts here, if so I'll defer to him)