[PATCH v9 0/3] fs: fat: add ioctl to modify fat filesystem partion volume label

[PATCH v9 0/3] fs: fat: add ioctl to modify fat filesystem partion volume label

[Chen Guanqiao]

The FAT filesystem partition volume label can be read with
FAT_IOCTL_GET_VOLUME_LABEL and written with FAT_IOCTL_SET_VOLUME_LABEL.

FAT volume label (volume name) is exactly same stored in boot sector and root
directory. Thus, the boot sector just needs to be upgrade when the label
writing.

And, Ignore the volume label in struct msdos_sb_info.

The volume label format reference from Pali Rohár <pali.rohar@gmail.com> testing
results in the previous mail.

v9:
1. Add necessary lock to protect write method.
2. Fix some code bug.

v8...v1:
write the patch

Chen Guanqiao (3):
  fs: fat: Add fat filesystem partition volume label in local structure
  fs: fat: Add volume label entry method function
  fs: fat: add ioctl method in fat filesystem driver

 fs/fat/dir.c                  |  47 ++++++++++++++
 fs/fat/fat.h                  |   6 ++
 fs/fat/file.c                 | 145 ++++++++++++++++++++++++++++++++++++++++++
 fs/fat/inode.c                |   6 ++
 include/uapi/linux/msdos_fs.h |   2 +
 5 files changed, 206 insertions(+)

--
2.14.3

[PATCH v9 1/3] fs: fat: Add fat filesystem partition volume label in local structure

[Chen Guanqiao]

Signed-off-by: Chen Guanqiao <chen.chenchacha@foxmail.com>
---
 fs/fat/fat.h                  | 6 ++++++
 fs/fat/inode.c                | 6 ++++++
 include/uapi/linux/msdos_fs.h | 2 ++
 3 files changed, 14 insertions(+)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 8fc1093da47d..11cf31802489 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -300,6 +300,12 @@ extern int fat_dir_empty(struct inode *dir);
 extern int fat_subdirs(struct inode *dir);
 extern int fat_scan(struct inode *dir, const unsigned char *name,
		    struct fat_slot_info *sinfo);
+extern int fat_get_volume_label_entry(struct inode *dir,
+				      struct buffer_head **bh,
+				      struct msdos_dir_entry **de);
+extern int fat_add_volume_label_entry(struct inode *dir,
+				      const unsigned char *name,
+				      struct timespec *ts);
 extern int fat_scan_logstart(struct inode *dir, int i_logstart,
			     struct fat_slot_info *sinfo);
 extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index ffbbf0520d9e..cae8215035af 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -46,12 +46,14 @@ struct fat_bios_param_block {

	u8	fat16_state;
	u32	fat16_vol_id;
+	u8      fat16_vol_label[11];

	u32	fat32_length;
	u32	fat32_root_cluster;
	u16	fat32_info_sector;
	u8	fat32_state;
	u32	fat32_vol_id;
+	u8      fat32_vol_label[11];
 };

 static int fat_default_codepage = CONFIG_FAT_DEFAULT_CODEPAGE;
@@ -1461,12 +1463,16 @@ static int fat_read_bpb(struct super_block *sb, struct fat_boot_sector *b,

	bpb->fat16_state = b->fat16.state;
	bpb->fat16_vol_id = get_unaligned_le32(b->fat16.vol_id);
+	memcpy(bpb->fat16_vol_label, b->fat16.vol_label,
+	       sizeof(bpb->fat16_vol_label));

	bpb->fat32_length = le32_to_cpu(b->fat32.length);
	bpb->fat32_root_cluster = le32_to_cpu(b->fat32.root_cluster);
	bpb->fat32_info_sector = le16_to_cpu(b->fat32.info_sector);
	bpb->fat32_state = b->fat32.state;
	bpb->fat32_vol_id = get_unaligned_le32(b->fat32.vol_id);
+	memcpy(bpb->fat32_vol_label, b->fat32.vol_label,
+	       sizeof(bpb->fat32_vol_label));

	/* Validate this looks like a FAT filesystem BPB */
	if (!bpb->fat_reserved) {
diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
index a45d0754102e..90d2d6f3d908 100644
--- a/include/uapi/linux/msdos_fs.h
+++ b/include/uapi/linux/msdos_fs.h
@@ -107,6 +107,8 @@ struct __fat_dirent {
 #define FAT_IOCTL_SET_ATTRIBUTES	_IOW('r', 0x11, __u32)
 /*Android kernel has used 0x12, so we use 0x13*/
 #define FAT_IOCTL_GET_VOLUME_ID		_IOR('r', 0x13, __u32)
+#define FAT_IOCTL_GET_VOLUME_LABEL	_IOR('r', 0x14, __u8[11])
+#define FAT_IOCTL_SET_VOLUME_LABEL	_IOW('r', 0x15, __u8[11])

 struct fat_boot_sector {
	__u8	ignored[3];	/* Boot strap short or near jump */
--
2.14.3

[PATCH v9 2/3] fs: fat: Add volume label entry method function

[Chen Guanqiao]

Signed-off-by: Chen Guanqiao <chen.chenchacha@foxmail.com>
---
 fs/fat/dir.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 8e100c3bf72c..d5286402c829 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -881,6 +881,53 @@ static int fat_get_short_entry(struct inode *dir, loff_t *pos,
	return -ENOENT;
 }

+int fat_get_volume_label_entry(struct inode *dir, struct buffer_head **bh,
+			       struct msdos_dir_entry **de)
+{
+	loff_t pos = 0;
+
+	*bh = NULL;
+	*de = NULL;
+	while (fat_get_entry(dir, &pos, bh, de) >= 0) {
+		if (((*de)->attr & ATTR_VOLUME) && ((*de)->attr != ATTR_EXT) &&
+		    !IS_FREE((*de)->name))
+			return 0;
+	}
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(fat_get_volume_label_entry);
+
+int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
+			       struct timespec *ts)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
+	struct msdos_dir_entry de;
+	struct fat_slot_info sinfo;
+	__le16 time, date;
+	int err;
+
+	memcpy(de.name, name, MSDOS_NAME);
+	de.attr = ATTR_VOLUME;
+	de.lcase = 0;
+	fat_time_unix2fat(sbi, ts, &time, &date, NULL);
+	de.cdate = de.adate = 0;
+	de.ctime = 0;
+	de.ctime_cs = 0;
+	de.time = time;
+	de.date = date;
+	fat_set_start(&de, 0);
+	de.size = 0;
+
+	err = fat_add_entries(dir, &de, 1, &sinfo);
+	if (err)
+		return err;
+
+	brelse(sinfo.bh);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fat_add_volume_label_entry);
+
 /*
  * The ".." entry can not provide the "struct fat_slot_info" information
  * for inode, nor a usable i_pos. So, this function provides some information
--
2.14.3

[PATCH v9 3/3] fs: fat: add ioctl method in fat filesystem driver

[Chen Guanqiao]

Signed-off-by: Chen Guanqiao <chen.chenchacha@foxmail.com>
---
 fs/fat/file.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/fs/fat/file.c b/fs/fat/file.c
index 4724cc9ad650..e87c5ae9208e 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -15,11 +15,46 @@
 #include <linux/fsnotify.h>
 #include <linux/security.h>
 #include <linux/falloc.h>
+#include <linux/ctype.h>
 #include "fat.h"

 static long fat_fallocate(struct file *file, int mode,
			  loff_t offset, loff_t len);

+static int fat_generate_volume_name(char *label, unsigned long len)
+{
+	unsigned int i;
+
+	/* For reasons, refer to namei_msdos.c:msdos_format_name() */
+	if (label[0] == 0xE5)
+		label[0] = 0x05;
+
+	for (i = 0; i < len; ++i) {
+		char c = label[i];
+		/* valid character contains d-characters and all extended ASCII
+		 * characters.
+		 * remaining empty spaces are padded by 0x20.
+		 */
+		if (c <= 0x7f)
+			switch (label[i]) {
+			case 'a' ... 'z':
+				label[i] = __toupper(label[i]);
+			case 'A' ... 'Z':
+			case '0' ... '9':
+			case '_':
+			case 0x20:
+				continue;
+			case 0:
+				label[i] = 0x20;
+				continue;
+			default:
+				return -EINVAL;
+			}
+	}
+
+	return 0;
+}
+
 static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
 {
	u32 attr;
@@ -121,10 +156,116 @@ static int fat_ioctl_get_volume_id(struct inode *inode, u32 __user *user_attr)
	return put_user(sbi->vol_id, user_attr);
 }

+
+static int fat_ioctl_get_volume_label(struct inode *inode, u8 __user *label)
+{
+	struct super_block *sb = inode->i_sb;
+	struct buffer_head *bh;
+	struct msdos_dir_entry *de;
+	int err;
+
+	inode = d_inode(sb->s_root);
+	inode_lock_shared(inode);
+
+	err = fat_get_volume_label_entry(inode, &bh, &de);
+	if (err)
+		goto out;
+
+	if (copy_to_user(label, de->name, MSDOS_NAME))
+		err = -EFAULT;
+
+	brelse(bh);
+out:
+	inode_unlock_shared(inode);
+
+	return err;
+}
+
+static int fat_ioctl_set_volume_label(struct file *file, u8 __user *label)
+{
+	struct inode *inode = file_inode(file);
+	struct super_block *sb = inode->i_sb;
+	struct timespec ts;
+	struct buffer_head *boot_bh;
+	struct buffer_head *vol_bh;
+	struct msdos_dir_entry *de;
+	struct fat_boot_sector *b;
+	u8 buf[MSDOS_NAME];
+	int err;
+
+	inode = d_inode(sb->s_root);
+
+	if (copy_from_user(buf, label, sizeof(buf))) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	err = fat_generate_volume_name(buf, sizeof(buf));
+	if (err)
+		goto out;
+
+	down_write(&sb->s_umount);
+	err = mnt_want_write_file(file);
+	if (err)
+		goto out;
+
+	inode_lock(inode);
+
+	/* Updates root directory's label */
+	err = fat_get_volume_label_entry(inode, &vol_bh, &de);
+	ts = current_time(inode);
+	if (err) {
+		/* Create volume label entry */
+		err = fat_add_volume_label_entry(inode, buf, &ts);
+		if (err)
+			goto out_vol_brelse;
+	} else {
+		/* Write to root directory */
+		memcpy(de->name, buf, sizeof(de->name));
+		inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
+
+		mark_buffer_dirty(vol_bh);
+	}
+
+	/* Update sector's label */
+	boot_bh = sb_bread(sb, 0);
+	if (boot_bh == NULL) {
+		fat_msg(sb, KERN_ERR,
+			"unable to read boot sector to write volume label");
+		err = -EIO;
+		goto out_boot_brelse;
+	}
+
+	b = (struct fat_boot_sector *)boot_bh->b_data;
+	if (MSDOS_SB(sb)->fat_bits == 32)
+		memcpy(b->fat32.vol_label, buf, sizeof(buf));
+	else
+		memcpy(b->fat16.vol_label, buf, sizeof(buf));
+
+	mark_buffer_dirty(boot_bh);
+
+	/* Synchronize the data together */
+	err = sync_dirty_buffer(boot_bh);
+	if (err)
+		goto out_boot_brelse;
+
+out_boot_brelse:
+	brelse(boot_bh);
+out_vol_brelse:
+	brelse(vol_bh);
+
+	inode_unlock(inode);
+	mnt_drop_write_file(file);
+	up_write(&sb->s_umount);
+out:
+	return err;
+}
+
 long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
	struct inode *inode = file_inode(filp);
	u32 __user *user_attr = (u32 __user *)arg;
+	u8 __user *user_vol_label = (u8 __user *)arg;

	switch (cmd) {
	case FAT_IOCTL_GET_ATTRIBUTES:
@@ -133,6 +274,10 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
		return fat_ioctl_set_attributes(filp, user_attr);
	case FAT_IOCTL_GET_VOLUME_ID:
		return fat_ioctl_get_volume_id(inode, user_attr);
+	case FAT_IOCTL_GET_VOLUME_LABEL:
+		return fat_ioctl_get_volume_label(inode, user_vol_label);
+	case FAT_IOCTL_SET_VOLUME_LABEL:
+		return fat_ioctl_set_volume_label(filp, user_vol_label);
	default:
		return -ENOTTY;	/* Inappropriate ioctl for device */
	}
--
2.14.3

Re: [PATCH v9 2/3] fs: fat: Add volume label entry method function

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

On Thursday 08 February 2018 00:14:06 Chen Guanqiao wrote:
> Signed-off-by: Chen Guanqiao <chen.chenchacha@foxmail.com>

Missing commit message and information what are the new proposed
functions suppose to do.

> ---
>  fs/fat/dir.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 8e100c3bf72c..d5286402c829 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -881,6 +881,53 @@ static int fat_get_short_entry(struct inode *dir, loff_t *pos,
> 	return -ENOENT;
>  }
> 
> +int fat_get_volume_label_entry(struct inode *dir, struct buffer_head **bh,
> +			       struct msdos_dir_entry **de)
> +{
> +	loff_t pos = 0;
> +
> +	*bh = NULL;
> +	*de = NULL;
> +	while (fat_get_entry(dir, &pos, bh, de) >= 0) {
> +		if (((*de)->attr & ATTR_VOLUME) && ((*de)->attr != ATTR_EXT) &&
> +		    !IS_FREE((*de)->name))
> +			return 0;
> +	}
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_get_volume_label_entry);

Why both functions are exported? In this patch series are used only in
file.c which is in the same object file.

> +
> +int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
> +			       struct timespec *ts)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
> +	struct msdos_dir_entry de;
> +	struct fat_slot_info sinfo;
> +	__le16 time, date;
> +	int err;
> +
> +	memcpy(de.name, name, MSDOS_NAME);
> +	de.attr = ATTR_VOLUME;
> +	de.lcase = 0;
> +	fat_time_unix2fat(sbi, ts, &time, &date, NULL);
> +	de.cdate = de.adate = 0;
> +	de.ctime = 0;
> +	de.ctime_cs = 0;
> +	de.time = time;
> +	de.date = date;
> +	fat_set_start(&de, 0);
> +	de.size = 0;
> +
> +	err = fat_add_entries(dir, &de, 1, &sinfo);
> +	if (err)
> +		return err;
> +
> +	brelse(sinfo.bh);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fat_add_volume_label_entry);

This function looks like copy+paste of function msdos_add_entry().
Please de-duplicate code as this would lead to problems in future.

Also who is supposed to format dos name? Caller or callee? There is no
information neither in commit message nor in comments.

> +
>  /*
>   * The ".." entry can not provide the "struct fat_slot_info" information
>   * for inode, nor a usable i_pos. So, this function provides some information
> --
> 2.14.3

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v9 3/3] fs: fat: add ioctl method in fat filesystem driver

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 6041 bytes --]

On Thursday 08 February 2018 00:14:07 Chen Guanqiao wrote:
> Signed-off-by: Chen Guanqiao <chen.chenchacha@foxmail.com>
> ---
>  fs/fat/file.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
> 
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index 4724cc9ad650..e87c5ae9208e 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -15,11 +15,46 @@
>  #include <linux/fsnotify.h>
>  #include <linux/security.h>
>  #include <linux/falloc.h>
> +#include <linux/ctype.h>
>  #include "fat.h"
> 
>  static long fat_fallocate(struct file *file, int mode,
> 			  loff_t offset, loff_t len);
> 
> +static int fat_generate_volume_name(char *label, unsigned long len)
> +{
> +	unsigned int i;
> +
> +	/* For reasons, refer to namei_msdos.c:msdos_format_name() */
> +	if (label[0] == 0xE5)
> +		label[0] = 0x05;
> +
> +	for (i = 0; i < len; ++i) {
> +		char c = label[i];
> +		/* valid character contains d-characters and all extended ASCII
> +		 * characters.
> +		 * remaining empty spaces are padded by 0x20.
> +		 */
> +		if (c <= 0x7f)

This check if tautology and do nothing. Maximal value in c for x86 is
127 and minimal value -128. (0x7f == 127)

> +			switch (label[i]) {
> +			case 'a' ... 'z':
> +				label[i] = __toupper(label[i]);

Why this is enforced? And ignored mount option nocase?

> +			case 'A' ... 'Z':
> +			case '0' ... '9':
> +			case '_':
> +			case 0x20:
> +				continue;
> +			case 0:
> +				label[i] = 0x20;
> +				continue;
> +			default:
> +				return -EINVAL;

Why are other characters disallowed (e.g. '!')? And completely ignored
mount option check?

> +			}
> +	}
> +
> +	return 0;
> +}

What is purpose of this function at all? There is already e.g.
msdos_format_name() which do lot of needed checks and formats.

> +
>  static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
>  {
> 	u32 attr;
> @@ -121,10 +156,116 @@ static int fat_ioctl_get_volume_id(struct inode *inode, u32 __user *user_attr)
> 	return put_user(sbi->vol_id, user_attr);
>  }
> 
> +
> +static int fat_ioctl_get_volume_label(struct inode *inode, u8 __user *label)
> +{
> +	struct super_block *sb = inode->i_sb;
> +	struct buffer_head *bh;
> +	struct msdos_dir_entry *de;
> +	int err;
> +
> +	inode = d_inode(sb->s_root);
> +	inode_lock_shared(inode);
> +
> +	err = fat_get_volume_label_entry(inode, &bh, &de);
> +	if (err)
> +		goto out;

Missing handling of leading 0xE5, leading spaces, etc...

> +	if (copy_to_user(label, de->name, MSDOS_NAME))
> +		err = -EFAULT;
> +
> +	brelse(bh);
> +out:
> +	inode_unlock_shared(inode);
> +
> +	return err;
> +}
> +
> +static int fat_ioctl_set_volume_label(struct file *file, u8 __user *label)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct super_block *sb = inode->i_sb;
> +	struct timespec ts;
> +	struct buffer_head *boot_bh;
> +	struct buffer_head *vol_bh;
> +	struct msdos_dir_entry *de;
> +	struct fat_boot_sector *b;
> +	u8 buf[MSDOS_NAME];
> +	int err;
> +
> +	inode = d_inode(sb->s_root);
> +
> +	if (copy_from_user(buf, label, sizeof(buf))) {
> +		err = -EFAULT;
> +		goto out;
> +	}
> +
> +	err = fat_generate_volume_name(buf, sizeof(buf));
> +	if (err)
> +		goto out;

This looks like a wrong API for userspace. Whole FAT driver for
userspace is working according to codepage= and iocharset=/utf8 mount
options, just this call fully ignores it. Seems very unintuitive.

What is the reason for such API?

Also there is missing handling of special cases, like empty file label,
etc...

> +	down_write(&sb->s_umount);
> +	err = mnt_want_write_file(file);
> +	if (err)
> +		goto out;
> +
> +	inode_lock(inode);
> +
> +	/* Updates root directory's label */
> +	err = fat_get_volume_label_entry(inode, &vol_bh, &de);
> +	ts = current_time(inode);
> +	if (err) {
> +		/* Create volume label entry */
> +		err = fat_add_volume_label_entry(inode, buf, &ts);
> +		if (err)
> +			goto out_vol_brelse;
> +	} else {
> +		/* Write to root directory */
> +		memcpy(de->name, buf, sizeof(de->name));
> +		inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
> +
> +		mark_buffer_dirty(vol_bh);
> +	}
> +
> +	/* Update sector's label */
> +	boot_bh = sb_bread(sb, 0);
> +	if (boot_bh == NULL) {
> +		fat_msg(sb, KERN_ERR,
> +			"unable to read boot sector to write volume label");
> +		err = -EIO;
> +		goto out_boot_brelse;
> +	}
> +
> +	b = (struct fat_boot_sector *)boot_bh->b_data;
> +	if (MSDOS_SB(sb)->fat_bits == 32)
> +		memcpy(b->fat32.vol_label, buf, sizeof(buf));
> +	else
> +		memcpy(b->fat16.vol_label, buf, sizeof(buf));

IIRC in boot sector is no 0xE5 <--> 0x05 conversion.

> +	mark_buffer_dirty(boot_bh);
> +
> +	/* Synchronize the data together */
> +	err = sync_dirty_buffer(boot_bh);
> +	if (err)
> +		goto out_boot_brelse;
> +
> +out_boot_brelse:
> +	brelse(boot_bh);
> +out_vol_brelse:
> +	brelse(vol_bh);
> +
> +	inode_unlock(inode);
> +	mnt_drop_write_file(file);
> +	up_write(&sb->s_umount);
> +out:
> +	return err;
> +}
> +
>  long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  {
> 	struct inode *inode = file_inode(filp);
> 	u32 __user *user_attr = (u32 __user *)arg;
> +	u8 __user *user_vol_label = (u8 __user *)arg;
> 
> 	switch (cmd) {
> 	case FAT_IOCTL_GET_ATTRIBUTES:
> @@ -133,6 +274,10 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 		return fat_ioctl_set_attributes(filp, user_attr);
> 	case FAT_IOCTL_GET_VOLUME_ID:
> 		return fat_ioctl_get_volume_id(inode, user_attr);
> +	case FAT_IOCTL_GET_VOLUME_LABEL:
> +		return fat_ioctl_get_volume_label(inode, user_vol_label);
> +	case FAT_IOCTL_SET_VOLUME_LABEL:
> +		return fat_ioctl_set_volume_label(filp, user_vol_label);
> 	default:
> 		return -ENOTTY;	/* Inappropriate ioctl for device */
> 	}
> --
> 2.14.3

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]