From cfd50daa28b27a17ce492b281cb080c009386214 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sun, 12 Jul 2026 19:48:03 -0500 Subject: [PATCH 1/5] 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 | 732 +++++++++++++++++++++++++++++++++----- cf-agent/nfs.h | 15 +- cf-agent/verify_storage.c | 96 ++++- libpromises/attributes.c | 3 + libpromises/cf3.defs.h | 6 +- libpromises/mod_storage.c | 3 + 6 files changed, 747 insertions(+), 108 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index 4ba7d095c7..6ce298487b 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -50,9 +50,18 @@ 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 char **ParseOptionsList(const char *opts); +static void FreeOptionsList(char **arr); +static bool InverseOptions(const char *a, const char *b); +static bool OptionPresent(const char *opt, char **actual); +static bool InversePresent(const char *opt, char **actual); +static char *RemountOptionString(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(char *mountpt); +static void ReplaceFstabEntry(Item **liststart, char *mountpt, char *new_entry); static const char *const VMOUNTCOMM[] = { @@ -174,6 +183,234 @@ static void GetHostAndSource(const char *buf, char *host, char *source) source[source_index] = '\0'; } +/* Parse comma-separated options string into an array of individual option strings. + * Returns a newly allocated array of char* pointers, terminated by NULL. + * Caller must free the array itself (not the individual strings). */ +static char **ParseOptionsList(const char *opts) +{ + if (opts == NULL || opts[0] == '\0') + { + char **arr = xcalloc(1, sizeof(char *)); + arr[0] = NULL; + return arr; + } + + /* First pass: count options */ + int count = 0; + for (const char *p = opts; *p; p++) + { + if (*p == ',') + count++; + } + count++; /* number of tokens = number of commas + 1 */ + + char **arr = xcalloc(count + 1, sizeof(char *)); + char *copy = xstrdup(opts); + int idx = 0; + char *token = strtok(copy, ","); + while (token != NULL && idx < count) + { + arr[idx++] = xstrdup(token); + token = strtok(NULL, ","); + } + free(copy); + return arr; +} + +static void FreeOptionsList(char **arr) +{ + if (arr == NULL) + return; + for (int i = 0; arr[i] != NULL; i++) + free(arr[i]); + free(arr); +} + +static bool InverseOptions(const char *a, const char *b) +/* 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). */ +{ + /* A "no"-prefixed option vs its bare form, in either direction. */ + if ((strncmp(a, "no", 2) == 0) && (strcmp(a + 2, b) == 0)) + { + return true; + } + if ((strncmp(b, "no", 2) == 0) && (strcmp(b + 2, a) == 0)) + { + 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 (((strcmp(a, pairs[i][0]) == 0) && (strcmp(b, pairs[i][1]) == 0)) || + ((strcmp(a, pairs[i][1]) == 0) && (strcmp(b, pairs[i][0]) == 0))) + { + return true; + } + } + return false; +} + +static bool OptionPresent(const char *opt, char **actual) +/* True if 'opt' is present in the live options, directly or via a + * tcp/udp<->proto= alias. */ +{ + for (int a = 0; actual[a] != NULL; a++) + { + if (strcmp(opt, actual[a]) == 0) + { + return true; + } + if ((strcmp(opt, "tcp") == 0 && strcmp(actual[a], "proto=tcp") == 0) || + (strcmp(opt, "udp") == 0 && strcmp(actual[a], "proto=udp") == 0) || + (strcmp(opt, "proto=tcp") == 0 && strcmp(actual[a], "tcp") == 0) || + (strcmp(opt, "proto=udp") == 0 && strcmp(actual[a], "udp") == 0)) + { + return true; + } + } + return false; +} + +static bool InversePresent(const char *opt, char **actual) +/* True if an option contradicting 'opt' is present in the live mount. */ +{ + for (int a = 0; actual[a] != NULL; a++) + { + if (InverseOptions(opt, actual[a])) + { + return true; + } + } + return false; +} + +static char *RemountOptionString(const char *opts) +/* Options for a `mount -o remount,...` command, expanding "defaults" to its + * positives rw,suid,dev,exec,async: util-linux ignores the flags implied by a + * bare "defaults" on a remount, so the explicit form is needed to restore a + * drifted mount in place. Newly allocated (caller frees), NULL if opts empty. */ +{ + if ((opts == NULL) || (opts[0] == '\0')) + { + return NULL; + } + + char **arr = ParseOptionsList(opts); + char buf[CF_BUFSIZE]; + size_t n = 0; + buf[0] = '\0'; + for (int i = 0; arr[i] != NULL; i++) + { + const char *tok = (strcmp(arr[i], "defaults") == 0) + ? "rw,suid,dev,exec,async" : arr[i]; + int w = snprintf(buf + n, sizeof(buf) - n, "%s%s", (i > 0) ? "," : "", tok); + if ((w < 0) || ((size_t) w >= sizeof(buf) - n)) + { + break; /* option list longer than CF_BUFSIZE; stop appending */ + } + n += (size_t) w; + } + FreeOptionsList(arr); + return xstrdup(buf); +} + +bool OptionsSubsetMatches(const char *promised_opts, const char *actual_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). */ +{ + if (promised_opts == NULL || promised_opts[0] == '\0') + { + return true; + } + + char **promised = ParseOptionsList(promised_opts); + char **actual = ParseOptionsList(actual_opts); + + /* Expand "defaults" in place, preserving order for the last-wins pass. */ + Seq *eff = SeqNew(16, free); + for (int p = 0; promised[p] != NULL; p++) + { + if (strcmp(promised[p], "defaults") == 0) + { + static const char *const comps[] = { "rw", "suid", "dev", "exec", "async" }; + for (size_t c = 0; c < sizeof(comps) / sizeof(comps[0]); c++) + { + SeqAppend(eff, xstrdup(comps[c])); + } + } + else + { + SeqAppend(eff, xstrdup(promised[p])); + } + } + + /* 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; + for (size_t i = 0; (i < SeqLength(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 < SeqLength(eff); j++) + { + const char *y = SeqAt(eff, j); + if (InverseOptions(x, y)) + { + Log(LOG_LEVEL_VERBOSE, "Mount option '%s' overridden by later '%s'", x, y); + overridden = true; + break; + } + if (strcmp(x, y) == 0) + { + overridden = true; + break; + } + } + if (overridden) + { + continue; + } + + bool is_default_on = false; + for (size_t d = 0; d < sizeof(default_on) / sizeof(default_on[0]); d++) + { + if (strcmp(x, default_on[d]) == 0) + { + is_default_on = true; + break; + } + } + + if (InversePresent(x, actual) || !(OptionPresent(x, actual) || is_default_on)) + { + mismatch = true; + } + } + + SeqDestroy(eff); + FreeOptionsList(promised); + FreeOptionsList(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,21 +583,41 @@ bool LoadMountInfo(Seq *list) Log(LOG_LEVEL_DEBUG, "LoadMountInfo: host '%s', source '%s', mounton '%s'", host, source, mounton); + /* Extract the actual mount options from the parenthesized portion of mount output. + * mount -va output format: host:source on /mountpoint type fstype (opts) */ + char mountopts[CF_BUFSIZE]; + mountopts[0] = '\0'; + char *paren = strstr(vbuff, "("); + if (paren != NULL) + { + char *end = strchr(paren, ')'); + if (end != NULL) + { + strlcpy(mountopts, paren + 1, sizeof(mountopts)); + /* Strip trailing whitespace */ + size_t len = strlen(mountopts); + while (len > 0 && (mountopts[len - 1] == ' ' || mountopts[len - 1] == '\t')) + { + mountopts[--len] = '\0'; + } + } + } + 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); } } @@ -373,7 +630,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 +649,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) { - entry->options = xstrdup(options); + 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->raw_opts = xstrdup(options); } SeqAppend(list, entry); @@ -408,25 +675,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 +787,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 +795,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 && strcmp(existing_opts, opts) != 0) + { + /* 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; @@ -618,7 +888,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); @@ -679,55 +949,65 @@ 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")) + { + 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); + } - 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 ((strstr(line, "busy")) || (strstr(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 +1028,51 @@ 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; - } + snprintf(comm, CF_BUFSIZE, "%s %s", VUNMOUNTCOMM[VSYSTEMHARDCLASS], mountpt); - 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 && ((strstr(line, "busy")) || (strstr(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 +1247,267 @@ static void DeleteThisItem(Item **liststart, Item *entry) } } +/*******************************************************************/ +/* CFE-90: Helper functions for fstab options comparison */ +/*******************************************************************/ + +static char *GetFstabEntryOptions(char *mountpt) +/* Extract the options field from the fstab entry matching mountpt. + * Returns a dynamically allocated string or NULL. */ +{ + for (Item *ip = FSTABLIST; ip != NULL; ip = ip->next) + { + if (ip->name == NULL || ip->name[0] == '#') + { + continue; + } + + /* Parse the fstab line to find the options field */ + char *orig = xstrdup(ip->name); + char *saveptr = NULL; + char *token = strtok_r(orig, " \t", &saveptr); + int field = 0; + bool found = false; + + while (token != NULL && !found) + { + if (field == 1) + { + if (strcmp(token, mountpt) == 0) + { + /* Found matching mountpoint - skip field 2 (fstype), return field 3 (options) */ + char *skip_tok = strtok_r(NULL, " \t", &saveptr); /* field 2: type */ + if (skip_tok != NULL) + { + char *tok = strtok_r(NULL, " \t", &saveptr); /* field 3: options */ + if (tok != NULL) + { + free(orig); + return xstrdup(tok); + } + } + } + } + field++; + token = strtok_r(NULL, " \t", &saveptr); + } + free(orig); + } + + return NULL; +} + +static void ReplaceFstabEntry(Item **liststart, char *mountpt, char *new_entry) +/* Replace the fstab entry for mountpt with new_entry */ +{ + for (Item *ip = *liststart; ip != NULL; ip = ip->next) + { + if (ip->name != NULL && ip->name[0] != '#') + { + char *orig = xstrdup(ip->name); + char *saveptr = NULL; + char *token = strtok_r(orig, " \t", &saveptr); + int field = 0; + bool found = false; + + while (token != NULL && !found) + { + if (field == 1) + { + if (strcmp(token, mountpt) == 0) + { + /* Found matching mountpoint - replace the entire line */ + free(ip->name); + ip->name = xstrdup(new_entry); + found = true; + } + } + field++; + token = strtok_r(NULL, " \t", &saveptr); + } + free(orig); + if (found) + { + break; + } + } + } +} + +/*******************************************************************/ +/* CFE-90: Remount reconciliation */ +/*******************************************************************/ + +static bool LiveMountConverged(const char *name, const Attributes *a) +/* 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. */ +{ + assert(a != NULL); + Seq *tmp = SeqNew(100, free); + bool converged = false; + + if (LoadMountInfo(tmp)) + { + for (size_t i = 0; i < SeqLength(tmp); i++) + { + Mount *mp = SeqAt(tmp, i); + if (mp == NULL || mp->mounton == NULL || strcmp(mp->mounton, name) != 0) + { + continue; + } + + /* Something is mounted here - check source, then options. */ + if ((a->mount.mount_source != NULL) + && ((mp->source == NULL) || (strcmp(mp->source, a->mount.mount_source) != 0))) + { + 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; +} + +PromiseResult ReconcileMountOptions(EvalContext *ctx, char *name, const Attributes *a, const Promise *pp) +/* 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. */ +{ + assert(a != NULL); + 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. */ + Seq *methods = SeqNew(4, NULL); /* borrows const char*, does not own them */ + 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; + for (size_t i = 0; (i < SeqLength(methods)) && !converged; i++) + { + const char *method = SeqAt(methods, i); + + if (strcmp(method, "remount") == 0) + { + char comm[CF_BUFSIZE]; + char *ropts = RemountOptionString(opts); + if (ropts != NULL) + { + snprintf(comm, CF_BUFSIZE, "%s -o remount,%s %s", + CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), ropts, name); + } + else + { + snprintf(comm, CF_BUFSIZE, "%s -o remount %s", + CommandArg0(VMOUNTCOMM[VSYSTEMHARDCLASS]), name); + } + 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 (strcmp(method, "unmount_mount") == 0) + { + /* 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..b3c88c21bf 100644 --- a/cf-agent/nfs.h +++ b/cf-agent/nfs.h @@ -28,7 +28,20 @@ #include #include // Seq -bool LoadMountInfo(Seq *list); +extern bool LoadMountInfo(Seq *list); + +/* Option subset matching for CFE-90 mount option verification. + * Checks whether all options in 'promised_opts' are present in + * 'actual_opts' (kernel-resolved options). Kernel-added NFS + * auto-negotiated options are ignored. Returns true if all + * user-specified options are satisfied. */ +extern bool OptionsSubsetMatches(const char *promised_opts, const char *actual_opts); + +/* CFE-90: Reconcile an already-mounted filesystem whose source or options + * diverge from the promise, using the mechanisms in a->mount.remount_methods + * (in order, verifying after each). Only invoked when remount is enabled. */ +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..57a3cfd6c4 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -341,6 +341,7 @@ 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); bool found = false; for (size_t i = 0; i < SeqLength(list); i++) @@ -366,11 +367,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 ? 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 +459,7 @@ 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); char dir[CF_BUFSIZE]; int changes = 0; @@ -454,13 +473,16 @@ 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)) { + /* 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 +492,59 @@ 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). */ + for (size_t i = 0; i < SeqLength(GetGlobalMountedFSList()); i++) + { + Mount *mp = SeqAt(GetGlobalMountedFSList(), i); + if (mp != NULL && mp->mounton != NULL && strcmp(name, mp->mounton) == 0) + { + 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 +555,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 +578,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..cb0869c80d 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", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; From ddd508708804d7c8dadbc953f2fda7b0813f93b4 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sun, 12 Jul 2026 19:48:05 -0500 Subject: [PATCH 2/5] 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 | 73 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/tests/unit/nfs_test.c b/tests/unit/nfs_test.c index 8e5d8ab78a..9b8e8d80bf 100644 --- a/tests/unit/nfs_test.c +++ b/tests/unit/nfs_test.c @@ -33,11 +33,84 @@ 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_RemountOptionString(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 = RemountOptionString("defaults"); + assert_true(strcmp(s, "rw,suid,dev,exec,async") == 0); free(s); + s = RemountOptionString("defaults,noatime"); + assert_true(strcmp(s, "rw,suid,dev,exec,async,noatime") == 0); free(s); + /* Everything else is passed through unchanged and order-preserved. */ + s = RemountOptionString("rw,noatime"); assert_true(strcmp(s, "rw,noatime") == 0); free(s); + s = RemountOptionString("ro"); assert_true(strcmp(s, "ro") == 0); free(s); + assert_true(RemountOptionString("") == NULL); + assert_true(RemountOptionString(NULL) == NULL); +} + int main() { PRINT_TEST_BANNER(); const UnitTest tests[] = { unit_test(test_MatchFSInFstab), + unit_test(test_OptionsSubsetMatches), + unit_test(test_RemountOptionString), }; return run_tests(tests); From 4cb0efcdba5f342561ba9e32720f1961e7198b20 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 11 Jul 2026 16:26:29 -0500 Subject: [PATCH 3/5] 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 6ce298487b..c08f15c657 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -797,7 +797,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 && strcmp(existing_opts, opts) != 0) { diff --git a/cf-agent/verify_storage.c b/cf-agent/verify_storage.c index 57a3cfd6c4..6e7465f4fe 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -574,7 +574,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 0c6866d09b66c952fa3ec59e576c35fe24a35195 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 11 Jul 2026 16:57:55 -0500 Subject: [PATCH 4/5] 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 6e7465f4fe..b037f0a8aa 100644 --- a/cf-agent/verify_storage.c +++ b/cf-agent/verify_storage.c @@ -397,8 +397,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; } } @@ -476,7 +478,8 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr PromiseResult result = PROMISE_RESULT_NOOP; if (!FileSystemMountedCorrectly(GetGlobalMountedFSList(), 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) @@ -531,20 +534,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 @@ -554,13 +551,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 c8ab003e58eb3e05ec66079744b799ef361d5d88 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 11 Jul 2026 17:20:29 -0500 Subject: [PATCH 5/5] 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 | 68 +++++++++++++++++++++++---------------- 2 files changed, 50 insertions(+), 27 deletions(-) diff --git a/cf-agent/nfs.c b/cf-agent/nfs.c index c08f15c657..0a7889211b 100644 --- a/cf-agent/nfs.c +++ b/cf-agent/nfs.c @@ -1375,6 +1375,15 @@ static bool LiveMountConverged(const char *name, const Attributes *a) { converged = false; } + else if ((a->mount.mount_server != NULL) + && ((mp->host == NULL) || (strcmp(mp->host, a->mount.mount_server) != 0))) + { + /* 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 b037f0a8aa..bb9eb5c9ba 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)) { @@ -368,6 +361,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) || (strcmp(mp->host, a->mount.mount_server) != 0))) + { + Log(LOG_LEVEL_INFO, + "Filesystem on '%s' is from server '%s', not the promised server '%s'", + name, mp->host ? 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). */ @@ -478,9 +484,20 @@ static PromiseResult VerifyMountPromise(EvalContext *ctx, char *name, const Attr PromiseResult result = PROMISE_RESULT_NOOP; if (!FileSystemMountedCorrectly(GetGlobalMountedFSList(), 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; + for (size_t i = 0; i < SeqLength(GetGlobalMountedFSList()); i++) + { + Mount *mp = SeqAt(GetGlobalMountedFSList(), i); + if (mp != NULL && mp->mounton != NULL && strcmp(name, mp->mounton) == 0) + { + already_mounted = true; + break; + } + } if (!a->mount.unmount) { @@ -495,18 +512,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). */ - for (size_t i = 0; i < SeqLength(GetGlobalMountedFSList()); i++) - { - Mount *mp = SeqAt(GetGlobalMountedFSList(), i); - if (mp != NULL && mp->mounton != NULL && strcmp(name, mp->mounton) == 0) - { - already_mounted = true; - break; - } - } - if (already_mounted) { /* CFE-90: mounted but not as promised. Correct the live mount @@ -546,7 +551,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); }