Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: 5UbE3DjmiAvdCTUSeO4HpV+gaSjGJLXE2ZY/LPPcDdFh3YGaHXA+ltdBXZlLufZ34+pwhPIu8M
 EghqQNUDYYR88TRqzuMoycSxCHiFGwJhKPHhqcU7ss6EOxLh3Pby0M9eSqF/dUQuFwMTA7NXMk
 AdCpnATNVjF7FoRo0+2uNTgoUWWB4DYtG6dXs3GDxaUHNaSpeBw97SwaDK6AcJ7chHEAcUtsEB
 7sG9hAAgwXoM6sBrbkNJzl7JgDBBKV/kDeB/uccRlFtDYLP/5rDzcGlUOzkGp56a8A1BqmmIk/
 EXE=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Jens Axboe <axboe@kernel.dk>,
        "yanfei.xu@windriver.com" <yanfei.xu@windriver.com>
CC:     "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] block: fix possible bd_size_lock deadlock
Thread-Topic: [PATCH] block: fix possible bd_size_lock deadlock
Thread-Index: AQHXFm/BBdsOQMCsFUCpesdiLb6LDg==
Date:   Fri, 12 Mar 2021 22:32:48 +0000
Message-ID: <BL0PR04MB651467F82F7B303829D2BB26E76F9@BL0PR04MB6514.namprd04.prod.outlook.com>
References: <20210311121139.205222-1-yanfei.xu@windriver.com>
 <af8f7d00-d612-c0d2-e254-ff3c967fb94c@kernel.dk>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6bf3a731-b373-4281-5a5e-08d8e5a6c41a
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Mar 2021 22:32:48.9611
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: d80c+rilVmacZ9ngOXME85050qepviRrA/j7jy7weOHaEEUEeOLNsJwZmGCSPJ8xwiYhfI25OCnX/UDKZiSEdQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR04MB4980
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021/03/13 4:37, Jens Axboe wrote:=0A=
> On 3/11/21 5:11 AM, yanfei.xu@windriver.com wrote:=0A=
>> From: Yanfei Xu <yanfei.xu@windriver.com>=0A=
>>=0A=
>> bd_size_lock spinlock could be taken in block softirq, thus we should=0A=
>> disable the softirq before taking the lock.=0A=
>>=0A=
>> WARNING: inconsistent lock state=0A=
>> 5.12.0-rc2-syzkaller #0 Not tainted=0A=
>> --------------------------------=0A=
>> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-R} usage.=0A=
>> kworker/u4:0/7 [HC0[0]:SC1[1]:HE0:SE0] takes:=0A=
>> 8f87826c (&inode->i_size_seqcount){+.+-}-{0:0}, at:=0A=
>> end_bio_bh_io_sync+0x38/0x54 fs/buffer.c:3006=0A=
>> {SOFTIRQ-ON-W} state was registered at:=0A=
>>   lock_acquire.part.0+0xf0/0x41c kernel/locking/lockdep.c:5510=0A=
>>   lock_acquire+0x6c/0x74 kernel/locking/lockdep.c:5483=0A=
>>   do_write_seqcount_begin_nested include/linux/seqlock.h:520 [inline]=0A=
>>   do_write_seqcount_begin include/linux/seqlock.h:545 [inline]=0A=
>>   i_size_write include/linux/fs.h:863 [inline]=0A=
>>   set_capacity+0x13c/0x1f8 block/genhd.c:50=0A=
>>   brd_alloc+0x130/0x180 drivers/block/brd.c:401=0A=
>>   brd_init+0xcc/0x1e0 drivers/block/brd.c:500=0A=
>>   do_one_initcall+0x8c/0x59c init/main.c:1226=0A=
>>   do_initcall_level init/main.c:1299 [inline]=0A=
>>   do_initcalls init/main.c:1315 [inline]=0A=
>>   do_basic_setup init/main.c:1335 [inline]=0A=
>>   kernel_init_freeable+0x2cc/0x330 init/main.c:1537=0A=
>>   kernel_init+0x10/0x120 init/main.c:1424=0A=
>>   ret_from_fork+0x14/0x20 arch/arm/kernel/entry-common.S:158=0A=
>>   0x0=0A=
>> irq event stamp: 2783413=0A=
>> hardirqs last  enabled at (2783412): [<802011ec>]=0A=
>> __do_softirq+0xf4/0x7ac kernel/softirq.c:329=0A=
>> hardirqs last disabled at (2783413): [<8277d260>]=0A=
>> __raw_read_lock_irqsave include/linux/rwlock_api_smp.h:157 [inline]=0A=
>> hardirqs last disabled at (2783413): [<8277d260>]=0A=
>> _raw_read_lock_irqsave+0x84/0x88 kernel/locking/spinlock.c:231=0A=
>> softirqs last  enabled at (2783410): [<826b5050>] spin_unlock_bh=0A=
>> include/linux/spinlock.h:399 [inline]=0A=
>> softirqs last  enabled at (2783410): [<826b5050>]=0A=
>> batadv_nc_purge_paths+0x10c/0x148 net/batman-adv/network-coding.c:467=0A=
>> softirqs last disabled at (2783411): [<8024ddfc>] do_softirq_own_stack=
=0A=
>> include/asm-generic/softirq_stack.h:10 [inline]=0A=
>> softirqs last disabled at (2783411): [<8024ddfc>] do_softirq=0A=
>> kernel/softirq.c:248 [inline]=0A=
>> softirqs last disabled at (2783411): [<8024ddfc>] do_softirq+0xd8/0xe4=
=0A=
>> kernel/softirq.c:235=0A=
>>=0A=
>> other info that might help us debug this:=0A=
>>  Possible unsafe locking scenario:=0A=
>>=0A=
>>        CPU0=0A=
>>        ----=0A=
>>   lock(&inode->i_size_seqcount);=0A=
>>   <Interrupt>=0A=
>>     lock(&inode->i_size_seqcount);=0A=
>>=0A=
>>  *** DEADLOCK ***=0A=
>>=0A=
>> 3 locks held by kworker/u4:0/7:=0A=
>>  #0: 88c622a8 ((wq_completion)bat_events){+.+.}-{0:0}, at: set_work_data=
=0A=
>> kernel/workqueue.c:615 [inline]=0A=
>>  #0: 88c622a8 ((wq_completion)bat_events){+.+.}-{0:0}, at:=0A=
>> set_work_pool_and_clear_pending kernel/workqueue.c:643 [inline]=0A=
>>  #0: 88c622a8 ((wq_completion)bat_events){+.+.}-{0:0}, at:=0A=
>> process_one_work+0x214/0x998 kernel/workqueue.c:2246=0A=
>>  #1: 85147ef8=0A=
>> ((work_completion)(&(&bat_priv->nc.work)->work)){+.+.}-{0:0}, at:=0A=
>> set_work_data kernel/workqueue.c:615 [inline]=0A=
>>  #1: 85147ef8=0A=
>> ((work_completion)(&(&bat_priv->nc.work)->work)){+.+.}-{0:0}, at:=0A=
>> set_work_pool_and_clear_pending kernel/workqueue.c:643 [inline]=0A=
>>  #1: 85147ef8=0A=
>> ((work_completion)(&(&bat_priv->nc.work)->work)){+.+.}-{0:0}, at:=0A=
>> process_one_work+0x214/0x998 kernel/workqueue.c:2246=0A=
>>  #2: 8f878010 (&ni->size_lock){...-}-{2:2}, at:=0A=
>> ntfs_end_buffer_async_read+0x6c/0x558 fs/ntfs/aops.c:66=0A=
> =0A=
> Damien? We have that revert queued up for this for 5.12, but looking=0A=
> at that, the state before that was kind of messy too.=0A=
=0A=
Indeed... I was thinking about this and I think I am with Christoph on this=
:=0A=
drivers should not call set_capacity() from command completion context. I t=
hink=0A=
the best thing to do would be to fix drivers that do that but that may not =
be RC=0A=
material ?=0A=
=0A=
Looking into more details of this case, it is slightly different though.=0A=
set_capacity() is here not called from soft IRQ context. It looks like a re=
gular=0A=
initialization, but one that seems way too early in the boot process when a=
=0A=
secondary core is being initialized with IRQ not yet enabled... I think. An=
d the=0A=
warnings come from i_size_write() calling preempt_disable() rather than=0A=
set_capacity() use of spin_lock(&bdev->bd_size_lock).=0A=
=0A=
I wonder how it is possible to have brd being initialized so early.=0A=
I am not sure how to fix that. It looks like arm arch code territory.=0A=
=0A=
For now, we could revert the revert as I do not think that Yanfei patch is=
=0A=
enough since completions may be from hard IRQ context too, which is not cov=
ered=0A=
with the spin_lock_bh() variants (c.f. a similar problem we are facing with=
 that=0A=
in scsi completion [1])=0A=
I do not have any good idea how to proceed though.=0A=
=0A=
[1]=0A=
https://lore.kernel.org/linux-scsi/PH0PR04MB7416C8330459E92D8AA21A889B6F9@P=
H0PR04MB7416.namprd04.prod.outlook.com/T/#t=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=