From 34f53e23aeb1b078a162fe1abf2c3116c3b09985 Mon Sep 17 00:00:00 2001 From: Dean Chen <862469039@qq.com> Date: Sat, 25 Jul 2026 18:14:07 +0500 Subject: [PATCH] collector/filesystem: fix readonly false positives under ro rootfs bind mount Since 65902fa (#3659) fixed the missing comma separator in mountOptionsString, isFilesystemReadOnly actually matches the per-mount "ro" flag. In the recommended containerized deployment the host root is bind-mounted read-only into the container (`-v /:/host:ro,rslave` with --path.rootfs=/host), so every host mount carries a per-mount "ro" flag in the exporter's mount namespace, and node_filesystem_readonly now reports 1 for host filesystems that are writable on the host. Only consider the superblock options, which are shared across mount namespaces, for mount points below --path.rootfs. Filesystems that really are read-only (superblock "ro", ext4 "emergency_ro") are still reported, and mounts outside the rootfs prefix keep the per-mount semantics from #3484. Fixes #3755 Signed-off-by: Dean Chen <862469039@qq.com> --- collector/filesystem_common.go | 4 ++ collector/filesystem_linux.go | 24 +++++-- collector/filesystem_linux_test.go | 69 +++++++++++++++++++ .../fixtures_bindmount_ro/proc/1/mountinfo | 6 ++ 4 files changed, 99 insertions(+), 4 deletions(-) create mode 100644 collector/fixtures_bindmount_ro/proc/1/mountinfo diff --git a/collector/filesystem_common.go b/collector/filesystem_common.go index 1cd4f3dc71..bdfacab2fe 100644 --- a/collector/filesystem_common.go +++ b/collector/filesystem_common.go @@ -81,6 +81,10 @@ type filesystemCollector struct { type filesystemLabels struct { device, mountPoint, fsType, mountOptions, superOptions, deviceError, major, minor string + // hostMount is true when the mount point was found below the configured + // --path.rootfs prefix, i.e. it is a host mount observed from within a + // container through a bind mount of the host root. + hostMount bool } type filesystemStats struct { diff --git a/collector/filesystem_linux.go b/collector/filesystem_linux.go index 038ff7970b..3f48cbe4cc 100644 --- a/collector/filesystem_linux.go +++ b/collector/filesystem_linux.go @@ -130,6 +130,7 @@ func (c *filesystemCollector) processStat(labels filesystemLabels) filesystemSta // twice, with different options (metrics would be recorded multiple times). labels.mountOptions = "" labels.superOptions = "" + labels.hostMount = false if err != nil { labels.deviceError = err.Error() @@ -208,15 +209,18 @@ func parseFilesystemLabels(mountInfo []*procfs.MountInfo) ([]filesystemLabels, e mount.MountPoint = strings.ReplaceAll(mount.MountPoint, "\\040", " ") mount.MountPoint = strings.ReplaceAll(mount.MountPoint, "\\011", "\t") + mountPoint := rootfsStripPrefix(mount.MountPoint) + filesystems = append(filesystems, filesystemLabels{ device: strings.ToValidUTF8(mount.Source, "�"), - mountPoint: strings.ToValidUTF8(rootfsStripPrefix(mount.MountPoint), "�"), + mountPoint: strings.ToValidUTF8(mountPoint, "�"), fsType: strings.ToValidUTF8(mount.FSType, "�"), mountOptions: mountOptionsString(mount.Options), superOptions: mountOptionsString(mount.SuperOptions), major: strconv.Itoa(major), minor: strconv.Itoa(minor), deviceError: "", + hostMount: mountPoint != mount.MountPoint, }) } @@ -227,14 +231,26 @@ func parseFilesystemLabels(mountInfo []*procfs.MountInfo) ([]filesystemLabels, e // if either mount or super options contain "ro" or the superblock contains "emergency_ro", // the filesystem is read-only func isFilesystemReadOnly(labels filesystemLabels) bool { - mountOptions := strings.Split(labels.mountOptions, ",") superOptions := strings.Split(labels.superOptions, ",") - if slices.Contains(mountOptions, "ro") || slices.Contains(superOptions, "ro") || slices.Contains(superOptions, "emergency_ro") { + // Super options are shared across mount namespaces: "ro" or + // "emergency_ro" there means the filesystem itself is read-only. + if slices.Contains(superOptions, "ro") || slices.Contains(superOptions, "emergency_ro") { return true } - return false + // For mount points below --path.rootfs the per-mount options describe the + // exporter's own view of the host mounts. With the recommended + // containerized deployment the host root is bind-mounted read-only into + // the container (`-v /:/host:ro,rslave`), so every host mount carries a + // per-mount "ro" flag in the exporter's mount namespace even though the + // filesystem is writable on the host. Ignore per-mount options for those + // mount points to avoid false positives (issue #3755). + if labels.hostMount { + return false + } + + return slices.Contains(strings.Split(labels.mountOptions, ","), "ro") } func mountOptionsString(m map[string]string) string { diff --git a/collector/filesystem_linux_test.go b/collector/filesystem_linux_test.go index 9e8869016a..1bbc0ec641 100644 --- a/collector/filesystem_linux_test.go +++ b/collector/filesystem_linux_test.go @@ -117,6 +117,32 @@ func Test_isFilesystemReadOnly(t *testing.T) { superOptions: "emergency_ro", }, expected: true, }, + // Host mount observed through a read-only bind mount of the host + // root (`-v /:/host:ro,rslave`): the per-mount "ro" belongs to the + // exporter's mount namespace, the filesystem itself is writable. + "/host/volume6": { + labels: filesystemLabels{ + mountOptions: "ro,relatime", + superOptions: "rw,errors=remount-ro", + hostMount: true, + }, expected: false, + }, + // Host filesystem that really is read-only on the superblock level. + "/host/volume7": { + labels: filesystemLabels{ + mountOptions: "ro,relatime", + superOptions: "ro", + hostMount: true, + }, expected: true, + }, + // Host ext4 filesystem after an emergency remount-ro (Linux 6.15+). + "/host/volume8": { + labels: filesystemLabels{ + mountOptions: "ro,relatime", + superOptions: "rw,emergency_ro", + hostMount: true, + }, expected: true, + }, } for _, tt := range tests { @@ -288,6 +314,49 @@ func TestMountOptionsStringReadOnlyDetection(t *testing.T) { } } +func TestPathRootfsReadOnly(t *testing.T) { + if _, err := kingpin.CommandLine.Parse([]string{"--path.procfs", "./fixtures_bindmount_ro/proc", "--path.rootfs", "/host"}); err != nil { + t.Fatal(err) + } + + // The fixture mimics the recommended containerized deployment with the + // host root bind-mounted read-only (`-v /:/host:ro,rslave`): every mount + // below /host carries a per-mount "ro" flag in the exporter's mount + // namespace, regardless of the state of the host filesystem (issue #3755). + expected := map[string]bool{ + // Writable on the host, per-mount "ro" only comes from the bind mount. + "/": false, + "/var/data": false, + // Read-only on the superblock level, must still be reported. + "/mnt/backup": true, + // ext4 emergency remount-ro (Linux 6.15+), must still be reported. + "/mnt/flaky": true, + // Mounts of the exporter's own container keep the per-mount semantics. + "/run/lock": false, + "/textfile_collector": true, + } + + filesystems, err := mountPointDetails(slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatal(err) + } + + if len(filesystems) != len(expected) { + t.Errorf("Expected %d mount points, got %d", len(expected), len(filesystems)) + } + + for _, fs := range filesystems { + want, ok := expected[fs.mountPoint] + if !ok { + t.Errorf("Got unexpected mount point %s", fs.mountPoint) + continue + } + if got := isFilesystemReadOnly(fs); got != want { + t.Errorf("Expected readonly=%t for mount point %s, got %t", want, fs.mountPoint, got) + } + } +} + func TestPathRootfs(t *testing.T) { if _, err := kingpin.CommandLine.Parse([]string{"--path.procfs", "./fixtures_bindmount/proc", "--path.rootfs", "/host"}); err != nil { t.Fatal(err) diff --git a/collector/fixtures_bindmount_ro/proc/1/mountinfo b/collector/fixtures_bindmount_ro/proc/1/mountinfo new file mode 100644 index 0000000000..7068c7a652 --- /dev/null +++ b/collector/fixtures_bindmount_ro/proc/1/mountinfo @@ -0,0 +1,6 @@ +29 1 259:0 / /host ro,relatime shared:1 - ext4 /dev/nvme1n0 rw +30 1 260:0 / /host/var/data ro,relatime shared:2 - ext4 /dev/mapper/data--vg0-data--lv--0 rw +31 1 261:0 / /host/mnt/backup ro,relatime shared:3 - ext4 /dev/sdb1 ro +32 1 262:0 / /host/mnt/flaky ro,relatime shared:4 - ext4 /dev/sdc1 rw,emergency_ro +33 28 0:27 / /run/lock rw,nosuid,nodev,noexec,relatime shared:6 - tmpfs tmpfs rw,size=5120k,inode64 +34 1 263:0 / /textfile_collector ro,relatime shared:7 - ext4 /dev/sdd1 rw