| Message ID | 15b6f52f-a90b-7056-8b2e-e2d4dde1ef5d@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs_admin: open with O_EXCL if we will be writing | expand |
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@redhat.com> > --- > > (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"), >
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@redhat.com> >> --- >> >> (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"), >> >
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") && + (x.isreadonly != LIBXFS_ISREADONLY)) + x.isreadonly = LIBXFS_EXCLUSIVELY; + x.bcache_flags = CACHE_MISCOMPARE_PURGE; if (!libxfs_init(&x)) { fputs(_("\nfatal error -- couldn't initialize XFS library\n"),
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@redhat.com> --- (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.)