[pull] master from libfuse:master#53
Merged
Merged
Conversation
The per-inode state is split across two mutexes for concurrency: Inode::mutex (content, mtime, dentries, nlookup) and Inode::attr_mutex (mode, uid, gid, atime). A chmod/chown/atime update should not serialize behind a write() resizing the content vector, so the two domains keep separate locks. The original split was inconsistent, which was the actual bug: write_content() updated content+mtime under mutex while truncate() did the same under attr_mutex, and get_attr() read content.size() under attr_mutex. So content and mtime were each touched under both locks depending on the path, and a getattr could race a concurrent write resizing the content. get_mode()/get_mtime() read mutable fields with no lock at all; get_children() and is_empty() iterated the dentry vector unlocked. Give each field a single owning lock. content, mtime, size: mutex (truncate and set_mtime move onto it). mode, uid, gid, atime: attr_mutex. ctime is set once at construction and never mutated, so it stays lock-free. get_attr() needs a consistent cross-domain snapshot, so it is the one place that holds both, always in the order mutex -> attr_mutex; no setter holds both, so there is no ABBA risk. memfs_setattr drops its outer lock()/unlock() because the setters and the final get_attr now self-lock; keeping it would deadlock the non-recursive mutex. A concurrent getattr may then observe a partially-applied setattr, which is acceptable for an example filesystem. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
Inodes::size() locked a separate std::mutex that protected nothing while reading the inode map, which is guarded by inodes_mutex. Concurrent create/erase could therefore reshape the map under an unsynchronized size() read. Take a shared lock on inodes_mutex instead and drop the stray mutex member, which had no other use. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
memfs_mkdir's out_cleanup path called erase_locked() without holding
inodes_mutex, an unsynchronized map erase on the failure path. Use the
locking erase() variant.
memfs_create ignored add_child()'s return value. Its early find_child()
EEXIST check and the later add_child() are two separate parent-lock
acquisitions, so two concurrent create("foo") calls can both pass
the check, both allocate distinct inodes, and both reach add_child();
the loser gets EEXIST, which was silently discarded. That leaked the
loser's Dentry and orphaned its inode (left in the map, no link) while
still replying success. Capture the return and, on failure, delete
the dentry and erase the orphan inode before replying the error. The
inode was never linked or returned to the kernel, so the unconditional
erase() is correct.
Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
memfs_link took inodes_mutex exclusively via Inodes.lock(), then resolved the source and parent inodes with Inodes.find(), which takes a shared lock on the same mutex -- a self-deadlock on a non-recursive shared_mutex. It then called parent_inode->add_child(), which re-locks the parent inode mutex already held via parent_inode->lock() -- a second self-deadlock. The new Dentry was owned by a unique_ptr whose raw pointer was handed to the parent and then freed at scope exit, leaving the parent with a dangling dentry. Resolve both inodes with find_locked() (inodes_mutex already held) and link with add_child_locked() (parent inode mutex already held). Heap-allocate the Dentry and hand ownership to the parent, which frees it in remove_child(), matching create()/mkdir(). On add_child_locked() failure, delete the dentry and undo the inc_nlink() before replying the error. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
Inodes::find() returned a raw Inode* after releasing its shared lock, so every handler dereferenced an inode with nothing preventing a concurrent forget from erasing and deleting it -- a use-after-free. Related defects: forget erased an inode at nlookup==0 regardless of nlink, freeing still-linked inodes; readdir dereferenced raw Dentry pointers snapshotted at opendir; lookup dereferenced a Dentry after dropping the parent lock; forget_multi resolved and decremented without a lock and never erased at zero. Make the inode table own std::shared_ptr<Inode> and have find()/find_locked()/create() return shared_ptr, so a handler holds a strong reference for the whole operation. A directory link (Dentry) now holds a shared_ptr too, so a linked inode cannot be freed while reachable. get_children()/DirHandle snapshot strong references, fixing the readdir UAF. lookup copies the child's shared_ptr under the parent lock before using it. An inode is erased only once it has no kernel references and no links (nlookup==0 && nlink==0); forget and the rewritten forget_multi both apply that guard under inodes_mutex. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
memfs_rename locked the source then destination directory positionally. Two concurrent renames in opposite directions could therefore deadlock if the global inodes_mutex were ever relaxed to allow them to run in parallel. Today the exclusive Inodes.lock() held for the whole operation serializes them, so the deadlock is masked, but the positional order is a latent hazard. Order the two directory locks by inode number -- always lock the lower ino first -- so any two renames acquire the per-directory locks in the same order. The ordering locals are declared with the other rename locals, before the first goto, because the validation-failure goto would otherwise cross an initialized declaration into its scope. The operation body still acts on the directories by role; both are locked regardless of which was taken first. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
memfs_link replies with fuse_reply_entry, which hands the kernel a new lookup reference on the inode, but it only bumped nlink and never bumped nlookup. Every other entry-replying handler (lookup, create, mkdir) accounts for that reference. Without it, the kernel's accumulated forget count for a hard-linked inode exceeds nlookup, and dec_lookup() underflows and panics with "Lookup count mismatch detected" -- readily reproduced by a concurrent stress loop that hard-links files. Increment nlookup on the successful link reply. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
When memfs_rename overwrites an existing target, it called remove_child(newname) -- which deletes the matched Dentry -- and then dereferenced existing_dentry again via existing_dentry->get_inode() to decrement the replaced inode's nlink, reading the freed Dentry: a use-after-free. Resolve the target inode before remove_child and use the saved pointer afterwards; the inode stays alive through the Inodes map reference. The same branch also decremented newparent's nlink twice when the target was a directory: once explicitly and again inside remove_child(), which already decrements a directory's parent. A directory-over-directory rename therefore left newparent's link count one too low (rmdir decrements the parent exactly once, via remove_child). Drop the redundant explicit decrement. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
For benchmarking the FUSE interface itself rather than memfs's std::vector content management, add a -o null_io mount option. Writes still consume the incoming buffer (folding it into a volatile sink so the reads are not elided) but discard it; reads return random data served from a scratch buffer filled once at startup. The content store is never touched on either path. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
Add a -o fuse_dio mount option that, on open and create, sets fi->direct_io and fi->parallel_direct_writes. parallel_direct_writes only takes effect alongside direct_io, so both are enabled together under the one option. Useful for benchmarking the direct-I/O path, where the kernel no longer serializes writes per inode and no longer clamps reads at i_size. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
memfs_write passed offset and size straight to Inode::write_content, which computes offset + size to size the content vector. A negative offset converts to a huge unsigned value in that expression and aborts the resize; an end position past the vector's index ceiling truncates on the resize/iterator arithmetic (size_t/ptrdiff_t), which on a 32-bit build is below the off_t range the kernel can hand us. memfs_read only guarded offset >= content_size(). On a 64-bit build the unsigned compare incidentally rejects a negative offset, but on 32-bit the comparison is signed, so a negative offset slips through, truncates in (size_t)offset, and indexes before the content buffer. Validate at both handlers before touching the content store: reject a negative offset with EINVAL, and in write reject an end position past PTRDIFF_MAX (the std::vector size/index limit on both 32- and 64-bit builds) with EFBIG. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
memfs only created regular files (via create) and directories (via mkdir), so making a FIFO, socket, or device node failed with ENOSYS. Add a mknod handler so these node types can be created. The new dev_t rdev field on Inode stores the device number for character and block special files and is reported back through get_attr's st_rdev. The type bits in the mode argument select the node kind, so the same handler covers all mknod-able types. Signed-off-by: Bernd Schubert <bernd@bsbernd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )