The IDE driver creates some passthru requests which never get
submitted to the block layer in such a way that blk_account_io_start()
gets called. However, the driver still calls __blk_mq_end_request() in
ide_end_rq() which will call blk_account_io_completion() which tries
to dereferences req->part which is never set. See ide_prep_sense() for
an example of where these requests come from.
To fix this, blk_account_io_completion() and blk_account_io_done()
should do nothing if req->part is not set.
The back trace of this bug is:
BUG: kernel NULL pointer dereference, address: 000002ac
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
*pde = 00000000
Oops: 0002 [#1]
CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted
5.4.0-rc2-00011-g48d9b0d43105e #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1
04/01/2014
Workqueue: kblockd drive_rq_insert_work
EIP: blk_account_io_completion+0x7a/0xf0
Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee
09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4
02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b
EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000
ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54
DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046
CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0
Call Trace:
<IRQ>
blk_update_request+0x85/0x420
ide_end_rq+0x38/0xa0
ide_complete_rq+0x3d/0x70
cdrom_newpc_intr+0x258/0xba0
ide_intr+0x135/0x250
__handle_irq_event_percpu+0x3e/0x250
handle_irq_event_percpu+0x1f/0x50
handle_irq_event+0x32/0x60
handle_level_irq+0x6c/0x110
handle_irq+0x72/0xa0
</IRQ>
do_IRQ+0x45/0xad
common_interrupt+0x115/0x11c
Fixes: 48d9b0d43105 ("block: account statistics for passthrough requests")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Logan Gunthorpe <[email protected]>
---
block/blk-core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index a1e228752083..68c309ce6735 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1310,7 +1310,7 @@ EXPORT_SYMBOL_GPL(blk_rq_err_bytes);
void blk_account_io_completion(struct request *req, unsigned int bytes)
{
- if (blk_do_io_stat(req)) {
+ if (req->part && blk_do_io_stat(req)) {
const int sgrp = op_stat_group(req_op(req));
struct hd_struct *part;
@@ -1328,7 +1328,8 @@ void blk_account_io_done(struct request *req, u64 now)
* normal IO on queueing nor completion. Accounting the
* containing request is enough.
*/
- if (blk_do_io_stat(req) && !(req->rq_flags & RQF_FLUSH_SEQ)) {
+ if (req->part && blk_do_io_stat(req) &&
+ !(req->rq_flags & RQF_FLUSH_SEQ)) {
const int sgrp = op_stat_group(req_op(req));
struct hd_struct *part;
--
2.20.1
On 12/10/19 11:47 AM, Logan Gunthorpe wrote:
> The IDE driver creates some passthru requests which never get
> submitted to the block layer in such a way that blk_account_io_start()
> gets called. However, the driver still calls __blk_mq_end_request() in
> ide_end_rq() which will call blk_account_io_completion() which tries
> to dereferences req->part which is never set. See ide_prep_sense() for
> an example of where these requests come from.
>
> To fix this, blk_account_io_completion() and blk_account_io_done()
> should do nothing if req->part is not set.
>
> The back trace of this bug is:
>
> BUG: kernel NULL pointer dereference, address: 000002ac
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0002) - not-present page
> *pde = 00000000
> Oops: 0002 [#1]
> CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted
> 5.4.0-rc2-00011-g48d9b0d43105e #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1
> 04/01/2014
> Workqueue: kblockd drive_rq_insert_work
> EIP: blk_account_io_completion+0x7a/0xf0
> Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee
> 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4
> 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b
> EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000
> ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54
> DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046
> CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0
> Call Trace:
> <IRQ>
> blk_update_request+0x85/0x420
> ide_end_rq+0x38/0xa0
> ide_complete_rq+0x3d/0x70
> cdrom_newpc_intr+0x258/0xba0
> ide_intr+0x135/0x250
> __handle_irq_event_percpu+0x3e/0x250
> handle_irq_event_percpu+0x1f/0x50
> handle_irq_event+0x32/0x60
> handle_level_irq+0x6c/0x110
> handle_irq+0x72/0xa0
> </IRQ>
> do_IRQ+0x45/0xad
> common_interrupt+0x115/0x11c
Why not just:
diff --git a/block/blk.h b/block/blk.h
index 6842f28c033e..d7407b5d0200 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -250,7 +250,7 @@ int blk_dev_init(void);
*/
static inline bool blk_do_io_stat(struct request *rq)
{
- return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
+ return rq->part && rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
}
static inline void req_set_nomerge(struct request_queue *q, struct request *req)
--
Jens Axboe
On 2019-12-10 11:59 a.m., Jens Axboe wrote:
> On 12/10/19 11:47 AM, Logan Gunthorpe wrote:
>> The IDE driver creates some passthru requests which never get
>> submitted to the block layer in such a way that blk_account_io_start()
>> gets called. However, the driver still calls __blk_mq_end_request() in
>> ide_end_rq() which will call blk_account_io_completion() which tries
>> to dereferences req->part which is never set. See ide_prep_sense() for
>> an example of where these requests come from.
>>
>> To fix this, blk_account_io_completion() and blk_account_io_done()
>> should do nothing if req->part is not set.
>>
>> The back trace of this bug is:
>>
>> BUG: kernel NULL pointer dereference, address: 000002ac
>> #PF: supervisor write access in kernel mode
>> #PF: error_code(0x0002) - not-present page
>> *pde = 00000000
>> Oops: 0002 [#1]
>> CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted
>> 5.4.0-rc2-00011-g48d9b0d43105e #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1
>> 04/01/2014
>> Workqueue: kblockd drive_rq_insert_work
>> EIP: blk_account_io_completion+0x7a/0xf0
>> Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee
>> 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4
>> 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b
>> EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000
>> ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54
>> DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046
>> CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0
>> Call Trace:
>> <IRQ>
>> blk_update_request+0x85/0x420
>> ide_end_rq+0x38/0xa0
>> ide_complete_rq+0x3d/0x70
>> cdrom_newpc_intr+0x258/0xba0
>> ide_intr+0x135/0x250
>> __handle_irq_event_percpu+0x3e/0x250
>> handle_irq_event_percpu+0x1f/0x50
>> handle_irq_event+0x32/0x60
>> handle_level_irq+0x6c/0x110
>> handle_irq+0x72/0xa0
>> </IRQ>
>> do_IRQ+0x45/0xad
>> common_interrupt+0x115/0x11c
>
> Why not just:
>
> diff --git a/block/blk.h b/block/blk.h
> index 6842f28c033e..d7407b5d0200 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -250,7 +250,7 @@ int blk_dev_init(void);
> */
> static inline bool blk_do_io_stat(struct request *rq)
> {
> - return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
> + return rq->part && rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
> }
Because blk_account_io_start() also checks blk_do_io_stat() and, in that
case, rq->part will never be set (seeing that's the function that
typically sets it); thus that solution would disable stats entirely.
Logan
On 12/10/19 12:02 PM, Logan Gunthorpe wrote:
>
>
> On 2019-12-10 11:59 a.m., Jens Axboe wrote:
>> On 12/10/19 11:47 AM, Logan Gunthorpe wrote:
>>> The IDE driver creates some passthru requests which never get
>>> submitted to the block layer in such a way that blk_account_io_start()
>>> gets called. However, the driver still calls __blk_mq_end_request() in
>>> ide_end_rq() which will call blk_account_io_completion() which tries
>>> to dereferences req->part which is never set. See ide_prep_sense() for
>>> an example of where these requests come from.
>>>
>>> To fix this, blk_account_io_completion() and blk_account_io_done()
>>> should do nothing if req->part is not set.
>>>
>>> The back trace of this bug is:
>>>
>>> BUG: kernel NULL pointer dereference, address: 000002ac
>>> #PF: supervisor write access in kernel mode
>>> #PF: error_code(0x0002) - not-present page
>>> *pde = 00000000
>>> Oops: 0002 [#1]
>>> CPU: 0 PID: 237 Comm: kworker/0:1H Not tainted
>>> 5.4.0-rc2-00011-g48d9b0d43105e #1
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1
>>> 04/01/2014
>>> Workqueue: kblockd drive_rq_insert_work
>>> EIP: blk_account_io_completion+0x7a/0xf0
>>> Code: 89 54 24 08 31 d2 89 4c 24 04 31 c9 c7 04 24 02 00 00 00 c1 ee
>>> 09 e8 f5 21 a6 ff e8 70 5c a7 ff 8b 53 60 8d 04 bd 00 00 00 00 <01> b4
>>> 02 ac 02 00 00 8b 9a 88 02 00 00 85 db 74 11 85 d2 74 51 8b
>>> EAX: 00000000 EBX: f5b80000 ECX: 00000000 EDX: 00000000
>>> ESI: 00000000 EDI: 00000000 EBP: f3031e70 ESP: f3031e54
>>> DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010046
>>> CR0: 80050033 CR2: 000002ac CR3: 03c25000 CR4: 000406d0
>>> Call Trace:
>>> <IRQ>
>>> blk_update_request+0x85/0x420
>>> ide_end_rq+0x38/0xa0
>>> ide_complete_rq+0x3d/0x70
>>> cdrom_newpc_intr+0x258/0xba0
>>> ide_intr+0x135/0x250
>>> __handle_irq_event_percpu+0x3e/0x250
>>> handle_irq_event_percpu+0x1f/0x50
>>> handle_irq_event+0x32/0x60
>>> handle_level_irq+0x6c/0x110
>>> handle_irq+0x72/0xa0
>>> </IRQ>
>>> do_IRQ+0x45/0xad
>>> common_interrupt+0x115/0x11c
>>
>> Why not just:
>>
>> diff --git a/block/blk.h b/block/blk.h
>> index 6842f28c033e..d7407b5d0200 100644
>> --- a/block/blk.h
>> +++ b/block/blk.h
>> @@ -250,7 +250,7 @@ int blk_dev_init(void);
>> */
>> static inline bool blk_do_io_stat(struct request *rq)
>> {
>> - return rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
>> + return rq->part && rq->rq_disk && (rq->rq_flags & RQF_IO_STAT);
>> }
>
> Because blk_account_io_start() also checks blk_do_io_stat() and, in that
> case, rq->part will never be set (seeing that's the function that
> typically sets it); thus that solution would disable stats entirely.
Gotcha. I'm fine with the patch you posted in that case.
--
Jens Axboe