On Nov 12, 2021, at 1:20 AM, Lukas Czerner <lczerner@xxxxxxxxxx> wrote:
>
> Implement support for FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls for
> online reading and setting of file system label.
>
> ext4_ioctl_getlabel() is simple, just get the label from the primary
> superblock bh. This might not be the first sb on the file system if
> 'sb=' mount option is used.
>
> In ext4_ioctl_setlabel() we update what ext4 currently views as a
> primary superblock and then proceed to update backup superblocks. There
> are two caveats:
> - the primary superblock might not be the first superblock and so it
> might not be the one used by userspace tools if read directly
> off the disk.
> - because the primary superblock might not be the first superblock we
> potentialy have to update it as part of backup superblock update.
> However the first sb location is a bit more complicated than the rest
> so we have to account for that.
>
> Tested with generic/492 with various configurations. I also checked the
> behavior with 'sb=' mount options, including very large file systems
> with and without sparse_super/sparse_super2.
>
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> ---
One minor issue/question inline.
> +static int ext4_ioctl_setlabel(struct file *filp, const char __user *user_label)
> +{
> + size_t len;
> + handle_t *handle;
> + ext4_group_t ngroups;
> + ext4_fsblk_t sb_block;
> + struct buffer_head *bh;
> + int ret = 0, ret2, grp;
> + unsigned long offset = 0;
> + char new_label[EXT4_LABEL_MAX + 1];
> + struct super_block *sb = file_inode(filp)->i_sb;
> + struct ext4_sb_info *sbi = EXT4_SB(sb);
> + struct ext4_super_block *es = sbi->s_es;
> +
> + /* Sanity check, this should never happen */
> + BUILD_BUG_ON(sizeof(es->s_volume_name) < EXT4_LABEL_MAX);
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> + /*
> + * Copy the maximum length allowed for ext4 label with one more to
> + * find the required terminating null byte in order to test the
> + * label length. The on disk label doesn't need to be null terminated.
> + */
> + if (copy_from_user(new_label, user_label, EXT4_LABEL_MAX + 1))
> + return -EFAULT;
> +
> + len = strnlen(new_label, EXT4_LABEL_MAX + 1);
> + if (len > EXT4_LABEL_MAX)
> + return -EINVAL;
> +
> + ret = mnt_want_write_file(filp);
> + if (ret)
> + return ret;
> +
> + handle = ext4_journal_start_sb(sb, EXT4_HT_MISC, EXT4_MAX_TRANS_DATA);
> + if (IS_ERR(handle)) {
> + ret = PTR_ERR(handle);
> + goto err_out;
> + }
> + /* Update the primary superblock first */
> + ret = ext4_journal_get_write_access(handle, sb,
> + sbi->s_sbh,
> + EXT4_JTR_NONE);
> + if (ret)
> + goto err_journal;
> +
> + lock_buffer(sbi->s_sbh);
> + memset(es->s_volume_name, 0, sizeof(es->s_volume_name));
> + memcpy(es->s_volume_name, new_label, len);
(minor) this introduces a very small window where s_volume_name is unset.
Since "new_label" is already a temporary buffer of the correct size, it
would be better IMHO to zero it out, copy the new label from userspace
into it, and then copy EXT4_LABEL_MAX bytes of new_label to s_volume_name.
It still isn't perfect, but reduces the window significantly.
> + /* Update backup superblocks */
> + ngroups = ext4_get_groups_count(sb);
> + for (grp = 0; grp < ngroups; grp++) {
:
:
> + ext4_debug("update backup superblock %llu\n", sb_block);
> + BUFFER_TRACE(bh, "get_write_access");
> + ret = ext4_journal_get_write_access(handle, sb,
> + bh,
> + EXT4_JTR_NONE);
> + if (ret) {
> + brelse(bh);
> + break;
> + }
> +
> + es = (struct ext4_super_block *) (bh->b_data + offset);
> + lock_buffer(bh);
> + if (ext4_has_metadata_csum(sb) &&
> + es->s_checksum != ext4_superblock_csum(sb, es)) {
> + ext4_msg(sb, KERN_ERR, "Invalid checksum for backup "
> + "superblock %llu\n", sb_block);
> + unlock_buffer(bh);
> + brelse(bh);
> + ret = -EFSBADCRC;
> + break;
> + }
> + memset(es->s_volume_name, 0, sizeof(es->s_volume_name));
> + memcpy(es->s_volume_name, new_label, len);
Same here.
The rest looks fine.
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
