Messages in this thread | | | From | OGAWA Hirofumi <> | Subject | Re: [PATCH v7 3/3] fs: fat: add ioctl method in fat filesystem driver | Date | Sun, 14 Jan 2018 22:17:44 +0900 |
| |
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 '0' ... '9': > + case 'A' ... 'Z': > + case '_': > + case 0x20: > + continue;
I didn't check though, what windows do if label = "a b c"? (I.e. invalid name as dirent name)
> +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; > + char buffer[MSDOS_NAME] = {0};
Why initialize buffer?
> + inode = d_inode(sb->s_root); > + > + err = fat_get_volume_label_entry(inode, &bh, &de); > + if (err) > + goto out;
The dir traverse has to be had lock.
> + inode_lock_shared(inode); > + memcpy(buffer, de->name, MSDOS_NAME); > + brelse(bh); > + inode_unlock_shared(inode); > + > + if (copy_to_user(vol_label, buffer, MSDOS_NAME)) > + err = -EFAULT;
No issue to copy to user memory under locking.
> +static int fat_ioctl_set_volume_label(struct file *file, > + u8 __user *vol_label) > +{
[...]
> + 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); > + inode_lock(inode);
BTW, lock order is tested with LOCKDEP?
> + /* Synchronize the data together */ > + err = sync_dirty_buffer(boot_bh); > + if (err) > + goto out_boot_brelse;
> + err = sync_dirty_buffer(vol_bh); > + if (err) > + goto out_boot_brelse;
Probably, we don't need to sync vol_bh. And in the case of adding new entry, now already doesn't sync. -- OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
| |