Chen Guanqiao <chen.chencha...@foxmail.com> writes:
> The FAT filesystem volume label can be read with FAT_IOCTL_GET_VOLUME_LABEL
> and written with FAT_IOCTL_SET_VOLUME_LABEL.
Those vol_label should be matching with volume label in root directory,
right? So I think handling only boot sector's vol_label would not work
as expected.
> +static int fat_ioctl_get_volume_label(struct inode *inode,
> + u32
> __user *user_attr)
Maybe you are using non-8 tab size, and so over 80 column.
> +{
> + struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> + u8 __user *vol_label = (u8 __user *)user_attr;
This should not use strange cast (u32 => u8), instead caller should cast
to proper one.
> + return copy_to_user(vol_label, sbi->vol_label, sizeof(sbi->vol_label));
> +}
Returning result of copy_to_user() is strange. Probably, it should
return 0 or -EFAULT.
> +static int fat_ioctl_set_volume_label(struct inode *inode,
> + u32
> __user *user_attr)
same indent issue.
> +{
> + struct buffer_head *bh;
> + struct fat_boot_sector *b;
> + struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> + u8 __user *vol_label = (u8 __user *)user_attr;
> + u8 label[11];
It should not allow to change for normal user that having only read access.
> + if (copy_from_user(label, vol_label, sizeof(label)))
> + return -EFAULT;
It should check invalid label early (e.g. lower case chars, invalid
chars, etc.).
> + if (sb_rdonly(inode->i_sb))
> + return -EFAULT;
-EROFS
> + bh = sb_bread(inode->i_sb, 0);
> + if (bh == NULL) {
> + fat_msg(inode->i_sb, KERN_ERR,
> + "unable to read boot sector to write volume
> label");
indent issue.
> + return -EFAULT;
> + }
It should take lock to prevent race.
> + b = (struct fat_boot_sector *) bh->b_data;
> +
> + if (sbi->fat_bits == 32)
> + memcpy(b->fat32.vol_label, label, sizeof(label));
> + else
> + memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> + mark_buffer_dirty(bh);
> + sync_dirty_buffer(bh);
It should check I/O error.
> long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long
> arg)
> {
u8 __user *user_vol_label = (u8 __user *)arg;
> + case FAT_IOCTL_GET_VOLUME_LABEL:
> + return fat_ioctl_get_volume_label(inode, user_attr);
> + case FAT_IOCTL_SET_VOLUME_LABEL:
> + return fat_ioctl_set_volume_label(inode, user_attr);
s/user_attr/user_vol_label/
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 20a0a89eaca5..9991500c98af 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -45,12 +45,14 @@ struct fat_bios_param_block {
>
> u8 fat16_state;
> u32 fat16_vol_id;
> + u8 fat16_vol_label[11];
> + u8 fat32_vol_label[11];
indent.
> +#define FAT_IOCTL_SET_VOLUME_LABEL _IOR('r', 0x15, __u8[11])
_IOW
Thanks.
--
OGAWA Hirofumi <hirof...@mail.parknet.co.jp>
