[PATCH v2 0/2] fat: Add FS_IOC_GETFSLABEL / FS_IOC_SETFSLABEL ioctls

[PATCH v2 0/2] fat: Add FS_IOC_GETFSLABEL / FS_IOC_SETFSLABEL ioctls

[Ethan Ferguson]

Add support for reading / writing to the volume label of a FAT filesystem
via the FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls.

Volume label changes are persisted in the volume label dentry in the root
directory as well as the bios parameter block.

v2:
* Create volume label dentry if not present
* Respect msdos shortname rules
* Add locking
* Fixed bugs (EFAULT from copy_to_user, uninitialized buffer_head)

Ethan Ferguson (2):
  fat: Add FS_IOC_GETFSLABEL ioctl
  fat: Add FS_IOC_SETFSLABEL ioctl

 fs/fat/dir.c         | 51 ++++++++++++++++++++++++++++
 fs/fat/fat.h         |  7 ++++
 fs/fat/file.c        | 79 ++++++++++++++++++++++++++++++++++++++++++++
 fs/fat/inode.c       | 26 +++++++++++++--
 fs/fat/namei_msdos.c |  4 +--
 5 files changed, 163 insertions(+), 4 deletions(-)

base-commit: 9f2693489ef8558240d9e80bfad103650daed0af
-- 
2.43.0

[PATCH v2 1/2] fat: Add FS_IOC_GETFSLABEL ioctl

[Ethan Ferguson]

Add support for reading the volume label of a FAT filesystem via the
FS_IOC_GETFSLABEL ioctl.

Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
---
 fs/fat/fat.h   |  1 +
 fs/fat/file.c  | 16 ++++++++++++++++
 fs/fat/inode.c | 11 +++++++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 0d269dba897b..4350c00dba34 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -89,6 +89,7 @@ struct msdos_sb_info {
 	int dir_per_block;	      /* dir entries per block */
 	int dir_per_block_bits;	      /* log2(dir_per_block) */
 	unsigned int vol_id;		/*volume ID*/
+	char vol_label[MSDOS_NAME];           /* volume label */
 
 	int fatent_shift;
 	const struct fatent_operations *fatent_ops;
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 124d9c5431c8..029b1750d1ec 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -153,6 +153,20 @@ static int fat_ioctl_fitrim(struct inode *inode, unsigned long arg)
 	return 0;
 }
 
+static int fat_ioctl_get_volume_label(struct super_block *sb, char __user *arg)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	int ret;
+
+	mutex_lock(&sbi->s_lock);
+	ret = copy_to_user(arg, sbi->vol_label, MSDOS_NAME);
+	mutex_unlock(&sbi->s_lock);
+	if (ret)
+		return -EFAULT;
+
+	return 0;
+}
+
 long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -165,6 +179,8 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return fat_ioctl_set_attributes(filp, user_attr);
 	case FAT_IOCTL_GET_VOLUME_ID:
 		return fat_ioctl_get_volume_id(inode, user_attr);
+	case FS_IOC_GETFSLABEL:
+		return fat_ioctl_get_volume_label(inode->i_sb, (char __user *) arg);
 	case FITRIM:
 		return fat_ioctl_fitrim(inode, arg);
 	default:
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 59fa90617b5b..6f9a8cc1ad2a 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -53,12 +53,14 @@ struct fat_bios_param_block {
 
 	u8	fat16_state;
 	u32	fat16_vol_id;
+	u8	fat16_vol_label[MSDOS_NAME];
 
 	u32	fat32_length;
 	u32	fat32_root_cluster;
 	u16	fat32_info_sector;
 	u8	fat32_state;
 	u32	fat32_vol_id;
+	u8	fat32_vol_label[MSDOS_NAME];
 };
 
 static int fat_default_codepage = CONFIG_FAT_DEFAULT_CODEPAGE;
@@ -1406,12 +1408,14 @@ static int fat_read_bpb(struct super_block *sb, struct fat_boot_sector *b,
 
 	bpb->fat16_state = b->fat16.state;
 	bpb->fat16_vol_id = get_unaligned_le32(b->fat16.vol_id);
+	memcpy(bpb->fat16_vol_label, b->fat16.vol_label, MSDOS_NAME);
 
 	bpb->fat32_length = le32_to_cpu(b->fat32.length);
 	bpb->fat32_root_cluster = le32_to_cpu(b->fat32.root_cluster);
 	bpb->fat32_info_sector = le16_to_cpu(b->fat32.info_sector);
 	bpb->fat32_state = b->fat32.state;
 	bpb->fat32_vol_id = get_unaligned_le32(b->fat32.vol_id);
+	memcpy(bpb->fat32_vol_label, b->fat32.vol_label, MSDOS_NAME);
 
 	/* Validate this looks like a FAT filesystem BPB */
 	if (!bpb->fat_reserved) {
@@ -1708,10 +1712,13 @@ int fat_fill_super(struct super_block *sb, struct fs_context *fc,
 	}
 
 	/* interpret volume ID as a little endian 32 bit integer */
-	if (is_fat32(sbi))
+	if (is_fat32(sbi)) {
 		sbi->vol_id = bpb.fat32_vol_id;
-	else /* fat 16 or 12 */
+		memcpy(sbi->vol_label, bpb.fat32_vol_label, MSDOS_NAME);
+	} else { /* fat 16 or 12 */
 		sbi->vol_id = bpb.fat16_vol_id;
+		memcpy(sbi->vol_label, bpb.fat16_vol_label, MSDOS_NAME);
+	}
 
 	__le32 vol_id_le = cpu_to_le32(sbi->vol_id);
 	super_set_uuid(sb, (void *) &vol_id_le, sizeof(vol_id_le));
-- 
2.43.0

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

[Ethan Ferguson]

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;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(fat_rename_volume_label_dentry);
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 4350c00dba34..3b75223fbe76 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -341,6 +341,8 @@ extern int fat_alloc_new_dir(struct inode *dir, struct timespec64 *ts);
 extern int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 			   struct fat_slot_info *sinfo);
 extern int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo);
+extern int fat_rename_volume_label_dentry(struct super_block *sb,
+					  char *vol_label);
 
 /* fat/fatent.c */
 struct fat_entry {
@@ -480,6 +482,10 @@ extern int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs);
 int fat_cache_init(void);
 void fat_cache_destroy(void);
 
+/* fat/namei/msdos.c */
+int msdos_format_name(const unsigned char *name, int len,
+		      unsigned char *res, struct fat_mount_options *opts);
+
 /* fat/nfs.c */
 extern const struct export_operations fat_export_ops;
 extern const struct export_operations fat_export_ops_nostale;
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 029b1750d1ec..5d445c2d8657 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -167,6 +167,67 @@ static int fat_ioctl_get_volume_label(struct super_block *sb, char __user *arg)
 	return 0;
 }
 
+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;
+
+	/*
+	 * '.' 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;
+
+	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;
+}
+
+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;
+
+	mutex_lock(&sbi->s_lock);
+	memcpy(sbi->vol_label, new_vol_label, MSDOS_NAME);
+	mutex_unlock(&sbi->s_lock);
+	return 0;
+}
+
 long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -181,6 +242,8 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return fat_ioctl_get_volume_id(inode, user_attr);
 	case FS_IOC_GETFSLABEL:
 		return fat_ioctl_get_volume_label(inode->i_sb, (char __user *) arg);
+	case FS_IOC_SETFSLABEL:
+		return fat_ioctl_set_volume_label(inode->i_sb, (char __user *) arg);
 	case FITRIM:
 		return fat_ioctl_fitrim(inode, arg);
 	default:
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);
 
 	fat_set_state(sb, 0, 0);
 
diff --git a/fs/fat/namei_msdos.c b/fs/fat/namei_msdos.c
index ba0152ed0810..92b5d387f88e 100644
--- a/fs/fat/namei_msdos.c
+++ b/fs/fat/namei_msdos.c
@@ -16,8 +16,8 @@ static unsigned char bad_chars[] = "*?<>|\"";
 static unsigned char bad_if_strict[] = "+=,; ";
 
 /***** Formats an MS-DOS file name. Rejects invalid names. */
-static int msdos_format_name(const unsigned char *name, int len,
-			     unsigned char *res, struct fat_mount_options *opts)
+int msdos_format_name(const unsigned char *name, int len, unsigned char *res,
+		      struct fat_mount_options *opts)
 	/*
 	 * name is the proposed name, len is its length, res is
 	 * the resulting name, opts->name_check is either (r)elaxed,
-- 
2.43.0

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

[kernel test robot]

Hi Ethan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9f2693489ef8558240d9e80bfad103650daed0af]

url:    https://github.com/intel-lab-lkp/linux/commits/Ethan-Ferguson/fat-Add-FS_IOC_GETFSLABEL-ioctl/20260218-071019
base:   9f2693489ef8558240d9e80bfad103650daed0af
patch link:    https://lore.kernel.org/r/20260217230628.719475-3-ethan.ferguson%40zetier.com
patch subject: [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl
config: arm-vexpress_defconfig (https://download.01.org/0day-ci/archive/20260218/202602181429.zMfoW0eg-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260218/202602181429.zMfoW0eg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602181429.zMfoW0eg-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: fs/fat/file.o: in function `fat_convert_volume_label_str':
   fs/fat/file.c:184:(.text+0x14c): undefined reference to `msdos_format_name'
>> arm-linux-gnueabi-ld: fs/fat/file.c:193:(.text+0x168): undefined reference to `msdos_format_name'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Ethan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9f2693489ef8558240d9e80bfad103650daed0af]

url:    https://github.com/intel-lab-lkp/linux/commits/Ethan-Ferguson/fat-Add-FS_IOC_GETFSLABEL-ioctl/20260218-071019
base:   9f2693489ef8558240d9e80bfad103650daed0af
patch link:    https://lore.kernel.org/r/20260217230628.719475-3-ethan.ferguson%40zetier.com
patch subject: [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl
config: i386-randconfig-014-20260218 (https://download.01.org/0day-ci/archive/20260218/202602181513.zGRm1yrD-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260218/202602181513.zGRm1yrD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602181513.zGRm1yrD-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: fs/fat/file.o: in function `fat_convert_volume_label_str':
   fs/fat/file.c:184:(.text+0x108): undefined reference to `msdos_format_name'
>> ld: fs/fat/file.c:193:(.text+0x12a): undefined reference to `msdos_format_name'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[OGAWA Hirofumi]

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>

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

[kernel test robot]

Hi Ethan,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9f2693489ef8558240d9e80bfad103650daed0af]

url:    https://github.com/intel-lab-lkp/linux/commits/Ethan-Ferguson/fat-Add-FS_IOC_GETFSLABEL-ioctl/20260218-071019
base:   9f2693489ef8558240d9e80bfad103650daed0af
patch link:    https://lore.kernel.org/r/20260217230628.719475-3-ethan.ferguson%40zetier.com
patch subject: [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl
config: x86_64-rhel-9.4-ltp (https://download.01.org/0day-ci/archive/20260218/202602181149.k8dhUKY6-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260218/202602181149.k8dhUKY6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602181149.k8dhUKY6-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "msdos_format_name" [fs/fat/fat.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Ethan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9f2693489ef8558240d9e80bfad103650daed0af]

url:    https://github.com/intel-lab-lkp/linux/commits/Ethan-Ferguson/fat-Add-FS_IOC_GETFSLABEL-ioctl/20260218-071019
base:   9f2693489ef8558240d9e80bfad103650daed0af
patch link:    https://lore.kernel.org/r/20260217230628.719475-3-ethan.ferguson%40zetier.com
patch subject: [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl
config: hexagon-randconfig-r121-20260218 (https://download.01.org/0day-ci/archive/20260219/202602190010.tPRKu8kq-lkp@intel.com/config)
compiler: clang version 23.0.0git (https://github.com/llvm/llvm-project e86750b29fa0ff207cd43213d66dabe565417638)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260219/202602190010.tPRKu8kq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602190010.tPRKu8kq-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> fs/fat/dir.c:1439:41: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [addressable] [assigned] [usertype] size @@     got unsigned char [addressable] [assigned] [usertype] lcase @@
   fs/fat/dir.c:1439:41: sparse:     expected restricted __le32 [addressable] [assigned] [usertype] size
   fs/fat/dir.c:1439:41: sparse:     got unsigned char [addressable] [assigned] [usertype] lcase
>> fs/fat/dir.c:1449:48: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [addressable] [assigned] [usertype] ctime @@     got unsigned char [addressable] [assigned] [usertype] ctime_cs @@
   fs/fat/dir.c:1449:48: sparse:     expected restricted __le16 [addressable] [assigned] [usertype] ctime
   fs/fat/dir.c:1449:48: sparse:     got unsigned char [addressable] [assigned] [usertype] ctime_cs

vim +1439 fs/fat/dir.c

  1426	
  1427	static int fat_create_volume_label_dentry(struct super_block *sb, char *vol_label)
  1428	{
  1429		struct msdos_sb_info *sbi = MSDOS_SB(sb);
  1430		struct inode *root_inode = sb->s_root->d_inode;
  1431		struct msdos_dir_entry de;
  1432		struct fat_slot_info sinfo;
  1433		struct timespec64 ts = current_time(root_inode);
  1434		__le16 date, time;
  1435		u8 time_cs;
  1436	
  1437		memcpy(de.name, vol_label, MSDOS_NAME);
  1438		de.attr = ATTR_VOLUME;
> 1439		de.starthi = de.start = de.size = de.lcase = 0;
  1440	
  1441		fat_time_unix2fat(sbi, &ts, &time, &date, &time_cs);
  1442		de.time = time;
  1443		de.date = date;
  1444		if (sbi->options.isvfat) {
  1445			de.cdate = de.adate = date;
  1446			de.ctime = time;
  1447			de.ctime_cs = time_cs;
  1448		} else
> 1449			de.cdate = de.adate = de.ctime = de.ctime_cs = 0;
  1450	
  1451		return fat_add_entries(root_inode, &de, 1, &sinfo);
  1452	}
  1453	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[syzbot ci] Re: fat: Add FS_IOC_GETFSLABEL / FS_IOC_SETFSLABEL ioctls

[syzbot ci]

syzbot ci has tested the following series

[v2] fat: Add FS_IOC_GETFSLABEL / FS_IOC_SETFSLABEL ioctls
https://lore.kernel.org/all/20260217230628.719475-1-ethan.ferguson@zetier.com
* [PATCH v2 1/2] fat: Add FS_IOC_GETFSLABEL ioctl
* [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl

and found the following issue:
KASAN: stack-out-of-bounds Write in msdos_format_name

Full report is available here:
https://ci.syzbot.org/series/da73a18c-15c6-438b-8ee3-f34978d4930c

***

KASAN: stack-out-of-bounds Write in msdos_format_name

tree:      bpf-next
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/bpf/bpf-next.git
base:      9f2693489ef8558240d9e80bfad103650daed0af
arch:      amd64
compiler:  Debian clang version 21.1.8 (++20251221033036+2078da43e25a-1~exp1~20251221153213.50), Debian LLD 21.1.8
config:    https://ci.syzbot.org/builds/972e08c3-3f0c-4dc8-b311-d7aec3efb56b/config
C repro:   https://ci.syzbot.org/findings/f100bebc-8929-4992-996b-73c5de2585ba/c_repro
syz repro: https://ci.syzbot.org/findings/f100bebc-8929-4992-996b-73c5de2585ba/syz_repro

loop0: rw=8912896, sector=1192, nr_sectors = 4 limit=256
syz.0.17: attempt to access beyond end of device
loop0: rw=8388608, sector=1192, nr_sectors = 4 limit=256
==================================================================
BUG: KASAN: stack-out-of-bounds in msdos_format_name+0x5fe/0xd90 fs/fat/namei_msdos.c:69
Write of size 1 at addr ffffc90003a27dcb by task syz.0.17/5956

CPU: 0 UID: 0 PID: 5956 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:378 [inline]
 print_report+0xba/0x230 mm/kasan/report.c:482
 kasan_report+0x117/0x150 mm/kasan/report.c:595
 msdos_format_name+0x5fe/0xd90 fs/fat/namei_msdos.c:69
 fat_convert_volume_label_str fs/fat/file.c:193 [inline]
 fat_ioctl_set_volume_label fs/fat/file.c:215 [inline]
 fat_generic_ioctl+0xebd/0x12a0 fs/fat/file.c:246
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:597 [inline]
 __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xe2/0xf80 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fa55ed9bf79
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe3b0c1748 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007fa55f015fa0 RCX: 00007fa55ed9bf79
RDX: 0000200000000240 RSI: 0000000041009432 RDI: 0000000000000004
RBP: 00007fa55ee327e0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007fa55f015fac R14: 00007fa55f015fa0 R15: 00007fa55f015fa0
 </TASK>

The buggy address belongs to stack of task syz.0.17/5956
 and is located at offset 427 in frame:
 fat_generic_ioctl+0x0/0x12a0

This frame has 4 objects:
 [32, 56) 'range.i'
 [96, 352) 'from_user.i'
 [416, 427) 'new_vol_label.i'
 [448, 528) 'ia.i'

The buggy address belongs to a 8-page vmalloc region starting at 0xffffc90003a20000 allocated at copy_process+0x508/0x3980 kernel/fork.c:2052
The buggy address belongs to the physical page:
page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1750f7
memcg:ffff888111b31502
flags: 0x57ff00000000000(node=1|zone=2|lastcpupid=0x7ff)
raw: 057ff00000000000 0000000000000000 dead000000000122 0000000000000000
raw: 0000000000000000 0000000000000000 00000001ffffffff ffff888111b31502
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x29c2(GFP_NOWAIT|__GFP_HIGHMEM|__GFP_IO|__GFP_FS|__GFP_ZERO), pid 41, tgid 41 (kworker/u10:2), ts 69338575178, free_ts 66424269276
 set_page_owner include/linux/page_owner.h:32 [inline]
 post_alloc_hook+0x228/0x280 mm/page_alloc.c:1884
 prep_new_page mm/page_alloc.c:1892 [inline]
 get_page_from_freelist+0x24dc/0x2580 mm/page_alloc.c:3945
 __alloc_frozen_pages_noprof+0x18d/0x380 mm/page_alloc.c:5240
 alloc_pages_mpol+0x232/0x4a0 mm/mempolicy.c:2486
 alloc_frozen_pages_noprof mm/mempolicy.c:2557 [inline]
 alloc_pages_noprof+0xa8/0x190 mm/mempolicy.c:2577
 vm_area_alloc_pages mm/vmalloc.c:3649 [inline]
 __vmalloc_area_node mm/vmalloc.c:3863 [inline]
 __vmalloc_node_range_noprof+0x79b/0x1730 mm/vmalloc.c:4051
 __vmalloc_node_noprof+0xc2/0x100 mm/vmalloc.c:4111
 alloc_thread_stack_node kernel/fork.c:354 [inline]
 dup_task_struct+0x228/0x9a0 kernel/fork.c:923
 copy_process+0x508/0x3980 kernel/fork.c:2052
 kernel_clone+0x248/0x870 kernel/fork.c:2651
 user_mode_thread+0x110/0x180 kernel/fork.c:2727
 call_usermodehelper_exec_sync kernel/umh.c:132 [inline]
 call_usermodehelper_exec_work+0x9c/0x230 kernel/umh.c:163
 process_one_work kernel/workqueue.c:3257 [inline]
 process_scheduled_works+0xaec/0x17a0 kernel/workqueue.c:3340
 worker_thread+0xda6/0x1360 kernel/workqueue.c:3421
 kthread+0x726/0x8b0 kernel/kthread.c:463
 ret_from_fork+0x51b/0xa40 arch/x86/kernel/process.c:158
page last free pid 5918 tgid 5918 stack trace:
 reset_page_owner include/linux/page_owner.h:25 [inline]
 free_pages_prepare mm/page_alloc.c:1433 [inline]
 __free_frozen_pages+0xbf8/0xd70 mm/page_alloc.c:2973
 discard_slab mm/slub.c:3346 [inline]
 __put_partials+0x146/0x170 mm/slub.c:3886
 __slab_free+0x294/0x320 mm/slub.c:5956
 qlink_free mm/kasan/quarantine.c:163 [inline]
 qlist_free_all+0x97/0x100 mm/kasan/quarantine.c:179
 kasan_quarantine_reduce+0x148/0x160 mm/kasan/quarantine.c:286
 __kasan_slab_alloc+0x22/0x80 mm/kasan/common.c:350
 kasan_slab_alloc include/linux/kasan.h:253 [inline]
 slab_post_alloc_hook mm/slub.c:4953 [inline]
 slab_alloc_node mm/slub.c:5263 [inline]
 __kmalloc_cache_noprof+0x36f/0x6e0 mm/slub.c:5775
 kmalloc_noprof include/linux/slab.h:957 [inline]
 kzalloc_noprof include/linux/slab.h:1094 [inline]
 ref_tracker_alloc+0x161/0x4d0 lib/ref_tracker.c:271
 __netdev_tracker_alloc include/linux/netdevice.h:4400 [inline]
 netdev_hold include/linux/netdevice.h:4429 [inline]
 netdev_queue_add_kobject net/core/net-sysfs.c:1994 [inline]
 netdev_queue_update_kobjects+0x1d1/0x6c0 net/core/net-sysfs.c:2056
 register_queue_kobjects net/core/net-sysfs.c:2119 [inline]
 netdev_register_kobject+0x258/0x310 net/core/net-sysfs.c:2362
 register_netdevice+0x12a0/0x1cd0 net/core/dev.c:11406
 register_netdev+0x40/0x60 net/core/dev.c:11522
 loopback_net_init+0x75/0x150 drivers/net/loopback.c:218
 ops_init+0x35c/0x5c0 net/core/net_namespace.c:137
 setup_net+0x118/0x340 net/core/net_namespace.c:446
 copy_net_ns+0x50e/0x730 net/core/net_namespace.c:581

Memory state around the buggy address:
 ffffc90003a27c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffffc90003a27d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffc90003a27d80: f2 f2 f2 f2 f2 f2 f2 f2 00 03 f2 f2 f8 f8 f8 f8
                                              ^
 ffffc90003a27e00: f8 f8 f8 f8 f8 f8 f3 f3 f3 f3 f3 f3 00 00 00 00
 ffffc90003a27e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.

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

[Ethan Ferguson]


> On Feb 18, 2026, at 02:22, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
> 
> 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?
I'm pretty sure ATTR_VOLUME is disallowed except for:
* Volume labels, where it is the only flag present
* Long file name entries, where it is /not/ the only flag present
This is why I check if attr == ATTR_VOLUME, not attr & ATTR_VOLUME
> What if label is marked as deleted?
As far as I know, a Volume label can never be marked as deleted, but if you want me to change the behavior of my patch, just let me know how you would like me to handle it and I'd be happy to change it.
> I'm not sure though, no need to update timestamps? (need to investigate
> spec or windows behavior)
It's not in the spec that I know either, I'm happy to remove if you deem this unnecessary.
>> +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?
When I implemented this in exfat, I returned -EINVAL for names that were longer than allowed, but only after converting from nls to UTF16. I can copy this behavior here as well.
> 
>> +    /*
>> +     * '.' 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?
Noted, will use, thanks.
>> +    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.
Fair. How would you implement checking the validity of the new volume label?
> 
>> +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)?
Sure, so should I only lock on sbi->s_lock through the whole function?
>> 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?
I can add a dirty bit to msdos_sb_info, and only write if it's present.
> Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[OGAWA Hirofumi]

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

>> I didn't check how to know the label though, the label is only if
>> ATTR_VOLUME? IOW, any other attributes are disallowed?
> I'm pretty sure ATTR_VOLUME is disallowed except for:
> * Volume labels, where it is the only flag present
> * Long file name entries, where it is /not/ the only flag present
> This is why I check if attr == ATTR_VOLUME, not attr & ATTR_VOLUME

What happen on Windows if ATTR_VOLUME | ATTR_ARCH, for example? There
are many strange behavior tools for FAT.

>> What if label is marked as deleted?
> As far as I know, a Volume label can never be marked as deleted, but if you want me to change the behavior of my patch, just let me know how you would like me to handle it and I'd be happy to change it.

That state is easily happen too, I'd like to emulate Windows behavior if
possible.

>> I'm not sure though, no need to update timestamps? (need to investigate
>> spec or windows behavior)
> It's not in the spec that I know either, I'm happy to remove if you deem this unnecessary.

Rather I'd like to know if Windows updates it or not.

>>> +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?
> When I implemented this in exfat, I returned -EINVAL for names that were longer than allowed, but only after converting from nls to UTF16. I can copy this behavior here as well.

I think we should avoid userspace adds workaround for per fs
behavior. So if possible, ioctl should behave same behavior or meaning
of error code.

>>> +    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.
> Fair. How would you implement checking the validity of the new volume label?

For example, move label verifier to fat module instead, and export to
msdos module if required.

>> This rename will have to take same or similar locks with rename(2)?
> Sure, so should I only lock on sbi->s_lock through the whole function?

Hm, I expected to take same locking with vfs what does for rename(2)
syscall path. Otherwise, this would be able to race with normal dir rename.

>>> 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?
> I can add a dirty bit to msdos_sb_info, and only write if it's present.

And why this update at unmount? Looks like it is natural to update like
normal rename.

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