From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aserp1040.oracle.com ([141.146.126.69]:31241 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750705AbaGGEEY (ORCPT ); Mon, 7 Jul 2014 00:04:24 -0400 Message-ID: <53BA1C39.9090407@oracle.com> Date: Mon, 07 Jul 2014 12:04:09 +0800 From: Anand Jain MIME-Version: 1.0 To: Miao Xie 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> In-Reply-To: <1404382933-26672-7-git-send-email-miaox@cn.fujitsu.com> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 03/07/2014 18:22, Miao Xie wrote: > From: Anand Jain > > 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++; > + } 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