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

[Ethan Ferguson]


> On Feb 18, 2026, at 02:22, OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> wrote:
> 
> Ethan Ferguson <ethan.ferguson@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxx>