On 2019/4/18 下午2:34, Nikolay Borisov wrote: > > > On 18.04.19 г. 9:28 ч., Qu Wenruo wrote: >> [BUG] >> With kmalloc failure injection for submit_one_bio(), btrfs can crash like: >> >> BUG: unable to handle kernel NULL pointer dereference at 0000000000000038 >> #PF error: [WRITE] >> PGD 0 P4D 0 >> Oops: 0002 [#1] PREEMPT SMP PTI >> CPU: 1 PID: 247 Comm: kworker/u8:4 Not tainted 5.1.0-rc5-custom+ #19 >> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 >> Workqueue: writeback wb_workfn (flush-btrfs-6) >> RIP: 0010:alloc_btrfs_bio+0x1e/0x30 [btrfs] >> Code: 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 0f 1f 44 00 00 48 63 f6 48 63 ff 48 8d 7c fe 18 be 40 8d 00 00 48 c1 e7 02 e8 a2 86 94 e0 <c7> 40 38 00 00 00 00 c7 00 01 00 00 00 c3 0f 1f 40 00 0f 1f 44 00 >> Call Trace: >> __btrfs_map_block+0x5ce/0x1210 [btrfs] >> ? btrfs_bio_counter_inc_blocked+0x3a/0xc0 [btrfs] >> btrfs_map_bio+0x9a/0x430 [btrfs] >> btree_submit_bio_hook+0x82/0xb0 [btrfs] >> submit_one_bio+0x95/0xc0 [btrfs] >> copy_oldmem_page_encrypted+0x20/0x20 >> ? write_one_eb+0x18f/0x2a0 [btrfs] >> ? end_extent_buffer_writeback+0x20/0x20 [btrfs] >> ? btree_write_cache_pages+0x12c/0x350 [btrfs] >> ? do_writepages+0x41/0xd0 >> ? __writeback_single_inode+0x54/0x650 >> ? writeback_sb_inodes+0x1f9/0x540 >> ? __writeback_inodes_wb+0x5d/0xb0 >> ? wb_writeback+0x340/0x4b0 >> ? wb_workfn+0x410/0x5d0 >> ? process_one_work+0x294/0x650 >> ? worker_thread+0x2d/0x3d0 >> ? process_one_work+0x650/0x650 >> ? kthread+0x112/0x130 >> ? kthread_park+0x80/0x80 >> ? ret_from_fork+0x3a/0x50 >> ---[ end trace b637169fb8b17c9c ]--- >> >> [CAUSE] >> We just forgot to check the return value of kmalloc. >> Surprisingly, all alloc_btrfs_bio() callers have handled memory >> allocation pretty well. >> > > The allocation uses the GFP_NOFAIL modified, which, according to the docs: > > * The VM implementation _must_ retry infinitely: the caller > * cannot handle allocation failures. The allocation could block > > * indefinitely but will never return with failure. Testing for > > * failure is pointless. Forgot the NOFAIL bit. > > The allocation requested is at least 128 bytes (assuming real_stripes is > 0). > > 96 + 24 * total_stripes + 4 * real_stripes + 8 * total_stripes > > Considering this I think it might be prudent to also remove the NOFAIL > flag altogether Definitely will remove NOFAIL flag for V2. Thanks, Qu > > >> [FIX] >> Check and return if we failed memory allocation. >> >> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx> > > Though the change is fine: > > Reviewed-by: Nikolay Borisov <nborisov@xxxxxxxx> > > >> --- >> fs/btrfs/volumes.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 78bab7803bda..875d0eee1785 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -5582,6 +5582,8 @@ static struct btrfs_bio *alloc_btrfs_bio(int total_stripes, int real_stripes) >> sizeof(u64) * (total_stripes), >> GFP_NOFS|__GFP_NOFAIL); >> >> + if (!bbio) >> + return NULL; >> atomic_set(&bbio->error, 0); >> refcount_set(&bbio->refs, 1); >> >>
