fuse: fix some subtle deadlocks and lock irregularities - #197
Merged
hbirth merged 5 commits intoAug 15, 2026
Conversation
fuse_write_inode() -> fuse_flush_times() pushes out the mtime/ctime that the kernel owns locally while the writeback cache is on. On the wire that request is indistinguishable from a userspace "touch -m": both arrive as SETATTR with FATTR_MTIME | FATTR_CTIME | FATTR_FH, because trust_local_cmtime makes iattr_to_fattr() send CTIME whenever the writeback cache is enabled. A server that wants to handle a cache flush differently from an explicit attribute change - skipping a cluster-wide lock, merging rather than overwriting - has no way to tell them apart. Add FATTR_WRITEBACK, a control bit in fuse_setattr_in.valid alongside the existing non-attribute bits FATTR_FH, FATTR_LOCKOWNER and FATTR_KILL_SUIDGID. It selects no attribute, it only states that the request originates from writeback. Bit 30 is used rather than the next free one. libfuse mirrors the wire bits into its own FUSE_SET_ATTR_* namespace, where bits 12 to 17 are already taken by library-internal flags, and it masks incoming requests against that namespace; a bit picked from the low end would collide there and need translating on the way in. Bit 30 is free on both sides and clear of the sign bit of the int that the libfuse setattr operation takes, so one value works end to end. The bit is negotiated at INIT time with FUSE_SETATTR_WRITEBACK and is only set on a connection whose server asked for it, so servers that do not know the bit never receive it. Only ->write_inode() is marked. The other kernel-initiated SETATTR, fuse_do_truncate() rolling back a failed extending direct-IO write, is deliberately left unmarked: it is a size correction rather than an attribute writeback, and conflating the two would make the flag ambiguous. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_reverse_inval_inode() invalidates with invalidate_inode_pages2_range(),
which waits out folios under writeback and launders dirty ones. Both need a
FUSE_WRITE reply, and while fi->writectr < 0 none can arrive:
fuse_flush_writepages() parks the request on fi->queued_writes until
fuse_release_nowrite().
A truncate holds that freeze across its whole SETATTR, and the server revokes
the truncated range from inside the SETATTR handler, so the notify blocks the
very thread that owes the reply lifting the freeze. generic/014 deadlocks
within seconds, in folio_wait_writeback() under fuse_launder_folio() under
fuse_reverse_inval_inode().
fuse_do_setattr() already states the rule ("Only call
invalidate_inode_pages2() after removing FUSE_NOWRITE, otherwise
fuse_launder_folio() would deadlock"). Give the notify path the same: while
frozen, use invalidate_mapping_pages(), which skips dirty and under-writeback
folios instead of waiting on them. The stale clean folios still go, the DLM
grant is revoked either way, and the freezes that span a request drop the
cache themselves when they finish.
Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_cache_write_iter() holds wb_inval_rwsem for read across the write, and the kiocb_modified() -> file_remove_privs() that precedes it runs under that same gate. Without handle_killpriv[_v2] the privilege kill asks the server (GETATTR, then SETATTR), and a server that invalidates the inode from inside such a handler blocks in percpu_down_write() draining the gate reader that is waiting for its reply. generic/193 hangs there. The gate only has to fence the page-cache dirtying, so run the write checks, the privilege kill and the timestamp update before entering it. task_io_account_write() stays behind the gate, so a write that the forced-DIO re-check reroutes is not counted twice. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
fuse_get_cache_mask() returns 0 once the connection has DLM, so every GETATTR reply overwrites i_size, mtime and ctime, and truncate_pagecache() then drops the tail the client still holds dirty. The only way a server can make that answer true is to revoke the client from inside the handler, which deadlocks against the coherency gate. A write grant already guarantees that no other node can touch the range, so keep the cached values while one is held: the size when the server reports less than i_size and [srv_size, i_size) is fully granted, mtime and ctime while the cache under the grant is still dirty. A remote truncate has to revoke first, so the smaller size that follows is applied as usual. The attribute-driven invalidation now keys off STATX_SIZE instead of the whole mask, so a reply that does shrink i_size still truncates the page cache when only the timestamps were served from the cache. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
Without handle_killpriv[_v2], fuse_setattr() kills the bits by asking the server, and fuse_do_setattr() freezes writepages around that SETATTR. fuse_set_nowrite() asserts BUG_ON(fi->writectr < 0) under fi->lock, which assumes the caller holds i_rwsem exclusive: with the writeback cache and DLM, buffered writes hold it only shared. Two writers to a suid file can then both pass dentry_needs_remove_privs() before either has cleared the bits, and the second one hits the assert. It oopses inside spin_lock(&fi->lock), so fi->lock stays held and the i_rwsem read count leaks: the inode wedges and the box follows. The race window is a full GETATTR plus SETATTR, so it is not narrow, and an unprivileged user can set the bit on a file it owns. Keep those writes off the writeback path, which is the one that relaxes i_rwsem to shared, the way handle_killpriv_v2 writes already are. Scoped to DLM connections, since every other configuration already holds i_rwsem exclusive for a buffered write, and only writes that still find the bits set pay for it. Note that the non-writeback path takes no DLM lock, so those writes leave clean folios in the page cache without a grant covering them. That gap already exists for handle_killpriv_v2 and is not addressed here. Signed-off-by: Horst Birthelmer <hbirthelmer@ddn.com>
hbirth
requested review from
achhenderson,
bsbernd,
cding-ddn,
hazhou-ddn and
yongzech
August 14, 2026 18:06
hbirth
merged commit Aug 15, 2026
7887305
into
DDNStorage:redfs-ubuntu-hwe-6.17.0-16.16-24.04.1
2 checks passed
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fix some lock irregularities which most of them were introduced by our previous patches adding the rwsem to avoid serializing writer on inode lock.
Add the writeback flag to setattr to mark writeback attribute changes.