Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754564AbcCJRhU (ORCPT <rfc822;w@1wt.eu>);
	Thu, 10 Mar 2016 12:37:20 -0500
Received: from mx2.suse.de ([195.135.220.15]:60992 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752618AbcCJRhI (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 10 Mar 2016 12:37:08 -0500
Date: Thu, 10 Mar 2016 18:37:27 +0100
From: Jan Kara <jack@suse.cz>
To: Rabin Vincent <rabin.vincent@axis.com>
Cc: viro@zeniv.linux.org.uk, jack@suse.cz, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, Rabin Vincent <rabinv@axis.com>,
        jaxboe@kernel.dk, hch@lst.de
Subject: Re: [PATCH] block: protect iterate_bdevs() against concurrent close
Message-ID: <20160310173727.GA19795@quack.suse.cz>
References: <1457612763-23196-1-git-send-email-rabin.vincent@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1457612763-23196-1-git-send-email-rabin.vincent@axis.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3503
Lines: 71

On Thu 10-03-16 13:26:03, Rabin Vincent wrote:
> From: Rabin Vincent <rabinv@axis.com>
> 
> If a block device is closed while iterate_bdevs() is handling it, the
> following NULL pointer dereference occurs because bdev->b_disk is NULL
> in bdev_get_queue(), which is called from blk_get_backing_dev_info() (in
> turn called by the mapping_cap_writeback_dirty() call in
> __filemap_fdatawrite_range()):
> 
>  BUG: unable to handle kernel NULL pointer dereference at 0000000000000508
>  IP: [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
>  PGD 9e62067 PUD 9ee8067 PMD 0
>  Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
>  Modules linked in:
>  CPU: 1 PID: 2422 Comm: sync Not tainted 4.5.0-rc7+ #400
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>  task: ffff880009f4d700 ti: ffff880009f5c000 task.ti: ffff880009f5c000
>  RIP: 0010:[<ffffffff81314790>]  [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
>  RSP: 0018:ffff880009f5fe68  EFLAGS: 00010246
>  RAX: 0000000000000000 RBX: ffff88000ec17a38 RCX: ffffffff81a4e940
>  RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: ffff88000ec176c0
>  RBP: ffff880009f5fe68 R08: 0000000000000000 R09: 0000000000000000
>  R10: 0000000000000001 R11: 0000000000000000 R12: ffff88000ec17860
>  R13: ffffffff811b25c0 R14: ffff88000ec178e0 R15: ffff88000ec17a38
>  FS:  00007faee505d700(0000) GS:ffff88000fb00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>  CR2: 0000000000000508 CR3: 0000000009e8a000 CR4: 00000000000006e0
>  Stack:
>   ffff880009f5feb8 ffffffff8112e7f5 0000000000000000 7fffffffffffffff
>   0000000000000000 0000000000000000 7fffffffffffffff 0000000000000001
>   ffff88000ec178e0 ffff88000ec17860 ffff880009f5fec8 ffffffff8112e81f
>  Call Trace:
>   [<ffffffff8112e7f5>] __filemap_fdatawrite_range+0x85/0x90
>   [<ffffffff8112e81f>] filemap_fdatawrite+0x1f/0x30
>   [<ffffffff811b25d6>] fdatawrite_one_bdev+0x16/0x20
>   [<ffffffff811bc402>] iterate_bdevs+0xf2/0x130
>   [<ffffffff811b2763>] sys_sync+0x63/0x90
>   [<ffffffff815d4272>] entry_SYSCALL_64_fastpath+0x12/0x76
>  Code: 0f 1f 44 00 00 48 8b 87 f0 00 00 00 55 48 89 e5 <48> 8b 80 08 05 00 00 5d
>  RIP  [<ffffffff81314790>] blk_get_backing_dev_info+0x10/0x20
>   RSP <ffff880009f5fe68>
>  CR2: 0000000000000508
>  ---[ end trace 2487336ceb3de62d ]---
> 
> The crash is easily reproducible by running the following command, if an
> msleep(100) is inserted before the call to func() in iterate_devs():
> 
>  while :; do head -c1 /dev/nullb0; done > /dev/null & while :; do sync; done
> 
> Fix it by holding the bd_mutex across the func() call and only calling
> func() if the bdev is opened.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rabin Vincent <rabinv@axis.com>

Thanks for spotting the problem. The patch will fix the problem you found.
But what prevents e.g. flusher thread from trying to writeback the block
device inode while that gets invalidated at the same moment? I.e., the
problem doesn't seem to be specific to iterate_bdevs() but rather hits
anything which writes back block device inodes, right?

Frankly it seems like a catch that mapping_cap_foo() requires block device
to be open (if mapping belongs to a block device) to be safe. The trouble
is we'd need to make inode_to_bdi() result stable while we work with the
block device inode. Christoph, do you have any idea how to cleanly achieve
that? All I'm able to come up with are ugly hacks...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR