[PATCH] e2fsck: fix checks done for mounted vs. read-only

[PATCH] e2fsck: fix checks done for mounted vs. read-only

[Andreas Dilger]

Currently, if e2fsck is run without the "-n" flag (i.e. it
might modify the filesystem), there is no guarantee that it will
open the filesystem with the EXCLUSIVE flag (i.e. O_EXCL) to
prevent the block device from being checked (in most cases this
means mounted, but it could also be an MD/LVM member device).

Conversely, if e2fsck is run with "-n" (i.e. read-only), and
/etc/mtab or /proc/mounts does not report the block device as
mounted then e2fsck thinks the filesystem is unmounted.  In this
case, e2fsck incorrectly sets the EXCLUSIVE flag, which causes
the check to fail, even though e2fsck is running read-only.

To fix this, do not open with EXCLUSIVE if it is a read-only check,
and always open with EXCLUSIVE if the filesystem might be changed.
This also prevents filesystem mounts while e2fsck is running.

Also refuse allow e2fsck to run at all if the filesystem is BUSY.
The e2fsck check_mount() was checking for MOUNTED, but not BUSY,
and it should refuse to run outright if the block device is BUSY.
The previous MOUNTED heuristics pre-date the O_EXCL reservation
by the kernel, so there could be uncertainty due to stale /etc/mtab
data, but with newer kernels a busy device should never be modified.

Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
---
 e2fsck/unix.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 53fcd04..6161e46 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -219,7 +219,7 @@ static void check_mount(e2fsck_t ctx)
 	 * filesystem and it's mounted read-only, and we're not doing
 	 * a read/write check, then everything's fine.
 	 */
-	if ((!(ctx->mount_flags & EXT2_MF_MOUNTED)) ||
+	if ((!(ctx->mount_flags & (EXT2_MF_MOUNTED | EXT2_MF_BUSY))) ||
 	    ((ctx->mount_flags & EXT2_MF_ISROOT) &&
 	     (ctx->mount_flags & EXT2_MF_READONLY) &&
 	     !(ctx->options & E2F_OPT_WRITECHECK)))
@@ -227,13 +227,16 @@ static void check_mount(e2fsck_t ctx)
 
 	if ((ctx->options & E2F_OPT_READONLY) &&
 	    !(ctx->options & E2F_OPT_WRITECHECK)) {
-		log_out(ctx, _("Warning!  %s is mounted.\n"),
-			ctx->filesystem_name);
+		log_out(ctx, _("Warning!  %s is %s.\n"),
+			ctx->filesystem_name,
+			ctx->mount_flags & EXT2_MF_MOUNTED ?
+				"mounted" : "in use");
 		return;
 	}
 
-	log_out(ctx, _("%s is mounted.  "), ctx->filesystem_name);
-	if (!ctx->interactive)
+	log_out(ctx, _("%s is %s.\n"), ctx->filesystem_name,
+		ctx->mount_flags & EXT2_MF_MOUNTED ? "mounted" : "in use");
+	if (!ctx->interactive || ctx->mount_flags & EXT2_MF_BUSY)
 		fatal_error(ctx, _("Cannot continue, aborting.\n\n"));
 	puts("\007\007\007\007");
 	log_out(ctx, _("\n\nWARNING!!!  "
@@ -1219,9 +1222,7 @@ restart:
 	if (!old_bitmaps)
 		flags |= EXT2_FLAG_64BITS;
 	if ((ctx->options & E2F_OPT_READONLY) == 0)
-		flags |= EXT2_FLAG_RW;
-	if ((ctx->mount_flags & EXT2_MF_MOUNTED) == 0)
-		flags |= EXT2_FLAG_EXCLUSIVE;
+		flags |= EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
 
 	retval = try_open_fs(ctx, flags, io_ptr, &fs);
 
-- 
1.7.3.4

Re: e2fsck: fix checks done for mounted vs. read-only

[Ted Ts'o]

On Thu, May 24, 2012 at 11:34:56AM -0000, Andreas Dilger wrote:
> Currently, if e2fsck is run without the "-n" flag (i.e. it
> might modify the filesystem), there is no guarantee that it will
> open the filesystem with the EXCLUSIVE flag (i.e. O_EXCL) to
> prevent the block device from being checked (in most cases this
> means mounted, but it could also be an MD/LVM member device).
> 
> Conversely, if e2fsck is run with "-n" (i.e. read-only), and
> /etc/mtab or /proc/mounts does not report the block device as
> mounted then e2fsck thinks the filesystem is unmounted.  In this
> case, e2fsck incorrectly sets the EXCLUSIVE flag, which causes
> the check to fail, even though e2fsck is running read-only.
> 
> To fix this, do not open with EXCLUSIVE if it is a read-only check,
> and always open with EXCLUSIVE if the filesystem might be changed.
> This also prevents filesystem mounts while e2fsck is running.
> 
> Also refuse allow e2fsck to run at all if the filesystem is BUSY.
> The e2fsck check_mount() was checking for MOUNTED, but not BUSY,
> and it should refuse to run outright if the block device is BUSY.
> The previous MOUNTED heuristics pre-date the O_EXCL reservation
> by the kernel, so there could be uncertainty due to stale /etc/mtab
> data, but with newer kernels a busy device should never be modified.
> 
> Signed-off-by: Andreas Dilger <adilger@whamcloud.com>

Thanks, applied.

						- Ted

Re: e2fsck: fix checks done for mounted vs. read-only

[Andreas Dilger]

On 2012-05-28, at 8:51 AM, Ted Ts'o wrote:
> On Thu, May 24, 2012 at 11:34:56AM -0000, Andreas Dilger wrote:
>> Currently, if e2fsck is run without the "-n" flag (i.e. it
>> might modify the filesystem), there is no guarantee that it will
>> open the filesystem with the EXCLUSIVE flag (i.e. O_EXCL) to
>> prevent the block device from being checked (in most cases this
>> means mounted, but it could also be an MD/LVM member device).
>> 
>> Conversely, if e2fsck is run with "-n" (i.e. read-only), and
>> /etc/mtab or /proc/mounts does not report the block device as
>> mounted then e2fsck thinks the filesystem is unmounted.  In this
>> case, e2fsck incorrectly sets the EXCLUSIVE flag, which causes
>> the check to fail, even though e2fsck is running read-only.
>> 
>> To fix this, do not open with EXCLUSIVE if it is a read-only check,
>> and always open with EXCLUSIVE if the filesystem might be changed.
>> This also prevents filesystem mounts while e2fsck is running.
>> 
>> Also refuse allow e2fsck to run at all if the filesystem is BUSY.
>> The e2fsck check_mount() was checking for MOUNTED, but not BUSY,
>> and it should refuse to run outright if the block device is BUSY.
>> The previous MOUNTED heuristics pre-date the O_EXCL reservation
>> by the kernel, so there could be uncertainty due to stale /etc/mtab
>> data, but with newer kernels a busy device should never be modified.
>> 
>> Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
> 
> Thanks, applied.

In late-breaking news, this change prevents checking the root
filesystem at mount time, because the device is BUSY, and all
filesystem-modifying checks use O_EXCL...

This was not a problem with our original patch, but I had changed
it in discussion with Eric to prevent people from shooting
themselves in the foot when bypassing "The filesystem is mounted.
Do you really want to continue (y/n)?" questions and corrupting
their filesystem.  The revised patch tested correctly on mounted
and unmounted regular filesystems, but breaks for the root fs.

Please revert this patch, and I'll come up with a better one.

Cheers, Andreas
--
Andreas Dilger                       Whamcloud, Inc.
Principal Lustre Engineer            http://www.whamcloud.com/

[PATCH] e2fsck: allow checking on mounted root filesystem

[Andreas Dilger]

Commit 732e26b98e5c79a4298dbe341f43b54b354bb241 added checks to
prevent e2fsck from being run in filesystem-modifying mode against
a mounted or otherwise busy device, due to several bug reports of
users doing this even with the verbose warnings in check_mount().

However, it also prevented e2fsck from checking a mounted root
filesystem, which will prevent the node from booting.  Once again
allow e2fsck to run against the mounted root filesystem.

Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
---
 e2fsck/unix.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 6161e46..1c1bae8 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1221,8 +1221,11 @@ restart:
 			    &old_bitmaps);
 	if (!old_bitmaps)
 		flags |= EXT2_FLAG_64BITS;
-	if ((ctx->options & E2F_OPT_READONLY) == 0)
-		flags |= EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
+	if ((ctx->options & E2F_OPT_READONLY) == 0) {
+		flags |= EXT2_FLAG_RW;
+		if (!(ctx->mount_flags & EXT2_MF_ISROOT))
+			flags |= EXT2_FLAG_EXCLUSIVE;
+	}
 
 	retval = try_open_fs(ctx, flags, io_ptr, &fs);
 
-- 
1.7.3.4

[PATCH] e2fsck: allow checking on mounted root fs (v2)

[Andreas Dilger]

Commit 732e26b98e5c79a4298dbe341f43b54b354bb241 added checks to
prevent e2fsck from being run in filesystem-modifying mode against
a mounted or otherwise busy device, due to several bug reports of
users doing this even with the verbose warnings in check_mount().

However, it also prevented e2fsck from checking a mounted root
filesystem, which will prevent the node from booting.  Once again
allow e2fsck to run against the mounted root filesystem if it is
also mounted read-only at the time.

Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
---
 e2fsck/unix.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 6161e46..94260bd 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1221,8 +1221,12 @@ restart:
 			    &old_bitmaps);
 	if (!old_bitmaps)
 		flags |= EXT2_FLAG_64BITS;
-	if ((ctx->options & E2F_OPT_READONLY) == 0)
-		flags |= EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
+	if ((ctx->options & E2F_OPT_READONLY) == 0) {
+		flags |= EXT2_FLAG_RW;
+		if (!(ctx->mount_flags & EXT2_MF_ISROOT &&
+		      ctx->mount_flags & EXT2_MF_READONLY))
+			flags |= EXT2_FLAG_EXCLUSIVE;
+	}
 
 	retval = try_open_fs(ctx, flags, io_ptr, &fs);
 
-- 
1.7.3.4

Re: [PATCH] e2fsck: allow checking on mounted root fs (v2)

[Ted Ts'o]

On Wed, May 30, 2012 at 04:55:04PM -0600, Andreas Dilger wrote:
> Commit 732e26b98e5c79a4298dbe341f43b54b354bb241 added checks to
> prevent e2fsck from being run in filesystem-modifying mode against
> a mounted or otherwise busy device, due to several bug reports of
> users doing this even with the verbose warnings in check_mount().
> 
> However, it also prevented e2fsck from checking a mounted root
> filesystem, which will prevent the node from booting.  Once again
> allow e2fsck to run against the mounted root filesystem if it is
> also mounted read-only at the time.
> 
> Signed-off-by: Andreas Dilger <adilger@whamcloud.com>

Thanks, applied.

						- Ted

Re: [PATCH] e2fsck: allow checking on mounted root filesystem

[Andreas Dilger]

On 2012-05-30, at 12:39 PM, Andreas Dilger wrote:
> Commit 732e26b98e5c79a4298dbe341f43b54b354bb241 added checks to
> prevent e2fsck from being run in filesystem-modifying mode against
> a mounted or otherwise busy device, due to several bug reports of
> users doing this even with the verbose warnings in check_mount().
> 
> However, it also prevented e2fsck from checking a mounted root
> filesystem, which will prevent the node from booting.  Once again
> allow e2fsck to run against the mounted root filesystem.
> 
> Signed-off-by: Andreas Dilger <adilger@whamcloud.com>
> ---
> e2fsck/unix.c |    7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 6161e46..1c1bae8 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1221,8 +1221,11 @@ restart:
> 			    &old_bitmaps);
> 	if (!old_bitmaps)
> 		flags |= EXT2_FLAG_64BITS;
> -	if ((ctx->options & E2F_OPT_READONLY) == 0)
> -		flags |= EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE;
> +	if ((ctx->options & E2F_OPT_READONLY) == 0) {
> +		flags |= EXT2_FLAG_RW;
> +		if (!(ctx->mount_flags & EXT2_MF_ISROOT))
> +			flags |= EXT2_FLAG_EXCLUSIVE;
> +	}

This should further exclude root filesystems that are not mounted
read-only.  Another patch on the way.

Cheers, Andreas
--
Andreas Dilger                       Whamcloud, Inc.
Principal Lustre Engineer            http://www.whamcloud.com/