Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751457AbdL0N3l (ORCPT ); Wed, 27 Dec 2017 08:29:41 -0500 Received: from mail.parknet.co.jp ([210.171.160.6]:34859 "EHLO mail.parknet.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750931AbdL0N3k (ORCPT ); Wed, 27 Dec 2017 08:29:40 -0500 From: OGAWA Hirofumi To: ChenGuanqiao Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 2/2] fs: fat: add ioctl method in fat filesystem driver References: <20171227091714.4566-1-chen.chenchacha@foxmail.com> <20171227091714.4566-3-chen.chenchacha@foxmail.com> Date: Wed, 27 Dec 2017 22:29:38 +0900 In-Reply-To: <20171227091714.4566-3-chen.chenchacha@foxmail.com> (ChenGuanqiao's message of "Wed, 27 Dec 2017 17:17:14 +0800") Message-ID: <87efngwb7h.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: 1948 Lines: 91 ChenGuanqiao writes: > +/* the characters in this field shall be d-characters, and unused byte shall be set to 0x20. */ > +static int fat_format_d_characters(char *label, unsigned long len) > +{ > + int i; > + > + for (i=0; i + switch (label[i]) { > + case '0' ... '9': > + case 'A' ... 'Z': > + case '_': > + continue; > + default: > + break; > + } > + break; > + } > + > + for (; i + label[i] = 0x20; > + > + return len; > +} It should not accept and overwrite invalid char, return -EINVAL. > +static int fat_ioctl_get_volume_label(struct inode *inode, > + u8 __user *vol_label) > +{ > + int err = 0; > + struct fat_slot_info sinfo; > + > + err = fat_scan_volume_label(inode, &sinfo); > + if (err) > + goto out; It needs to take inode read lock. > + if (copy_to_user(vol_label, sinfo.de->name, MSDOS_NAME)) > + err = -EFAULT; > + > + brelse(sinfo.bh); > +out: > + return err; > +} > + > +static int fat_ioctl_set_volume_label(struct file *file, > + u8 __user *vol_label) > +{ [...] > + b = (struct fat_boot_sector *)bh->b_data; > + > + lock_buffer(bh); Probably, sb->s_umount to exclusive with remount update. > + 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(bh); > + unlock_buffer(bh); > + err = sync_dirty_buffer(bh); > + brelse(bh); > + if (err) > + goto out_drop_file; It should not update before preparing is done and checking error. > + /* updates root directory's vol_label */ > + err = fat_scan_volume_label(inode, &sinfo); > + if (err) > + goto out_drop_file; It should add entry if no volume label. > + lock_buffer(bh); inode write lock. > + memcpy(sinfo.de->name, label, sizeof(sinfo.de->name)); > + mark_buffer_dirty(bh); > + unlock_buffer(bh); Thanks. -- OGAWA Hirofumi