* [PATCH] btrfs: volumes: Check memory allocation failure for alloc_btrfs_bio()
@ 2019-04-18 6:28 Qu Wenruo
2019-04-18 6:34 ` Nikolay Borisov
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2019-04-18 6:28 UTC (permalink / raw)
To: linux-btrfs
[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.
[FIX]
Check and return if we failed memory allocation.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
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);
--
2.21.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: volumes: Check memory allocation failure for alloc_btrfs_bio()
2019-04-18 6:28 [PATCH] btrfs: volumes: Check memory allocation failure for alloc_btrfs_bio() Qu Wenruo
@ 2019-04-18 6:34 ` Nikolay Borisov
2019-04-18 6:49 ` Qu Wenruo
0 siblings, 1 reply; 3+ messages in thread
From: Nikolay Borisov @ 2019-04-18 6:34 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
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.
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
> [FIX]
> Check and return if we failed memory allocation.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
Though the change is fine:
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 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);
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: volumes: Check memory allocation failure for alloc_btrfs_bio()
2019-04-18 6:34 ` Nikolay Borisov
@ 2019-04-18 6:49 ` Qu Wenruo
0 siblings, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2019-04-18 6:49 UTC (permalink / raw)
To: Nikolay Borisov, Qu Wenruo, linux-btrfs
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@suse.com>
>
> Though the change is fine:
>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
>
>> ---
>> 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);
>>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-04-18 6:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-18 6:28 [PATCH] btrfs: volumes: Check memory allocation failure for alloc_btrfs_bio() Qu Wenruo
2019-04-18 6:34 ` Nikolay Borisov
2019-04-18 6:49 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).