> On Nov 29, 2021, at 2:36 AM, Lukas Czerner <lczerner@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxx>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
