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

["Pali Rohár" <pali.rohar@gmail.com>]

[-- 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 --]