[PATCH 0/2] fs: fat: add ioctl method to read voluem label in fat filesystem driver

[PATCH 0/2] fs: fat: add ioctl method to read voluem label in fat filesystem driver

[chenchacha]

This patch add fat ioctl command FAT_IOCTL_GET_VOLUME_LABEL.

We can get the volume label with this command:

	ioctl(fd, FAT_IOCTL_GET_VOLUME_LABEL, &volume_label);

FAT volume label (volume name) is exactly same stored in boot sector and root
directory. And this patch read label only from the root directory. If label in
root directory is missing then disk would be treated as without label. Label
from boot sector would not be read.

The volume label format reference from Pali Rohár testing results
(https://www.spinics.net/lists/kernel/msg2645732.html) in the previous mail.

chenchacha (2):
  Add FAT_IOCTL_GET_VOLUME_LABEL in fat_generic_ioctl()
  Add a new function to get root directory with ATTR_VOLUME

 fs/fat/dir.c                  | 13 +++++++++++++
 fs/fat/fat.h                  |  2 ++
 fs/fat/file.c                 | 22 ++++++++++++++++++++++
 include/uapi/linux/msdos_fs.h |  1 +
 4 files changed, 38 insertions(+)

-- 
2.19.0

[PATCH 2/2] Add a new function to get root directory with ATTR_VOLUME

[chenchacha]

Signed-off-by: chenchacha <chen.chenchacha@foxmail.com>
---
 fs/fat/dir.c | 13 +++++++++++++
 fs/fat/fat.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 7f5f3699fc6c..4fdcc1200f2b 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -881,6 +881,19 @@ static int fat_get_short_entry(struct inode *dir, loff_t *pos,
 	return -ENOENT;
 }
 
+int fat_get_root_entry(struct inode *dir, struct buffer_head **bh,
+		       struct msdos_dir_entry **de)
+{
+	loff_t offset = 0;
+
+	*de = NULL;
+	while (fat_get_entry(dir, &offset, bh, de) >= 0) {
+		if (!IS_FREE((*de)->name) && (*de)->attr & ATTR_VOLUME)
+			return 0;
+	}
+	return -ENOENT;
+}
+
 /*
  * The ".." entry can not provide the "struct fat_slot_info" information
  * for inode, nor a usable i_pos. So, this function provides some information
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index be012de96f65..4195cb1e891a 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -302,6 +302,8 @@ extern int fat_scan(struct inode *dir, const unsigned char *name,
 		    struct fat_slot_info *sinfo);
 extern int fat_scan_logstart(struct inode *dir, int i_logstart,
 			     struct fat_slot_info *sinfo);
+extern int fat_get_root_entry(struct inode *dir, struct buffer_head **bh,
+			      struct msdos_dir_entry **de);
 extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
 				struct msdos_dir_entry **de);
 extern int fat_alloc_new_dir(struct inode *dir, struct timespec64 *ts);
-- 
2.19.0

Re: [PATCH 2/2] Add a new function to get root directory with ATTR_VOLUME

[Pali Rohár]

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

On Friday 05 October 2018 01:21:01 chenchacha wrote:
> Signed-off-by: chenchacha <chen.chenchacha@foxmail.com>
> ---
>  fs/fat/dir.c | 13 +++++++++++++
>  fs/fat/fat.h |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 7f5f3699fc6c..4fdcc1200f2b 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -881,6 +881,19 @@ static int fat_get_short_entry(struct inode *dir, loff_t *pos,
>  	return -ENOENT;
>  }
>  
> +int fat_get_root_entry(struct inode *dir, struct buffer_head **bh,
> +		       struct msdos_dir_entry **de)

fat_get_root_volume_entry would be better name. It does not return root
entry, but volume label entry.

> +{
> +	loff_t offset = 0;
> +
> +	*de = NULL;
> +	while (fat_get_entry(dir, &offset, bh, de) >= 0) {
> +		if (!IS_FREE((*de)->name) && (*de)->attr & ATTR_VOLUME)

You should check that cluster number is zero. As e.g. first entry with
ATTR_VOLUME can be also LFN entry and not real volume label. So you
should properly check mask and filter out also LFN entries which have
ATTR_VOLUME flag set too.

> +			return 0;
> +	}
> +	return -ENOENT;
> +}
> +
>  /*
>   * The ".." entry can not provide the "struct fat_slot_info" information
>   * for inode, nor a usable i_pos. So, this function provides some information
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index be012de96f65..4195cb1e891a 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -302,6 +302,8 @@ extern int fat_scan(struct inode *dir, const unsigned char *name,
>  		    struct fat_slot_info *sinfo);
>  extern int fat_scan_logstart(struct inode *dir, int i_logstart,
>  			     struct fat_slot_info *sinfo);
> +extern int fat_get_root_entry(struct inode *dir, struct buffer_head **bh,
> +			      struct msdos_dir_entry **de);
>  extern int fat_get_dotdot_entry(struct inode *dir, struct buffer_head **bh,
>  				struct msdos_dir_entry **de);
>  extern int fat_alloc_new_dir(struct inode *dir, struct timespec64 *ts);
> -- 
> 2.19.0
> 

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

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

Re: [PATCH 0/2] fs: fat: add ioctl method to read voluem label in fat filesystem driver

[Pali Rohár]

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

On Friday 05 October 2018 01:20:59 chenchacha wrote:
> This patch add fat ioctl command FAT_IOCTL_GET_VOLUME_LABEL.
> 
> We can get the volume label with this command:
> 
> 	ioctl(fd, FAT_IOCTL_GET_VOLUME_LABEL, &volume_label);

Hi! What is purpose of having such functionality in kernel? Userspace
can read it without any kernel support. (e.g. mlabel or fatlabel)

> FAT volume label (volume name) is exactly same stored in boot sector and root
> directory. And this patch read label only from the root directory. If label in
> root directory is missing then disk would be treated as without label. Label
> from boot sector would not be read.
> 
> The volume label format reference from Pali Rohár testing results
> (https://www.spinics.net/lists/kernel/msg2645732.html) in the previous mail.
> 
> chenchacha (2):
>   Add FAT_IOCTL_GET_VOLUME_LABEL in fat_generic_ioctl()
>   Add a new function to get root directory with ATTR_VOLUME
> 
>  fs/fat/dir.c                  | 13 +++++++++++++
>  fs/fat/fat.h                  |  2 ++
>  fs/fat/file.c                 | 22 ++++++++++++++++++++++
>  include/uapi/linux/msdos_fs.h |  1 +
>  4 files changed, 38 insertions(+)
> 

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

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

Re: [PATCH 1/2] Add FAT_IOCTL_GET_VOLUME_LABEL in fat_generic_ioctl()

[Pali Rohár]

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

On Friday 05 October 2018 01:21:00 chenchacha wrote:
> Signed-off-by: chenchacha <chen.chenchacha@foxmail.com>
> ---
>  fs/fat/file.c                 | 22 ++++++++++++++++++++++
>  include/uapi/linux/msdos_fs.h |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index 4724cc9ad650..56db0b5a8df1 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -121,10 +121,30 @@ 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 inode *root_inode = d_inode(sb->s_root);
> +	struct buffer_head *bh = NULL;
> +	struct msdos_dir_entry *de;
> +	int err;
> +
> +	inode_lock_shared(root_inode);
> +	err = fat_get_root_entry(root_inode, &bh, &de);
> +	if (err == 0) {
> +		if (copy_to_user(label, de->name, MSDOS_NAME))

You need to convert entry name from 8.3 format to label in correct
encoding specified by codepage mount option. Plus needs to handle
leading 0x03 byte.

> +			err = -EFAULT;
> +	}
> +	inode_unlock_shared(root_inode);
> +
> +	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 +153,8 @@ 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);
>  	default:
>  		return -ENOTTY;	/* Inappropriate ioctl for device */
>  	}
> diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
> index fde753735aba..8ba8e2d7bafe 100644
> --- a/include/uapi/linux/msdos_fs.h
> +++ b/include/uapi/linux/msdos_fs.h
> @@ -109,6 +109,7 @@ 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[MSDOS_NAME])
>  
>  struct fat_boot_sector {
>  	__u8	ignored[3];	/* Boot strap short or near jump */

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

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

Re: [PATCH 1/2] Add FAT_IOCTL_GET_VOLUME_LABEL in fat_generic_ioctl()

[Pali Rohár]

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

On Saturday 06 October 2018 16:33:10 chen.chenchacha wrote:
> On Thu, 2018-10-04 at 19:33 +0200, Pali Rohár wrote:
> > On Friday 05 October 2018 01:21:00 chenchacha wrote:
> > > Signed-off-by: chenchacha <chen.chenchacha@foxmail.com>
> > > ---
> > >  fs/fat/file.c                 | 22 ++++++++++++++++++++++
> > >  include/uapi/linux/msdos_fs.h |  1 +
> > >  2 files changed, 23 insertions(+)
> > > 
> > > diff --git a/fs/fat/file.c b/fs/fat/file.c
> > > index 4724cc9ad650..56db0b5a8df1 100644
> > > --- a/fs/fat/file.c
> > > +++ b/fs/fat/file.c
> > > @@ -121,10 +121,30 @@ 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 inode *root_inode = d_inode(sb->s_root);
> > > +	struct buffer_head *bh = NULL;
> > > +	struct msdos_dir_entry *de;
> > > +	int err;
> > > +
> > > +	inode_lock_shared(root_inode);
> > > +	err = fat_get_root_entry(root_inode, &bh, &de);
> > > +	if (err == 0) {
> > > +		if (copy_to_user(label, de->name, MSDOS_NAME))
> > 
> > You need to convert entry name from 8.3 format to label in correct
> > encoding specified by codepage mount option. Plus needs to handle
> > leading 0x03 byte.
> > 
> I think it might be better to put the decoder/encoder on the
> application layter.

I do not think so. On all other places in msdos.ko and vfat.ko driver
which communicate with userspace is that decoder/encoder active and user
does not see raw bytes.

It is really bad to mix encodings and API of different calls.

If in mount option I specified that I want to use XYZ encoding, why
driver does not going to respect it?

> Similarly, the handle leading 0x03 byte is in the
> write function. And make read operation pure, just to carry the volume
> label in root directory to the user.
> 
> What do you think, Pali?
> 

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

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