Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Moderate issues remain in stat flags and hung-FUSE test timeouts; a naming nit also remains.
Review effort: Lite
Findings: None
What changed in this PR
This pull request hardens lsfd against hung filesystems using non-synchronizing statx, map_files, and a new abort option.
Changes:
- Adds shared safe-stat helpers and
lsfdstat wrappers. - Adds
-b/--abort-if-blockableand map-files inspection. - Adds documentation, completion support, CI dependencies, and hung-FUSE regression tests.
Outstanding comments request AT_NO_AUTOMOUNT, bounded execution in both hung-FUSE test invocations, and clearer capability-check naming.
| File | Summary |
|---|---|
tests/ts/lsfd/option-abort-if-blockable |
Tests abort behavior when non-blocking statx is unavailable. |
tests/ts/lsfd/mkfds-fuse-hungfs |
Tests file and mapping inspection on hung FUSE filesystems. |
tests/helpers/test_sysinfo.c |
Detects statx support. |
tests/expected/lsfd/option-abort-if-blockable |
Expected abort-option output. |
tests/expected/lsfd/mkfds-fuse-hungfs-mem |
Expected mapped-file output. |
tests/expected/lsfd/mkfds-fuse-hungfs-fd |
Expected file-descriptor output. |
lsfd-cmd/util.c |
Implements statx capability checks and wrappers. |
lsfd-cmd/sock.c |
Uses non-blocking socket statistics. |
lsfd-cmd/sock-xinfo.c |
Uses non-blocking statistics for namespace data. |
lsfd-cmd/lsfd.h |
Declares new APIs and status code. |
lsfd-cmd/lsfd.c |
Adds option handling and map_files inspection. |
lsfd-cmd/lsfd.1.adoc |
Documents the new option and behavior. |
lsfd-cmd/file.c |
Updates file metadata lookups. |
lsfd-cmd/cdev.c |
Uses non-blocking descriptor statistics. |
libmount/src/utils.c |
Reuses the shared safe-stat helper. |
lib/fileutils.c |
Implements safe statx helpers and conversion. |
include/fileutils.h |
Declares stat helpers. |
bash-completion/lsfd |
Completes the new option. |
.github/workflows/cibuild-setup-ubuntu.sh |
Adds FUSE development support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
50f20d5 to
f67da3e
Compare
…_stat libmount has safe_stat() which uses statx(2) with AT_STATX_DONT_SYNC to retrieve file information without blocking on unresponsive remote or FUSE file systems. Extract the non-blocking statx logic into a common ul_safe_stat() helper in include/fileutils.h and lib/fileutils.c. Reimplement libmount's safe_stat (renamed to safer_stat) to call ul_safe_stat(), keeping the fallback to fstatat() or stat() when statx is unsupported. Addresses: util-linux#4586 (comment) Signed-off-by: Masatake YAMATO <yamato@redhat.com>
…stat() Introduce ul_statx_to_stat() to convert struct statx into struct stat. Extend ul_safe_stat() with an all_basic parameter so callers can request either partial attributes (STATX_TYPE | STATX_MODE | STATX_INO) or full basic attributes (STATX_BASIC_STATS), as required by lsfd. Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Introduce lsfd_stat(), lsfd_path_stat(), lsfd_path_statf(), and lsfd_fstat() wrappers as a preparation step for introducing non-blocking file inspection. At startup, lsfd probes whether statx(2) is supported. When supported, these wrappers invoke statx(2) / ul_path_statx() and convert the result back to struct stat. Otherwise, they call traditional stat(2) / fstat(2). Signed-off-by: Masatake YAMATO <yamato@redhat.com>
…ppers Use AT_STATX_DONT_SYNC in lsfd_path_stat() and lsfd_fstat(), and utilize ul_safe_stat() in lsfd_stat() (which indirectly introduces AT_STATX_DONT_SYNC), to avoid synchronizing file attributes with remote servers or FUSE file systems, reducing the risk of hanging when inspecting file descriptors. In addition, specify AT_NO_AUTOMOUNT in lsfd_path_stat() to prevent triggering automounts and blocking on automounted (e.g. autofs) paths. Also add -b/--abort-if-blockable option to exit immediately with status 2 (LSFD_EX_NONBLOCK_UNAVAIL) if AT_STATX_DONT_SYNC is not supported by the kernel or the build. Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Verify that lsfd with -b/--abort-if-blockable exits with status 2 (LSFD_EX_NONBLOCK_UNAVAIL) and prints an error message when statx(2) with AT_STATX_DONT_SYNC is unsupported (simulated via enosys helper). Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Add tests/ts/lsfd/mkfds-fuse-hungfs to verify that lsfd does not hang when inspecting a file descriptor open on a stalled FUSE file system. To avoid test suite hangs on older kernels or platforms where statx(2) with AT_STATX_DONT_SYNC is unsupported, add "statx-dontsync-ok" helper to tests/helpers/test_sysinfo.c and skip the test when unsupported. Also add libfuse-dev to .github/workflows/cibuild-setup-ubuntu.sh so that FUSE-related tests are automatically built and executed in GitHub Actions CI. Signed-off-by: Masatake YAMATO <yamato@redhat.com>
…ings In parse_maps_line(), prefer inspecting memory-mapped files via /proc/PID/map_files/ magic symlinks rather than pathnames from /proc/PID/maps. Using map_files directly references the kernel struct file via procfs, bypassing pathname resolution (link_path_walk) and avoiding potential hangs in dentry revalidation on unresponsive file systems (such as FUSE or NFS). If map_files is not accessible (e.g. unprivileged execution or CONFIG_CHECKPOINT_RESTORE disabled), fall back to stat-by-pathname from /proc/PID/maps, unless -b/--abort-if-blockable is specified. Signed-off-by: Masatake YAMATO <yamato@redhat.com>
…filesystem Add "mem" subtest to tests/ts/lsfd/mkfds-fuse-hungfs to verify that lsfd does not hang when inspecting memory-mapped files backed by a stalled FUSE file system. Signed-off-by: Masatake YAMATO <yamato@redhat.com>
f67da3e to
cf1f39b
Compare
|
Thanks, this is a good improvement and the lsfd side looks solid. One note up front so the request below doesn't read as criticism: the non-blocking statx logic you extracted is my own old libmount code, and you moved it faithfully -- nothing wrong on your side. Seeing it lifted out of libmount context is what made me realise we can do better than the original. So I'd like to split this. The first two commits are generic API I'm going to implement#if defined(HAVE_STATX) && defined(HAVE_STRUCT_STATX)
# define UL_STATX_ESSENTIAL (STATX_TYPE | STATX_MODE | STATX_INO)
# define UL_STATX_BASIC (STATX_BASIC_STATS)
extern void ul_statx_to_stat(const struct statx *stx, struct stat *st);
#else
# define UL_STATX_ESSENTIAL 0
# define UL_STATX_BASIC 0
#endif
extern int ul_safe_stat(const char *target, struct stat *st, int nofollow,
unsigned int mask);Three differences from the current version:
libmount will then call Notes on the lsfd partNothing blocking, CI is all green.
The commit split, the tests (nice use of the — assisted by Claude Code |
Move the non-blocking statx() based stat() alternative out of libmount,
so that other tools can use it too. lsfd needs it to avoid blocking on
hung filesystems.
ul_safe_statx() is the primitive; it adds AT_STATX_DONT_SYNC and
AT_NO_AUTOMOUNT (and AT_EMPTY_PATH for an empty path) to the caller's
flags, so there is one place that knows what "do not block" means.
ul_safe_stat() converts the result to struct stat. It's an improved
version of the original libmount code:
* the caller says what it needs by a statx(2) attribute mask, see
UL_STATX_{ESSENTIAL,BASIC}. These are plain statx masks, so they can
be composed, for example UL_STATX_ESSENTIAL | STATX_ATIME.
UL_STATX_ESSENTIAL is always added, so 0 is a valid request for the
minimum.
* ul_statx_to_stat() honours stx_mask. statx(2) is not obliged to
return everything we ask for, and with AT_STATX_DONT_SYNC that's
exactly the case we care about. The original code copied the fields
unconditionally, which could report garbage as a valid attribute.
* the optional @Retmask returns what the kernel really provided, so
the caller can tell which fields in the struct stat are usable.
* it fails with EOPNOTSUPP when the kernel does not provide the
essential attributes, so the caller can fall back to stat().
Both functions return 0 or a negative errno, and they set errno too.
ul_safe_statx() always zeroizes the struct statx, so the caller never
sees stale data on an error path. The HAVE_UL_SAFE_STAT{,X} macros hide
the HAVE_STATX, HAVE_STRUCT_STATX and AT_* feature tests from the
callers.
libmount's safe_stat() and get_mnt_id() now use the new functions. Note
that this changes mnt_id_from_path(), mnt_id_from_fd() and the internal
mnt_safe_stat()/mnt_safe_lstat() to return -errno rather than -1. All
in-tree callers only test for non-zero, and the public functions are
documented as "<0 on error" only.
get_mnt_id() honours stx_mask now too. The kernel silently ignores
unsupported mask bits rather than failing, so the old "errno == EINVAL"
probe for STATX_MNT_ID_UNIQUE never triggered; statx(2) returns EINVAL
only for STATX__RESERVED. On kernels older than 6.8 that made
mnt_id_from_path() report the legacy non-unique mount ID as if it were
the unique one. Both IDs are returned only when the kernel advertises
them in stx_mask, otherwise -ENOSYS -- which is what mnt_fs_fetch_ids()
already expects for its fallback to the old mount ID.
Add test_fileutils --safe-stat and tests/ts/misc/safe-stat. The stat
attributes differ between systems, so the test program does not print
them; it compares the ul_safe_stat() result with the classic stat() and
prints OK/FAILED per attribute. The reported attributes depend on the
requested mask only, not on how generous the kernel is.
Addresses: util-linux#4647
Signed-off-by: Karel Zak <kzak@redhat.com>

lsfd: do not block on hung file systems (statx dont_sync & map_files)
This patch set improves
lsfd's resilience against stalled or hung file systems (such as unresponsive FUSE or NFS mounts), preventinglsfdfrom blocking indefinitely while inspecting file descriptors and memory mappings.Summary of Changes
1. Extract non-blocking
statxfrom libmount asul_safe_stat(refactor)statxlogic usingAT_STATX_DONT_SYNCfrom libmount into a commonul_safe_stat()helper ininclude/fileutils.handlib/fileutils.c.safe_stat(renamed tosafer_stat) to callul_safe_stat(), keeping the fallback tofstatat()orstat()whenstatxis unsupported.2. Introduce
ul_statx_to_stat()and addall_basictoul_safe_stat()(enhance)ul_statx_to_stat()helper to convertstruct statxtostruct stat.ul_safe_stat()with anall_basicparameter to allow callers (such aslsfd) to request full basic stats (STATX_BASIC_STATS).3. Stat wrappers with
AT_STATX_DONT_SYNClsfd_stat(),lsfd_fstat(),lsfd_path_stat(), andlsfd_path_statf()wrappers that usestatx(2)withAT_STATX_DONT_SYNC.-b/--abort-if-blockableoption to exit immediately (status 2) if non-blocking attribute retrieval is unsupported by the kernel or build.-bmode, skip resolving mountpoint pathnames from/proc/pid/mountinfoto prevent potential pathname resolution hangs.tests/ts/lsfd/option-abort-if-blockabletest case to verify thatlsfd -bexits with status 2 whenstatxis unavailable.4. Inspect memory mappings via
map_filesparse_maps_line(), prefer inspecting memory-mapped files via/proc/PID/map_files/magic symlinks rather than pathnames from/proc/PID/maps.struct filevia procfs bypasses pathname resolution (link_path_walk) and prevents hangs caused by dentry revalidation (d_revalidate) on hung file systems.-bis specified.5. Regression tests with
hungfshungfstesting infrastructure intest_mkfds, add regression test cases:tests/ts/lsfd/mkfds-fuse-hungfs(fdsubtest): verify inspection of open files on a hung FUSE mount point.tests/ts/lsfd/mkfds-fuse-hungfs(memsubtest): verify inspection of mmap'ed files on a hung FUSE mount point.Commits (8)
fileutils, libmount: (refactor) extract non-blocking statx as ul_safe_statfileutils: introduce ul_statx_to_stat() and add all_basic to ul_safe_stat()lsfd: (refactor) introduce stat wrappers using statx as preparationlsfd: do not synchronize with remote or FUSE file systems in stat wrapperstests: (lsfd) add test case for -b/--abort-if-blockable optiontests: (lsfd) add test case for inspecting file in hung FUSE filesystemlsfd: prefer map_files over maps pathname when inspecting memory mappingstests: (lsfd) add test case for inspecting mmap'ed file in hung FUSE filesystemDiffstat