Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752339AbdLWKXc (ORCPT ); Sat, 23 Dec 2017 05:23:32 -0500 Received: from mail.parknet.co.jp ([210.171.160.6]:34362 "EHLO mail.parknet.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750713AbdLWKXa (ORCPT ); Sat, 23 Dec 2017 05:23:30 -0500 From: OGAWA Hirofumi To: Chen Guanqiao Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 2/2] fs: fat: add ioctl method in fat filesystem driver References: <25bdc7c814dedce538cb419804fdf0c0ca9d62cd.1514017201.git.chen.chenchacha@foxmail.com> Date: Sat, 23 Dec 2017 19:23:27 +0900 In-Reply-To: <25bdc7c814dedce538cb419804fdf0c0ca9d62cd.1514017201.git.chen.chenchacha@foxmail.com> (Chen Guanqiao's message of "Sat, 23 Dec 2017 16:25:12 +0800") Message-ID: <874loh4uao.fsf@mail.parknet.co.jp> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1943 Lines: 87 Chen Guanqiao writes: > +static int fat_check_volume_label(const char *label) > +{ > + int i; > + > + for (i=0; i<11; ++i) { > + switch (label[i]) { > + case 0x20: > + case 'A' ... 'Z': > + case '0' ... '9': > + continue; > + case 0: > + return 0; > + default: > + return -EINVAL; > + } > + } > + return -EINVAL; > +} This check is really work? Especially what Windows show if '\0' is included in label? > +static int fat_ioctl_get_volume_label(struct inode *inode, > + u8 __user *vol_label) > +{ > + int ret = 0; > + struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb); > + > + if (copy_to_user(vol_label, sbi->vol_label, sizeof(sbi->vol_label))) > + ret = -EFAULT; > + > + return ret; > +} This should handle the label in root dir too. > + inode_lock(inode); Lock is not enough. > + if (sb_rdonly(inode->i_sb)) { > + err = -EROFS; > + goto out_unlock_inode; > + } mnt_want_write_file() checks it already. > + /* handling sector's vol_label */ > + bh = sb_bread(inode->i_sb, 0); > + if (bh == NULL) { > + fat_msg(inode->i_sb, KERN_ERR, > + "unable to read boot sector to write volume label"); > + err = -EFAULT; -EIO > + if (b->fat16.signature == 0x28 || b->fat32.signature == 0x28) { > + fat_msg(inode->i_sb, KERN_ERR, > + "volume label supported since OS/2 1.2 and MS-DOS 4.0 " > + "and higher"); > + err = -EFAULT; > + goto out_unlock_inode; > + } I don't know though, is this check necessary? > + /* handling root directory's vol_label */ > + bh = sb_bread(sb, sbi->dir_start); > + entry = &((struct msdos_dir_entry *)(bh->b_data))[0]; > + > + lock_buffer(bh); > + memcpy(entry->name, label, sizeof(entry->name)); > + mark_buffer_dirty(bh); > + unlock_buffer(bh); > + brelse(bh); This totally doesn't work. Please check other implementations how to handle label. > + out_unlock_inode: > + inode_unlock(inode); Missing release mnt_want_write_file(). Thanks.