[PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Ethan Ferguson]

Add support for reading / writing to the exfat volume label from the
FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls.

Implemented in similar ways to other fs drivers, namely btrfs and ext4,
where the ioctls are performed on file inodes.

v4:
Implement allocating a new cluster when the current dentry cluster would
be full as a result of inserting a volume label dentry.
v3:
Add lazy-loading of volume label into superblock.
Use better UTF-16 conversions to detect invalid characters.
If no volume label entry exists, overwrite a deleted dentry,
or create a new dentry if the cluster has space.
Link: https://lore.kernel.org/all/20250821150926.1025302-1-ethan.ferguson@zetier.com/
v2:
Fix endianness conversion as reported by kernel test robot
Link: https://lore.kernel.org/all/20250817003046.313497-1-ethan.ferguson@zetier.com/
v1:
Link: https://lore.kernel.org/all/20250815171056.103751-1-ethan.ferguson@zetier.com/

Ethan Ferguson (1):
  exfat: Add support for FS_IOC_{GET,SET}FSLABEL

 fs/exfat/exfat_fs.h  |   5 +
 fs/exfat/exfat_raw.h |   6 ++
 fs/exfat/file.c      |  88 +++++++++++++++++
 fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 323 insertions(+)

-- 
2.34.1

[PATCH v4 1/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Ethan Ferguson]

Add support for reading / writing to the exfat volume label from the
FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls

Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
---
 fs/exfat/exfat_fs.h  |   5 +
 fs/exfat/exfat_raw.h |   6 ++
 fs/exfat/file.c      |  88 +++++++++++++++++
 fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 323 insertions(+)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index f8ead4d47ef0..ed4b5ecb952b 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -267,6 +267,7 @@ struct exfat_sb_info {
 	struct buffer_head **vol_amap; /* allocation bitmap */
 
 	unsigned short *vol_utbl; /* upcase table */
+	unsigned short *volume_label; /* volume name */
 
 	unsigned int clu_srch_ptr; /* cluster search pointer */
 	unsigned int used_clusters; /* number of used clusters */
@@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct inode *inode)
 /* super.c */
 int exfat_set_volume_dirty(struct super_block *sb);
 int exfat_clear_volume_dirty(struct super_block *sb);
+int exfat_read_volume_label(struct super_block *sb);
+int exfat_write_volume_label(struct super_block *sb,
+			     struct exfat_uni_name *uniname,
+			     struct inode *root_inode);
 
 /* fatent.c */
 #define exfat_get_next_cluster(sb, pclu) exfat_ent_get(sb, *(pclu), pclu)
diff --git a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h
index 971a1ccd0e89..4082fa7b8c14 100644
--- a/fs/exfat/exfat_raw.h
+++ b/fs/exfat/exfat_raw.h
@@ -80,6 +80,7 @@
 #define BOOTSEC_OLDBPB_LEN		53
 
 #define EXFAT_FILE_NAME_LEN		15
+#define EXFAT_VOLUME_LABEL_LEN		11
 
 #define EXFAT_MIN_SECT_SIZE_BITS		9
 #define EXFAT_MAX_SECT_SIZE_BITS		12
@@ -159,6 +160,11 @@ struct exfat_dentry {
 			__le32 start_clu;
 			__le64 size;
 		} __packed upcase; /* up-case table directory entry */
+		struct {
+			__u8 char_count;
+			__le16 volume_label[EXFAT_VOLUME_LABEL_LEN];
+			__u8 reserved[8];
+		} __packed volume_label; /* volume label directory entry */
 		struct {
 			__u8 flags;
 			__u8 vendor_guid[16];
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index 538d2b6ac2ec..970e3ee57c43 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -12,6 +12,7 @@
 #include <linux/security.h>
 #include <linux/msdos_fs.h>
 #include <linux/writeback.h>
+#include "../nls/nls_ucs2_utils.h"
 
 #include "exfat_raw.h"
 #include "exfat_fs.h"
@@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block *sb, unsigned long arg)
 	return exfat_force_shutdown(sb, flags);
 }
 
+static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned long arg)
+{
+	int ret;
+	char utf8[FSLABEL_MAX] = {0};
+	struct exfat_uni_name *uniname;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+
+	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
+	if (!uniname)
+		return -ENOMEM;
+
+	ret = exfat_read_volume_label(sb);
+	if (ret < 0)
+		goto cleanup;
+
+	memcpy(uniname->name, sbi->volume_label,
+	       EXFAT_VOLUME_LABEL_LEN * sizeof(short));
+	uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
+	uniname->name_len = UniStrnlen(uniname->name, EXFAT_VOLUME_LABEL_LEN);
+
+	ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
+	if (ret < 0)
+		goto cleanup;
+
+	if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
+		ret = -EFAULT;
+		goto cleanup;
+	}
+
+	ret = 0;
+
+cleanup:
+	kfree(uniname);
+	return ret;
+}
+
+static int exfat_ioctl_set_volume_label(struct super_block *sb,
+					unsigned long arg,
+					struct inode *root_inode)
+{
+	int ret, lossy;
+	char utf8[FSLABEL_MAX];
+	struct exfat_uni_name *uniname;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
+	if (!uniname)
+		return -ENOMEM;
+
+	if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
+		ret = -EFAULT;
+		goto cleanup;
+	}
+
+	if (utf8[0]) {
+		ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8, FSLABEL_MAX),
+					 uniname, &lossy);
+		if (ret < 0)
+			goto cleanup;
+		else if (lossy & NLS_NAME_LOSSY) {
+			ret = -EINVAL;
+			goto cleanup;
+		}
+	} else {
+		uniname->name[0] = 0x0000;
+		uniname->name_len = 0;
+	}
+
+	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
+		exfat_info(sb, "Volume label length too long, truncating");
+		uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
+	}
+
+	ret = exfat_write_volume_label(sb, uniname, root_inode);
+
+cleanup:
+	kfree(uniname);
+	return ret;
+}
+
 long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
 	u32 __user *user_attr = (u32 __user *)arg;
+	struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
 
 	switch (cmd) {
 	case FAT_IOCTL_GET_ATTRIBUTES:
@@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return exfat_ioctl_shutdown(inode->i_sb, arg);
 	case FITRIM:
 		return exfat_ioctl_fitrim(inode, arg);
+	case FS_IOC_GETFSLABEL:
+		return exfat_ioctl_get_volume_label(inode->i_sb, arg);
+	case FS_IOC_SETFSLABEL:
+		return exfat_ioctl_set_volume_label(inode->i_sb, arg, root_inode);
 	default:
 		return -ENOTTY;
 	}
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 8926e63f5bb7..7931cdb4a1d1 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -18,6 +18,7 @@
 #include <linux/nls.h>
 #include <linux/buffer_head.h>
 #include <linux/magic.h>
+#include "../nls/nls_ucs2_utils.h"
 
 #include "exfat_raw.h"
 #include "exfat_fs.h"
@@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct super_block *sb)
 	return 0;
 }
 
+static int exfat_get_volume_label_ptrs(struct super_block *sb,
+				       struct buffer_head **out_bh,
+				       struct exfat_dentry **out_dentry,
+				       struct inode *root_inode)
+{
+	int i, ret;
+	unsigned int type, old_clu;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	struct exfat_chain clu;
+	struct exfat_dentry *ep, *deleted_ep = NULL;
+	struct buffer_head *bh, *deleted_bh;
+
+	clu.dir = sbi->root_dir;
+	clu.flags = ALLOC_FAT_CHAIN;
+
+	while (clu.dir != EXFAT_EOF_CLUSTER) {
+		for (i = 0; i < sbi->dentries_per_clu; i++) {
+			ep = exfat_get_dentry(sb, &clu, i, &bh);
+
+			if (!ep) {
+				ret = -EIO;
+				goto end;
+			}
+
+			type = exfat_get_entry_type(ep);
+			if (type == TYPE_DELETED && !deleted_ep && root_inode) {
+				deleted_ep = ep;
+				deleted_bh = bh;
+				continue;
+			}
+
+			if (type == TYPE_UNUSED) {
+				if (!root_inode) {
+					brelse(bh);
+					ret = -ENOENT;
+					goto end;
+				}
+
+				if (deleted_ep) {
+					brelse(bh);
+					goto end;
+				}
+
+				if (i < sbi->dentries_per_clu - 1) {
+					deleted_ep = ep;
+					deleted_bh = bh;
+
+					ep = exfat_get_dentry(sb, &clu,
+							      i + 1, &bh);
+					memset(ep, 0,
+					       sizeof(struct exfat_dentry));
+					ep->type = EXFAT_UNUSED;
+					exfat_update_bh(bh, true);
+					brelse(bh);
+
+					goto end;
+				}
+
+				// Last dentry in cluster
+				clu.size = 0;
+				old_clu = clu.dir;
+				ret = exfat_alloc_cluster(root_inode, 1,
+							  &clu, true);
+				if (ret < 0) {
+					brelse(bh);
+					goto end;
+				}
+
+				ret = exfat_ent_set(sb, old_clu, clu.dir);
+				if (ret < 0) {
+					exfat_free_cluster(root_inode, &clu);
+					brelse(bh);
+					goto end;
+				}
+
+				ret = exfat_zeroed_cluster(root_inode, clu.dir);
+				if (ret < 0) {
+					exfat_free_cluster(root_inode, &clu);
+					brelse(bh);
+					goto end;
+				}
+
+				deleted_ep = ep;
+				deleted_bh = bh;
+				goto end;
+			}
+
+			if (type == TYPE_VOLUME) {
+				*out_bh = bh;
+				*out_dentry = ep;
+
+				if (deleted_ep)
+					brelse(deleted_bh);
+
+				return 0;
+			}
+
+			brelse(bh);
+		}
+
+		if (exfat_get_next_cluster(sb, &(clu.dir))) {
+			ret = -EIO;
+			goto end;
+		}
+	}
+
+	ret = -EIO;
+
+end:
+	if (deleted_ep) {
+		*out_bh = deleted_bh;
+		*out_dentry = deleted_ep;
+		memset((*out_dentry), 0, sizeof(struct exfat_dentry));
+		(*out_dentry)->type = EXFAT_VOLUME;
+		return 0;
+	}
+
+	*out_bh = NULL;
+	*out_dentry = NULL;
+	return ret;
+}
+
+static int exfat_alloc_volume_label(struct super_block *sb)
+{
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+
+	if (sbi->volume_label)
+		return 0;
+
+
+	mutex_lock(&sbi->s_lock);
+	sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN,
+						     sizeof(short), GFP_KERNEL);
+	mutex_unlock(&sbi->s_lock);
+
+	if (!sbi->volume_label)
+		return -ENOMEM;
+
+	return 0;
+}
+
+int exfat_read_volume_label(struct super_block *sb)
+{
+	int ret, i;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	struct buffer_head *bh = NULL;
+	struct exfat_dentry *ep = NULL;
+
+	ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL);
+	// ENOENT signifies that a volume label dentry doesn't exist
+	// We will treat this as an empty volume label and not fail.
+	if (ret < 0 && ret != -ENOENT)
+		goto cleanup;
+
+	ret = exfat_alloc_volume_label(sb);
+	if (ret < 0)
+		goto cleanup;
+
+	mutex_lock(&sbi->s_lock);
+	if (!ep)
+		memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN);
+	else
+		for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
+			sbi->volume_label[i] = le16_to_cpu(ep->dentry.volume_label.volume_label[i]);
+	mutex_unlock(&sbi->s_lock);
+
+	ret = 0;
+
+cleanup:
+	if (bh)
+		brelse(bh);
+
+	return ret;
+}
+
+int exfat_write_volume_label(struct super_block *sb,
+			     struct exfat_uni_name *uniname,
+			     struct inode *root_inode)
+{
+	int ret, i;
+	struct exfat_sb_info *sbi = EXFAT_SB(sb);
+	struct buffer_head *bh = NULL;
+	struct exfat_dentry *ep = NULL;
+
+	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
+		ret = -EINVAL;
+		goto cleanup;
+	}
+
+	ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode);
+	if (ret < 0)
+		goto cleanup;
+
+	ret = exfat_alloc_volume_label(sb);
+	if (ret < 0)
+		goto cleanup;
+
+	memcpy(sbi->volume_label, uniname->name,
+	       uniname->name_len * sizeof(short));
+
+	mutex_lock(&sbi->s_lock);
+	for (i = 0; i < uniname->name_len; i++)
+		ep->dentry.volume_label.volume_label[i] =
+			cpu_to_le16(sbi->volume_label[i]);
+	// Fill the rest of the str with 0x0000
+	for (; i < EXFAT_VOLUME_LABEL_LEN; i++)
+		ep->dentry.volume_label.volume_label[i] = 0x0000;
+
+	ep->dentry.volume_label.char_count = uniname->name_len;
+	mutex_unlock(&sbi->s_lock);
+
+	ret = 0;
+
+cleanup:
+	if (bh) {
+		exfat_update_bh(bh, true);
+		brelse(bh);
+	}
+
+	return ret;
+}
+
 /* mount the file system volume */
 static int __exfat_fill_super(struct super_block *sb,
 		struct exfat_chain *root_clu)
@@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p)
 
 	unload_nls(sbi->nls_io);
 	exfat_free_upcase_table(sbi);
+	kfree(sbi->volume_label);
 	exfat_free_sbi(sbi);
 }
 
-- 
2.34.1

RE: [PATCH v4 1/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Sungjong Seo]

Hi,
> Add support for reading / writing to the exfat volume label from the
> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
> 
> Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
> ---
>  fs/exfat/exfat_fs.h  |   5 +
>  fs/exfat/exfat_raw.h |   6 ++
>  fs/exfat/file.c      |  88 +++++++++++++++++
>  fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 323 insertions(+)
> 
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index f8ead4d47ef0..ed4b5ecb952b 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -267,6 +267,7 @@ struct exfat_sb_info {
>  	struct buffer_head **vol_amap; /* allocation bitmap */
> 
>  	unsigned short *vol_utbl; /* upcase table */
> +	unsigned short *volume_label; /* volume name */
Why is it necessary to allocate and cache it? I didn't find where to reuse
it.
Is there a reason why uniname is not used directly as an argument?
Is there something I missed?

> 
>  	unsigned int clu_srch_ptr; /* cluster search pointer */
>  	unsigned int used_clusters; /* number of used clusters */
> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
> inode *inode)
[snip]
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 538d2b6ac2ec..970e3ee57c43 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -12,6 +12,7 @@
>  #include <linux/security.h>
>  #include <linux/msdos_fs.h>
>  #include <linux/writeback.h>
> +#include "../nls/nls_ucs2_utils.h"
> 
>  #include "exfat_raw.h"
>  #include "exfat_fs.h"
> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
> *sb, unsigned long arg)
>  	return exfat_force_shutdown(sb, flags);
>  }
> 
> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
> long arg)
> +{
> +	int ret;
> +	char utf8[FSLABEL_MAX] = {0};
> +	struct exfat_uni_name *uniname;
> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +
> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
> +	if (!uniname)
> +		return -ENOMEM;
> +
> +	ret = exfat_read_volume_label(sb);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	memcpy(uniname->name, sbi->volume_label,
> +	       EXFAT_VOLUME_LABEL_LEN * sizeof(short));
> +	uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
> +	uniname->name_len = UniStrnlen(uniname->name,
> EXFAT_VOLUME_LABEL_LEN);
The volume label length is stored on-disk. It makes sense to retrieve
it directly. This way, there is no need to unnecessarily include the 
NLS utility header file.

> +
> +	ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
> +		ret = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +	ret = 0;
> +
> +cleanup:
> +	kfree(uniname);
> +	return ret;
> +}
> +
> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
> +					unsigned long arg,
> +					struct inode *root_inode)
> +{
> +	int ret, lossy;
> +	char utf8[FSLABEL_MAX];
> +	struct exfat_uni_name *uniname;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
> +	if (!uniname)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
> +		ret = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +	if (utf8[0]) {
> +		ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
> FSLABEL_MAX),
> +					 uniname, &lossy);
> +		if (ret < 0)
> +			goto cleanup;
> +		else if (lossy & NLS_NAME_LOSSY) {
> +			ret = -EINVAL;
> +			goto cleanup;
> +		}
> +	} else {
> +		uniname->name[0] = 0x0000;
> +		uniname->name_len = 0;
> +	}
> +
> +	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
> +		exfat_info(sb, "Volume label length too long, truncating");
> +		uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
> +	}
> +
> +	ret = exfat_write_volume_label(sb, uniname, root_inode);
> +
> +cleanup:
> +	kfree(uniname);
> +	return ret;
> +}
> +
>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>  	struct inode *inode = file_inode(filp);
>  	u32 __user *user_attr = (u32 __user *)arg;
> +	struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
From this point, there is no need to pass root_inode. The root_inode can be
obtained directly from sb->s_root->d_inode within the function.

> 
>  	switch (cmd) {
>  	case FAT_IOCTL_GET_ATTRIBUTES:
> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
>  		return exfat_ioctl_shutdown(inode->i_sb, arg);
>  	case FITRIM:
>  		return exfat_ioctl_fitrim(inode, arg);
> +	case FS_IOC_GETFSLABEL:
> +		return exfat_ioctl_get_volume_label(inode->i_sb, arg);
> +	case FS_IOC_SETFSLABEL:
> +		return exfat_ioctl_set_volume_label(inode->i_sb, arg,
> root_inode);
>  	default:
>  		return -ENOTTY;
>  	}
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 8926e63f5bb7..7931cdb4a1d1 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -18,6 +18,7 @@
>  #include <linux/nls.h>
>  #include <linux/buffer_head.h>
>  #include <linux/magic.h>
> +#include "../nls/nls_ucs2_utils.h"
> 
>  #include "exfat_raw.h"
>  #include "exfat_fs.h"
> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
> super_block *sb)
>  	return 0;
>  }
> 
> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
> +				       struct buffer_head **out_bh,
> +				       struct exfat_dentry **out_dentry,
> +				       struct inode *root_inode)
Instead of passing root_inode, it seems more helpful to pass the
need_create condition to better understand the function's behavior.
As mentioned earlier, the root_inode can be found directly from
sb->s_root->d_inode.

> +{
> +	int i, ret;
> +	unsigned int type, old_clu;
> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +	struct exfat_chain clu;
> +	struct exfat_dentry *ep, *deleted_ep = NULL;
> +	struct buffer_head *bh, *deleted_bh;
> +
> +	clu.dir = sbi->root_dir;
> +	clu.flags = ALLOC_FAT_CHAIN;
> +
> +	while (clu.dir != EXFAT_EOF_CLUSTER) {
> +		for (i = 0; i < sbi->dentries_per_clu; i++) {
> +			ep = exfat_get_dentry(sb, &clu, i, &bh);
> +
> +			if (!ep) {
> +				ret = -EIO;
> +				goto end;
> +			}
> +
> +			type = exfat_get_entry_type(ep);
> +			if (type == TYPE_DELETED && !deleted_ep &&
root_inode)
> {
> +				deleted_ep = ep;
> +				deleted_bh = bh;
> +				continue;
> +			}
> +
> +			if (type == TYPE_UNUSED) {
> +				if (!root_inode) {
> +					brelse(bh);
> +					ret = -ENOENT;
> +					goto end;
> +				}
Too many unnecessary operations are being performed here.
1. Since the VOLUME_LABEL entry requires only one empty entry, if a DELETED
    or UNUSED entry is found, it can be used directly.
2. According to the exFAT specification, all entries after UNUSED are
    guaranteed to be UNUSED.

Therefore, there is no need to allocate additional clusters or
mark the next entry as UNUSED here.

In the case of need_create(as of now, root_inode is not null),
if the next cluster is EOF and TYPE_VOLUME, TYPE_DELETED, or TYPE_UNUSED
entries are not found, then a new cluster should be allocated.

Lastly, if a new VOLUME_LABEL entry is created, initialization of
hint_femp is required.

> +
> +				if (deleted_ep) {
> +					brelse(bh);
> +					goto end;
> +				}
> +
> +				if (i < sbi->dentries_per_clu - 1) {

> +					deleted_ep = ep;
> +					deleted_bh = bh;
> +
> +					ep = exfat_get_dentry(sb, &clu,
> +							      i + 1, &bh);
> +					memset(ep, 0,
> +					       sizeof(struct exfat_dentry));
> +					ep->type = EXFAT_UNUSED;
> +					exfat_update_bh(bh, true);
> +					brelse(bh);
> +
> +					goto end;
> +				}
> +
> +				// Last dentry in cluster
> +				clu.size = 0;
> +				old_clu = clu.dir;
> +				ret = exfat_alloc_cluster(root_inode, 1,
> +							  &clu, true);
> +				if (ret < 0) {
> +					brelse(bh);
> +					goto end;
> +				}
> +
> +				ret = exfat_ent_set(sb, old_clu, clu.dir);
> +				if (ret < 0) {
> +					exfat_free_cluster(root_inode,
&clu);
> +					brelse(bh);
> +					goto end;
> +				}
> +
> +				ret = exfat_zeroed_cluster(root_inode,
clu.dir);
> +				if (ret < 0) {
> +					exfat_free_cluster(root_inode,
&clu);
> +					brelse(bh);
> +					goto end;
> +				}
> +
> +				deleted_ep = ep;
> +				deleted_bh = bh;
> +				goto end;
> +			}
> +
> +			if (type == TYPE_VOLUME) {
> +				*out_bh = bh;
> +				*out_dentry = ep;
> +
> +				if (deleted_ep)
> +					brelse(deleted_bh);
> +
> +				return 0;
> +			}
> +
> +			brelse(bh);
> +		}
> +
> +		if (exfat_get_next_cluster(sb, &(clu.dir))) {
> +			ret = -EIO;
> +			goto end;
> +		}
> +	}
> +
> +	ret = -EIO;
> +
> +end:
> +	if (deleted_ep) {
> +		*out_bh = deleted_bh;
> +		*out_dentry = deleted_ep;
> +		memset((*out_dentry), 0, sizeof(struct exfat_dentry));
> +		(*out_dentry)->type = EXFAT_VOLUME;
> +		return 0;
> +	}
> +
> +	*out_bh = NULL;
> +	*out_dentry = NULL;
> +	return ret;
> +}
> +
> +static int exfat_alloc_volume_label(struct super_block *sb)
> +{
> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +
> +	if (sbi->volume_label)
> +		return 0;
> +
> +
> +	mutex_lock(&sbi->s_lock);
> +	sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN,
> +						     sizeof(short),
GFP_KERNEL);
> +	mutex_unlock(&sbi->s_lock);
> +
> +	if (!sbi->volume_label)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +int exfat_read_volume_label(struct super_block *sb)
> +{
> +	int ret, i;
> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +	struct buffer_head *bh = NULL;
> +	struct exfat_dentry *ep = NULL;
> +
> +	ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL);
> +	// ENOENT signifies that a volume label dentry doesn't exist
> +	// We will treat this as an empty volume label and not fail.
> +	if (ret < 0 && ret != -ENOENT)
> +		goto cleanup;
> +
> +	ret = exfat_alloc_volume_label(sb);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	mutex_lock(&sbi->s_lock);
The sbi->s_lock should be acquired from the beginning of the function.

> +	if (!ep)
> +		memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN);
If sbi->volume_label remains, a memset operation is required for
EXFAT_VOLUME_LABEL_LEN * sizeof(short).

> +	else
> +		for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
> +			sbi->volume_label[i] = le16_to_cpu(ep-
> >dentry.volume_label.volume_label[i]);
> +	mutex_unlock(&sbi->s_lock);
> +
> +	ret = 0;
> +
> +cleanup:
> +	if (bh)
> +		brelse(bh);
> +
> +	return ret;
> +}
> +
> +int exfat_write_volume_label(struct super_block *sb,
> +			     struct exfat_uni_name *uniname,
> +			     struct inode *root_inode)
> +{
> +	int ret, i;
> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +	struct buffer_head *bh = NULL;
> +	struct exfat_dentry *ep = NULL;
> +
> +	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
> +		ret = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	ret = exfat_alloc_volume_label(sb);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	memcpy(sbi->volume_label, uniname->name,
> +	       uniname->name_len * sizeof(short));
> +
> +	mutex_lock(&sbi->s_lock);
The sbi->s_lock should be acquired from the beginning of the function.

> +	for (i = 0; i < uniname->name_len; i++)
> +		ep->dentry.volume_label.volume_label[i] =
> +			cpu_to_le16(sbi->volume_label[i]);
> +	// Fill the rest of the str with 0x0000
> +	for (; i < EXFAT_VOLUME_LABEL_LEN; i++)
> +		ep->dentry.volume_label.volume_label[i] = 0x0000;
> +
> +	ep->dentry.volume_label.char_count = uniname->name_len;
> +	mutex_unlock(&sbi->s_lock);
> +
> +	ret = 0;
> +
> +cleanup:
> +	if (bh) {
> +		exfat_update_bh(bh, true);
> +		brelse(bh);
> +	}
> +
> +	return ret;
> +}
> +
>  /* mount the file system volume */
>  static int __exfat_fill_super(struct super_block *sb,
>  		struct exfat_chain *root_clu)
> @@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p)
> 
>  	unload_nls(sbi->nls_io);
>  	exfat_free_upcase_table(sbi);
> +	kfree(sbi->volume_label);
>  	exfat_free_sbi(sbi);
>  }
> 
> --
> 2.34.1

RE: [PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Ethan Ferguson]

On 8/31/25 05:50, Sungjong Seo wrote:
> Hi,
>> Add support for reading / writing to the exfat volume label from the
>> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
>>
>> Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
>> ---
>>  fs/exfat/exfat_fs.h  |   5 +
>>  fs/exfat/exfat_raw.h |   6 ++
>>  fs/exfat/file.c      |  88 +++++++++++++++++
>>  fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 323 insertions(+)
>>
>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> index f8ead4d47ef0..ed4b5ecb952b 100644
>> --- a/fs/exfat/exfat_fs.h
>> +++ b/fs/exfat/exfat_fs.h
>> @@ -267,6 +267,7 @@ struct exfat_sb_info {
>>  	struct buffer_head **vol_amap; /* allocation bitmap */
>>
>>  	unsigned short *vol_utbl; /* upcase table */
>> +	unsigned short *volume_label; /* volume name */
> Why is it necessary to allocate and cache it? I didn't find where to reuse
> it.
> Is there a reason why uniname is not used directly as an argument?
> Is there something I missed?
>
I thought it might be prudent to store it in the sbi, in case other
patches in the future might want to use the volume label. However, since
this is not necessary, I can remove it if needed.

I chose not to use a uniname, as that would require allocating lots of
data, when the maximum amount of bytes present in a volume label is 22.

>>
>>  	unsigned int clu_srch_ptr; /* cluster search pointer */
>>  	unsigned int used_clusters; /* number of used clusters */
>> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
>> inode *inode)
> [snip]
>> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
>> index 538d2b6ac2ec..970e3ee57c43 100644
>> --- a/fs/exfat/file.c
>> +++ b/fs/exfat/file.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/security.h>
>>  #include <linux/msdos_fs.h>
>>  #include <linux/writeback.h>
>> +#include "../nls/nls_ucs2_utils.h"
>>
>>  #include "exfat_raw.h"
>>  #include "exfat_fs.h"
>> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
>> *sb, unsigned long arg)
>>  	return exfat_force_shutdown(sb, flags);
>>  }
>>
>> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
>> long arg)
>> +{
>> +	int ret;
>> +	char utf8[FSLABEL_MAX] = {0};
>> +	struct exfat_uni_name *uniname;
>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +
>> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>> +	if (!uniname)
>> +		return -ENOMEM;
>> +
>> +	ret = exfat_read_volume_label(sb);
>> +	if (ret < 0)
>> +		goto cleanup;
>> +
>> +	memcpy(uniname->name, sbi->volume_label,
>> +	       EXFAT_VOLUME_LABEL_LEN * sizeof(short));
>> +	uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
>> +	uniname->name_len = UniStrnlen(uniname->name,
>> EXFAT_VOLUME_LABEL_LEN);
> The volume label length is stored on-disk. It makes sense to retrieve
> it directly. This way, there is no need to unnecessarily include the 
> NLS utility header file.
>
That's true, given I'll be removing the volume_label field from the
superblock info struct, I can rework this function to output to a
uniname struct.
>> +
>> +	ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
>> +	if (ret < 0)
>> +		goto cleanup;
>> +
>> +	if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
>> +		ret = -EFAULT;
>> +		goto cleanup;
>> +	}
>> +
>> +	ret = 0;
>> +
>> +cleanup:
>> +	kfree(uniname);
>> +	return ret;
>> +}
>> +
>> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
>> +					unsigned long arg,
>> +					struct inode *root_inode)
>> +{
>> +	int ret, lossy;
>> +	char utf8[FSLABEL_MAX];
>> +	struct exfat_uni_name *uniname;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>> +	if (!uniname)
>> +		return -ENOMEM;
>> +
>> +	if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
>> +		ret = -EFAULT;
>> +		goto cleanup;
>> +	}
>> +
>> +	if (utf8[0]) {
>> +		ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
>> FSLABEL_MAX),
>> +					 uniname, &lossy);
>> +		if (ret < 0)
>> +			goto cleanup;
>> +		else if (lossy & NLS_NAME_LOSSY) {
>> +			ret = -EINVAL;
>> +			goto cleanup;
>> +		}
>> +	} else {
>> +		uniname->name[0] = 0x0000;
>> +		uniname->name_len = 0;
>> +	}
>> +
>> +	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
>> +		exfat_info(sb, "Volume label length too long, truncating");
>> +		uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
>> +	}
>> +
>> +	ret = exfat_write_volume_label(sb, uniname, root_inode);
>> +
>> +cleanup:
>> +	kfree(uniname);
>> +	return ret;
>> +}
>> +
>>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  {
>>  	struct inode *inode = file_inode(filp);
>>  	u32 __user *user_attr = (u32 __user *)arg;
>> +	struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
> From this point, there is no need to pass root_inode. The root_inode can be
> obtained directly from sb->s_root->d_inode within the function.
>
Must have missed that, thank you!
>>
>>  	switch (cmd) {
>>  	case FAT_IOCTL_GET_ATTRIBUTES:
>> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>  		return exfat_ioctl_shutdown(inode->i_sb, arg);
>>  	case FITRIM:
>>  		return exfat_ioctl_fitrim(inode, arg);
>> +	case FS_IOC_GETFSLABEL:
>> +		return exfat_ioctl_get_volume_label(inode->i_sb, arg);
>> +	case FS_IOC_SETFSLABEL:
>> +		return exfat_ioctl_set_volume_label(inode->i_sb, arg,
>> root_inode);
>>  	default:
>>  		return -ENOTTY;
>>  	}
>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>> index 8926e63f5bb7..7931cdb4a1d1 100644
>> --- a/fs/exfat/super.c
>> +++ b/fs/exfat/super.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/nls.h>
>>  #include <linux/buffer_head.h>
>>  #include <linux/magic.h>
>> +#include "../nls/nls_ucs2_utils.h"
>>
>>  #include "exfat_raw.h"
>>  #include "exfat_fs.h"
>> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
>> super_block *sb)
>>  	return 0;
>>  }
>>
>> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
>> +				       struct buffer_head **out_bh,
>> +				       struct exfat_dentry **out_dentry,
>> +				       struct inode *root_inode)
> Instead of passing root_inode, it seems more helpful to pass the
> need_create condition to better understand the function's behavior.
> As mentioned earlier, the root_inode can be found directly from
> sb->s_root->d_inode.
>
Given I can now obtain the root inode due to your above suggestion, I
can use need_create from my previous patch (v3).
>> +{
>> +	int i, ret;
>> +	unsigned int type, old_clu;
>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +	struct exfat_chain clu;
>> +	struct exfat_dentry *ep, *deleted_ep = NULL;
>> +	struct buffer_head *bh, *deleted_bh;
>> +
>> +	clu.dir = sbi->root_dir;
>> +	clu.flags = ALLOC_FAT_CHAIN;
>> +
>> +	while (clu.dir != EXFAT_EOF_CLUSTER) {
>> +		for (i = 0; i < sbi->dentries_per_clu; i++) {
>> +			ep = exfat_get_dentry(sb, &clu, i, &bh);
>> +
>> +			if (!ep) {
>> +				ret = -EIO;
>> +				goto end;
>> +			}
>> +
>> +			type = exfat_get_entry_type(ep);
>> +			if (type == TYPE_DELETED && !deleted_ep &&
> root_inode)
>> {
>> +				deleted_ep = ep;
>> +				deleted_bh = bh;
>> +				continue;
>> +			}
>> +
>> +			if (type == TYPE_UNUSED) {
>> +				if (!root_inode) {
>> +					brelse(bh);
>> +					ret = -ENOENT;
>> +					goto end;
>> +				}
> Too many unnecessary operations are being performed here.
> 1. Since the VOLUME_LABEL entry requires only one empty entry, if a DELETED
>     or UNUSED entry is found, it can be used directly.
> 2. According to the exFAT specification, all entries after UNUSED are
>     guaranteed to be UNUSED.
> 
> Therefore, there is no need to allocate additional clusters or
> mark the next entry as UNUSED here.
> 
> In the case of need_create(as of now, root_inode is not null),
> if the next cluster is EOF and TYPE_VOLUME, TYPE_DELETED, or TYPE_UNUSED
> entries are not found, then a new cluster should be allocated.
> 
> Lastly, if a new VOLUME_LABEL entry is created, initialization of
> hint_femp is required.
>
Just so I'm sure, if the last dentry in a cluster is TYPE_UNUSED, we can
safely overwrite that with the volume label (assuming no other
TYPE_VOLUME or TYPE_DELETED have been found previously), and we don't
have to allocate a new cluster? And we would only have to allocate a
new cluster if there are no TYPE_UNUSED, TYPE_VOLUME, or TYPE_DELETED
anywhere in the chain?

As for hint_femp, should I set the root_inode's hint_femp field to
EXFAT_HINT_NONE?
>> +
>> +				if (deleted_ep) {
>> +					brelse(bh);
>> +					goto end;
>> +				}
>> +
>> +				if (i < sbi->dentries_per_clu - 1) {
> 
>> +					deleted_ep = ep;
>> +					deleted_bh = bh;
>> +
>> +					ep = exfat_get_dentry(sb, &clu,
>> +							      i + 1, &bh);
>> +					memset(ep, 0,
>> +					       sizeof(struct exfat_dentry));
>> +					ep->type = EXFAT_UNUSED;
>> +					exfat_update_bh(bh, true);
>> +					brelse(bh);
>> +
>> +					goto end;
>> +				}
>> +
>> +				// Last dentry in cluster
>> +				clu.size = 0;
>> +				old_clu = clu.dir;
>> +				ret = exfat_alloc_cluster(root_inode, 1,
>> +							  &clu, true);
>> +				if (ret < 0) {
>> +					brelse(bh);
>> +					goto end;
>> +				}
>> +
>> +				ret = exfat_ent_set(sb, old_clu, clu.dir);
>> +				if (ret < 0) {
>> +					exfat_free_cluster(root_inode,
> &clu);
>> +					brelse(bh);
>> +					goto end;
>> +				}
>> +
>> +				ret = exfat_zeroed_cluster(root_inode,
> clu.dir);
>> +				if (ret < 0) {
>> +					exfat_free_cluster(root_inode,
> &clu);
>> +					brelse(bh);
>> +					goto end;
>> +				}
>> +
>> +				deleted_ep = ep;
>> +				deleted_bh = bh;
>> +				goto end;
>> +			}
>> +
>> +			if (type == TYPE_VOLUME) {
>> +				*out_bh = bh;
>> +				*out_dentry = ep;
>> +
>> +				if (deleted_ep)
>> +					brelse(deleted_bh);
>> +
>> +				return 0;
>> +			}
>> +
>> +			brelse(bh);
>> +		}
>> +
>> +		if (exfat_get_next_cluster(sb, &(clu.dir))) {
>> +			ret = -EIO;
>> +			goto end;
>> +		}
>> +	}
>> +
>> +	ret = -EIO;
>> +
>> +end:
>> +	if (deleted_ep) {
>> +		*out_bh = deleted_bh;
>> +		*out_dentry = deleted_ep;
>> +		memset((*out_dentry), 0, sizeof(struct exfat_dentry));
>> +		(*out_dentry)->type = EXFAT_VOLUME;
>> +		return 0;
>> +	}
>> +
>> +	*out_bh = NULL;
>> +	*out_dentry = NULL;
>> +	return ret;
>> +}
>> +
>> +static int exfat_alloc_volume_label(struct super_block *sb)
>> +{
>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +
>> +	if (sbi->volume_label)
>> +		return 0;
>> +
>> +
>> +	mutex_lock(&sbi->s_lock);
>> +	sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN,
>> +						     sizeof(short),
> GFP_KERNEL);
>> +	mutex_unlock(&sbi->s_lock);
>> +
>> +	if (!sbi->volume_label)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +int exfat_read_volume_label(struct super_block *sb)
>> +{
>> +	int ret, i;
>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +	struct buffer_head *bh = NULL;
>> +	struct exfat_dentry *ep = NULL;
>> +
>> +	ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL);
>> +	// ENOENT signifies that a volume label dentry doesn't exist
>> +	// We will treat this as an empty volume label and not fail.
>> +	if (ret < 0 && ret != -ENOENT)
>> +		goto cleanup;
>> +
>> +	ret = exfat_alloc_volume_label(sb);
>> +	if (ret < 0)
>> +		goto cleanup;
>> +
>> +	mutex_lock(&sbi->s_lock);
> The sbi->s_lock should be acquired from the beginning of the function.
>
This would be better, I agree.
>> +	if (!ep)
>> +		memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN);
> If sbi->volume_label remains, a memset operation is required for
> EXFAT_VOLUME_LABEL_LEN * sizeof(short).
> 
>> +	else
>> +		for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
>> +			sbi->volume_label[i] = le16_to_cpu(ep-
>>> dentry.volume_label.volume_label[i]);
>> +	mutex_unlock(&sbi->s_lock);
>> +
>> +	ret = 0;
>> +
>> +cleanup:
>> +	if (bh)
>> +		brelse(bh);
>> +
>> +	return ret;
>> +}
>> +
>> +int exfat_write_volume_label(struct super_block *sb,
>> +			     struct exfat_uni_name *uniname,
>> +			     struct inode *root_inode)
>> +{
>> +	int ret, i;
>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +	struct buffer_head *bh = NULL;
>> +	struct exfat_dentry *ep = NULL;
>> +
>> +	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
>> +		ret = -EINVAL;
>> +		goto cleanup;
>> +	}
>> +
>> +	ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode);
>> +	if (ret < 0)
>> +		goto cleanup;
>> +
>> +	ret = exfat_alloc_volume_label(sb);
>> +	if (ret < 0)
>> +		goto cleanup;
>> +
>> +	memcpy(sbi->volume_label, uniname->name,
>> +	       uniname->name_len * sizeof(short));
>> +
>> +	mutex_lock(&sbi->s_lock);
> The sbi->s_lock should be acquired from the beginning of the function.
>
Noted, thanks!
>> +	for (i = 0; i < uniname->name_len; i++)
>> +		ep->dentry.volume_label.volume_label[i] =
>> +			cpu_to_le16(sbi->volume_label[i]);
>> +	// Fill the rest of the str with 0x0000
>> +	for (; i < EXFAT_VOLUME_LABEL_LEN; i++)
>> +		ep->dentry.volume_label.volume_label[i] = 0x0000;
>> +
>> +	ep->dentry.volume_label.char_count = uniname->name_len;
>> +	mutex_unlock(&sbi->s_lock);
>> +
>> +	ret = 0;
>> +
>> +cleanup:
>> +	if (bh) {
>> +		exfat_update_bh(bh, true);
>> +		brelse(bh);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  /* mount the file system volume */
>>  static int __exfat_fill_super(struct super_block *sb,
>>  		struct exfat_chain *root_clu)
>> @@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p)
>>
>>  	unload_nls(sbi->nls_io);
>>  	exfat_free_upcase_table(sbi);
>> +	kfree(sbi->volume_label);
>>  	exfat_free_sbi(sbi);
>>  }
>>
>> --
>> 2.34.1
> 
> 

Thanks you for the sugestions,
Ethan Ferguson

Re: [PATCH v4 1/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Yuezhang.Mo]

Hi,

I have 3 more comments.

> Add support for reading / writing to the exfat volume label from the
> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
>
> Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
> ---
>  fs/exfat/exfat_fs.h  |   5 +
>  fs/exfat/exfat_raw.h |   6 ++
>  fs/exfat/file.c      |  88 +++++++++++++++++
>  fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 323 insertions(+)
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index f8ead4d47ef0..ed4b5ecb952b 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -267,6 +267,7 @@ struct exfat_sb_info {
>       struct buffer_head **vol_amap; /* allocation bitmap */
>
>       unsigned short *vol_utbl; /* upcase table */
> +     unsigned short *volume_label; /* volume name */
>
>       unsigned int clu_srch_ptr; /* cluster search pointer */
>       unsigned int used_clusters; /* number of used clusters */
> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
> inode *inode)
[snip]
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 538d2b6ac2ec..970e3ee57c43 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -12,6 +12,7 @@
>  #include <linux/security.h>
>  #include <linux/msdos_fs.h>
>  #include <linux/writeback.h>
> +#include "../nls/nls_ucs2_utils.h"
>
>  #include "exfat_raw.h"
>  #include "exfat_fs.h"
> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
> *sb, unsigned long arg)
>       return exfat_force_shutdown(sb, flags);
>  }
>
> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
> long arg)
> +{
> +     int ret;
> +     char utf8[FSLABEL_MAX] = {0};
> +     struct exfat_uni_name *uniname;
> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +
> +     uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
> +     if (!uniname)
> +             return -ENOMEM;
> +
> +     ret = exfat_read_volume_label(sb);
> +     if (ret < 0)
> +             goto cleanup;
> +
> +     memcpy(uniname->name, sbi->volume_label,
> +            EXFAT_VOLUME_LABEL_LEN * sizeof(short));
> +     uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
> +     uniname->name_len = UniStrnlen(uniname->name,
> EXFAT_VOLUME_LABEL_LEN);
> +
> +     ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
> +     if (ret < 0)
> +             goto cleanup;
> +
> +     if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
> +             ret = -EFAULT;
> +             goto cleanup;
> +     }
> +
> +     ret = 0;
> +
> +cleanup:
> +     kfree(uniname);
> +     return ret;
> +}
> +
> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
> +                                     unsigned long arg,
> +                                     struct inode *root_inode)
> +{
> +     int ret, lossy;
> +     char utf8[FSLABEL_MAX];
> +     struct exfat_uni_name *uniname;
> +
> +     if (!capable(CAP_SYS_ADMIN))
> +             return -EPERM;
> +
> +     uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
> +     if (!uniname)
> +             return -ENOMEM;
> +
> +     if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
> +             ret = -EFAULT;
> +             goto cleanup;
> +     }
> +
> +     if (utf8[0]) {
> +             ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
> FSLABEL_MAX),
> +                                      uniname, &lossy);
> +             if (ret < 0)
> +                     goto cleanup;
> +             else if (lossy & NLS_NAME_LOSSY) {
> +                     ret = -EINVAL;
> +                     goto cleanup;
> +             }
> +     } else {
> +             uniname->name[0] = 0x0000;
> +             uniname->name_len = 0;
> +     }
> +
> +     if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
> +             exfat_info(sb, "Volume label length too long, truncating");
> +             uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
> +     }
> +
> +     ret = exfat_write_volume_label(sb, uniname, root_inode);
> +
> +cleanup:
> +     kfree(uniname);
> +     return ret;
> +}
> +
>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
>       struct inode *inode = file_inode(filp);
>       u32 __user *user_attr = (u32 __user *)arg;
> +     struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
>
>       switch (cmd) {
>       case FAT_IOCTL_GET_ATTRIBUTES:
> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
>               return exfat_ioctl_shutdown(inode->i_sb, arg);
>       case FITRIM:
>               return exfat_ioctl_fitrim(inode, arg);
> +     case FS_IOC_GETFSLABEL:
> +             return exfat_ioctl_get_volume_label(inode->i_sb, arg);
> +     case FS_IOC_SETFSLABEL:
> +             return exfat_ioctl_set_volume_label(inode->i_sb, arg,
> root_inode);
>       default:
>               return -ENOTTY;
>       }
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 8926e63f5bb7..7931cdb4a1d1 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -18,6 +18,7 @@
>  #include <linux/nls.h>
>  #include <linux/buffer_head.h>
>  #include <linux/magic.h>
> +#include "../nls/nls_ucs2_utils.h"
>
>  #include "exfat_raw.h"
>  #include "exfat_fs.h"
> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
> super_block *sb)
>       return 0;
>  }
>
> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
> +                                    struct buffer_head **out_bh,
> +                                    struct exfat_dentry **out_dentry,
> +                                    struct inode *root_inode)
> +{
> +     int i, ret;
> +     unsigned int type, old_clu;
> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +     struct exfat_chain clu;
> +     struct exfat_dentry *ep, *deleted_ep = NULL;
> +     struct buffer_head *bh, *deleted_bh;
> +
> +     clu.dir = sbi->root_dir;
> +     clu.flags = ALLOC_FAT_CHAIN;
> +
> +     while (clu.dir != EXFAT_EOF_CLUSTER) {
> +             for (i = 0; i < sbi->dentries_per_clu; i++) {
> +                     ep = exfat_get_dentry(sb, &clu, i, &bh);
> +
> +                     if (!ep) {
> +                             ret = -EIO;
> +                             goto end;
> +                     }
> +
> +                     type = exfat_get_entry_type(ep);
> +                     if (type == TYPE_DELETED && !deleted_ep && root_inode)
> {
> +                             deleted_ep = ep;
> +                             deleted_bh = bh;
> +                             continue;
> +                     }
> +
> +                     if (type == TYPE_UNUSED) {
> +                             if (!root_inode) {
> +                                     brelse(bh);
> +                                     ret = -ENOENT;
> +                                     goto end;
> +                             }
> +
> +                             if (deleted_ep) {
> +                                     brelse(bh);
> +                                     goto end;
> +                             }
> +
> +                             if (i < sbi->dentries_per_clu - 1) {
> +                                     deleted_ep = ep;
> +                                     deleted_bh = bh;
> +
> +                                     ep = exfat_get_dentry(sb, &clu,
> +                                                           i + 1, &bh);
> +                                     memset(ep, 0,
> +                                            sizeof(struct exfat_dentry));
> +                                     ep->type = EXFAT_UNUSED;
> +                                     exfat_update_bh(bh, true);
> +                                     brelse(bh);
> +
> +                                     goto end;
> +                             }
> +
> +                             // Last dentry in cluster

Please use /* */ to comment.

> +                             clu.size = 0;
> +                             old_clu = clu.dir;
> +                             ret = exfat_alloc_cluster(root_inode, 1,
> +                                                       &clu, true);
> +                             if (ret < 0) {
> +                                     brelse(bh);
> +                                     goto end;
> +                             }

In exFAT, directory size is limited to 256MB. Please add a check to return -ENOSPC
instead of allocating a new cluster if the root directory size had reached this limit. 

> +
> +                             ret = exfat_ent_set(sb, old_clu, clu.dir);
> +                             if (ret < 0) {
> +                                     exfat_free_cluster(root_inode, &clu);
> +                                     brelse(bh);
> +                                     goto end;
> +                             }
> +
> +                             ret = exfat_zeroed_cluster(root_inode, clu.dir);
> +                             if (ret < 0) {
> +                                     exfat_free_cluster(root_inode, &clu);
> +                                     brelse(bh);
> +                                     goto end;
> +                             }

After allocating a new cluster for the root directory, its size needs to be updated.

> +
> +                             deleted_ep = ep;
> +                             deleted_bh = bh;
> +                             goto end;
> +                     }
> +
> +                     if (type == TYPE_VOLUME) {
> +                             *out_bh = bh;
> +                             *out_dentry = ep;
> +
> +                             if (deleted_ep)
> +                                     brelse(deleted_bh);
> +
> +                             return 0;
> +                     }
> +
> +                     brelse(bh);
> +             }
> +
> +             if (exfat_get_next_cluster(sb, &(clu.dir))) {
> +                     ret = -EIO;
> +                     goto end;
> +             }
> +     }
> +
> +     ret = -EIO;
> +
> +end:
> +     if (deleted_ep) {
> +             *out_bh = deleted_bh;
> +             *out_dentry = deleted_ep;
> +             memset((*out_dentry), 0, sizeof(struct exfat_dentry));
> +             (*out_dentry)->type = EXFAT_VOLUME;
> +             return 0;
> +     }
> +
> +     *out_bh = NULL;
> +     *out_dentry = NULL;
> +     return ret;
> +}
> +
> +static int exfat_alloc_volume_label(struct super_block *sb)
> +{
> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +
> +     if (sbi->volume_label)
> +             return 0;
> +
> +
> +     mutex_lock(&sbi->s_lock);
> +     sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN,
> +                                                  sizeof(short), GFP_KERNEL);
> +     mutex_unlock(&sbi->s_lock);
> +
> +     if (!sbi->volume_label)
> +             return -ENOMEM;
> +
> +     return 0;
> +}
> +
> +int exfat_read_volume_label(struct super_block *sb)
> +{
> +     int ret, i;
> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +     struct buffer_head *bh = NULL;
> +     struct exfat_dentry *ep = NULL;
> +
> +     ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL);
> +     // ENOENT signifies that a volume label dentry doesn't exist
> +     // We will treat this as an empty volume label and not fail.
> +     if (ret < 0 && ret != -ENOENT)
> +             goto cleanup;
> +
> +     ret = exfat_alloc_volume_label(sb);
> +     if (ret < 0)
> +             goto cleanup;
> +
> +     mutex_lock(&sbi->s_lock);
> +     if (!ep)
> +             memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN);
> +     else
> +             for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
> +                     sbi->volume_label[i] = le16_to_cpu(ep-
> >dentry.volume_label.volume_label[i]);
> +     mutex_unlock(&sbi->s_lock);
> +
> +     ret = 0;
> +
> +cleanup:
> +     if (bh)
> +             brelse(bh);
> +
> +     return ret;
> +}
> +
> +int exfat_write_volume_label(struct super_block *sb,
> +                          struct exfat_uni_name *uniname,
> +                          struct inode *root_inode)
> +{
> +     int ret, i;
> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
> +     struct buffer_head *bh = NULL;
> +     struct exfat_dentry *ep = NULL;
> +
> +     if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
> +             ret = -EINVAL;
> +             goto cleanup;
> +     }
> +
> +     ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode);
> +     if (ret < 0)
> +             goto cleanup;
> +
> +     ret = exfat_alloc_volume_label(sb);
> +     if (ret < 0)
> +             goto cleanup;
> +
> +     memcpy(sbi->volume_label, uniname->name,
> +            uniname->name_len * sizeof(short));
> +
> +     mutex_lock(&sbi->s_lock);
> +     for (i = 0; i < uniname->name_len; i++)
> +             ep->dentry.volume_label.volume_label[i] =
> +                     cpu_to_le16(sbi->volume_label[i]);
> +     // Fill the rest of the str with 0x0000
> +     for (; i < EXFAT_VOLUME_LABEL_LEN; i++)
> +             ep->dentry.volume_label.volume_label[i] = 0x0000;
> +
> +     ep->dentry.volume_label.char_count = uniname->name_len;
> +     mutex_unlock(&sbi->s_lock);
> +
> +     ret = 0;
> +
> +cleanup:
> +     if (bh) {
> +             exfat_update_bh(bh, true);
> +             brelse(bh);
> +     }
> +
> +     return ret;
> +}
> +
>  /* mount the file system volume */
>  static int __exfat_fill_super(struct super_block *sb,
>               struct exfat_chain *root_clu)
> @@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p)
>
>       unload_nls(sbi->nls_io);
>       exfat_free_upcase_table(sbi);
> +     kfree(sbi->volume_label);
>       exfat_free_sbi(sbi);
>  }
>
> --
> 2.34.1

Re: [PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Sungjong Seo]

On 25. 9. 2. 03:02, Ethan Ferguson wrote:
> On 8/31/25 05:50, Sungjong Seo wrote:
>> Hi,
>>> Add support for reading / writing to the exfat volume label from the
>>> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
>>>
>>> Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
>>> ---
>>>  fs/exfat/exfat_fs.h  |   5 +
>>>  fs/exfat/exfat_raw.h |   6 ++
>>>  fs/exfat/file.c      |  88 +++++++++++++++++
>>>  fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 323 insertions(+)
>>>
>>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>>> index f8ead4d47ef0..ed4b5ecb952b 100644
>>> --- a/fs/exfat/exfat_fs.h
>>> +++ b/fs/exfat/exfat_fs.h
>>> @@ -267,6 +267,7 @@ struct exfat_sb_info {
>>>  	struct buffer_head **vol_amap; /* allocation bitmap */
>>>
>>>  	unsigned short *vol_utbl; /* upcase table */
>>> +	unsigned short *volume_label; /* volume name */
>> Why is it necessary to allocate and cache it? I didn't find where to reuse
>> it.
>> Is there a reason why uniname is not used directly as an argument?
>> Is there something I missed?
>>
> I thought it might be prudent to store it in the sbi, in case other
> patches in the future might want to use the volume label. However, since
> this is not necessary, I can remove it if needed.
> 
> I chose not to use a uniname, as that would require allocating lots of
> data, when the maximum amount of bytes present in a volume label is 22.
> 

What I meant was that since uniname is already assigned within the new
ioctl, I think we can use it right away. Anyway, this variable in
sbi right now seems unnecessary.

>>>
>>>  	unsigned int clu_srch_ptr; /* cluster search pointer */
>>>  	unsigned int used_clusters; /* number of used clusters */
>>> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
>>> inode *inode)
[snip[
>>> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
>>> index 538d2b6ac2ec..970e3ee57c43 100644
>>> --- a/fs/exfat/file.c
>>> +++ b/fs/exfat/file.c
>>> @@ -12,6 +12,7 @@
>>>  #include <linux/security.h>
>>>  #include <linux/msdos_fs.h>
>>>  #include <linux/writeback.h>
>>> +#include "../nls/nls_ucs2_utils.h"
>>>
>>>  #include "exfat_raw.h"
>>>  #include "exfat_fs.h"
>>> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
>>> *sb, unsigned long arg)
>>>  	return exfat_force_shutdown(sb, flags);
>>>  }
>>>
>>> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
>>> long arg)
>>> +{
>>> +	int ret;
>>> +	char utf8[FSLABEL_MAX] = {0};
>>> +	struct exfat_uni_name *uniname;
>>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>>> +
>>> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>>> +	if (!uniname)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = exfat_read_volume_label(sb);
>>> +	if (ret < 0)
>>> +		goto cleanup;
>>> +
>>> +	memcpy(uniname->name, sbi->volume_label,
>>> +	       EXFAT_VOLUME_LABEL_LEN * sizeof(short));
>>> +	uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
>>> +	uniname->name_len = UniStrnlen(uniname->name,
>>> EXFAT_VOLUME_LABEL_LEN);
>> The volume label length is stored on-disk. It makes sense to retrieve
>> it directly. This way, there is no need to unnecessarily include the 
>> NLS utility header file.
>>
> That's true, given I'll be removing the volume_label field from the
> superblock info struct, I can rework this function to output to a
> uniname struct.
>>> +
>>> +	ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
>>> +	if (ret < 0)
>>> +		goto cleanup;
>>> +
>>> +	if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
>>> +		ret = -EFAULT;
>>> +		goto cleanup;
>>> +	}
>>> +
>>> +	ret = 0;
>>> +
>>> +cleanup:
>>> +	kfree(uniname);
>>> +	return ret;
>>> +}
>>> +
>>> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
>>> +					unsigned long arg,
>>> +					struct inode *root_inode)
>>> +{
>>> +	int ret, lossy;
>>> +	char utf8[FSLABEL_MAX];
>>> +	struct exfat_uni_name *uniname;
>>> +
>>> +	if (!capable(CAP_SYS_ADMIN))
>>> +		return -EPERM;
>>> +
>>> +	uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>>> +	if (!uniname)
>>> +		return -ENOMEM;
>>> +
>>> +	if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
>>> +		ret = -EFAULT;
>>> +		goto cleanup;
>>> +	}
>>> +
>>> +	if (utf8[0]) {
>>> +		ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
>>> FSLABEL_MAX),
>>> +					 uniname, &lossy);
>>> +		if (ret < 0)
>>> +			goto cleanup;
>>> +		else if (lossy & NLS_NAME_LOSSY) {
>>> +			ret = -EINVAL;
>>> +			goto cleanup;
>>> +		}
>>> +	} else {
>>> +		uniname->name[0] = 0x0000;
>>> +		uniname->name_len = 0;
>>> +	}
>>> +
>>> +	if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
>>> +		exfat_info(sb, "Volume label length too long, truncating");
>>> +		uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
>>> +	}
>>> +
>>> +	ret = exfat_write_volume_label(sb, uniname, root_inode);
>>> +
>>> +cleanup:
>>> +	kfree(uniname);
>>> +	return ret;
>>> +}
>>> +
>>>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>>  {
>>>  	struct inode *inode = file_inode(filp);
>>>  	u32 __user *user_attr = (u32 __user *)arg;
>>> +	struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
>> From this point, there is no need to pass root_inode. The root_inode can be
>> obtained directly from sb->s_root->d_inode within the function.
>>
> Must have missed that, thank you!
>>>
>>>  	switch (cmd) {
>>>  	case FAT_IOCTL_GET_ATTRIBUTES:
>>> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
>>> unsigned long arg)
>>>  		return exfat_ioctl_shutdown(inode->i_sb, arg);
>>>  	case FITRIM:
>>>  		return exfat_ioctl_fitrim(inode, arg);
>>> +	case FS_IOC_GETFSLABEL:
>>> +		return exfat_ioctl_get_volume_label(inode->i_sb, arg);
>>> +	case FS_IOC_SETFSLABEL:
>>> +		return exfat_ioctl_set_volume_label(inode->i_sb, arg,
>>> root_inode);
>>>  	default:
>>>  		return -ENOTTY;
>>>  	}
>>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>>> index 8926e63f5bb7..7931cdb4a1d1 100644
>>> --- a/fs/exfat/super.c
>>> +++ b/fs/exfat/super.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/nls.h>
>>>  #include <linux/buffer_head.h>
>>>  #include <linux/magic.h>
>>> +#include "../nls/nls_ucs2_utils.h"
>>>
>>>  #include "exfat_raw.h"
>>>  #include "exfat_fs.h"
>>> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
>>> super_block *sb)
>>>  	return 0;
>>>  }
>>>
>>> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
>>> +				       struct buffer_head **out_bh,
>>> +				       struct exfat_dentry **out_dentry,
>>> +				       struct inode *root_inode)
>> Instead of passing root_inode, it seems more helpful to pass the
>> need_create condition to better understand the function's behavior.
>> As mentioned earlier, the root_inode can be found directly from
>> sb->s_root->d_inode.
>>
> Given I can now obtain the root inode due to your above suggestion, I
> can use need_create from my previous patch (v3).
>>> +{
>>> +	int i, ret;
>>> +	unsigned int type, old_clu;
>>> +	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>>> +	struct exfat_chain clu;
>>> +	struct exfat_dentry *ep, *deleted_ep = NULL;
>>> +	struct buffer_head *bh, *deleted_bh;
>>> +
>>> +	clu.dir = sbi->root_dir;
>>> +	clu.flags = ALLOC_FAT_CHAIN;
>>> +
>>> +	while (clu.dir != EXFAT_EOF_CLUSTER) {
>>> +		for (i = 0; i < sbi->dentries_per_clu; i++) {
>>> +			ep = exfat_get_dentry(sb, &clu, i, &bh);
>>> +
>>> +			if (!ep) {
>>> +				ret = -EIO;
>>> +				goto end;
>>> +			}
>>> +
>>> +			type = exfat_get_entry_type(ep);
>>> +			if (type == TYPE_DELETED && !deleted_ep &&
>> root_inode)
>>> {
>>> +				deleted_ep = ep;
>>> +				deleted_bh = bh;
>>> +				continue;
>>> +			}
>>> +
>>> +			if (type == TYPE_UNUSED) {
>>> +				if (!root_inode) {
>>> +					brelse(bh);
>>> +					ret = -ENOENT;
>>> +					goto end;
>>> +				}
>> Too many unnecessary operations are being performed here.
>> 1. Since the VOLUME_LABEL entry requires only one empty entry, if a DELETED
>>     or UNUSED entry is found, it can be used directly.
>> 2. According to the exFAT specification, all entries after UNUSED are
>>     guaranteed to be UNUSED.
>>
>> Therefore, there is no need to allocate additional clusters or
>> mark the next entry as UNUSED here.
>>
>> In the case of need_create(as of now, root_inode is not null),
>> if the next cluster is EOF and TYPE_VOLUME, TYPE_DELETED, or TYPE_UNUSED
>> entries are not found, then a new cluster should be allocated.
>>
>> Lastly, if a new VOLUME_LABEL entry is created, initialization of
>> hint_femp is required.
>>
> Just so I'm sure, if the last dentry in a cluster is TYPE_UNUSED, we can
> safely overwrite that with the volume label (assuming no other
> TYPE_VOLUME or TYPE_DELETED have been found previously), and we don't
> have to allocate a new cluster? And we would only have to allocate a
> new cluster if there are no TYPE_UNUSED, TYPE_VOLUME, or TYPE_DELETED
> anywhere in the chain?
Yes, that's right. If the root directory has an empty entry to use as
a volume label, no additional cluster is required at this point.

> 
> As for hint_femp, should I set the root_inode's hint_femp field to
> EXFAT_HINT_NONE?
I think so.

> Thanks you for the sugestions,
> Ethan Ferguson

Thanks!

RE: [PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Ethan Ferguson]

On 9/2/25 00:55, Yuezhang.Mo@sony.com wrote:
> Hi,
> 
> I have 3 more comments.
> 
>> Add support for reading / writing to the exfat volume label from the
>> FS_IOC_GETFSLABEL and FS_IOC_SETFSLABEL ioctls
>>
>> Signed-off-by: Ethan Ferguson <ethan.ferguson@zetier.com>
>> ---
>>  fs/exfat/exfat_fs.h  |   5 +
>>  fs/exfat/exfat_raw.h |   6 ++
>>  fs/exfat/file.c      |  88 +++++++++++++++++
>>  fs/exfat/super.c     | 224 +++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 323 insertions(+)
>>
>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> index f8ead4d47ef0..ed4b5ecb952b 100644
>> --- a/fs/exfat/exfat_fs.h
>> +++ b/fs/exfat/exfat_fs.h
>> @@ -267,6 +267,7 @@ struct exfat_sb_info {
>>       struct buffer_head **vol_amap; /* allocation bitmap */
>>
>>       unsigned short *vol_utbl; /* upcase table */
>> +     unsigned short *volume_label; /* volume name */
>>
>>       unsigned int clu_srch_ptr; /* cluster search pointer */
>>       unsigned int used_clusters; /* number of used clusters */
>> @@ -431,6 +432,10 @@ static inline loff_t exfat_ondisk_size(const struct
>> inode *inode)
> [snip]
>> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
>> index 538d2b6ac2ec..970e3ee57c43 100644
>> --- a/fs/exfat/file.c
>> +++ b/fs/exfat/file.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/security.h>
>>  #include <linux/msdos_fs.h>
>>  #include <linux/writeback.h>
>> +#include "../nls/nls_ucs2_utils.h"
>>
>>  #include "exfat_raw.h"
>>  #include "exfat_fs.h"
>> @@ -486,10 +487,93 @@ static int exfat_ioctl_shutdown(struct super_block
>> *sb, unsigned long arg)
>>       return exfat_force_shutdown(sb, flags);
>>  }
>>
>> +static int exfat_ioctl_get_volume_label(struct super_block *sb, unsigned
>> long arg)
>> +{
>> +     int ret;
>> +     char utf8[FSLABEL_MAX] = {0};
>> +     struct exfat_uni_name *uniname;
>> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +
>> +     uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>> +     if (!uniname)
>> +             return -ENOMEM;
>> +
>> +     ret = exfat_read_volume_label(sb);
>> +     if (ret < 0)
>> +             goto cleanup;
>> +
>> +     memcpy(uniname->name, sbi->volume_label,
>> +            EXFAT_VOLUME_LABEL_LEN * sizeof(short));
>> +     uniname->name[EXFAT_VOLUME_LABEL_LEN] = 0x0000;
>> +     uniname->name_len = UniStrnlen(uniname->name,
>> EXFAT_VOLUME_LABEL_LEN);
>> +
>> +     ret = exfat_utf16_to_nls(sb, uniname, utf8, FSLABEL_MAX);
>> +     if (ret < 0)
>> +             goto cleanup;
>> +
>> +     if (copy_to_user((char __user *)arg, utf8, FSLABEL_MAX)) {
>> +             ret = -EFAULT;
>> +             goto cleanup;
>> +     }
>> +
>> +     ret = 0;
>> +
>> +cleanup:
>> +     kfree(uniname);
>> +     return ret;
>> +}
>> +
>> +static int exfat_ioctl_set_volume_label(struct super_block *sb,
>> +                                     unsigned long arg,
>> +                                     struct inode *root_inode)
>> +{
>> +     int ret, lossy;
>> +     char utf8[FSLABEL_MAX];
>> +     struct exfat_uni_name *uniname;
>> +
>> +     if (!capable(CAP_SYS_ADMIN))
>> +             return -EPERM;
>> +
>> +     uniname = kmalloc(sizeof(struct exfat_uni_name), GFP_KERNEL);
>> +     if (!uniname)
>> +             return -ENOMEM;
>> +
>> +     if (copy_from_user(utf8, (char __user *)arg, FSLABEL_MAX)) {
>> +             ret = -EFAULT;
>> +             goto cleanup;
>> +     }
>> +
>> +     if (utf8[0]) {
>> +             ret = exfat_nls_to_utf16(sb, utf8, strnlen(utf8,
>> FSLABEL_MAX),
>> +                                      uniname, &lossy);
>> +             if (ret < 0)
>> +                     goto cleanup;
>> +             else if (lossy & NLS_NAME_LOSSY) {
>> +                     ret = -EINVAL;
>> +                     goto cleanup;
>> +             }
>> +     } else {
>> +             uniname->name[0] = 0x0000;
>> +             uniname->name_len = 0;
>> +     }
>> +
>> +     if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
>> +             exfat_info(sb, "Volume label length too long, truncating");
>> +             uniname->name_len = EXFAT_VOLUME_LABEL_LEN;
>> +     }
>> +
>> +     ret = exfat_write_volume_label(sb, uniname, root_inode);
>> +
>> +cleanup:
>> +     kfree(uniname);
>> +     return ret;
>> +}
>> +
>>  long exfat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  {
>>       struct inode *inode = file_inode(filp);
>>       u32 __user *user_attr = (u32 __user *)arg;
>> +     struct inode *root_inode = filp->f_path.mnt->mnt_root->d_inode;
>>
>>       switch (cmd) {
>>       case FAT_IOCTL_GET_ATTRIBUTES:
>> @@ -500,6 +584,10 @@ long exfat_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>               return exfat_ioctl_shutdown(inode->i_sb, arg);
>>       case FITRIM:
>>               return exfat_ioctl_fitrim(inode, arg);
>> +     case FS_IOC_GETFSLABEL:
>> +             return exfat_ioctl_get_volume_label(inode->i_sb, arg);
>> +     case FS_IOC_SETFSLABEL:
>> +             return exfat_ioctl_set_volume_label(inode->i_sb, arg,
>> root_inode);
>>       default:
>>               return -ENOTTY;
>>       }
>> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
>> index 8926e63f5bb7..7931cdb4a1d1 100644
>> --- a/fs/exfat/super.c
>> +++ b/fs/exfat/super.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/nls.h>
>>  #include <linux/buffer_head.h>
>>  #include <linux/magic.h>
>> +#include "../nls/nls_ucs2_utils.h"
>>
>>  #include "exfat_raw.h"
>>  #include "exfat_fs.h"
>> @@ -573,6 +574,228 @@ static int exfat_verify_boot_region(struct
>> super_block *sb)
>>       return 0;
>>  }
>>
>> +static int exfat_get_volume_label_ptrs(struct super_block *sb,
>> +                                    struct buffer_head **out_bh,
>> +                                    struct exfat_dentry **out_dentry,
>> +                                    struct inode *root_inode)
>> +{
>> +     int i, ret;
>> +     unsigned int type, old_clu;
>> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +     struct exfat_chain clu;
>> +     struct exfat_dentry *ep, *deleted_ep = NULL;
>> +     struct buffer_head *bh, *deleted_bh;
>> +
>> +     clu.dir = sbi->root_dir;
>> +     clu.flags = ALLOC_FAT_CHAIN;
>> +
>> +     while (clu.dir != EXFAT_EOF_CLUSTER) {
>> +             for (i = 0; i < sbi->dentries_per_clu; i++) {
>> +                     ep = exfat_get_dentry(sb, &clu, i, &bh);
>> +
>> +                     if (!ep) {
>> +                             ret = -EIO;
>> +                             goto end;
>> +                     }
>> +
>> +                     type = exfat_get_entry_type(ep);
>> +                     if (type == TYPE_DELETED && !deleted_ep && root_inode)
>> {
>> +                             deleted_ep = ep;
>> +                             deleted_bh = bh;
>> +                             continue;
>> +                     }
>> +
>> +                     if (type == TYPE_UNUSED) {
>> +                             if (!root_inode) {
>> +                                     brelse(bh);
>> +                                     ret = -ENOENT;
>> +                                     goto end;
>> +                             }
>> +
>> +                             if (deleted_ep) {
>> +                                     brelse(bh);
>> +                                     goto end;
>> +                             }
>> +
>> +                             if (i < sbi->dentries_per_clu - 1) {
>> +                                     deleted_ep = ep;
>> +                                     deleted_bh = bh;
>> +
>> +                                     ep = exfat_get_dentry(sb, &clu,
>> +                                                           i + 1, &bh);
>> +                                     memset(ep, 0,
>> +                                            sizeof(struct exfat_dentry));
>> +                                     ep->type = EXFAT_UNUSED;
>> +                                     exfat_update_bh(bh, true);
>> +                                     brelse(bh);
>> +
>> +                                     goto end;
>> +                             }
>> +
>> +                             // Last dentry in cluster
> 
> Please use /* */ to comment.
> 
>> +                             clu.size = 0;
>> +                             old_clu = clu.dir;
>> +                             ret = exfat_alloc_cluster(root_inode, 1,
>> +                                                       &clu, true);
>> +                             if (ret < 0) {
>> +                                     brelse(bh);
>> +                                     goto end;
>> +                             }
> 
> In exFAT, directory size is limited to 256MB. Please add a check to return -ENOSPC
> instead of allocating a new cluster if the root directory size had reached this limit. 
>
Noted. I am switching over to using exfat_find_empty_entry, which
checks for this.
>> +
>> +                             ret = exfat_ent_set(sb, old_clu, clu.dir);
>> +                             if (ret < 0) {
>> +                                     exfat_free_cluster(root_inode, &clu);
>> +                                     brelse(bh);
>> +                                     goto end;
>> +                             }
>> +
>> +                             ret = exfat_zeroed_cluster(root_inode, clu.dir);
>> +                             if (ret < 0) {
>> +                                     exfat_free_cluster(root_inode, &clu);
>> +                                     brelse(bh);
>> +                                     goto end;
>> +                             }
> 
> After allocating a new cluster for the root directory, its size needs to be updated.
>
Where would I update the size? I don't think the root directory has a
Stream Extension dentry, would I increment the exfat_inode_info.dir.size
field?
>> +
>> +                             deleted_ep = ep;
>> +                             deleted_bh = bh;
>> +                             goto end;
>> +                     }
>> +
>> +                     if (type == TYPE_VOLUME) {
>> +                             *out_bh = bh;
>> +                             *out_dentry = ep;
>> +
>> +                             if (deleted_ep)
>> +                                     brelse(deleted_bh);
>> +
>> +                             return 0;
>> +                     }
>> +
>> +                     brelse(bh);
>> +             }
>> +
>> +             if (exfat_get_next_cluster(sb, &(clu.dir))) {
>> +                     ret = -EIO;
>> +                     goto end;
>> +             }
>> +     }
>> +
>> +     ret = -EIO;
>> +
>> +end:
>> +     if (deleted_ep) {
>> +             *out_bh = deleted_bh;
>> +             *out_dentry = deleted_ep;
>> +             memset((*out_dentry), 0, sizeof(struct exfat_dentry));
>> +             (*out_dentry)->type = EXFAT_VOLUME;
>> +             return 0;
>> +     }
>> +
>> +     *out_bh = NULL;
>> +     *out_dentry = NULL;
>> +     return ret;
>> +}
>> +
>> +static int exfat_alloc_volume_label(struct super_block *sb)
>> +{
>> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +
>> +     if (sbi->volume_label)
>> +             return 0;
>> +
>> +
>> +     mutex_lock(&sbi->s_lock);
>> +     sbi->volume_label = kcalloc(EXFAT_VOLUME_LABEL_LEN,
>> +                                                  sizeof(short), GFP_KERNEL);
>> +     mutex_unlock(&sbi->s_lock);
>> +
>> +     if (!sbi->volume_label)
>> +             return -ENOMEM;
>> +
>> +     return 0;
>> +}
>> +
>> +int exfat_read_volume_label(struct super_block *sb)
>> +{
>> +     int ret, i;
>> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +     struct buffer_head *bh = NULL;
>> +     struct exfat_dentry *ep = NULL;
>> +
>> +     ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, NULL);
>> +     // ENOENT signifies that a volume label dentry doesn't exist
>> +     // We will treat this as an empty volume label and not fail.
>> +     if (ret < 0 && ret != -ENOENT)
>> +             goto cleanup;
>> +
>> +     ret = exfat_alloc_volume_label(sb);
>> +     if (ret < 0)
>> +             goto cleanup;
>> +
>> +     mutex_lock(&sbi->s_lock);
>> +     if (!ep)
>> +             memset(sbi->volume_label, 0, EXFAT_VOLUME_LABEL_LEN);
>> +     else
>> +             for (i = 0; i < EXFAT_VOLUME_LABEL_LEN; i++)
>> +                     sbi->volume_label[i] = le16_to_cpu(ep-
>>> dentry.volume_label.volume_label[i]);
>> +     mutex_unlock(&sbi->s_lock);
>> +
>> +     ret = 0;
>> +
>> +cleanup:
>> +     if (bh)
>> +             brelse(bh);
>> +
>> +     return ret;
>> +}
>> +
>> +int exfat_write_volume_label(struct super_block *sb,
>> +                          struct exfat_uni_name *uniname,
>> +                          struct inode *root_inode)
>> +{
>> +     int ret, i;
>> +     struct exfat_sb_info *sbi = EXFAT_SB(sb);
>> +     struct buffer_head *bh = NULL;
>> +     struct exfat_dentry *ep = NULL;
>> +
>> +     if (uniname->name_len > EXFAT_VOLUME_LABEL_LEN) {
>> +             ret = -EINVAL;
>> +             goto cleanup;
>> +     }
>> +
>> +     ret = exfat_get_volume_label_ptrs(sb, &bh, &ep, root_inode);
>> +     if (ret < 0)
>> +             goto cleanup;
>> +
>> +     ret = exfat_alloc_volume_label(sb);
>> +     if (ret < 0)
>> +             goto cleanup;
>> +
>> +     memcpy(sbi->volume_label, uniname->name,
>> +            uniname->name_len * sizeof(short));
>> +
>> +     mutex_lock(&sbi->s_lock);
>> +     for (i = 0; i < uniname->name_len; i++)
>> +             ep->dentry.volume_label.volume_label[i] =
>> +                     cpu_to_le16(sbi->volume_label[i]);
>> +     // Fill the rest of the str with 0x0000
>> +     for (; i < EXFAT_VOLUME_LABEL_LEN; i++)
>> +             ep->dentry.volume_label.volume_label[i] = 0x0000;
>> +
>> +     ep->dentry.volume_label.char_count = uniname->name_len;
>> +     mutex_unlock(&sbi->s_lock);
>> +
>> +     ret = 0;
>> +
>> +cleanup:
>> +     if (bh) {
>> +             exfat_update_bh(bh, true);
>> +             brelse(bh);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>>  /* mount the file system volume */
>>  static int __exfat_fill_super(struct super_block *sb,
>>               struct exfat_chain *root_clu)
>> @@ -791,6 +1014,7 @@ static void delayed_free(struct rcu_head *p)
>>
>>       unload_nls(sbi->nls_io);
>>       exfat_free_upcase_table(sbi);
>> +     kfree(sbi->volume_label);
>>       exfat_free_sbi(sbi);
>>  }
>>
>> --
>> 2.34.1

Re: [PATCH v4 0/1] exfat: Add support for FS_IOC_{GET,SET}FSLABEL

[Yuezhang.Mo]

> >> +                             clu.size = 0;
> >> +                             old_clu = clu.dir;
> >> +                             ret = exfat_alloc_cluster(root_inode, 1,
> >> +                                                       &clu, true);
> >> +                             if (ret < 0) {
> >> +                                     brelse(bh);
> >> +                                     goto end;
> >> +                             }
> > 
> > In exFAT, directory size is limited to 256MB. Please add a check to return -ENOSPC
> > instead of allocating a new cluster if the root directory size had reached this limit. 
> >
> Noted. I am switching over to using exfat_find_empty_entry, which
> checks for this.

I think it is good way to use exfat_find_empty_entry.
In exfat_get_volume_label_ptrs, if an empty dentry is found, ei->hint_femp should be
set to avoid repeated traversal.

> >> +
> >> +                             ret = exfat_ent_set(sb, old_clu, clu.dir);
> >> +                             if (ret < 0) {
> >> +                                     exfat_free_cluster(root_inode, &clu);
> >> +                                     brelse(bh);
> >> +                                     goto end;
> >> +                             }
> >> +
> >> +                             ret = exfat_zeroed_cluster(root_inode, clu.dir);
> >> +                             if (ret < 0) {
> >> +                                     exfat_free_cluster(root_inode, &clu);
> >> +                                     brelse(bh);
> >> +                                     goto end;
> >> +                             }
> >
> > After allocating a new cluster for the root directory, its size needs to be updated.
> >
> Where would I update the size? I don't think the root directory has a
> Stream Extension dentry, would I increment the exfat_inode_info.dir.size
> field?

The root directory does not have a Stream Extension dentry.  We just need to update
the new size to root_inode->i_size. If exfat_find_empty_entry is used, root_inode->i_size
will be updated by it.