On Tue, Feb 15, 2022 at 11:35:23PM -0600, Eric Sandeen wrote:
> So, coreOS has a systemd unit which changes the UUID of a filesystem
> on first boot, and they're currently racing that with mount.
>
> This leads to corruption and mount failures.
>
> If xfs_db is running as xfs_admin in a mode that can write to the
> device, open that device exclusively.
>
> This might still lead to mount failures if xfs_admin wins the open race,
> but at least it won't corrupt the filesystem along the way.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> (this opens plain files O_EXCL is well, which is undefined without O_CREAT.
> I'm not sure if we need to worry about that.)
>
> diff --git a/db/init.c b/db/init.c
> index eec65d0..f43be6e 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -97,6 +97,14 @@ init(
> else
> x.dname = fsdevice;
>
> + /*
> + * If running as xfs_admin in RW mode, prevent concurrent
> + * opens of a block device.
> + */
> + if (!strcmp(progname, "xfs_admin") &&
Hmm, it seems like sort of a hack to key this off the program name.
Though Eric mentioned on IRC that Dave or someone expressed a preference
for xfs_db not being gated on O_EXCL when a user is trying to run the
program for *debugging*.
Perhaps "if (strcmp(progname, "xfs_db") &&" here? Just in case we add
more shell script wrappers for xfs_db in the future? I prefer loosening
restrictions as new functionality asks for them, rather than risk
breaking scripts when we discover holes in new code later on.
> + (x.isreadonly != LIBXFS_ISREADONLY))
At first I wondered about the -i case where ISREADONLY and ISINACTIVE
are set, but then I realized that -i ("do it even if mounted") isn't
used by xfs_admin and expressly forbids the use of O_EXCL. So I guess
the equivalence test and the assignment below are ok, since x.isreadonly
is zero at the start of xfs_db's init() function, and we'll never have
to deal with other flags combinations that might've snuck in from
somewhere else. Right?
> + x.isreadonly = LIBXFS_EXCLUSIVELY;
But this is still a mess. Apparently libxfs_init_t.isdirect is for
LIBXFS_DIRECT, but libxfs_init_t.isreadonly is for other four flags?
But it doesn't really make much difference to libxfs_init() because it
combines both fields?
Can we turn this into a single flags field? Not necessarily here, but
as a general cleanup?
> +
> x.bcache_flags = CACHE_MISCOMPARE_PURGE;
...and maybe teach libxlog not to have this global variable?
--D
> if (!libxfs_init(&x)) {
> fputs(_("\nfatal error -- couldn't initialize XFS library\n"),
>
