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

[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

[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

[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

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>