Re: [PATCH] tune2fs: implement support for set/get label iocts

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 7269 bytes --]


> On Nov 29, 2021, at 2:36 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> 
> On Sat, Nov 27, 2021 at 02:23:32PM -0700, Andreas Dilger wrote:
>> On Nov 24, 2021, at 6:45 AM, Lukas Czerner <lczerner@redhat.com> wrote:
>>> 
>>> Implement support for FS_IOC_SETFSLABEL and FS_IOC_GETFSLABEL ioctls.
>>> Try to use the ioctls if possible even before we open the file system
>>> since we don't need it. Only fall back to the old method in the case the
>>> file system is not mounted, is mounted read only in the set label case,
>>> or the ioctls are not suppported by the kernel.
>>> 
>>> The new ioctls can also be supported by file system drivers other than
>>> ext4. As a result tune2fs and e2label will work for those file systems
>>> as well as long as the file system is mounted. Note that we still truncate
>>> the label exceeds the supported lenghth on extN file system family, while
>>> we keep the label intact for others.
>>> 
>>> Update tune2fs and e2label as well.
>>> 
>>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>>> ---
>>> lib/ext2fs/ext2fs.h    |  1 +
>>> lib/ext2fs/ismounted.c |  5 +++
>>> misc/e2label.8.in      |  7 ++-
>>> misc/tune2fs.8.in      |  8 +++-
>>> misc/tune2fs.c         | 96 ++++++++++++++++++++++++++++++++++++++++++
>>> 5 files changed, 114 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
>>> index 0ee0e7d0..68f9c1fe 100644
>>> --- a/lib/ext2fs/ext2fs.h
>>> +++ b/lib/ext2fs/ext2fs.h
>>> @@ -531,6 +531,7 @@ typedef struct ext2_struct_inode_scan *ext2_inode_scan;
>>> #define EXT2_MF_READONLY	4
>>> #define EXT2_MF_SWAP		8
>>> #define EXT2_MF_BUSY		16
>>> +#define EXT2_MF_EXTFS		32
>>> 
>>> /*
>>> * Ext2/linux mode flags.  We define them here so that we don't need
>>> diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c
>>> index aee7d726..c73273b8 100644
>>> --- a/lib/ext2fs/ismounted.c
>>> +++ b/lib/ext2fs/ismounted.c
>>> @@ -207,6 +207,11 @@ is_root:
>>> 			close(fd);
>>> 		(void) unlink(TEST_FILE);
>>> 	}
>>> +
>>> +	if (!strcmp(mnt->mnt_type, "ext4") ||
>>> +	    !strcmp(mnt->mnt_type, "ext3") ||
>>> +	    !strcmp(mnt->mnt_type, "ext2"))
>> 
>> IMHO, using "!strcmp(...)" reads like "not matching the string ...", so I prefer
>> to use "strcmp(...) == 0".
> 
> Hi Andreas, thanks for thre review!
> 
> Ok, I can change that.
> 
>> 
>>> +		*mount_flags |= EXT2_MF_EXTFS;
>>> 	retval = 0;
>>> errout:
>>> 	endmntent (f);
>>> diff --git a/misc/e2label.8.in b/misc/e2label.8.in
>>> index 1dc96199..fa5294c4 100644
>>> --- a/misc/e2label.8.in
>>> +++ b/misc/e2label.8.in
>>> @@ -33,7 +33,12 @@ Ext2 volume labels can be at most 16 characters long; if
>>> .I volume-label
>>> is longer than 16 characters,
>>> .B e2label
>>> -will truncate it and print a warning message.
>>> +will truncate it and print a warning message.  For other file systems that
>>> +support online label manipulation and are mounted
>>> +.B e2label
>>> +will work as well, but it will not attempt to truncate the
>>> +.I volume-label
>>> +at all.
>>> .PP
>>> It is also possible to set the volume label using the
>>> .B \-L
>>> diff --git a/misc/tune2fs.8.in b/misc/tune2fs.8.in
>>> index 1e026e5f..628dcdc0 100644
>>> --- a/misc/tune2fs.8.in
>>> +++ b/misc/tune2fs.8.in
>>> @@ -457,8 +457,12 @@ Ext2 file system labels can be at most 16 characters long; if
>>> .I volume-label
>>> is longer than 16 characters,
>>> .B tune2fs
>>> -will truncate it and print a warning.  The volume label can be used
>>> -by
>>> +will truncate it and print a warning.  For other file systems that
>>> +support online label manipulation and are mounted
>>> +.B tune2fs
>>> +will work as well, but it will not attempt to truncate the
>>> +.I volume-label
>>> +at all.  The volume label can be used by
>>> .BR mount (8),
>>> .BR fsck (8),
>>> and
>>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
>>> index 71a8e99b..6c162ba5 100644
>>> --- a/misc/tune2fs.c
>>> +++ b/misc/tune2fs.c
>>> @@ -52,6 +52,9 @@ extern int optind;
>>> #include <sys/types.h>
>>> #include <libgen.h>
>>> #include <limits.h>
>>> +#ifdef HAVE_SYS_IOCTL_H
>>> +#include <sys/ioctl.h>
>>> +#endif
>>> 
>>> #include "ext2fs/ext2_fs.h"
>>> #include "ext2fs/ext2fs.h"
>>> @@ -70,6 +73,15 @@ extern int optind;
>>> #define QOPT_ENABLE	(1)
>>> #define QOPT_DISABLE	(-1)
>>> 
>>> +#ifndef FS_IOC_SETFSLABEL
>>> +#define FSLABEL_MAX 256
>>> +#define FS_IOC_SETFSLABEL	_IOW(0x94, 50, char[FSLABEL_MAX])
>>> +#endif
>>> +
>>> +#ifndef FS_IOC_GETFSLABEL
>>> +#define FS_IOC_GETFSLABEL	_IOR(0x94, 49, char[FSLABEL_MAX])
>>> +#endif
>>> +
>>> extern int ask_yn(const char *string, int def);
>>> 
>>> const char *program_name = "tune2fs";
>>> @@ -2997,6 +3009,75 @@ fs_update_journal_user(struct ext2_super_block *sb, __u8 old_uuid[UUID_SIZE])
>>> 	return 0;
>>> }
>>> 
>>> +/*
>>> + * Use FS_IOC_SETFSLABEL or FS_IOC_GETFSLABEL to set/get file system label
>>> + * Return:	0 on success
>>> + *		1 on error
>>> + *		-1 when the old method should be used
>>> + */
>>> +int handle_fslabel(int setlabel) {
>>> +	errcode_t ret;
>>> +	int mnt_flags, fd;
>>> +	char label[FSLABEL_MAX];
>>> +	int maxlen = FSLABEL_MAX - 1;
>>> +	char mntpt[PATH_MAX + 1];
>>> +
>>> +	ret = ext2fs_check_mount_point(device_name, &mnt_flags,
>>> +					  mntpt, sizeof(mntpt));
>>> +	if (ret) {
>>> +		com_err(device_name, ret, _("while checking mount status"));
>>> +		return 1;
>>> +	}
>>> +	if (!(mnt_flags & EXT2_MF_MOUNTED) ||
>>> +	    (setlabel && (mnt_flags & EXT2_MF_READONLY)))
>>> +		return -1;
>>> +
>>> +	if (!mntpt[0]) {
>>> +		fprintf(stderr,_("Unknown mount point for %s\n"), device_name);
>>> +		return 1;
>>> +	}
>>> +
>>> +	fd = open(mntpt, O_RDONLY);
>> 
>> Opening read-only to change the label is a bit strange?  It would be better
>> to open in write mode, and verify in the kernel that this is the case:
>> 
>> 	fd = open(mntpt, setlabel ? O_WRONLY : O_RDONLY);
> 
> I am not convinced about this. Sure it may feel strange, but:
> 
> - we're not operating on the file itself but the file system in general
>   and that needs to be rw mounted; kernel will check that
> - no other fslabel implementation requires the file to be opened for
>   writing.
> - we don't even require file to be opened to writing for the most of our
>   own special ioctls if they don't deal with the file itself such as
>   EXT4_IOC_MOVE_EXT and EXT4_IOC_SWAP_BOOT
> - btrfs-progs uses O_RDONLY of setting label, fstrim uses O_RDONLY for
>   FITRIM and I am sure there are plenty more examples.
> 
> So AFAICT the standard seems to be not to require it and just open
> O_RDONLY if we really want a handle of a file system, not the file
> itself. I don't really care either way, but I am not willing to change
> what to me seems to be a standard way of doing this.
> 
> So if you insist I'll change the code here, but I won't change it on the
> kernel side to require FMODE_WRITE.

I see in the kernel patch that it is checking for CAP_SYS_ADMIN, so that
should be enough protection.

You can add my:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]