On 2/16/22 11:15 AM, Darrick J. Wong wrote:
> 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.
I was just thinking about that last night. I agree, thanks.
>
>> + (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?
Hm, ok fair, let me give that more thought.
[ -i|r|x|F ]
sounds exclusive but I don't think it's enforced. I think it was safe for
xfs_admin, but maybe not for db in general. I'll give it another look,
thanks.
>> + 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?
yeah, ugly isn't it. :) I definitely cringed at "isreadonly = EXCLSUSIVELY" -
wut?
> 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?
And a pony. :)
All good points, thanks.
-Eric
> --D
>
>> if (!libxfs_init(&x)) {
>> fputs(_("\nfatal error -- couldn't initialize XFS library\n"),
>>
>
