[PATCH] tune2fs: check whether dev is mounted or in use before setting

[PATCH] tune2fs: check whether dev is mounted or in use before setting

[Zhiqiang Liu]

From: Zhiqiang Liu <liuzhiqiang26@huawei.com>

tune2fs is used to adjust various tunable filesystem pars, which
may conflict with kernel operations. So we should check whether
device is mounted or in use at the begin similar to e2fsck and mke2fs.

Of course, we can ignore this check if -f is set.

Reported-by: Baokun Li <libaokun1@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
---
 misc/tune2fs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 458f7cf6..b667e1f4 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -3327,6 +3327,22 @@ retry_open:
 		goto closefs;
 	}

+	if (open_flag & EXT2_FLAG_RW) {
+		if (mount_flags & EXT2_MF_MOUNTED) {
+			fprintf(stderr, _("Warning! %s is mounted.\n"), device_name);
+			if (!f_flag) {
+				rc = 1;
+				goto closefs;
+			}
+		} else if (mount_flags & EXT2_MF_BUSY) {
+			fprintf(stderr, _("Warning! %s is in use by the system.\n"),
+					device_name);
+			if (!f_flag) {
+				rc = 1;
+				goto closefs;
+			}
+		}
+	}
 #ifdef NO_RECOVERY
 	/* Warn if file system needs recovery and it is opened for writing. */
 	if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) &&
-- 
2.33.0

Re: [PATCH] tune2fs: check whether dev is mounted or in use before setting

[Theodore Ts'o]

On Sat, Mar 18, 2023 at 11:36:03AM +0800, Zhiqiang Liu wrote:
> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> 
> tune2fs is used to adjust various tunable filesystem pars, which
> may conflict with kernel operations. So we should check whether
> device is mounted or in use at the begin similar to e2fsck and mke2fs.
> 
> Of course, we can ignore this check if -f is set.

Tune2fs is designed to work on mounted file systems.  There are
individual checks for those changes which can not be safely done on
mounted file systems, but most changes are safe to do on mounted file
systems.

Cheers,

					- Ted

Re: [PATCH] tune2fs: check whether dev is mounted or in use before setting

[Zhiqiang Liu]

On 2023/3/19 0:24, Theodore Ts'o wrote:
> On Sat, Mar 18, 2023 at 11:36:03AM +0800, Zhiqiang Liu wrote:
>> From: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>
>> tune2fs is used to adjust various tunable filesystem pars, which
>> may conflict with kernel operations. So we should check whether
>> device is mounted or in use at the begin similar to e2fsck and mke2fs.
>>
>> Of course, we can ignore this check if -f is set.
> 
> Tune2fs is designed to work on mounted file systems.  There are
> individual checks for those changes which can not be safely done on
> mounted file systems, but most changes are safe to do on mounted file
> systems.
> 
Thanks for your reply.

Does quota setting is safely done on mounted or busy filesystems?

We have met a problem as follows,
# mkfs.ext4 /dev/sdd
# mount /dev/sdd /test
			# /test mountpoint is used in other namespace
# umount /dev/sdd
# tune2fs -O project,quota /dev/sdd
# mount -o prjquota /dev/sdd /test
# mount | grep sdd
/dev/sdd on /test type ext4 (rw,relatime,seclabel,prjquota)
# quotaon -Ppv /test
quotaon: Mountpoint (or device) /test not found or has no quota enabled

Here, tune2fs only check whether /test is mountted when setting project,quota,
it does not check whether /test is busy (/test is mounted in other namespace).
Users will be very confused about why prjquota does no take effect.

Should we check whether mountpoint is busy when setting quota?

Zhiqiang Liu.

> Cheers,
> 
> 					- Ted
> 
> .
> 

Re: [PATCH] tune2fs: check whether dev is mounted or in use before setting

[Theodore Ts'o]

On Sun, Mar 19, 2023 at 01:44:57PM +0800, Zhiqiang Liu wrote:
> 
> Does quota setting is safely done on mounted or busy filesystems?

No, and tune2fs already disallows this.

	if (Q_flag) {
		if (mount_flags & EXT2_MF_MOUNTED) {
			fputs(_("The quota feature may only be changed when "
				"the filesystem is unmounted.\n"), stderr);
			rc = 1;
			goto closefs;
		}


> We have met a problem as follows,
> # mkfs.ext4 /dev/sdd
> # mount /dev/sdd /test
> 			# /test mountpoint is used in other namespace
> # umount /dev/sdd
> # tune2fs -O project,quota /dev/sdd

First of all, you unmounted /dev/sdd above.  If it had remained
mounted...

root@kvm-xfstests:~# mkfs.ext4 -q /dev/vdc
root@kvm-xfstests:~# mount /dev/vdc /vdc
[  103.424437] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: none.
root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc
tune2fs 1.47.0 (5-Feb-2023)
The quota feature may only be changed when the filesystem is unmounted.
root@kvm-xfstests:~# 

> # mount -o prjquota /dev/sdd /test
> # mount | grep sdd
> /dev/sdd on /test type ext4 (rw,relatime,seclabel,prjquota)
> # quotaon -Ppv /test
> quotaon: Mountpoint (or device) /test not found or has no quota enabled

Your problem is that a problem of understanding.  The quotaon and
quotaoff commands are not needed and are not supported once you use
the ext4 "quota as a first class feature".

Before the existence of the ext4 quota feature, ext4 had quota
support, but it was done using visible files (aquota.user and
aquota.group in the top-level directory of the file system).  This
older quota system had a lot of problems.  The top-level files were
visible, and could be corrupted by users.  Since you had to explicitly
enable quota support, the quota files could easily get out of sync
with reality, which required use of a separate quotacheck command;
since it ran on the mounted file system, it was (a) slow, and (b)
while quotacheck was running, if anything else created or deleted
files, the quota files could be out of sync with reality even before
the quotacheck was completed.

The new ext4 quota feature means that the moment the file system is
mounted, the quota information is updated.  You don't have the option
of turning off quota tracking (other than unmounting the file system,
and removing the quota feature, of course).  You also don't need to
run quotacheck; if the quota information is out of date, e2fsck will
notice and correct the problem.

For example:

root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc 
tune2fs 1.47.0 (5-Feb-2023)
root@kvm-xfstests:~# mount /dev/vdc /vdc
[   18.920024] EXT4-fs (vdc): recovery complete
[   18.920342] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: journalled.
root@kvm-xfstests:~# mkdir /vdc/quota
root@kvm-xfstests:~# chattr -p 123 /vdc/quota
root@kvm-xfstests:~# cp /etc/issue /vdc/quota
root@kvm-xfstests:~# repquota -P /vdc
*** Report for project quotas on device /dev/vdc
Block grace time: 7days; Inode grace time: 7days
                        Block limits                File limits
Project         used    soft    hard  grace    used  soft  hard  grace
----------------------------------------------------------------------
#0        --      24       0       0              3     0     0       
#123      --       4       0       0              1     0     0       

See?  No need to use quotaon!

> Here, tune2fs only check whether /test is mountted when setting project,quota,
> it does not check whether /test is busy (/test is mounted in other namespace).
> Users will be very confused about why prjquota does no take effect.

The question is whether or not /test is mounted, but whether or not
the device is mounted.  In your example, you actually formatted the
file system, so the device was clearly not mounted:

> # mkfs.ext4 /dev/sdd
> # mount /dev/sdd /test

Are you saying that the problem is after this point, you created
addditional mount namespaces, which where cloned off of the existing
mount namespace, and left /dev/sdd mounted there?

#1, don't do that.  #2, your patch wouldn't have helped, since you
were also only checking EXT2_MF_MOUNTED, and it works by checking
/proc/mounts.  If you are using mount namespaces, yes, it's possible
for ext2fs_check_if_mounted() to give incorrect results.  So I'm
pretty sure you must not have tested your patch before you fired it
off to me.  :-)

Now, what we *can* do if we want to bullet-proof against people using
mount namespaces and doing stupid things would be to change the test

	(mount_flags & EXT2_MF_MOUNTED)

to

	(mount_flags & (EXT2_MF_BUSY | EXT2_MF_MOUNTED)))

The EXT2_MF_BUSY flag will indicates that an attempt to open the
device with O_EXCL will fail.  We do this in some places in tune2fs.c,
but we missed this for a couple of cases, including in the tests for
I_flag and Q_flag.

Cheers,

					- Ted

Re: [PATCH] tune2fs: check whether dev is mounted or in use before setting

[Zhiqiang Liu]

On 2023/3/19 21:02, Theodore Ts'o wrote:
> On Sun, Mar 19, 2023 at 01:44:57PM +0800, Zhiqiang Liu wrote:
>>
>> Does quota setting is safely done on mounted or busy filesystems?
> 
> No, and tune2fs already disallows this.
> 
> 	if (Q_flag) {
> 		if (mount_flags & EXT2_MF_MOUNTED) {
> 			fputs(_("The quota feature may only be changed when "
> 				"the filesystem is unmounted.\n"), stderr);
> 			rc = 1;
> 			goto closefs;
> 		}
> 
> 
>> We have met a problem as follows,
>> # mkfs.ext4 /dev/sdd
>> # mount /dev/sdd /test
>> 			# /test mountpoint is used in other namespace
>> # umount /dev/sdd
>> # tune2fs -O project,quota /dev/sdd
> 
> First of all, you unmounted /dev/sdd above.  If it had remained
> mounted...
> 
> root@kvm-xfstests:~# mkfs.ext4 -q /dev/vdc
> root@kvm-xfstests:~# mount /dev/vdc /vdc
> [  103.424437] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: none.
> root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc
> tune2fs 1.47.0 (5-Feb-2023)
> The quota feature may only be changed when the filesystem is unmounted.
> root@kvm-xfstests:~# 
> 
>> # mount -o prjquota /dev/sdd /test
>> # mount | grep sdd
>> /dev/sdd on /test type ext4 (rw,relatime,seclabel,prjquota)
>> # quotaon -Ppv /test
>> quotaon: Mountpoint (or device) /test not found or has no quota enabled
> 
> Your problem is that a problem of understanding.  The quotaon and
> quotaoff commands are not needed and are not supported once you use
> the ext4 "quota as a first class feature".
> 
> Before the existence of the ext4 quota feature, ext4 had quota
> support, but it was done using visible files (aquota.user and
> aquota.group in the top-level directory of the file system).  This
> older quota system had a lot of problems.  The top-level files were
> visible, and could be corrupted by users.  Since you had to explicitly
> enable quota support, the quota files could easily get out of sync
> with reality, which required use of a separate quotacheck command;
> since it ran on the mounted file system, it was (a) slow, and (b)
> while quotacheck was running, if anything else created or deleted
> files, the quota files could be out of sync with reality even before
> the quotacheck was completed.
> 
> The new ext4 quota feature means that the moment the file system is
> mounted, the quota information is updated.  You don't have the option
> of turning off quota tracking (other than unmounting the file system,
> and removing the quota feature, of course).  You also don't need to
> run quotacheck; if the quota information is out of date, e2fsck will
> notice and correct the problem.
> 
> For example:
> 
> root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc 
> tune2fs 1.47.0 (5-Feb-2023)
> root@kvm-xfstests:~# mount /dev/vdc /vdc
> [   18.920024] EXT4-fs (vdc): recovery complete
> [   18.920342] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: journalled.
> root@kvm-xfstests:~# mkdir /vdc/quota
> root@kvm-xfstests:~# chattr -p 123 /vdc/quota
> root@kvm-xfstests:~# cp /etc/issue /vdc/quota
> root@kvm-xfstests:~# repquota -P /vdc
> *** Report for project quotas on device /dev/vdc
> Block grace time: 7days; Inode grace time: 7days
>                         Block limits                File limits
> Project         used    soft    hard  grace    used  soft  hard  grace
> ----------------------------------------------------------------------
> #0        --      24       0       0              3     0     0       
> #123      --       4       0       0              1     0     0       
> 
> See?  No need to use quotaon!
> 
Thank you very much for your patient explanation.


>> Here, tune2fs only check whether /test is mountted when setting project,quota,
>> it does not check whether /test is busy (/test is mounted in other namespace).
>> Users will be very confused about why prjquota does no take effect.
> 
> The question is whether or not /test is mounted, but whether or not
> the device is mounted.  In your example, you actually formatted the
> file system, so the device was clearly not mounted:
> 
>> # mkfs.ext4 /dev/sdd
>> # mount /dev/sdd /test
> 
> Are you saying that the problem is after this point, you created
> addditional mount namespaces, which where cloned off of the existing
> mount namespace, and left /dev/sdd mounted there?
Yes, that's exactly what I meant.
I'm so sorry for my confusing expression.

> 
> #1, don't do that.  #2, your patch wouldn't have helped, since you
> were also only checking EXT2_MF_MOUNTED, and it works by checking
> /proc/mounts.  If you are using mount namespaces, yes, it's possible
> for ext2fs_check_if_mounted() to give incorrect results.  So I'm
> pretty sure you must not have tested your patch before you fired it
> off to me.  :-)
Thanks for pointing out it.
In fact, I have tested my patch before sending it to you. In my patch,
I check both EXT2_MF_MOUNTED and EXT2_MF_BUSY flags similar to check_mount().
In my test, the /dev/sdd is unmounted in current namespace, but it was left
mounted in other mount namespace. So only checking EXT2_MF_MOUNTED does not
make sense. Actually, current tune2fs has already check EXT2_MF_MOUNTED flag
when Q_flag is set. So, I refer to check_mount() to add EXT2_MF_BUSY check.

The details as follows,

--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -3327,6 +3327,22 @@ retry_open:
 		goto closefs;
 	}

+	if (open_flag & EXT2_FLAG_RW) {
+		if (mount_flags & EXT2_MF_MOUNTED) {
+			fprintf(stderr, _("Warning! %s is mounted.\n"), device_name);
+			if (!f_flag) {
+				rc = 1;
+				goto closefs;
+			}
+		} else if (mount_flags & EXT2_MF_BUSY) {
+			fprintf(stderr, _("Warning! %s is in use by the system.\n"),
+					device_name);
+			if (!f_flag) {
+				rc = 1;
+				goto closefs;
+			}
+		}

> 
> Now, what we *can* do if we want to bullet-proof against people using
> mount namespaces and doing stupid things would be to change the test
> 
> 	(mount_flags & EXT2_MF_MOUNTED)
> 
> to
> 
> 	(mount_flags & (EXT2_MF_BUSY | EXT2_MF_MOUNTED)))
> 
> The EXT2_MF_BUSY flag will indicates that an attempt to open the
> device with O_EXCL will fail.  We do this in some places in tune2fs.c,
> but we missed this for a couple of cases, including in the tests for
> I_flag and Q_flag.
> 
> Cheers,
> 
> 					- Ted
Thanks for your suggestion.
At the begin, I wanted to add EXT2_MF_BUSY check only in the Q_flag test.
Because I didn't understand that tune2fs is designed for mounted file systems,
I added the EXT2_MF_BUSY and EXT2_MF_MOUNTED before all tests.

I will follow your suggestions to add EXT2_MF_BUSY check
in both I_flag and Q_flag tests, and send a new patch.

Thank you again for your patient explanation.

Zhiqiang Liu.

> .
>