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

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

[ChenGuanqiao]

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.

v8:
1. struct msdos_sb_info reduction.
2. remove MSDOS_ROOT_INO check in label entry search.
3. replace uppercase in the label with lowercase, like windows.
4. put the copy_to_user() back into the lock.
5. delete unnecessary lock.
v7...v1:
write the patch

ChenGuanqiao (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                  |   5 ++
 fs/fat/file.c                 | 132 ++++++++++++++++++++++++++++++++++++++++++
 fs/fat/inode.c                |   6 ++
 include/uapi/linux/msdos_fs.h |   2 +
 5 files changed, 192 insertions(+)

--
2.11.0

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

[ChenGuanqiao]

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

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 8fc1093da47d..22cded6a2780 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -300,6 +300,11 @@ 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..40f1dd23e937 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.11.0

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

[ChenGuanqiao]

Signed-off-by: ChenGuanqiao <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.11.0

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

[ChenGuanqiao]

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

diff --git a/fs/fat/file.c b/fs/fat/file.c
index 4724cc9ad650..a885f92df1eb 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -15,11 +15,33 @@
 #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_check_d_characters(char *label, unsigned long len)
+{
+	int i;
+
+	for (i = 0; i < len; ++i) {
+		switch (label[i]) {
+		case 'a' ... 'z':
+			label[i] = __toupper(label[i]);
+		case 'A' ... 'Z':
+		case '0' ... '9':
+		case '_':
+		case 0x20:
+			continue;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int fat_ioctl_get_attributes(struct inode *inode, u32 __user *user_attr)
 {
	u32 attr;
@@ -121,10 +143,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 *vol_label)
+{
+	int err = 0;
+	struct buffer_head *bh;
+	struct msdos_dir_entry *de;
+	struct super_block *sb = inode->i_sb;
+
+	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(vol_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 *vol_label)
+{
+	int err = 0;
+	u8 label[MSDOS_NAME];
+	struct timespec ts;
+	struct buffer_head *boot_bh;
+	struct buffer_head *vol_bh;
+	struct msdos_dir_entry *de;
+	struct fat_boot_sector *b;
+	struct inode *inode = file_inode(file);
+	struct super_block *sb = inode->i_sb;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+
+	inode = d_inode(sb->s_root);
+
+	if (copy_from_user(label, vol_label, sizeof(label))) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	err = fat_check_d_characters(label, sizeof(label));
+	if (err)
+		goto out;
+
+	err = mnt_want_write_file(file);
+	if (err)
+		goto out;
+
+	down_write(&sb->s_umount);
+
+	/* Updates root directory's vol_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, label, &ts);
+		if (err)
+			goto out_vol_brelse;
+	} else {
+		/* Write to root directory */
+		memcpy(de->name, label, sizeof(de->name));
+		inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
+
+		mark_buffer_dirty(vol_bh);
+	}
+
+	/* Update sector's vol_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 (sbi->fat_bits == 32)
+		memcpy(b->fat32.vol_label, label, sizeof(label));
+	else
+		memcpy(b->fat16.vol_label, label, sizeof(label));
+
+	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);
+
+	up_write(&sb->s_umount);
+	mnt_drop_write_file(file);
+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 +261,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.11.0

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

[OGAWA Hirofumi]

ChenGuanqiao <chen.chenchacha@foxmail.com> writes:

> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; ++i) {
> +		switch (label[i]) {
> +		case 'a' ... 'z':
> +			label[i] = __toupper(label[i]);
> +		case 'A' ... 'Z':
> +		case '0' ... '9':
> +		case '_':
> +		case 0x20:
> +			continue;
> +		default:
> +			return -EINVAL;
> +		}

Same question with previous though, what windows do if label = "a b c"?
(this is including space other than end of name or extension.)

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

> +static int fat_ioctl_set_volume_label(struct file *file,
> +				      u8 __user *vol_label)
> +{
> +	int err = 0;
> +	u8 label[MSDOS_NAME];
> +	struct timespec ts;
> +	struct buffer_head *boot_bh;
> +	struct buffer_head *vol_bh;
> +	struct msdos_dir_entry *de;
> +	struct fat_boot_sector *b;
> +	struct inode *inode = file_inode(file);
> +	struct super_block *sb = inode->i_sb;
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +

[...]

> +	err = mnt_want_write_file(file);
> +	if (err)
> +		goto out;
> +
> +	down_write(&sb->s_umount);

Looks like inode_lock() for rootdir is gone. It is necessary to
traverse+modify.

> +	/* Updates root directory's vol_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, label, &ts);
> +		if (err)
> +			goto out_vol_brelse;
> +	} else {
> +		/* Write to root directory */
> +		memcpy(de->name, label, sizeof(de->name));
> +		inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
> +
> +		mark_buffer_dirty(vol_bh);
> +	}
> +
> +	/* Update sector's vol_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 (sbi->fat_bits == 32)
> +		memcpy(b->fat32.vol_label, label, sizeof(label));
> +	else
> +		memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> +	mark_buffer_dirty(boot_bh);
> +
> +	/* Synchronize the data together */
> +	err = sync_dirty_buffer(boot_bh);
> +	if (err)
> +		goto out_boot_brelse;
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[OGAWA Hirofumi]

chenchacha <chen.chenchacha@foxmail.com> writes:

> On 01/29/2018 09:02 PM, OGAWA Hirofumi wrote:
>> ChenGuanqiao <chen.chenchacha@foxmail.com> writes:
>>
>>> +static int fat_check_d_characters(char *label, unsigned long len)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < len; ++i) {
>>> +		switch (label[i]) {
>>> +		case 'a' ... 'z':
>>> +			label[i] = __toupper(label[i]);
>>> +		case 'A' ... 'Z':
>>> +		case '0' ... '9':
>>> +		case '_':
>>> +		case 0x20:
>>> +			continue;
>>> +		default:
>>> +			return -EINVAL;
>>> +		}
>> Same question with previous though, what windows do if label = "a b c"?
>> (this is including space other than end of name or extension.)
> In win7, the volume label will be capitalized, and leaving spaces.
> Or, you mean I need to fill the rest of the space with "0x20"?

I see. However, what win7 stored, BTW? It was "A B C      ", or anything
other?

>>> +static int fat_ioctl_set_volume_label(struct file *file,
>>> +				      u8 __user *vol_label)
>>> +{
>>> +	int err = 0;
>>> +	u8 label[MSDOS_NAME];
>>> +	struct timespec ts;
>>> +	struct buffer_head *boot_bh;
>>> +	struct buffer_head *vol_bh;
>>> +	struct msdos_dir_entry *de;
>>> +	struct fat_boot_sector *b;
>>> +	struct inode *inode = file_inode(file);
>>> +	struct super_block *sb = inode->i_sb;
>>> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>> +
>> [...]
> I need remove "struct msdos_sb_info *sbi"?

If you didn't use sbi anymore, you should remove.

>>> +	err = mnt_want_write_file(file);
>>> +	if (err)
>>> +		goto out;
>>> +
>>> +	down_write(&sb->s_umount);
>> Looks like inode_lock() for rootdir is gone. It is necessary to
>> traverse+modify.
> Is it wrong for me to use the inode_lock() in patch v7? I need to lock 
> inode here, and turn off immediately after

inode_lock() is necessary to protect race with other dir operations.

I asked at v7, locking order to prevent the AB locking order bug.

I.e.
	mnt_want_write_file => down_write => inode_lock()
vs
	down_write => mnt_want_write_file => inode_lock()

Which is right one?

> mark_buffer_dirty(vol_bh)?

What is this asking?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[Pali Rohár]

On Tuesday 30 January 2018 20:23:03 OGAWA Hirofumi wrote:
> chenchacha <chen.chenchacha@foxmail.com> writes:
> 
> > On 01/29/2018 09:02 PM, OGAWA Hirofumi wrote:
> >> ChenGuanqiao <chen.chenchacha@foxmail.com> writes:
> >>
> >>> +static int fat_check_d_characters(char *label, unsigned long len)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	for (i = 0; i < len; ++i) {
> >>> +		switch (label[i]) {
> >>> +		case 'a' ... 'z':
> >>> +			label[i] = __toupper(label[i]);
> >>> +		case 'A' ... 'Z':
> >>> +		case '0' ... '9':
> >>> +		case '_':
> >>> +		case 0x20:
> >>> +			continue;
> >>> +		default:
> >>> +			return -EINVAL;
> >>> +		}
> >> Same question with previous though, what windows do if label = "a b c"?
> >> (this is including space other than end of name or extension.)
> > In win7, the volume label will be capitalized, and leaving spaces.
> > Or, you mean I need to fill the rest of the space with "0x20"?
> 
> I see. However, what win7 stored, BTW? It was "A B C      ", or anything
> other?

Yes, as in FAT, all directory entries are padded by spaces.

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

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

[Andy Shevchenko]

+Cc: Pali, who AFAIRC is interested in FAT labeling mess.

On Wed, Jan 17, 2018 at 12:43 PM, ChenGuanqiao
<chen.chenchacha@foxmail.com> wrote:

Commit message?

> Signed-off-by: ChenGuanqiao <chen.chenchacha@foxmail.com>

>  #include <linux/fsnotify.h>
>  #include <linux/security.h>
>  #include <linux/falloc.h>
> +#include <linux/ctype.h>

It would be better to squeeze it to have order (to some extent) preserved.

> +static int fat_check_d_characters(char *label, unsigned long len)
> +{
> +       int i;
> +
> +       for (i = 0; i < len; ++i) {

Oy vey, unsigned long len vs. int i.

> +               switch (label[i]) {
> +               case 'a' ... 'z':
> +                       label[i] = __toupper(label[i]);
> +               case 'A' ... 'Z':
> +               case '0' ... '9':
> +               case '_':
> +               case 0x20:
> +                       continue;

I'm not sure this is best approach. And since there is no commit
message I can't be constructive.

> +               default:
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}

> +static int fat_ioctl_get_volume_label(struct inode *inode,
> +                                     u8 __user *vol_label)
> +{

> +       int err = 0;

Redundant assignment.

> +       struct buffer_head *bh;
> +       struct msdos_dir_entry *de;
> +       struct super_block *sb = inode->i_sb;

Moreover, perhaps reversed tree order for the definition block?

> +
> +       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(vol_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 *vol_label)

Perhaps vol_label -> label, and fit one line?

> +{
> +       int err = 0;
> +       u8 label[MSDOS_NAME];
> +       struct timespec ts;
> +       struct buffer_head *boot_bh;
> +       struct buffer_head *vol_bh;
> +       struct msdos_dir_entry *de;
> +       struct fat_boot_sector *b;
> +       struct inode *inode = file_inode(file);
> +       struct super_block *sb = inode->i_sb;
> +       struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> +       inode = d_inode(sb->s_root);
> +
> +       if (copy_from_user(label, vol_label, sizeof(label))) {
> +               err = -EFAULT;
> +               goto out;
> +       }
> +
> +       err = fat_check_d_characters(label, sizeof(label));
> +       if (err)
> +               goto out;
> +
> +       err = mnt_want_write_file(file);
> +       if (err)
> +               goto out;
> +
> +       down_write(&sb->s_umount);
> +
> +       /* Updates root directory's vol_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, label, &ts);
> +               if (err)
> +                       goto out_vol_brelse;
> +       } else {
> +               /* Write to root directory */
> +               memcpy(de->name, label, sizeof(de->name));
> +               inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
> +
> +               mark_buffer_dirty(vol_bh);
> +       }
> +
> +       /* Update sector's vol_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 (sbi->fat_bits == 32)
> +               memcpy(b->fat32.vol_label, label, sizeof(label));
> +       else
> +               memcpy(b->fat16.vol_label, label, sizeof(label));
> +
> +       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);
> +
> +       up_write(&sb->s_umount);
> +       mnt_drop_write_file(file);
> +out:
> +       return err;
> +}



-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

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

On Monday 29 January 2018 15:18:42 Andy Shevchenko wrote:
> +Cc: Pali, who AFAIRC is interested in FAT labeling mess.

Yes, please CC me for FAT labeling discussing in future.

> On Wed, Jan 17, 2018 at 12:43 PM, ChenGuanqiao
> <chen.chenchacha@foxmail.com> wrote:
> 
> Commit message?
> 
> > Signed-off-by: ChenGuanqiao <chen.chenchacha@foxmail.com>
> 
> >  #include <linux/fsnotify.h>
> >  #include <linux/security.h>
> >  #include <linux/falloc.h>
> > +#include <linux/ctype.h>
> 
> It would be better to squeeze it to have order (to some extent) preserved.
> 
> > +static int fat_check_d_characters(char *label, unsigned long len)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < len; ++i) {
> 
> Oy vey, unsigned long len vs. int i.
> 
> > +               switch (label[i]) {
> > +               case 'a' ... 'z':
> > +                       label[i] = __toupper(label[i]);

Why only lower a..z? á or ó are also valid (according to some OEM
codepage) and are lower case.

Also please look at my testing results. Even old DOS versions are able
to correctly read lower case label. Therefore I do not see any reason
why to have such "force" code in kernel.

https://www.spinics.net/lists/kernel/msg2645732.html

Here I proposed how should be linux tools unified which manipulates with
fat label.

> > +               case 'A' ... 'Z':
> > +               case '0' ... '9':
> > +               case '_':
> > +               case 0x20:
> > +                       continue;
> 
> I'm not sure this is best approach. And since there is no commit
> message I can't be constructive.
> 
> > +               default:
> > +                       return -EINVAL;

And what about other valid characters? Why are ignored and returned
error?

> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> 
> > +static int fat_ioctl_get_volume_label(struct inode *inode,
> > +                                     u8 __user *vol_label)
> > +{
> 
> > +       int err = 0;
> 
> Redundant assignment.
> 
> > +       struct buffer_head *bh;
> > +       struct msdos_dir_entry *de;
> > +       struct super_block *sb = inode->i_sb;
> 
> Moreover, perhaps reversed tree order for the definition block?
> 
> > +
> > +       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(vol_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 *vol_label)
> 
> Perhaps vol_label -> label, and fit one line?
> 
> > +{
> > +       int err = 0;
> > +       u8 label[MSDOS_NAME];
> > +       struct timespec ts;
> > +       struct buffer_head *boot_bh;
> > +       struct buffer_head *vol_bh;
> > +       struct msdos_dir_entry *de;
> > +       struct fat_boot_sector *b;
> > +       struct inode *inode = file_inode(file);
> > +       struct super_block *sb = inode->i_sb;
> > +       struct msdos_sb_info *sbi = MSDOS_SB(sb);
> > +
> > +       inode = d_inode(sb->s_root);
> > +
> > +       if (copy_from_user(label, vol_label, sizeof(label))) {
> > +               err = -EFAULT;
> > +               goto out;
> > +       }
> > +
> > +       err = fat_check_d_characters(label, sizeof(label));
> > +       if (err)
> > +               goto out;
> > +
> > +       err = mnt_want_write_file(file);
> > +       if (err)
> > +               goto out;
> > +
> > +       down_write(&sb->s_umount);
> > +
> > +       /* Updates root directory's vol_label */
> > +       err = fat_get_volume_label_entry(inode, &vol_bh, &de);
> > +       ts = current_time(inode);
> > +       if (err) {
> > +               /* Create volume label entry */

Not handling situation when buffer is empty which means that user what
to remove label. Entry name in FAT directory cannot be empty string.

> > +               err = fat_add_volume_label_entry(inode, label, &ts);
> > +               if (err)
> > +                       goto out_vol_brelse;
> > +       } else {
> > +               /* Write to root directory */
> > +               memcpy(de->name, label, sizeof(de->name));

Really? You forgot for 0x05/0xE5 handling. And also conversion from VFS
NLS encoding to OEM code page.

Anyway, kernel's fat driver already has this conversion implemented in
some function, so it is better to not reinvent wheel.

> > +               inode->i_ctime = inode->i_mtime = inode->i_atime = ts;
> > +
> > +               mark_buffer_dirty(vol_bh);
> > +       }
> > +
> > +       /* Update sector's vol_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 (sbi->fat_bits == 32)
> > +               memcpy(b->fat32.vol_label, label, sizeof(label));
> > +       else
> > +               memcpy(b->fat16.vol_label, label, sizeof(label));

Missing NO NAME handling.

> > +       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);
> > +
> > +       up_write(&sb->s_umount);
> > +       mnt_drop_write_file(file);
> > +out:
> > +       return err;
> > +}
> 
> 
> 

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

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

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

[Pali Rohár]

On Tuesday 30 January 2018 14:33:49 Chen Guanqiao wrote:
> On Mon, 2018-01-29 at 17:43 +0100, Pali Rohár wrote:
> > On Monday 29 January 2018 15:18:42 Andy Shevchenko wrote:
> > > +Cc: Pali, who AFAIRC is interested in FAT labeling mess.
> > 
> > Yes, please CC me for FAT labeling discussing in future.
> > 
> > > On Wed, Jan 17, 2018 at 12:43 PM, ChenGuanqiao
> > > <chen.chenchacha@foxmail.com> wrote:
> > > 
> > > Commit message?
> > > 
> > > > Signed-off-by: ChenGuanqiao <chen.chenchacha@foxmail.com>
> > > >  #include <linux/fsnotify.h>
> > > >  #include <linux/security.h>
> > > >  #include <linux/falloc.h>
> > > > +#include <linux/ctype.h>
> > > 
> > > It would be better to squeeze it to have order (to some extent)
> > > preserved.
> > > 
> > > > +static int fat_check_d_characters(char *label, unsigned long
> > > > len)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < len; ++i) {
> > > 
> > > Oy vey, unsigned long len vs. int i.
> > > 
> > > > +               switch (label[i]) {
> > > > +               case 'a' ... 'z':
> > > > +                       label[i] = __toupper(label[i]);
> > 
> > Why only lower a..z? á or ó are also valid (according to some OEM
> > codepage) and are lower case.
> > 
> > Also please look at my testing results. Even old DOS versions are
> > able
> > to correctly read lower case label. Therefore I do not see any reason
> > why to have such "force" code in kernel.
> > 
> > https://www.spinics.net/lists/kernel/msg2645732.html
> > 
> > Here I proposed how should be linux tools unified which manipulates
> > with
> > fat label.
> In character detection, I refer to the FAT standard document "Ecma-
> 107".

So please stick with implementation which is already used in kernel
driver or refer to one of two MS documentations for FAT12/16/32. We do
not want to have part of kernel fs driver to be written according one
documentation, other parts according to another documentation and
remaining parts according to guess or something else. We already have
compatibility with MS FAT, so inventing something new does not make
sense. Also in past there was found mistakes in MS documentation (like
incorrect formulas for counting clusters, etc.), so beware of facts that
documentation/specification does not have to be 100% correct.

In above email I did investigation for FAT labels and come to the
conclusion which seems to make sense. So if you disagree with it, please
open discussion what is wrong with proposed solution for unifying
handling label on Linux. Instead of inventing fully new way how to
handle FAT label in Linux. It really does not make any sense to use
fully different way how to handle FAT label which is incompatible with
existing Linux and Windows implementations.

In namei_msdos.c you can find already used functions. Others should be
available in dir.c.

> The document limits the volume label to a range of characters
> called "d-characters". The "d-characters" only includes uppercase
> letter, '-' and numbers.

msdos.ko and vfat.ko already handles also other characters.

> And I have found the volume label can be written as extended ASCII,
> double-byte characters in Windows. such as Chinese characters, Arabic
> characters, and others are not all characters that control characters
> and punctuation mark in ASCII. This is obviously no correct if we copy
> the implementation of windows completely, let the maximum length of
> volume label is variable?

Label is stored in two locations: boot sector and as entry in root
directory structure. For directory entry there are strict rules which
msdos.ko/vfat.ko uses and which are verified by years of usage. Maximum
length is fixed to 11 bytes due to length of directory entry. Also note
that label cannot be stored as LFN entry.

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