From 348722a06b49ffc8db34332ec7599c1e7a975cc9 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sun, 12 Jul 2026 19:48:03 -0500 Subject: [PATCH 1/6] Added opt-in remount reconciliation for storage mount options Previously mount_options only affected the initial fstab write and the initial mount; a filesystem already mounted with the wrong options was never corrected, because `mount -a` skips already-mounted filesystems. This adds opt-in reconciliation of a live mount when its options drift from the promise. New mount body attributes: - remount (default false): manage the options of an already-mounted filesystem. When false a mounted filesystem with the correct source is considered kept regardless of option drift (backwards compatible; options still drive the initial mount and, with edit_fstab, the fstab entry). - remount_methods (default { remount }): ordered mechanisms tried in turn, re-reading the live mount after each to verify it satisfies the promise. Defaults to the non-disruptive in-place remount only; the disruptive unmount_mount (which tears the filesystem down and back up, interrupting anything using it) is opt-in and is required to change options a live remount cannot, e.g. NFS-negotiated vers=/rsize= or the server. The kernel returns success from a remount even when it silently ignores NFS-negotiated options, so the resulting state is verified rather than the command exit status trusted. - remount_timeout: bounds the unmount/mount path against a hung or unreachable server. When reconciliation is enabled the live mount is corrected first; if edit_fstab is set the fstab entry is then updated regardless of the live outcome (recording intent moves the system closer to the desired state, and the live result is reported as its own promise outcome). The promise is matched against the live mount by first resolving it with util-linux "last wins" semantics - exactly as `mount -o` applies a list, a later option overrides an earlier conflicting one (so "defaults,ro" is a read-only mount and "ro,rw" is rw) - and then checking each surviving option. Only the options the promise names are enforced; any option it does not name is left unpoliced, because its provenance is unknown (kernel-negotiated like vers=/rsize=, or added by a prior manual mount, indistinguishable). An option the promise does specify - including a negotiated one such as rsize=8192 - must be present in the live mount, with its inverse absent. Inverse pairs (noatime/relatime, hard/soft, ro/rw, sync/async, generic no/) and tcp/udp<->proto= aliases are recognized. The "defaults" pseudo-option (never echoed by the kernel) expands to its checkable components rw,suid,dev,exec, async (mount(8): defaults = rw,suid,dev,exec,auto,nouser,async; auto/nouser are fstab/permission concepts, not runtime state, so they are not enforced) - so "defaults" holds unless ro/nosuid/nodev/noexec/sync is present, and a later explicit option can override any of those components. A correctly-mounted filesystem thus converges instead of being reported changed every run. When a later option overrides an earlier conflicting one, that is logged at verbose. When reconciling via an in-place remount, "defaults" is likewise expanded to "rw,suid,dev,exec,async" in the mount command (util-linux does not apply the options implied by a bare "defaults" on a remount), so a filesystem that has drifted to read-only (or nosuid, etc.) is restored in place rather than only via the disruptive unmount_mount; mount's own last-wins then applies any trailing override (e.g. "defaults,ro" remounts as ...,ro). Mount and unmount side effects are gated on the promise action (MakingInternalChanges), not a bare !DONTDO: a dry-run (-n) or action_policy => "warn" promise now reports WARN without defining the classes body's promise_repaired set, so a dependent promise keyed on those classes no longer fires on a no-op run (CFE-3366). A live remount is a real side effect outside the simulate sandbox, so the normal-mode gate is used. Also corrects the mount-info handling this relies on: - GetFstabEntryOptions returned the fstab type field instead of the options field, causing a spurious fstab rewrite and reported change every run. - ReplaceFstabEntry leaked the previous entry string. - The mounted-FS scan no longer dropped the fstype used by IsForeignFileSystem; fstype and kernel-resolved options are now stored separately (options vs raw_opts). - Restored unconditional creation of the mount point directory before mounting, and stopped arming `mount -a` for an already-mounted filesystem. - "device busy" interruptions are logged at LOG_LEVEL_ERR (the outcome is INTERRUPTED, cf. RecordInterruption), and the leaked options strings on the error paths are freed. - VerifyUnmount consulted feof() after cf_pclose() had already closed the stream (a use-after-close): a silent, successful unmount then logged a bogus "Unable to read output of unmount command" and returned before the success cfPS(), reporting NOOP with no promise_repaired class instead of a change. feof() is now captured before cf_pclose() invalidates the stream. The unmount_mount remount method depends on this outcome being correct. References for the kernel/OS behavior asserted in the option-matching and convergence code: mount(8) (`mount -a` skips already-mounted filesystems; `defaults` = rw,suid,dev,exec,auto,nouser,async) and nfs(5) (options negotiated by client and server are reported in /proc/mounts; NFS-specific options cannot be changed by a remount). Ticket: CFE-90 Ticket: CFE-1864 Ticket: CFE-3366 Changelog: Title Co-Authored-By: Claude Opus 4.8 (1M context) --- cf-agent/nfs.c | 698 +++++++++++++++++++++++++++++++++----- cf-agent/nfs.h | 2 + cf-agent/verify_storage.c | 108 +++++- libpromises/attributes.c | 3 + libpromises/cf3.defs.h | 6 +- libpromises/mod_storage.c | 3 + 6 files changed, 713 insertions(+), 107 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 4ba7d095c7..ad5671271b 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -50,9 +50,16 @@ static Item *FSTABLIST = NULL; /* GLOBAL_X */ static void GetHostAndSource(const char *buf, char *host, char *source); -static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *options); +static bool ConflictingOptions(const char *a, const char *b); +static bool OptionPresent(const char *opt, const Seq *actual); +static bool ConflictingOptionPresent(const char *opt, const Seq *actual); +static char *OptionStringExpandDefaults(const char *opts); + +static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *fstype, char *options); static bool MatchFSInFstab(char *match); static void DeleteThisItem(Item **liststart, Item *entry); +static char *GetFstabEntryOptions(const char *mountpt); +static void ReplaceFstabEntry(Item *liststart, const char *mountpt, const char *new_entry); static const char *const VMOUNTCOMM[] = { @@ -174,6 +181,208 @@ static void GetHostAndSource(const char *buf, char *host, char *source) source[source_index] = '\0'; } +/** + * True if 'a' and 'b' are mutually exclusive mount options that cannot both + * hold: ro/rw, hard/soft, sync/async, noatime/relatime, or a "no"-prefixed + * option and its bare form (e.g. noexec/exec). + */ +static bool ConflictingOptions(const char *a, const char *b) +{ + /* A "no"-prefixed option vs its bare form, in either direction. */ + if (StringEqualN(a, "no", 2) && StringEqual(a + 2, b)) + { + return true; + } + if (StringEqualN(b, "no", 2) && StringEqual(b + 2, a)) + { + return true; + } + + /* Inverse pairs that do not share the "no" prefix. */ + static const char *const pairs[][2] = { + { "noatime", "relatime" }, + { "hard", "soft" }, + { "sync", "async" }, + { "ro", "rw" }, + }; + for (size_t i = 0; i < sizeof(pairs) / sizeof(pairs[0]); i++) + { + if ((StringEqual(a, pairs[i][0]) && StringEqual(b, pairs[i][1])) || + (StringEqual(a, pairs[i][1]) && StringEqual(b, pairs[i][0]))) + { + return true; + } + } + return false; +} + +/** + * True if 'opt' is present in the live options, directly or via a + * tcp/udp<->proto= alias. + */ +static bool OptionPresent(const char *opt, const Seq *actual) +{ + const size_t len = SeqLength(actual); + for (size_t i = 0; i < len; i++) + { + const char *act = SeqAt(actual, i); + if (StringEqual(opt, act)) + { + return true; + } + if ((StringEqual(opt, "tcp") && StringEqual(act, "proto=tcp")) || + (StringEqual(opt, "udp") && StringEqual(act, "proto=udp")) || + (StringEqual(opt, "proto=tcp") && StringEqual(act, "tcp")) || + (StringEqual(opt, "proto=udp") && StringEqual(act, "udp"))) + { + return true; + } + } + return false; +} + +/** + * True if an option contradicting 'opt' is present in the live mount. + */ +static bool ConflictingOptionPresent(const char *opt, const Seq *actual) +{ + const size_t len = SeqLength(actual); + for (size_t i = 0; i < len; i++) + { + if (ConflictingOptions(opt, SeqAt(actual, i))) + { + return true; + } + } + return false; +} + +/** + * Expand a bare "defaults" in an option string to its positives + * rw,suid,dev,exec,async: util-linux ignores the flags implied by a bare + * "defaults" on a `mount -o remount,...`, so the explicit form is needed both to + * restore a drifted mount in place and to compare against the live options. + * Newly allocated (caller frees), NULL if opts empty. + */ +static char *OptionStringExpandDefaults(const char *opts) +{ + if ((opts == NULL) || (opts[0] == '\0')) + { + return NULL; + } + + Seq *in = SeqStringFromString(opts, ','); + const size_t len = SeqLength(in); + for (size_t i = 0; i < len; i++) + { + if (StringEqual(SeqAt(in, i), "defaults")) + { + SeqSet(in, i, xstrdup("rw,suid,dev,exec,async")); + } + } + char *result = StringJoin(in, ","); + SeqDestroy(in); + return result; +} + +/** + * Extract the options from the parenthesized tail of a mount(8) output line + * ("host:/source on /mountpoint type nfs (rw,noatime)"), without the enclosing + * parentheses and with surrounding whitespace trimmed. The closing parenthesis + * must not be left on the last option or it will never compare equal to the + * promised one. + * Newly allocated (caller frees), NULL if the line has no "(...)" section. + */ +static char *MountOptionsFromLine(const char *line) +{ + const char *paren = strchr(line, '('); + if (paren == NULL) + { + return NULL; + } + const char *end = strchr(paren, ')'); + if (end == NULL) + { + return NULL; + } + + /* TrimWhitespace() can return an interior pointer, which is not free()-able. */ + char *tail = xstrndup(paren + 1, (size_t) (end - paren - 1)); + char *opts = xstrdup(TrimWhitespace(tail)); + free(tail); + return opts; +} + +/** + * Whether the live mount (actual_opts, from /proc/mounts) satisfies the promise. + * Only named options are enforced; unnamed ones are left alone. The promise is + * resolved "last wins" like `mount -o` (defaults,ro is read-only, ro,rw is rw), + * with "defaults" expanded to rw,suid,dev,exec,async (auto/nouser aren't runtime + * state). Each surviving option must then hold: its inverse absent, and it + * present or a default-on flag (suid/dev/exec/async only show when negated). + */ +bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts) +{ + if (promised_opts == NULL || promised_opts[0] == '\0') + { + return true; + } + + Seq *actual = SeqStringFromString(actual_opts, ','); + + /* Expand a bare "defaults" to its positives (rw,suid,dev,exec,async), + * preserving order for the last-wins pass, then split into options. */ + char *expanded = OptionStringExpandDefaults(promised_opts); + Seq *eff = SeqStringFromString(expanded, ','); + free(expanded); + + /* On-by-default flags the kernel does not echo (only their negatives show). */ + static const char *const default_on[] = { "suid", "dev", "exec", "async" }; + + bool mismatch = false; + const size_t n_eff = SeqLength(eff); + for (size_t i = 0; (i < n_eff) && !mismatch; i++) + { + const char *x = SeqAt(eff, i); + + /* Last wins: skip this option if a later one overrides it (its inverse) + * or repeats it. */ + bool overridden = false; + for (size_t j = i + 1; j < n_eff; j++) + { + const char *y = SeqAt(eff, j); + if (ConflictingOptions(x, y)) + { + Log(LOG_LEVEL_VERBOSE, "Mount option '%s' overridden by later '%s'", x, y); + overridden = true; + break; + } + if (StringEqual(x, y)) + { + overridden = true; + break; + } + } + if (overridden) + { + continue; + } + + const bool is_default_on = IsStringInArray( + x, default_on, sizeof(default_on) / sizeof(default_on[0])); + + if (ConflictingOptionPresent(x, actual) || !(OptionPresent(x, actual) || is_default_on)) + { + mismatch = true; + } + } + + SeqDestroy(eff); + SeqDestroy(actual); + + return !mismatch; +} + bool LoadMountInfo(Seq *list) /* This is, in fact, the most portable way to read the mount info! */ /* Depressing, isn't it? */ @@ -346,22 +555,26 @@ bool LoadMountInfo(Seq *list) Log(LOG_LEVEL_DEBUG, "LoadMountInfo: host '%s', source '%s', mounton '%s'", host, source, mounton); + char *mountopts = MountOptionsFromLine(vbuff); + if (panfs) { - AugmentMountInfo(list, host, source, mounton, "panfs"); + AugmentMountInfo(list, host, source, mounton, "panfs", mountopts); } else if (nfs) { - AugmentMountInfo(list, host, source, mounton, "nfs"); + AugmentMountInfo(list, host, source, mounton, "nfs", mountopts); } else if (cifs) { - AugmentMountInfo(list, host, source, mounton, "cifs"); + AugmentMountInfo(list, host, source, mounton, "cifs", mountopts); } else { - AugmentMountInfo(list, host, source, mounton, NULL); + AugmentMountInfo(list, host, source, mounton, NULL, mountopts); } + + free(mountopts); } free(vbuff); @@ -373,7 +586,7 @@ bool LoadMountInfo(Seq *list) /*******************************************************************/ -static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *options) +static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, char *fstype, char *options) { Mount *entry = xcalloc(1, sizeof(Mount)); @@ -392,9 +605,19 @@ static void AugmentMountInfo(Seq *list, char *host, char *source, char *mounton, entry->mounton = xstrdup(mounton); } - if (options) + /* Store the fstype in options so IsForeignFileSystem can detect + * foreign filesystems via strstr(entry->options, "nfs"/"panfs"/"cifs"). */ + if (fstype != NULL) + { + entry->options = xstrdup(fstype); + } + + /* Store the full kernel-resolved options in raw_opts. + * For unmounted filesystems (options == NULL or empty), raw_opts stays NULL + * and will be checked in FileSystemMountedCorrectly as "not mounted". */ + if (options != NULL && options[0] != '\0') { - entry->options = xstrdup(options); + entry->raw_opts = xstrdup(options); } SeqAppend(list, entry); @@ -408,25 +631,11 @@ void DeleteMountInfo(Seq *list) { Mount *entry = SeqAt(list, i); - if (entry->host) - { - free(entry->host); - } - - if (entry->source) - { - free(entry->source); - } - - if (entry->mounton) - { - free(entry->mounton); - } - - if (entry->options) - { - free(entry->options); - } + free(entry->host); + free(entry->source); + free(entry->mounton); + free(entry->options); + free(entry->raw_opts); } SeqClear(list); @@ -534,6 +743,7 @@ int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promi if (!MatchFSInFstab(mountpt)) { + /* CFE-90: Entry not in fstab - add it */ AppendItem(&FSTABLIST, fstab, NULL); FSTAB_EDITS++; cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, "Adding file system entry '%s' to '%s'", fstab, @@ -541,6 +751,22 @@ int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promi *result = PromiseResultUpdate(*result, PROMISE_RESULT_CHANGE); changes += 1; } + else + { + /* CFE-90: Entry exists - check if options differ and update if needed */ + char *existing_opts = GetFstabEntryOptions(mountpt); + if (existing_opts != NULL && !StringEqual(existing_opts, opts)) + { + /* Replace the entire fstab entry with the corrected options */ + ReplaceFstabEntry(FSTABLIST, mountpt, fstab); + FSTAB_EDITS++; + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, "Updating file system entry for '%s' in '%s' (options: '%s' -> '%s')", + mountpt, VFSTAB[VSYSTEMHARDCLASS], existing_opts, opts); + *result = PromiseResultUpdate(*result, PROMISE_RESULT_CHANGE); + changes += 1; + } + free(existing_opts); + } free(opts); return changes; @@ -679,55 +905,68 @@ PromiseResult VerifyMount(EvalContext *ctx, char *name, const Attributes *a, con } PromiseResult result = PROMISE_RESULT_NOOP; - if (!DONTDO) + + /* CFE-3366: gate the mount on the promise action, not just DONTDO, so a + * dry-run (or action_policy => "warn") reports a warning and does not + * define promise_repaired for a run that changes nothing. */ + if (!MakingInternalChanges(ctx, pp, a, &result, "mount '%s' to keep promise", mountpt)) { - if (StringEqual(a->mount.mount_type, "panfs")) - { - snprintf(comm, CF_BUFSIZE, "%s -t panfs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); - } - else if (StringEqual(a->mount.mount_type, "cifs")) - { - snprintf(comm, CF_BUFSIZE, "%s -t cifs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); - } - else - { - snprintf(comm, CF_BUFSIZE, "%s -o %s %s:%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); - } + free(opts); + return result; + } - if ((pfp = cf_popen(comm, "r", true)) == NULL) - { - Log(LOG_LEVEL_ERR, "Failed to open pipe from '%s'", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS])); - return PROMISE_RESULT_FAIL; - } + if (StringEqual(a->mount.mount_type, "panfs")) + { + NDEBUG_UNUSED int ret = snprintf(comm, CF_BUFSIZE, "%s -t panfs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); + assert(ret >= 0 && ret < CF_BUFSIZE); + } + else if (StringEqual(a->mount.mount_type, "cifs")) + { + NDEBUG_UNUSED int ret = snprintf(comm, CF_BUFSIZE, "%s -t cifs -o %s %s%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); + assert(ret >= 0 && ret < CF_BUFSIZE); + } + else + { + NDEBUG_UNUSED int ret = snprintf(comm, CF_BUFSIZE, "%s -o %s %s:%s %s", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), opts, host, rmountpt, mountpt); + assert(ret >= 0 && ret < CF_BUFSIZE); + } - size_t line_size = CF_BUFSIZE; - char *line = xmalloc(line_size); + if ((pfp = cf_popen(comm, "r", true)) == NULL) + { + Log(LOG_LEVEL_ERR, "Failed to open pipe from '%s'", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS])); + free(opts); + return PROMISE_RESULT_FAIL; + } - ssize_t res = CfReadLine(&line, &line_size, pfp); + size_t line_size = CF_BUFSIZE; + char *line = xmalloc(line_size); - if (res == -1) - { - if (!feof(pfp)) - { - Log(LOG_LEVEL_ERR, "Unable to read output of mount command. (fread: %s)", GetErrorStr()); - cf_pclose(pfp); - free(line); - return PROMISE_RESULT_FAIL; - } - } - else if ((strstr(line, "busy")) || (strstr(line, "Busy"))) + ssize_t res = CfReadLine(&line, &line_size, pfp); + + if (res == -1) + { + if (!feof(pfp)) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be mounted", mountpt); - result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); + Log(LOG_LEVEL_ERR, "Unable to read output of mount command. (fread: %s)", GetErrorStr()); cf_pclose(pfp); free(line); - return 1; + free(opts); + return PROMISE_RESULT_FAIL; } - - free(line); + } + else if (StringContains(line, "busy") || StringContains(line, "Busy")) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be mounted", mountpt); + result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); cf_pclose(pfp); + free(line); + free(opts); + return result; } + free(line); + cf_pclose(pfp); + /* Since opts is either Rlist2String or xstrdup'd, we need to always free it */ free(opts); @@ -748,40 +987,52 @@ PromiseResult VerifyUnmount(EvalContext *ctx, char *name, const Attributes *a, c mountpt = name; PromiseResult result = PROMISE_RESULT_NOOP; - if (!DONTDO) + + /* CFE-3366: gate the unmount on the promise action, not just DONTDO, so a + * dry-run (or action_policy => "warn") reports a warning and does not + * define promise_repaired for a run that changes nothing. */ + if (!MakingInternalChanges(ctx, pp, a, &result, "unmount '%s' to keep promise", mountpt)) { - snprintf(comm, CF_BUFSIZE, "%s %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS], mountpt); + return result; + } - if ((pfp = cf_popen(comm, "r", true)) == NULL) - { - Log(LOG_LEVEL_ERR, "Failed to open pipe from %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS]); - return result; - } + NDEBUG_UNUSED int ret = snprintf(comm, CF_BUFSIZE, "%s %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS], mountpt); + assert(ret >= 0 && ret < CF_BUFSIZE); - size_t line_size = CF_BUFSIZE; - char *line = xmalloc(line_size); + if ((pfp = cf_popen(comm, "r", true)) == NULL) + { + Log(LOG_LEVEL_ERR, "Failed to open pipe from %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS]); + return result; + } - ssize_t res = CfReadLine(&line, &line_size, pfp); - if (res == -1) - { - cf_pclose(pfp); - free(line); + size_t line_size = CF_BUFSIZE; + char *line = xmalloc(line_size); - if (!feof(pfp)) - { - Log(LOG_LEVEL_ERR, "Unable to read output of unmount command. (fread: %s)", GetErrorStr()); - return result; - } - } - else if (res > 0 && ((strstr(line, "busy")) || (strstr(line, "Busy")))) + ssize_t res = CfReadLine(&line, &line_size, pfp); + if (res == -1) + { + /* CfReadLine() returns -1 both at end-of-output and on a read error. A + * successful unmount is silent, so EOF is the normal case here; only a + * genuine read error should be reported. feof() must be consulted + * before cf_pclose() closes and invalidates the stream. */ + bool read_error = !feof(pfp); + cf_pclose(pfp); + free(line); + + if (read_error) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be unmounted", mountpt); - result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); - cf_pclose(pfp); - free(line); + Log(LOG_LEVEL_ERR, "Unable to read output of unmount command. (fread: %s)", GetErrorStr()); return result; } } + else if (res > 0 && (StringContains(line, "busy") || StringContains(line, "Busy"))) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be unmounted", mountpt); + result = PromiseResultUpdate(result, PROMISE_RESULT_INTERRUPTED); + cf_pclose(pfp); + free(line); + return result; + } cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, "Unmounting '%s' to keep promise", mountpt); result = PromiseResultUpdate(result, PROMISE_RESULT_CHANGE); @@ -956,6 +1207,275 @@ static void DeleteThisItem(Item **liststart, Item *entry) } } +/*******************************************************************/ +/* CFE-90: Helper functions for fstab options comparison */ +/*******************************************************************/ + +/* fstab fields, 1-based: device, mount point, type, options, dump, pass. */ +#define FSTAB_FIELD_MOUNTPOINT 2 +#define FSTAB_FIELD_OPTIONS 4 + +/** + * Field 'n' (1-based) of a whitespace-separated fstab line. + * Newly allocated (caller frees), NULL for a comment line or one with fewer + * than 'n' fields. + */ +static char *FstabField(const char *line, size_t n) +{ + if ((line == NULL) || (line[0] == '#')) + { + return NULL; + } + + char *copy = xstrdup(line); + char *saveptr = NULL; + char *token = strtok_r(copy, " \t", &saveptr); + for (size_t i = 1; (i < n) && (token != NULL); i++) + { + token = strtok_r(NULL, " \t", &saveptr); + } + char *field = (token != NULL) ? xstrdup(token) : NULL; + + free(copy); + return field; +} + +/** + * True if the fstab line is the entry for 'mountpt'. + */ +static bool FstabEntryIsFor(const char *line, const char *mountpt) +{ + char *mp = FstabField(line, FSTAB_FIELD_MOUNTPOINT); + bool match = (mp != NULL) && StringEqual(mp, mountpt); + free(mp); + return match; +} + +/** + * Extract the options field from the fstab entry matching mountpt. + * Newly allocated (caller frees), NULL if there is no such entry. + */ +static char *GetFstabEntryOptions(const char *mountpt) +{ + for (Item *ip = FSTABLIST; ip != NULL; ip = ip->next) + { + if (FstabEntryIsFor(ip->name, mountpt)) + { + return FstabField(ip->name, FSTAB_FIELD_OPTIONS); + } + } + + return NULL; +} + +/** + * Replace the fstab entry for mountpt with new_entry. + */ +static void ReplaceFstabEntry(Item *liststart, const char *mountpt, const char *new_entry) +{ + for (Item *ip = liststart; ip != NULL; ip = ip->next) + { + if (FstabEntryIsFor(ip->name, mountpt)) + { + free(ip->name); + ip->name = xstrdup(new_entry); + return; + } + } +} + +/*******************************************************************/ +/* CFE-90: Remount reconciliation */ +/*******************************************************************/ + +/** + * Re-read the mount table from scratch (the cached global list is stale after + * a mount operation) and report whether the filesystem now mounted at 'name' + * satisfies the promise: correct source (when specified) and, when options are + * promised, a superset of the promised options. NFS-specific options + * (transport, NFS version, etc.) cannot be changed by a remount - see nfs(5) + * "THE REMOUNT OPTION": https://man7.org/linux/man-pages/man5/nfs.5.html + * The remount call can still return success without applying them, so we + * verify the resulting state rather than trust the command's exit status. + */ +static bool LiveMountConverged(const char *name, const Attributes *a) +{ + assert(a != NULL); + if (a == NULL) + { + return false; + } + Seq *tmp = SeqNew(100, free); + bool converged = false; + + if (LoadMountInfo(tmp)) + { + const size_t n_mounted = SeqLength(tmp); + for (size_t i = 0; i < n_mounted; i++) + { + Mount *mp = SeqAt(tmp, i); + if (mp == NULL || mp->mounton == NULL || !StringEqual(mp->mounton, name)) + { + continue; + } + + /* Something is mounted here - check source, then options. */ + if ((a->mount.mount_source != NULL) + && ((mp->source == NULL) || !StringEqual(mp->source, a->mount.mount_source))) + { + converged = false; + } + else if (a->mount.mount_options != NULL) + { + char *opts = Rlist2String(a->mount.mount_options, ","); + converged = (mp->raw_opts != NULL) && OptionsSubsetMatches(opts, mp->raw_opts); + free(opts); + } + else + { + converged = true; + } + break; + } + } + + DeleteMountInfo(tmp); + SeqDestroy(tmp); + return converged; +} + +/** + * CFE-90: Reconcile an already-mounted filesystem that has drifted from the + * promise, trying each a->mount.remount_methods mechanism in order (default: + * just "remount"; the disruptive "unmount_mount" is opt-in) and re-checking + * after each. Honors DONTDO; reports its own cfPS outcome. + */ +PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp) +{ + assert(a != NULL); + if (a == NULL) + { + return PROMISE_RESULT_NOOP; + } + PromiseResult result = PROMISE_RESULT_NOOP; + char *opts = Rlist2String(a->mount.mount_options, ","); + int timeout = (a->mount.remount_timeout != CF_NOINT) ? a->mount.remount_timeout : RPCTIMEOUT; + + /* Ordered method list: promise-specified, else the default remount. */ + /* Borrows the const char *, does not own them. */ + Seq *methods = SeqNew( + (a->mount.remount_methods != NULL) ? RlistLen(a->mount.remount_methods) : 1, NULL); + if (a->mount.remount_methods != NULL) + { + for (const Rlist *rp = a->mount.remount_methods; rp != NULL; rp = rp->next) + { + SeqAppend(methods, RlistScalarValue(rp)); + } + } + else + { + /* Default: in-place remount only. unmount_mount tears the filesystem + * down and back up, so it is opt-in (needed for options a remount + * can't change, e.g. NFS-negotiated vers=/rsize= or the server). */ + SeqAppend(methods, "remount"); + } + + /* CFE-3366: gate on the promise action, not just DONTDO, so a dry-run or + * action_policy => "warn" reports a warning without defining + * promise_repaired on a run that changes nothing. */ + if (!MakingInternalChanges(ctx, pp, a, &result, + "reconcile mount '%s' to promised options '%s'", name, + (opts != NULL) ? opts : "")) + { + SeqDestroy(methods); + free(opts); + return result; + } + + bool converged = false; + const size_t n_methods = SeqLength(methods); + for (size_t i = 0; (i < n_methods) && !converged; i++) + { + const char *method = SeqAt(methods, i); + + if (StringEqual(method, "remount")) + { + char comm[CF_BUFSIZE]; + char *ropts = OptionStringExpandDefaults(opts); + if (ropts != NULL) + { + NDEBUG_UNUSED int ret = snprintf(comm, CF_BUFSIZE, "%s -o remount,%s %s", + CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), ropts, name); + assert(ret >= 0 && ret < CF_BUFSIZE); + } + else + { + NDEBUG_UNUSED int ret = snprintf(comm, CF_BUFSIZE, "%s -o remount %s", + CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), name); + assert(ret >= 0 && ret < CF_BUFSIZE); + } + free(ropts); + + Log(LOG_LEVEL_VERBOSE, "Reconciling '%s' via remount: %s", name, comm); + SetTimeOut(timeout); + + FILE *pfp = cf_popen(comm, "r", true); + if (pfp == NULL) + { + Log(LOG_LEVEL_ERR, "Failed to open pipe from '%s'", CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS])); + } + else + { + size_t line_size = CF_BUFSIZE; + char *line = xmalloc(line_size); + while (CfReadLine(&line, &line_size, pfp) != -1) + { + /* drain command output */ + } + free(line); + cf_pclose(pfp); + } + } + else if (StringEqual(method, "unmount_mount")) + { + /* Reuse the tested helpers - both honor DONTDO and build the correct + * per-fstype command. This also handles a wrong-source mount, which + * an in-place remount cannot fix. */ + Log(LOG_LEVEL_VERBOSE, "Reconciling '%s' via unmount + mount", name); + SetTimeOut(timeout); + result = PromiseResultUpdate(result, VerifyUnmount(ctx, name, a, pp)); + result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); + } + else + { + Log(LOG_LEVEL_WARNING, "Unknown remount_method '%s' for '%s' - skipping", method, name); + continue; + } + + /* Verify-after-act: confirm the live mount actually satisfies the promise. */ + if (LiveMountConverged(name, a)) + { + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_CHANGE, pp, a, + "Reconciled mount '%s' to promised options '%s' via '%s'", name, + (opts != NULL) ? opts : "", method); + result = PromiseResultUpdate(result, PROMISE_RESULT_CHANGE); + converged = true; + } + } + + if (!converged) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "Could not reconcile mount '%s' to promised options '%s'", name, + (opts != NULL) ? opts : ""); + result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); + } + + SeqDestroy(methods); + free(opts); + return result; +} + void CleanupNFS(void) { Log(LOG_LEVEL_VERBOSE, "Number of changes observed in '%s' is %d", VFSTAB[VSYSTEMHARDCLASS], FSTAB_EDITS); diff --git a/cf-agent/nfs.h b/cf-agent/nfs.h index 03567c7b35..1ea51bde5b 100644 --- a/cf-agent/nfs.h +++ b/cf-agent/nfs.h @@ -29,6 +29,8 @@ #include // Seq bool LoadMountInfo(Seq *list); +bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts); +PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp); void DeleteMountInfo(Seq *list); int VerifyNotInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp, PromiseResult *result); int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp, PromiseResult *result); diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 698585b9d8..fbe7c1760b 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -341,6 +341,11 @@ static PromiseResult VolumeScanArrivals(ARG_UNUSED char *file, ARG_UNUSED const #if !defined(__MINGW32__) static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes *a) { + assert(a != NULL); + if (a == NULL) + { + return false; + } bool found = false; for (size_t i = 0; i < SeqLength(list); i++) @@ -366,11 +371,29 @@ static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes * mp->host, mp->source, name); return false; } - else + + /* CFE-90: option drift is only managed when 'remount' is on; without + * it a mount with the correct source is "mounted correctly" whatever + * the options (they still drive the initial mount and fstab). */ + if (a->mount.remount && a->mount.mount_options != NULL) { - Log(LOG_LEVEL_VERBOSE, "File system '%s' seems to be mounted correctly", mp->source); - break; + char *opts = Rlist2String(a->mount.mount_options, ","); + if (mp->raw_opts == NULL || mp->raw_opts[0] == '\0' + || !OptionsSubsetMatches(opts, mp->raw_opts)) + { + Log(LOG_LEVEL_INFO, + "Mount options for '%s' do not match promise (actual: '%s', promised: '%s')", + name, + (mp->raw_opts != NULL) ? mp->raw_opts : "(none)", + opts); + free(opts); + return false; + } + free(opts); } + + Log(LOG_LEVEL_VERBOSE, "File system '%s' seems to be mounted correctly", mp->source); + break; } } @@ -440,7 +463,11 @@ static bool IsForeignFileSystem(struct stat *childstat, char *dir) static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp) { - char *options; + assert(a != NULL); + if (a == NULL) + { + return PROMISE_RESULT_NOOP; + } char dir[CF_BUFSIZE]; int changes = 0; @@ -454,13 +481,17 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr return PROMISE_RESULT_INTERRUPTED; } - options = Rlist2String(a->mount.mount_options, ","); - PromiseResult result = PROMISE_RESULT_NOOP; - if (!FileSystemMountedCorrectly(GetGlobalMountedFSList(), name, a)) + Seq *mounted_fs = GetGlobalMountedFSList(); + if (!FileSystemMountedCorrectly(mounted_fs, name, a)) { + /* Declared at this scope so the CF_MOUNTALL guard below can see it. */ + bool already_mounted = false; + if (!a->mount.unmount) { + /* Ensure the mount point exists before mounting or remounting. + * dir is "/.", so this creates the mount point directory. */ if (!MakeParentDirectory(dir, a->move_obstructions, NULL)) { // Could not create parent directory, assume this is okay, @@ -470,18 +501,60 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr dir); } - if (a->mount.editfstab) + /* CFE-90: distinguish "not mounted at all" from "mounted, but not as + * promised" (wrong source, or drifted options with remount enabled). */ + const size_t n_mounted = SeqLength(mounted_fs); + for (size_t i = 0; i < n_mounted; i++) + { + Mount *mp = SeqAt(mounted_fs, i); + if (mp != NULL && mp->mounton != NULL && StringEqual(name, mp->mounton)) + { + already_mounted = true; + break; + } + } + + if (already_mounted) { - changes += VerifyInFstab(ctx, name, a, pp, &result); + /* CFE-90: mounted but not as promised. Correct the live mount + * first (only when remount is enabled), then update fstab. */ + if (a->mount.remount) + { + result = PromiseResultUpdate(result, ReconcileMountOptions(ctx, name, a, pp)); + changes++; + } + else + { + /* Reachable only for a wrong-source mount: option drift with + * remount disabled is reported as mounted correctly. */ + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "A different filesystem is mounted on '%s' than promised; enable 'remount' to correct", name); + result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); + } + + /* Persist to fstab regardless of the live outcome (reported + * above); it converges at the next remount/reboot. */ + if (a->mount.editfstab) + { + changes += VerifyInFstab(ctx, name, a, pp, &result); + } } else { - cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, - "Filesystem '%s' was not mounted as promised, and no edits were promised in '%s'", name, - VFSTAB[VSYSTEMHARDCLASS]); - result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); - // Mount explicitly - result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); + /* Not mounted at all - mount it (historical behavior). */ + if (a->mount.editfstab) + { + changes += VerifyInFstab(ctx, name, a, pp, &result); + } + else + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, + "Filesystem '%s' was not mounted as promised, and no edits were promised in '%s'", name, + VFSTAB[VSYSTEMHARDCLASS]); + result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); + // Mount explicitly + result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); + } } } else @@ -492,7 +565,9 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } } - if (changes > 0) + /* mount -a can only help filesystems that are NOT already mounted; it + * never remounts or changes options on a live mount. */ + if (changes > 0 && !already_mounted) { CF_MOUNTALL = true; } @@ -513,7 +588,6 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } } - free(options); return result; } diff --git a/libpromises/attributes.c b/libpromises/attributes.c index ddc3138e8e..50851aaada 100644 --- a/libpromises/attributes.c +++ b/libpromises/attributes.c @@ -1694,6 +1694,9 @@ StorageMount GetMountConstraints(const EvalContext *ctx, const Promise *pp) m.mount_options = PromiseGetConstraintAsList(ctx, "mount_options", pp); m.editfstab = PromiseGetConstraintAsBoolean(ctx, "edit_fstab", pp); m.unmount = PromiseGetConstraintAsBoolean(ctx, "unmount", pp); + m.remount = PromiseGetConstraintAsBoolean(ctx, "remount", pp); + m.remount_methods = PromiseGetConstraintAsList(ctx, "remount_methods", pp); + m.remount_timeout = PromiseGetConstraintAsInt(ctx, "remount_timeout", pp); return m; } diff --git a/libpromises/cf3.defs.h b/libpromises/cf3.defs.h index fd53bf8a68..b3bd138507 100644 --- a/libpromises/cf3.defs.h +++ b/libpromises/cf3.defs.h @@ -913,7 +913,8 @@ typedef struct char *host; char *source; char *mounton; - char *options; + char *options; /* fstype string (e.g. "nfs", "panfs", "cifs") for foreign-FS detection */ + char *raw_opts; /* full kernel-resolved options from /proc/mounts */ int unmount; } Mount; @@ -1292,6 +1293,9 @@ typedef struct Rlist *mount_options; int editfstab; int unmount; + int remount; + Rlist *remount_methods; + int remount_timeout; } StorageMount; typedef struct diff --git a/libpromises/mod_storage.c b/libpromises/mod_storage.c index cd663a0f4a..4ed8f51b5a 100644 --- a/libpromises/mod_storage.c +++ b/libpromises/mod_storage.c @@ -48,6 +48,9 @@ static const ConstraintSyntax mount_constraints[] = ConstraintSyntaxNewString("mount_server", "", "Hostname or IP or remote file system server", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewStringList("mount_options", "", "List of option strings to add to the file system table (\"fstab\")", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewBool("unmount", "true/false unmount a previously mounted filesystem. Default value: false", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewBool("remount", "true/false correct the options of an already-mounted filesystem when they differ from the promise. Default value: false", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewOptionList("remount_methods", "remount,unmount_mount", "Ordered list of mechanisms to reconcile a mounted filesystem with the promise (tried in order). Default: remount", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewInt("remount_timeout", CF_VALRANGE, "Timeout in seconds for each remount_method. Default value: 60 seconds", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; From 60dc8457e15dc0167e96e028dff17a1736e5dc53 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sun, 12 Jul 2026 19:48:05 -0500 Subject: [PATCH 2/6] Added unit tests for mount option matching Covers OptionsSubsetMatches: named-option-only enforcement (kernel-added options ignored), order independence for non-conflicting options, inverse pairs (noatime/relatime, ro/rw, hard/soft, sync/async, generic no/), tcp/udp<->proto= aliases, the "defaults" expansion (held unless ro/nosuid/nodev/noexec/sync is present), and the "last wins" resolution of conflicting options (defaults,ro -> read-only; ro,rw -> rw; a later explicit option overrides a "defaults" component). Also covers RemountOptionString (expanding "defaults" to rw,suid,dev,exec,async for the remount command). Runs unprivileged in CI via "make -C tests/unit check"; the behavioral mount/remount test belongs in the system-testing repo (needs root + a real NFS server). Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/nfs_test.c | 145 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/tests/unit/nfs_test.c b/tests/unit/nfs_test.c index 8e5d8ab78a..2e0c403e81 100644 --- a/tests/unit/nfs_test.c +++ b/tests/unit/nfs_test.c @@ -33,11 +33,156 @@ static void test_MatchFSInFstab(void) assert_false(MatchFSInFstab("/mnt/fileserver3/vol1")); } +static void test_OptionsSubsetMatches(void) +{ + /* Empty/NULL promise is always satisfied. */ + assert_true(OptionsSubsetMatches(NULL, "rw,noatime")); + assert_true(OptionsSubsetMatches("", "rw,noatime")); + + /* Subset: all promised present, kernel-added options ignored. */ + assert_true(OptionsSubsetMatches("rw,noatime", + "rw,noatime,vers=4.2,rsize=524288,wsize=524288,hard,proto=tcp,addr=10.0.0.1")); + + /* Order-insensitive. */ + assert_true(OptionsSubsetMatches("noatime,rw", "rw,noatime,vers=4.2")); + + /* A promised option that is simply absent -> mismatch. */ + assert_false(OptionsSubsetMatches("rw,noatime,acl", "rw,noatime,vers=4.2")); + + /* Inverse pairs contradict. */ + assert_false(OptionsSubsetMatches("noatime", "rw,relatime,vers=4.2")); + assert_false(OptionsSubsetMatches("ro", "rw,relatime")); + assert_false(OptionsSubsetMatches("rw", "ro,relatime")); + assert_false(OptionsSubsetMatches("hard", "rw,soft")); + assert_false(OptionsSubsetMatches("sync", "rw,async")); + + /* Generic "no" vs "" contradiction. */ + assert_false(OptionsSubsetMatches("nodev", "rw,dev")); + assert_false(OptionsSubsetMatches("atime", "rw,noatime")); + + /* Protocol aliases, both directions. */ + assert_true(OptionsSubsetMatches("tcp", "rw,proto=tcp,vers=4.2")); + assert_true(OptionsSubsetMatches("proto=tcp", "rw,tcp")); + assert_true(OptionsSubsetMatches("udp", "rw,proto=udp")); + + /* "defaults" is never echoed by the kernel; it holds iff none of the + * negatives that would violate it (ro/nosuid/nodev/noexec/sync) are + * present. atime/relatime and kernel-added options are irrelevant to it. */ + assert_true(OptionsSubsetMatches("defaults", "rw,relatime,vers=4.2,hard")); + assert_true(OptionsSubsetMatches("defaults,noatime", "rw,noatime,vers=4.2")); + assert_true(OptionsSubsetMatches("defaults", "rw,noatime,relatime,vers=4.2")); + assert_false(OptionsSubsetMatches("defaults", "ro,relatime,vers=4.2")); + assert_false(OptionsSubsetMatches("defaults", "rw,nosuid,vers=4.2")); + assert_false(OptionsSubsetMatches("defaults", "rw,noexec")); + assert_false(OptionsSubsetMatches("defaults", "rw,sync")); + + /* Last wins: a later option overrides an earlier conflicting one, exactly + * as `mount -o` applies the list (defaults,ro is read-only; ro,rw is rw). */ + assert_true(OptionsSubsetMatches("defaults,ro", "ro,relatime,vers=4.2")); /* ro overrides defaults' rw */ + assert_false(OptionsSubsetMatches("defaults,ro", "rw,relatime,vers=4.2")); /* wants ro, mount is rw */ + assert_true(OptionsSubsetMatches("ro,rw", "rw,relatime")); /* rw wins */ + assert_false(OptionsSubsetMatches("ro,rw", "ro,relatime")); /* wants rw, mount is ro */ + assert_true(OptionsSubsetMatches("rw,ro", "ro,relatime")); /* ro wins */ + assert_true(OptionsSubsetMatches("defaults,nosuid", "rw,nosuid,vers=4.2")); /* nosuid overrides defaults' suid */ +} + +static void test_OptionStringExpandDefaults(void) +{ + char *s; + + /* "defaults" expands to its checkable positives so an in-place remount + * restores a mount that drifted to ro/nosuid/etc (util-linux does not honor + * the options implied by "defaults" on a remount). */ + s = OptionStringExpandDefaults("defaults"); + assert_true(StringEqual(s, "rw,suid,dev,exec,async")); + free(s); + s = OptionStringExpandDefaults("defaults,noatime"); + assert_true(StringEqual(s, "rw,suid,dev,exec,async,noatime")); + free(s); + /* Everything else is passed through unchanged and order-preserved. */ + s = OptionStringExpandDefaults("rw,noatime"); + assert_true(StringEqual(s, "rw,noatime")); + free(s); + s = OptionStringExpandDefaults("ro"); + assert_true(StringEqual(s, "ro")); + free(s); + assert_true(OptionStringExpandDefaults("") == NULL); + assert_true(OptionStringExpandDefaults(NULL) == NULL); +} + +static void test_MountOptionsFromLine(void) +{ + char *s; + + /* The closing parenthesis must not survive on the last option: it would + * never compare equal to the promised one, so a correctly mounted + * filesystem would be reported as drifted (and, with remount enabled, + * remounted on every run without ever converging). */ + s = MountOptionsFromLine("srv:/vol on /mnt/x type nfs (rw,noatime)"); + assert_true(StringEqual(s, "rw,noatime")); + free(s); + s = MountOptionsFromLine("/dev/sda1 on /boot type ext4 (ro,nosuid)"); + assert_true(StringEqual(s, "ro,nosuid")); + free(s); + + /* A long NFS line keeps its last (kernel-negotiated) option intact. */ + s = MountOptionsFromLine( + "10.0.0.1:/export on /mnt/data type nfs4 (rw,relatime,vers=4.2,hard,addr=10.0.0.1)"); + assert_true(StringEqual(s, "rw,relatime,vers=4.2,hard,addr=10.0.0.1")); + free(s); + + /* Whitespace in the parentheses is not part of an option. The leading case + * also catches an interior pointer being returned: the free() then aborts. */ + s = MountOptionsFromLine("srv:/vol on /mnt/x type nfs (rw,noatime \t)"); + assert_true(StringEqual(s, "rw,noatime")); + free(s); + s = MountOptionsFromLine("srv:/vol on /mnt/x type nfs ( \trw,noatime )"); + assert_true(StringEqual(s, "rw,noatime")); + free(s); + + /* Only whitespace between the parentheses: empty, not NULL. */ + s = MountOptionsFromLine("srv:/vol on /mnt/x type nfs ( )"); + assert_true(StringEqual(s, "")); + free(s); + + /* Nothing to extract: no parentheses, and an unterminated one. */ + assert_true(MountOptionsFromLine("srv:/vol on /mnt/x type nfs") == NULL); + assert_true(MountOptionsFromLine("srv:/vol on /mnt/x type nfs (rw") == NULL); +} + +static void test_GetFstabEntryOptions(void) +{ + /* Own entries, so this does not depend on another test having run. */ + AppendItem(&FSTABLIST, "srv:/vol/a\t/mnt/opts/tabs\tnfs\trw,intr,noatime\t0 0", NULL); + AppendItem(&FSTABLIST, "UUID=1234 /mnt/opts/spaces ext4 errors=remount-ro 0 1", NULL); + AppendItem(&FSTABLIST, "#srv:/vol/c /mnt/opts/commented nfs rw,noatime 0 0", NULL); + AppendItem(&FSTABLIST, "srv:/vol/d /mnt/opts/short", NULL); + + /* Field 4 is the options — not the fstype in field 3. Tabs and runs of + * spaces both separate fields. */ + char *opts = GetFstabEntryOptions("/mnt/opts/tabs"); + assert_true(StringEqual(opts, "rw,intr,noatime")); + free(opts); + opts = GetFstabEntryOptions("/mnt/opts/spaces"); + assert_true(StringEqual(opts, "errors=remount-ro")); + free(opts); + + /* A commented-out entry is not an entry; nor is a truncated one or an + * unknown mount point. */ + assert_true(GetFstabEntryOptions("/mnt/opts/commented") == NULL); + assert_true(GetFstabEntryOptions("/mnt/opts/short") == NULL); + assert_true(GetFstabEntryOptions("/mnt/opts/nosuchmount") == NULL); +} + int main() { PRINT_TEST_BANNER(); const UnitTest tests[] = { unit_test(test_MatchFSInFstab), + unit_test(test_OptionsSubsetMatches), + unit_test(test_OptionStringExpandDefaults), + unit_test(test_MountOptionsFromLine), + unit_test(test_GetFstabEntryOptions), }; return run_tests(tests); From c009afa116ed36c85fece148ea422ecbd1d96751 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 11 Jul 2026 16:26:29 -0500 Subject: [PATCH 3/6] Maintained fstab entries for already-mounted filesystems A storage promise whose filesystem was already mounted with the correct source reported "mounted as promised" and never touched fstab, so a missing fstab entry was not restored and an options change was not written to fstab (CFE-1539). VerifyInFstab now also runs on the mounted-correctly path (when edit_fstab is set), independent of the opt-in live 'remount': keeping fstab correct is the documented behavior of mount_options, while remounting a live filesystem stays the disruptive, opt-in part gated by 'remount'. The fstab options comparison is left exact (order-sensitive): fstab option order is significant for duplicated/conflicting options, and the earlier GetFstabEntryOptions fix already stops the every-run rewrites. Ticket: CFE-1539 Changelog: Title --- cf-agent/nfs.c | 9 ++++++++- cf-agent/verify_storage.c | 13 ++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index ad5671271b..bc5a600e3b 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -753,7 +753,14 @@ int VerifyInFstab(EvalContext *ctx, char *name, const Attributes *a, const Promi } else { - /* CFE-90: Entry exists - check if options differ and update if needed */ + /* CFE-90: Entry exists - rewrite it if the options differ. The compare + * is exact (order-sensitive) on purpose: for duplicated/conflicting + * options the kernel uses the last one, so option order is significant + * in an fstab line and must not be normalized away. Since + * GetFstabEntryOptions now reads the real options field (it previously + * returned the fstype), this converges - a differently-ordered entry is + * rewritten once to the promised form, then matches - rather than being + * rewritten on every run. */ char *existing_opts = GetFstabEntryOptions(mountpt); if (existing_opts != NULL && !StringEqual(existing_opts, opts)) { diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index fbe7c1760b..281f92ad90 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -584,7 +584,18 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } else { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_NOOP, pp, a, "Filesystem '%s' seems to be mounted as promised", name); + /* CFE-1539: mounted correctly, but still maintain fstab (add a + * missing entry, correct drifted options) so the promise persists + * across reboots. Independent of 'remount' - keeping fstab correct + * is documented mount_options behavior; remounting live is not. */ + if (a->mount.editfstab) + { + changes += VerifyInFstab(ctx, name, a, pp, &result); + } + if (changes == 0) + { + cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_NOOP, pp, a, "Filesystem '%s' seems to be mounted as promised", name); + } } } From e98492e17dd0bdbd9a0b01e3467821b9a6f3c2f0 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 11 Jul 2026 16:57:55 -0500 Subject: [PATCH 4/6] Mounted a single filesystem surgically instead of running mount -a A storage promise for a not-yet-mounted filesystem armed CF_MOUNTALL, so CFEngine ran 'mount -a'/'mount -va' at the end of the pass. That mounts every unmounted entry in fstab - unrelated devices and foreign filesystem types included - as a side effect of a single promise (CFE-1863). The not-mounted path now mounts just the promised filesystem with VerifyMount (the existing surgical primitive), then persists it to fstab when edit_fstab is set. CF_MOUNTALL / MountAll are kept for the explicit 'mountfilesystems' agent control, which still means "mount everything in fstab". Ticket: CFE-1863 Changelog: Title --- cf-agent/verify_storage.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 281f92ad90..c44c14da4c 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -401,8 +401,10 @@ static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes * { if (!a->mount.unmount) { + /* CFE-1863: do not arm MountAll ('mount -a') here - the caller + * mounts this one filesystem surgically. CF_MOUNTALL is reserved + * for the explicit 'mountfilesystems' agent control. */ Log(LOG_LEVEL_VERBOSE, "File system '%s' seems not to be mounted correctly", name); - CF_MOUNTALL = true; } } @@ -485,7 +487,8 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr Seq *mounted_fs = GetGlobalMountedFSList(); if (!FileSystemMountedCorrectly(mounted_fs, name, a)) { - /* Declared at this scope so the CF_MOUNTALL guard below can see it. */ + /* Whether something is already mounted at the promiser (vs. nothing + * mounted there at all). */ bool already_mounted = false; if (!a->mount.unmount) @@ -541,20 +544,14 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } else { - /* Not mounted at all - mount it (historical behavior). */ + /* CFE-1863: not mounted - mount just THIS filesystem instead of + * arming MountAll ('mount -a'), which would also mount every + * other unmounted fstab entry. Then persist to fstab. */ + result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); if (a->mount.editfstab) { changes += VerifyInFstab(ctx, name, a, pp, &result); } - else - { - cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, - "Filesystem '%s' was not mounted as promised, and no edits were promised in '%s'", name, - VFSTAB[VSYSTEMHARDCLASS]); - result = PromiseResultUpdate(result, PROMISE_RESULT_FAIL); - // Mount explicitly - result = PromiseResultUpdate(result, VerifyMount(ctx, name, a, pp)); - } } } else @@ -564,13 +561,6 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr changes += VerifyNotInFstab(ctx, name, a, pp, &result); } } - - /* mount -a can only help filesystems that are NOT already mounted; it - * never remounts or changes options on a live mount. */ - if (changes > 0 && !already_mounted) - { - CF_MOUNTALL = true; - } } else { From 841d48738175b672d34cf5a0b9f3ede35ad08705 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 11 Jul 2026 17:20:29 -0500 Subject: [PATCH 5/6] Allowed mount promises to target a specific server An unmount promise that named mount_source/mount_server was warned about as "probably an error", and the server was never used to select which mount to act on - so a policy could not unmount one specific mount (e.g. from a server being migrated away) without affecting others (CFE-2350). The bogus warning is removed, and the server (host) is now part of the "mounted correctly" identity check, gated on remount or unmount so it only engages when the promise opts into disruptive mount management. An unmount promise that finds a *different* filesystem at the mount point now leaves it - and its fstab entry - untouched. LiveMountConverged likewise treats the server as part of identity, so a remount-in-place that cannot change the server escalates to unmount_mount. Ticket: CFE-2350 Changelog: Title --- cf-agent/nfs.c | 9 +++++ cf-agent/verify_storage.c | 70 +++++++++++++++++++++++---------------- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index bc5a600e3b..7b850c33c5 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -1332,6 +1332,15 @@ static bool LiveMountConverged(const char *name, const Attributes *a) { converged = false; } + else if ((a->mount.mount_server != NULL) + && ((mp->host == NULL) || !StringEqual(mp->host, a->mount.mount_server))) + { + /* CFE-2350: the server is part of the mount identity, so a + * remounted-in-place mount that kept the old server has not + * converged - a remount in place cannot change the server, so + * reconciling it requires unmount_mount in remount_methods. */ + converged = false; + } else if (a->mount.mount_options != NULL) { char *opts = Rlist2String(a->mount.mount_options, ","); diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index c44c14da4c..77b910c662 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -101,18 +101,11 @@ PromiseResult VerifyStoragePromise(EvalContext *ctx, char *path, const Promise * /* No parameter conflicts here */ - if (a.mount.unmount) - { - if ((a.mount.mount_source)) - { - Log(LOG_LEVEL_VERBOSE, "An unmount promise indicates a mount-source information - probably an error"); - } - if ((a.mount.mount_server)) - { - Log(LOG_LEVEL_VERBOSE, "An unmount promise indicates a mount-server information - probably an error"); - } - } - else if (a.havemount) + /* CFE-2350: mount_source / mount_server on an unmount promise are not an + * error - they select which specific mount to unmount (e.g. one from a + * particular server) rather than every mount of a type. Only a *mount* + * promise needs both. */ + if (!a.mount.unmount && a.havemount) { if ((a.mount.mount_source == NULL) || (a.mount.mount_server == NULL)) { @@ -372,6 +365,19 @@ static bool FileSystemMountedCorrectly(Seq *list, char *name, const Attributes * return false; } + /* CFE-2350: the server is part of mount identity, but acting on a + * mismatch is disruptive (umount+mount), so only check it when the + * promise opts in via remount or unmount. */ + if ((a->mount.remount || a->mount.unmount) + && (a->mount.mount_server != NULL) + && ((mp->host == NULL) || !StringEqual(mp->host, a->mount.mount_server))) + { + Log(LOG_LEVEL_INFO, + "Filesystem on '%s' is from server '%s', not the promised server '%s'", + name, (mp->host != NULL) ? mp->host : "(unknown)", a->mount.mount_server); + return false; + } + /* CFE-90: option drift is only managed when 'remount' is on; without * it a mount with the correct source is "mounted correctly" whatever * the options (they still drive the initial mount and fstab). */ @@ -487,9 +493,21 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr Seq *mounted_fs = GetGlobalMountedFSList(); if (!FileSystemMountedCorrectly(mounted_fs, name, a)) { - /* Whether something is already mounted at the promiser (vs. nothing - * mounted there at all). */ + /* Whether something is mounted at the promiser at all (vs. nothing + * mounted there). Computed for both mount and unmount: the unmount + * path uses it to tell "a different filesystem is mounted here" from + * "nothing is mounted here". */ bool already_mounted = false; + const size_t n_mounted = SeqLength(mounted_fs); + for (size_t i = 0; i < n_mounted; i++) + { + Mount *mp = SeqAt(mounted_fs, i); + if (mp != NULL && mp->mounton != NULL && StringEqual(name, mp->mounton)) + { + already_mounted = true; + break; + } + } if (!a->mount.unmount) { @@ -504,19 +522,6 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr dir); } - /* CFE-90: distinguish "not mounted at all" from "mounted, but not as - * promised" (wrong source, or drifted options with remount enabled). */ - const size_t n_mounted = SeqLength(mounted_fs); - for (size_t i = 0; i < n_mounted; i++) - { - Mount *mp = SeqAt(mounted_fs, i); - if (mp != NULL && mp->mounton != NULL && StringEqual(name, mp->mounton)) - { - already_mounted = true; - break; - } - } - if (already_mounted) { /* CFE-90: mounted but not as promised. Correct the live mount @@ -556,7 +561,16 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr } else { - if (a->mount.editfstab) + /* CFE-2350: a *different* filesystem here is not ours to unmount - + * leave it (and its fstab entry) alone; otherwise nothing of ours + * is mounted, so just ensure it is not in fstab. */ + if (already_mounted) + { + cfPS(ctx, LOG_LEVEL_VERBOSE, PROMISE_RESULT_NOOP, pp, a, + "A different filesystem is mounted on '%s' than the unmount promise targets; leaving it untouched", + name); + } + else if (a->mount.editfstab) { changes += VerifyNotInFstab(ctx, name, a, pp, &result); } From 6af74569089fb56cc1d2dbfdd46d2c0bb01f0a42 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 24 Jul 2026 16:44:29 -0500 Subject: [PATCH 6/6] Logged a busy-device fstab-removal failure at ERR, not INFO VerifyNotInFstab logged the "device busy, cannot remove from fstab" interruption at INFO. An INTERRUPTED outcome should surface at ERR so it is visible without verbose logging. Split out from the CFE-90 work as an unrelated log-level correction. --- cf-agent/nfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 7b850c33c5..da8b5cd78a 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -851,7 +851,7 @@ int VerifyNotInFstab(EvalContext *ctx, char *name, const Attributes *a, const Pr if (strstr(line, "busy")) { - cfPS(ctx, LOG_LEVEL_INFO, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be removed from '%s'", + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_INTERRUPTED, pp, a, "The device under '%s' cannot be removed from '%s'", mountpt, VFSTAB[VSYSTEMHARDCLASS]); *result = PromiseResultUpdate(*result, PROMISE_RESULT_INTERRUPTED); free(line);