Re: [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Ethan Ferguson <ethan.ferguson@zetier.com> writes:

> Add support for writing to the volume label of a FAT filesystem via the
> FS_IOC_SETFSLABEL ioctl.
>
> Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
> ---
>  fs/fat/dir.c         | 51 +++++++++++++++++++++++++++++++++++
>  fs/fat/fat.h         |  6 +++++
>  fs/fat/file.c        | 63 ++++++++++++++++++++++++++++++++++++++++++++
>  fs/fat/inode.c       | 15 +++++++++++
>  fs/fat/namei_msdos.c |  4 +--
>  5 files changed, 137 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 07d95f1442c8..1b11713309ae 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -1425,3 +1425,54 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(fat_add_entries);
> +
> +static int fat_create_volume_label_dentry(struct super_block *sb, char *vol_label)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	struct inode *root_inode = sb->s_root->d_inode;
> +	struct msdos_dir_entry de;
> +	struct fat_slot_info sinfo;
> +	struct timespec64 ts = current_time(root_inode);
> +	__le16 date, time;
> +	u8 time_cs;
> +
> +	memcpy(de.name, vol_label, MSDOS_NAME);
> +	de.attr = ATTR_VOLUME;
> +	de.starthi = de.start = de.size = de.lcase = 0;
> +
> +	fat_time_unix2fat(sbi, &ts, &time, &date, &time_cs);
> +	de.time = time;
> +	de.date = date;
> +	if (sbi->options.isvfat) {
> +		de.cdate = de.adate = date;
> +		de.ctime = time;
> +		de.ctime_cs = time_cs;
> +	} else
> +		de.cdate = de.adate = de.ctime = de.ctime_cs = 0;
> +
> +	return fat_add_entries(root_inode, &de, 1, &sinfo);
> +}
> +
> +int fat_rename_volume_label_dentry(struct super_block *sb, char *vol_label)
> +{
> +	struct inode *root_inode = sb->s_root->d_inode;
> +	struct buffer_head *bh = NULL;
> +	struct msdos_dir_entry *de;
> +	loff_t cpos = 0;
> +	int err = 0;
> +
> +	while (1) {
> +		if (fat_get_entry(root_inode, &cpos, &bh, &de) == -1)
> +			return fat_create_volume_label_dentry(sb, vol_label);
> +
> +		if (de->attr == ATTR_VOLUME) {
> +			memcpy(de->name, vol_label, MSDOS_NAME);
> +			mark_buffer_dirty_inode(bh, root_inode);
> +			if (IS_DIRSYNC(root_inode))
> +				err = sync_dirty_buffer(bh);
> +			brelse(bh);
> +			return err;
> +		}
> +	}

I didn't check how to know the label though, the label is only if
ATTR_VOLUME? IOW, any other attributes are disallowed?

What if label is marked as deleted?

I'm not sure though, no need to update timestamps? (need to investigate
spec or windows behavior)

> +static int fat_convert_volume_label_str(struct msdos_sb_info *sbi, char *in,
> +					char *out)
> +{
> +	int ret, in_len = max(strnlen(in, FSLABEL_MAX), 11);
> +	char *needle;

Silently truncate is the common way for this ioctl?

> +	/*
> +	 * '.' is not included in any bad_chars list in this driver,
> +	 * but it is specifically not allowed for volume labels
> +	 */
> +	for (needle = in; needle - in < in_len; needle++)
> +		if (*needle == '.')
> +			return -EINVAL;

memchr() or such?

> +	ret = msdos_format_name(in, in_len, out, &sbi->options);
> +	if (ret)
> +		return ret;

> +	/*
> +	 * msdos_format_name assumes we're translating an 8.3 name, but
> +	 * we can handle 11 chars
> +	 */
> +	if (in_len > 8)
> +		ret = msdos_format_name(in + 8, in_len - 8, out + 8,
> +					&sbi->options);
> +	return ret;

fat module should not import msdos module.

> +static int fat_ioctl_set_volume_label(struct super_block *sb, char __user *arg)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	struct inode *root_inode = sb->s_root->d_inode;
> +	char from_user[FSLABEL_MAX];
> +	char new_vol_label[MSDOS_NAME];
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (sb_rdonly(sb))
> +		return -EROFS;
> +
> +	if (copy_from_user(from_user, arg, FSLABEL_MAX))
> +		return -EFAULT;
> +
> +	ret = fat_convert_volume_label_str(sbi, from_user, new_vol_label);
> +	if (ret)
> +		return ret;
> +
> +	inode_lock(root_inode);
> +	ret = fat_rename_volume_label_dentry(sb, new_vol_label);
> +	inode_unlock(root_inode);
> +	if (ret)
> +		return ret;

This rename will have to take same or similar locks with rename(2)?

> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 6f9a8cc1ad2a..a7528937383b 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -736,6 +736,21 @@ static void delayed_free(struct rcu_head *p)
>  static void fat_put_super(struct super_block *sb)
>  {
>  	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	struct buffer_head *bh = NULL;
> +	struct fat_boot_sector *bs;
> +
> +	bh = sb_bread(sb, 0);
> +	if (bh == NULL)
> +		fat_msg(sb, KERN_ERR, "unable to read boot sector");
> +	else if (!sb_rdonly(sb)) {
> +		bs = (struct fat_boot_sector *)bh->b_data;
> +		if (is_fat32(sbi))
> +			memcpy(bs->fat32.vol_label, sbi->vol_label, MSDOS_NAME);
> +		else
> +			memcpy(bs->fat16.vol_label, sbi->vol_label, MSDOS_NAME);
> +		mark_buffer_dirty(bh);
> +	}
> +	brelse(bh);

Why this unconditionally update the vol_label at unmount?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>