From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp1040.oracle.com ([141.146.126.69]:38924 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751706AbaGGJ4P (ORCPT ); Mon, 7 Jul 2014 05:56:15 -0400 Message-ID: <53BA6EBD.9060004@oracle.com> Date: Mon, 07 Jul 2014 17:56:13 +0800 From: Anand Jain MIME-Version: 1.0 To: miaox@cn.fujitsu.com CC: linux-btrfs@vger.kernel.org Subject: Re: [PATCH V2 7/9] btrfs: fix null pointer dereference in clone_fs_devices when name is null References: <1404382933-26672-1-git-send-email-miaox@cn.fujitsu.com> <1404382933-26672-7-git-send-email-miaox@cn.fujitsu.com> <53BA1C39.9090407@oracle.com> <53BA206D.4040301@cn.fujitsu.com> In-Reply-To: <53BA206D.4040301@cn.fujitsu.com> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 07/07/2014 12:22, Miao Xie wrote: > On Mon, 7 Jul 2014 12:04:09 +0800, Anand Jain wrote: >>> when one of the device path is missing btrfs_device name is null. So this >>> patch will check for that. >>> >>> stack: >>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 >>> IP: [] strlen+0x0/0x30 >>> [] ? clone_fs_devices+0xaa/0x160 [btrfs] >>> [] btrfs_init_new_device+0x317/0xca0 [btrfs] >>> [] ? __kmalloc_track_caller+0x15a/0x1a0 >>> [] btrfs_ioctl+0xaa3/0x2860 [btrfs] >>> [] ? handle_mm_fault+0x48c/0x9c0 >>> [] ? __blkdev_put+0x171/0x180 >>> [] ? __do_page_fault+0x4ac/0x590 >>> [] ? blkdev_put+0x106/0x110 >>> [] ? mntput+0x35/0x40 >>> [] do_vfs_ioctl+0x460/0x4a0 >>> [] ? ____fput+0xe/0x10 >>> [] ? task_work_run+0xb3/0xd0 >>> [] SyS_ioctl+0x57/0x90 >>> [] ? do_page_fault+0xe/0x10 >>> [] system_call_fastpath+0x16/0x1b >>> >>> reproducer: >>> mkfs.btrfs -draid1 -mraid1 /dev/sdg1 /dev/sdg2 >>> btrfstune -S 1 /dev/sdg1 >>> modprobe -r btrfs && modprobe btrfs >>> mount -o degraded /dev/sdg1 /btrfs >>> btrfs dev add /dev/sdg3 /btrfs >>> >>> Signed-off-by: Anand Jain >>> Signed-off-by: Miao Xie >>> --- >>> Changelog v1->v2: >>> - Fix the problem that we forgot to set the missing flag for the cloned device >>> --- >>> fs/btrfs/volumes.c | 25 ++++++++++++++++--------- >>> 1 file changed, 16 insertions(+), 9 deletions(-) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 1891541..4731bd6 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -598,16 +598,23 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig) >>> if (IS_ERR(device)) >>> goto error; >>> >>> - /* >>> - * This is ok to do without rcu read locked because we hold the >>> - * uuid mutex so nothing we touch in here is going to disappear. >>> - */ >>> - name = rcu_string_strdup(orig_dev->name->str, GFP_NOFS); >>> - if (!name) { >>> - kfree(device); >>> - goto error; >>> + if (orig_dev->missing) { >>> + device->missing = 1; >>> + fs_devices->missing_devices++; >> >> as mentioned in some places we just check name (for missing device) >> and don't set the missing flag so it better to .. >> >> if (orig_dev->missing || !orig_dev->name) { >> device->missing = 1; >> fs_devices->missing_devices++; > > I don't think we need check name pointer here because only missing device > doesn't have its own name. Or there is something wrong in the code, so > I add assert in else branch. Am I right? At few critical code, the below and I guess in the chunk/strips function as well, we don't make use of missing flag, but rather ->name. ----- btrfsic_process_superblock :: if (!device->bdev || !device->name) continue; ----- But here without !orig_dev->name check, is also good enough. Thanks, Anand >>> + } else { >>> + ASSERT(orig_dev->name); >>> + /* >>> + * This is ok to do without rcu read locked because >>> + * we hold the uuid mutex so nothing we touch in here >>> + * is going to disappear. >>> + */ >>> + name = rcu_string_strdup(orig_dev->name->str, GFP_NOFS); >>> + if (!name) { >>> + kfree(device); >>> + goto error; >>> + } >>> + rcu_assign_pointer(device->name, name); >>> } >>> - rcu_assign_pointer(device->name, name); >>> >>> list_add(&device->dev_list, &fs_devices->devices); >>> device->fs_devices = fs_devices; >>> >> >> Thanks, Anand >> . >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >