* [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
@ 2017-05-31 21:43 ` Bart Van Assche
2017-06-01 19:09 ` Jens Axboe
2017-06-13 17:54 ` Ross Zwisler
2017-05-31 21:43 ` [PATCH v2 2/6] block: Introduce queue flag QUEUE_FLAG_SCSI_PASSTHROUGH Bart Van Assche
` (6 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Bart Van Assche @ 2017-05-31 21:43 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Bart Van Assche, Jan Kara, stable
Since the introduction of .init_rq_fn() and .exit_rq_fn() it is
essential that the memory allocated for struct request_queue
stays around until all blk_exit_rl() calls have finished. Hence
make blk_init_rl() take a reference on struct request_queue.
This patch fixes the following crash:
general protection fault: 0000 [#2] SMP
CPU: 3 PID: 28 Comm: ksoftirqd/3 Tainted: G D 4.12.0-rc2-dbg+ #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
task: ffff88013a108040 task.stack: ffffc9000071c000
RIP: 0010:free_request_size+0x1a/0x30
RSP: 0018:ffffc9000071fd38 EFLAGS: 00010202
RAX: 6b6b6b6b6b6b6b6b RBX: ffff880067362a88 RCX: 0000000000000003
RDX: ffff880067464178 RSI: ffff880067362a88 RDI: ffff880135ea4418
RBP: ffffc9000071fd40 R08: 0000000000000000 R09: 0000000100180009
R10: ffffc9000071fd38 R11: ffffffff81110800 R12: ffff88006752d3d8
R13: ffff88006752d3d8 R14: ffff88013a108040 R15: 000000000000000a
FS: 0000000000000000(0000) GS:ffff88013fd80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa8ec1edb00 CR3: 0000000138ee8000 CR4: 00000000001406e0
Call Trace:
mempool_destroy.part.10+0x21/0x40
mempool_destroy+0xe/0x10
blk_exit_rl+0x12/0x20
blkg_free+0x4d/0xa0
__blkg_release_rcu+0x59/0x170
rcu_process_callbacks+0x260/0x4e0
__do_softirq+0x116/0x250
smpboot_thread_fn+0x123/0x1e0
kthread+0x109/0x140
ret_from_fork+0x31/0x40
Fixes: commit e9c787e65c0c ("scsi: allocate scsi_cmnd structures as part of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Jan Kara <jack@suse.cz>
Cc: <stable@vger.kernel.org> # v4.11+
---
block/blk-cgroup.c | 2 +-
block/blk-core.c | 10 ++++++++--
block/blk-sysfs.c | 2 +-
block/blk.h | 2 +-
4 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 7c2947128f58..0480892e97e5 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -74,7 +74,7 @@ static void blkg_free(struct blkcg_gq *blkg)
blkcg_policy[i]->pd_free_fn(blkg->pd[i]);
if (blkg->blkcg != &blkcg_root)
- blk_exit_rl(&blkg->rl);
+ blk_exit_rl(blkg->q, &blkg->rl);
blkg_rwstat_exit(&blkg->stat_ios);
blkg_rwstat_exit(&blkg->stat_bytes);
diff --git a/block/blk-core.c b/block/blk-core.c
index c7068520794b..a7421b772d0e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -648,13 +648,19 @@ int blk_init_rl(struct request_list *rl, struct request_queue *q,
if (!rl->rq_pool)
return -ENOMEM;
+ if (rl != &q->root_rl)
+ WARN_ON_ONCE(!blk_get_queue(q));
+
return 0;
}
-void blk_exit_rl(struct request_list *rl)
+void blk_exit_rl(struct request_queue *q, struct request_list *rl)
{
- if (rl->rq_pool)
+ if (rl->rq_pool) {
mempool_destroy(rl->rq_pool);
+ if (rl != &q->root_rl)
+ blk_put_queue(q);
+ }
}
struct request_queue *blk_alloc_queue(gfp_t gfp_mask)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 712b018e9f54..283da7fbe034 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -809,7 +809,7 @@ static void blk_release_queue(struct kobject *kobj)
blk_free_queue_stats(q->stats);
- blk_exit_rl(&q->root_rl);
+ blk_exit_rl(q, &q->root_rl);
if (q->queue_tags)
__blk_queue_free_tags(q);
diff --git a/block/blk.h b/block/blk.h
index 2ed70228e44f..83c8e1100525 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -59,7 +59,7 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
int blk_init_rl(struct request_list *rl, struct request_queue *q,
gfp_t gfp_mask);
-void blk_exit_rl(struct request_list *rl);
+void blk_exit_rl(struct request_queue *q, struct request_list *rl);
void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
struct bio *bio);
void blk_queue_bypass_start(struct request_queue *q);
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-05-31 21:43 ` [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free Bart Van Assche
@ 2017-06-01 19:09 ` Jens Axboe
2017-06-13 17:54 ` Ross Zwisler
1 sibling, 0 replies; 21+ messages in thread
From: Jens Axboe @ 2017-06-01 19:09 UTC (permalink / raw)
To: Bart Van Assche; +Cc: linux-block, Christoph Hellwig, Jan Kara, stable
On 05/31/2017 02:43 PM, Bart Van Assche wrote:
> Since the introduction of .init_rq_fn() and .exit_rq_fn() it is
> essential that the memory allocated for struct request_queue
> stays around until all blk_exit_rl() calls have finished. Hence
> make blk_init_rl() take a reference on struct request_queue.
>
> This patch fixes the following crash:
>
> general protection fault: 0000 [#2] SMP
> CPU: 3 PID: 28 Comm: ksoftirqd/3 Tainted: G D 4.12.0-rc2-dbg+ #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> task: ffff88013a108040 task.stack: ffffc9000071c000
> RIP: 0010:free_request_size+0x1a/0x30
> RSP: 0018:ffffc9000071fd38 EFLAGS: 00010202
> RAX: 6b6b6b6b6b6b6b6b RBX: ffff880067362a88 RCX: 0000000000000003
> RDX: ffff880067464178 RSI: ffff880067362a88 RDI: ffff880135ea4418
> RBP: ffffc9000071fd40 R08: 0000000000000000 R09: 0000000100180009
> R10: ffffc9000071fd38 R11: ffffffff81110800 R12: ffff88006752d3d8
> R13: ffff88006752d3d8 R14: ffff88013a108040 R15: 000000000000000a
> FS: 0000000000000000(0000) GS:ffff88013fd80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa8ec1edb00 CR3: 0000000138ee8000 CR4: 00000000001406e0
> Call Trace:
> mempool_destroy.part.10+0x21/0x40
> mempool_destroy+0xe/0x10
> blk_exit_rl+0x12/0x20
> blkg_free+0x4d/0xa0
> __blkg_release_rcu+0x59/0x170
> rcu_process_callbacks+0x260/0x4e0
> __do_softirq+0x116/0x250
> smpboot_thread_fn+0x123/0x1e0
> kthread+0x109/0x140
> ret_from_fork+0x31/0x40
>
> Fixes: commit e9c787e65c0c ("scsi: allocate scsi_cmnd structures as part of struct request")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Jan Kara <jack@suse.cz>
> Cc: <stable@vger.kernel.org> # v4.11+
Added this one for 4.12.
--
Jens Axboe
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-05-31 21:43 ` [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free Bart Van Assche
2017-06-01 19:09 ` Jens Axboe
@ 2017-06-13 17:54 ` Ross Zwisler
2017-06-14 15:19 ` Bart Van Assche
1 sibling, 1 reply; 21+ messages in thread
From: Ross Zwisler @ 2017-06-13 17:54 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Jan Kara, stable
On Wed, May 31, 2017 at 3:43 PM, Bart Van Assche
<bart.vanassche@sandisk.com> wrote:
> Since the introduction of .init_rq_fn() and .exit_rq_fn() it is
> essential that the memory allocated for struct request_queue
> stays around until all blk_exit_rl() calls have finished. Hence
> make blk_init_rl() take a reference on struct request_queue.
>
> This patch fixes the following crash:
>
> general protection fault: 0000 [#2] SMP
> CPU: 3 PID: 28 Comm: ksoftirqd/3 Tainted: G D 4.12.0-rc2-dbg+ #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.0.0-prebuilt.qemu-project.org 04/01/2014
> task: ffff88013a108040 task.stack: ffffc9000071c000
> RIP: 0010:free_request_size+0x1a/0x30
> RSP: 0018:ffffc9000071fd38 EFLAGS: 00010202
> RAX: 6b6b6b6b6b6b6b6b RBX: ffff880067362a88 RCX: 0000000000000003
> RDX: ffff880067464178 RSI: ffff880067362a88 RDI: ffff880135ea4418
> RBP: ffffc9000071fd40 R08: 0000000000000000 R09: 0000000100180009
> R10: ffffc9000071fd38 R11: ffffffff81110800 R12: ffff88006752d3d8
> R13: ffff88006752d3d8 R14: ffff88013a108040 R15: 000000000000000a
> FS: 0000000000000000(0000) GS:ffff88013fd80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa8ec1edb00 CR3: 0000000138ee8000 CR4: 00000000001406e0
> Call Trace:
> mempool_destroy.part.10+0x21/0x40
> mempool_destroy+0xe/0x10
> blk_exit_rl+0x12/0x20
> blkg_free+0x4d/0xa0
> __blkg_release_rcu+0x59/0x170
> rcu_process_callbacks+0x260/0x4e0
> __do_softirq+0x116/0x250
> smpboot_thread_fn+0x123/0x1e0
> kthread+0x109/0x140
> ret_from_fork+0x31/0x40
>
> Fixes: commit e9c787e65c0c ("scsi: allocate scsi_cmnd structures as part of struct request")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Jan Kara <jack@suse.cz>
> Cc: <stable@vger.kernel.org> # v4.11+
> ---
> block/blk-cgroup.c | 2 +-
> block/blk-core.c | 10 ++++++++--
> block/blk-sysfs.c | 2 +-
> block/blk.h | 2 +-
> 4 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 7c2947128f58..0480892e97e5 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -74,7 +74,7 @@ static void blkg_free(struct blkcg_gq *blkg)
> blkcg_policy[i]->pd_free_fn(blkg->pd[i]);
>
> if (blkg->blkcg != &blkcg_root)
> - blk_exit_rl(&blkg->rl);
> + blk_exit_rl(blkg->q, &blkg->rl);
>
> blkg_rwstat_exit(&blkg->stat_ios);
> blkg_rwstat_exit(&blkg->stat_bytes);
> diff --git a/block/blk-core.c b/block/blk-core.c
> index c7068520794b..a7421b772d0e 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -648,13 +648,19 @@ int blk_init_rl(struct request_list *rl, struct request_queue *q,
> if (!rl->rq_pool)
> return -ENOMEM;
>
> + if (rl != &q->root_rl)
> + WARN_ON_ONCE(!blk_get_queue(q));
> +
> return 0;
> }
>
> -void blk_exit_rl(struct request_list *rl)
> +void blk_exit_rl(struct request_queue *q, struct request_list *rl)
> {
> - if (rl->rq_pool)
> + if (rl->rq_pool) {
> mempool_destroy(rl->rq_pool);
> + if (rl != &q->root_rl)
> + blk_put_queue(q);
> + }
> }
This commit is causing the following kernel BUG for me when I shut
down my systems:
BUG: sleeping function called from invalid context at kernel/workqueue.c:2790
in_atomic(): 1, irqs_disabled(): 0, pid: 41, name: rcuop/3
1 lock held by rcuop/3/41:
#0: (rcu_callback){......}, at: [<ffffffff8111f9a2>]
rcu_nocb_kthread+0x282/0x500
Preemption disabled at:
[<ffffffff8111f9b3>] rcu_nocb_kthread+0x293/0x500
CPU: 2 PID: 41 Comm: rcuop/3 Not tainted 4.12.0-rc5 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.9.3-1.fc25 04/01/2014
Call Trace:
dump_stack+0x86/0xcf
___might_sleep+0x174/0x260
__might_sleep+0x4a/0x80
flush_work+0x7e/0x2e0
? blk_throtl_update_limit_valid.isra.14+0x192/0x240
? blkg_destroy_all+0x63/0xc0
? __cancel_work_timer+0x123/0x1c0
__cancel_work_timer+0x143/0x1c0
? blkcg_exit_queue+0x2d/0x40
? _raw_spin_unlock_irq+0x2c/0x60
cancel_work_sync+0x10/0x20
blk_throtl_exit+0x25/0x60
blkcg_exit_queue+0x35/0x40
blk_release_queue+0x42/0x130
kobject_put+0xa9/0x190
kauditd_printk_skb: 60 callbacks suppressed
audit: type=1131 audit(1497375715.762:263): pid=1 uid=0
auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0
msg='unit=auditd comm="systemd" exe="/usr/lib/systemd/systemd"
hostname=? addr=? terminal=? res=success'
audit: type=1131 audit(1497375715.765:264): pid=1 uid=0
auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0
msg='unit=systemd-tmpfiles-setup comm="systemd"
exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=?
res=success'
audit: type=1131 audit(1497375715.767:265): pid=1 uid=0
auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0
msg='unit=fedora-import-state comm="systemd"
exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=?
res=success'
blk_exit_rl+0x35/0x40
blkg_free+0x56/0xb0
__blkg_release_rcu+0x5e/0x190
? blkcg_css_offline+0xa0/0xa0
rcu_nocb_kthread+0x33d/0x500
kthread+0x117/0x150
? rcu_all_qs+0xe0/0xe0
? kthread_create_on_node+0x70/0x70
ret_from_fork+0x2a/0x40
BUG: scheduling while atomic: rcuop/3/41/0x00000201
1 lock held by rcuop/3/41:
#0: (rcu_callback){......}, at: [<ffffffff8111f9a2>]
rcu_nocb_kthread+0x282/0x500
Modules linked in: nd_pmem nd_btt dax_pmem device_dax nfit libnvdimm
Preemption disabled at:
[<ffffffff8111f9b3>] rcu_nocb_kthread+0x293/0x500
CPU: 2 PID: 41 Comm: rcuop/3 Tainted: G W 4.12.0-rc5 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.9.3-1.fc25 04/01/2014
Call Trace:
dump_stack+0x86/0xcf
? rcu_nocb_kthread+0x293/0x500
__schedule_bug+0x88/0xe0
__schedule+0x77a/0xb10
? wait_for_completion+0x10b/0x1a0
schedule+0x40/0x90
schedule_timeout+0x2ac/0x510
? wait_for_completion+0x122/0x1a0
? wait_for_completion+0x122/0x1a0
? _raw_spin_unlock_irq+0x2c/0x60
? wait_for_completion+0x10b/0x1a0
? wait_for_completion+0x10b/0x1a0
wait_for_completion+0x12a/0x1a0
? wait_for_completion+0x12a/0x1a0
? wake_up_q+0x80/0x80
__wait_rcu_gp+0xca/0x100
synchronize_rcu.part.67+0x41/0x60
? rcu_barrier+0x20/0x20
? trace_raw_output_rcu_utilization+0x50/0x50
? wait_for_completion+0x47/0x1a0
synchronize_rcu+0x2c/0xa0
blk_queue_bypass_start+0x7f/0xa0
blkcg_deactivate_policy+0x110/0x120
blk_throtl_exit+0x34/0x60
blkcg_exit_queue+0x35/0x40
blk_release_queue+0x42/0x130
kobject_put+0xa9/0x190
blk_exit_rl+0x35/0x40
blkg_free+0x56/0xb0
__blkg_release_rcu+0x5e/0x190
? blkcg_css_offline+0xa0/0xa0
rcu_nocb_kthread+0x33d/0x500
kthread+0x117/0x150
? rcu_all_qs+0xe0/0xe0
? kthread_create_on_node+0x70/0x70
ret_from_fork+0x2a/0x40
NOHZ: local_softirq_pending 202
DEBUG_LOCKS_WARN_ON(val > preempt_count())
------------[ cut here ]------------
WARNING: CPU: 2 PID: 41 at kernel/sched/core.c:3202
preempt_count_sub+0x5f/0xa0
Modules linked in: nd_pmem nd_btt dax_pmem device_dax nfit libnvdimm
CPU: 2 PID: 41 Comm: rcuop/3 Tainted: G W 4.12.0-rc5 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.9.3-1.fc25 04/01/2014
task: ffff880210790000 task.stack: ffffc90000dbc000
RIP: 0010:preempt_count_sub+0x5f/0xa0
RSP: 0000:ffffc90000dbfe58 EFLAGS: 00010082
RAX: 000000000000002a RBX: 0000000000000200 RCX: 0000000000000001
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff8110dad7
RBP: ffffc90000dbfe58 R08: 0000000000000000 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff8111fa08
R13: 0000000000000026 R14: ffff880210790000 R15: ffff88020880c4c0
FS: 0000000000000000(0000) GS:ffff880211400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055c7110f4148 CR3: 000000020c1bc000 CR4: 00000000000006e0
Call Trace:
__local_bh_enable_ip+0x52/0xb0
rcu_nocb_kthread+0x2fe/0x500
kthread+0x117/0x150
? rcu_all_qs+0xe0/0xe0
? kthread_create_on_node+0x70/0x70
ret_from_fork+0x2a/0x40
Code: 37 f4 7e 5d c3 e8 52 d2 56 00 85 c0 74 f5 8b 15 b8 2b 51 02 85
d2 75 eb 48 c7 c6 9a f9 ef 81 48 c7 c7 c2 87 ee 81 e8 e8 34 12 00 <0f>
ff 5d c3 84 d2 75 c7 e8 24 d2 56 00 85 c0 74 c7 8b 05 8a 2b
---[ end trace f7877221a24ce5d5 ]---
I think essentially the problem is that blk_exit_rl() is called from
atomic context, and the work done by the newly added blk_put_queue()
call can sleep.
This is reproducible 100% of the time by running a single xfstest
(generic/085 is what I've been using) against a pair simulated of PMEM
block devices (without DAX), and by shutting down or rebooting the
system.
Sometimes it is relatively harmless, and you just get a splat during shutdown.
Sometimes the shutdown stops making forward progress and the machine
fails to reboot.
I am able to reproduce this consistently with v4.12-rc5, and reverting
this one commit removes the behavior.
- Ross
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-06-13 17:54 ` Ross Zwisler
@ 2017-06-14 15:19 ` Bart Van Assche
2017-06-14 18:04 ` Ross Zwisler
2017-06-14 19:28 ` Jens Axboe
0 siblings, 2 replies; 21+ messages in thread
From: Bart Van Assche @ 2017-06-14 15:19 UTC (permalink / raw)
To: Ross Zwisler; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Jan Kara, stable
On 06/13/17 10:54, Ross Zwisler wrote:
> This commit is causing the following kernel BUG for me when I shut
> down my systems:
>
> BUG: sleeping function called from invalid context at kernel/workqueue.c:2790
> in_atomic(): 1, irqs_disabled(): 0, pid: 41, name: rcuop/3
Thanks Ross for the testing and for the report. Can you check whether
the patch below is sufficient to fix this?
Subject: [PATCH] block: Fix a blk_exit_rl() regression
Avoid that the following complaint is reported:
BUG: sleeping function called from invalid context at kernel/workqueue.c:2790
in_atomic(): 1, irqs_disabled(): 0, pid: 41, name: rcuop/3
1 lock held by rcuop/3/41:
#0: (rcu_callback){......}, at: [<ffffffff8111f9a2>] rcu_nocb_kthread+0x282/0x500
Call Trace:
dump_stack+0x86/0xcf
___might_sleep+0x174/0x260
__might_sleep+0x4a/0x80
flush_work+0x7e/0x2e0
__cancel_work_timer+0x143/0x1c0
cancel_work_sync+0x10/0x20
blk_throtl_exit+0x25/0x60
blkcg_exit_queue+0x35/0x40
blk_release_queue+0x42/0x130
kobject_put+0xa9/0x190
Reported-by: Ross Zwisler <zwisler@gmail.com>
Fixes: commit b425e5049258 ("block: Avoid that blk_exit_rl() triggers a use-after-free")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Ross Zwisler <zwisler@gmail.com>
---
block/blk-sysfs.c | 34 ++++++++++++++++++++++------------
include/linux/blkdev.h | 2 ++
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 283da7fbe034..27aceab1cc31 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -777,24 +777,25 @@ static void blk_free_queue_rcu(struct rcu_head *rcu_head)
}
/**
- * blk_release_queue: - release a &struct request_queue when it is no longer needed
- * @kobj: the kobj belonging to the request queue to be released
+ * __blk_release_queue - release a request queue when it is no longer needed
+ * @work: pointer to the release_work member of the request queue to be released
*
* Description:
- * blk_release_queue is the pair to blk_init_queue() or
- * blk_queue_make_request(). It should be called when a request queue is
- * being released; typically when a block device is being de-registered.
- * Currently, its primary task it to free all the &struct request
- * structures that were allocated to the queue and the queue itself.
+ * blk_release_queue is the counterpart of blk_init_queue(). It should be
+ * called when a request queue is being released; typically when a block
+ * device is being de-registered. Its primary task it to free the queue
+ * itself.
*
- * Note:
+ * Notes:
* The low level driver must have finished any outstanding requests first
* via blk_cleanup_queue().
- **/
-static void blk_release_queue(struct kobject *kobj)
+ *
+ * Although blk_release_queue() may be called with preemption disabled,
+ * __blk_release_queue() may sleep.
+ */
+static void __blk_release_queue(struct work_struct *work)
{
- struct request_queue *q =
- container_of(kobj, struct request_queue, kobj);
+ struct request_queue *q = container_of(work, typeof(*q), release_work);
if (test_bit(QUEUE_FLAG_POLL_STATS, &q->queue_flags))
blk_stat_remove_callback(q, q->poll_cb);
@@ -834,6 +835,15 @@ static void blk_release_queue(struct kobject *kobj)
call_rcu(&q->rcu_head, blk_free_queue_rcu);
}
+static void blk_release_queue(struct kobject *kobj)
+{
+ struct request_queue *q =
+ container_of(kobj, struct request_queue, kobj);
+
+ INIT_WORK(&q->release_work, __blk_release_queue);
+ schedule_work(&q->release_work);
+}
+
static const struct sysfs_ops queue_sysfs_ops = {
.show = queue_attr_show,
.store = queue_attr_store,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index cc75407881e4..07a26414fdfc 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -590,6 +590,8 @@ struct request_queue {
size_t cmd_size;
void *rq_alloc_data;
+
+ struct work_struct release_work;
};
#define QUEUE_FLAG_QUEUED 1 /* uses generic tag queueing */
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-06-14 15:19 ` Bart Van Assche
@ 2017-06-14 18:04 ` Ross Zwisler
2017-06-14 19:28 ` Jens Axboe
1 sibling, 0 replies; 21+ messages in thread
From: Ross Zwisler @ 2017-06-14 18:04 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Jan Kara, stable
On Wed, Jun 14, 2017 at 9:19 AM, Bart Van Assche
<bart.vanassche@sandisk.com> wrote:
> On 06/13/17 10:54, Ross Zwisler wrote:
>> This commit is causing the following kernel BUG for me when I shut
>> down my systems:
>>
>> BUG: sleeping function called from invalid context at kernel/workqueue.c:2790
>> in_atomic(): 1, irqs_disabled(): 0, pid: 41, name: rcuop/3
>
> Thanks Ross for the testing and for the report. Can you check whether
> the patch below is sufficient to fix this?
>
>
> Subject: [PATCH] block: Fix a blk_exit_rl() regression
>
> Avoid that the following complaint is reported:
>
> BUG: sleeping function called from invalid context at kernel/workqueue.c:2790
> in_atomic(): 1, irqs_disabled(): 0, pid: 41, name: rcuop/3
> 1 lock held by rcuop/3/41:
> #0: (rcu_callback){......}, at: [<ffffffff8111f9a2>] rcu_nocb_kthread+0x282/0x500
> Call Trace:
> dump_stack+0x86/0xcf
> ___might_sleep+0x174/0x260
> __might_sleep+0x4a/0x80
> flush_work+0x7e/0x2e0
> __cancel_work_timer+0x143/0x1c0
> cancel_work_sync+0x10/0x20
> blk_throtl_exit+0x25/0x60
> blkcg_exit_queue+0x35/0x40
> blk_release_queue+0x42/0x130
> kobject_put+0xa9/0x190
>
> Reported-by: Ross Zwisler <zwisler@gmail.com>
> Fixes: commit b425e5049258 ("block: Avoid that blk_exit_rl() triggers a use-after-free")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Ross Zwisler <zwisler@gmail.com>
Yep, this solves the issue for me, thanks!
Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-06-14 15:19 ` Bart Van Assche
2017-06-14 18:04 ` Ross Zwisler
@ 2017-06-14 19:28 ` Jens Axboe
2017-06-14 19:32 ` Bart Van Assche
1 sibling, 1 reply; 21+ messages in thread
From: Jens Axboe @ 2017-06-14 19:28 UTC (permalink / raw)
To: Bart Van Assche, Ross Zwisler
Cc: linux-block, Christoph Hellwig, Jan Kara, stable
On 06/14/2017 09:19 AM, Bart Van Assche wrote:
> Subject: [PATCH] block: Fix a blk_exit_rl() regression
>
> Avoid that the following complaint is reported:
>
> BUG: sleeping function called from invalid context at kernel/workqueue.c:2790
> in_atomic(): 1, irqs_disabled(): 0, pid: 41, name: rcuop/3
> 1 lock held by rcuop/3/41:
> #0: (rcu_callback){......}, at: [<ffffffff8111f9a2>] rcu_nocb_kthread+0x282/0x500
> Call Trace:
> dump_stack+0x86/0xcf
> ___might_sleep+0x174/0x260
> __might_sleep+0x4a/0x80
> flush_work+0x7e/0x2e0
> __cancel_work_timer+0x143/0x1c0
> cancel_work_sync+0x10/0x20
> blk_throtl_exit+0x25/0x60
> blkcg_exit_queue+0x35/0x40
> blk_release_queue+0x42/0x130
> kobject_put+0xa9/0x190
I added this, but the above is really a horrible changelog. It doesn't
say how the problem is fixed. I added some verbiage to that effect.
--
Jens Axboe
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free
2017-06-14 19:28 ` Jens Axboe
@ 2017-06-14 19:32 ` Bart Van Assche
0 siblings, 0 replies; 21+ messages in thread
From: Bart Van Assche @ 2017-06-14 19:32 UTC (permalink / raw)
To: Jens Axboe, Ross Zwisler; +Cc: linux-block, Christoph Hellwig, Jan Kara, stable
On 06/14/17 12:28, Jens Axboe wrote:
> I added this, but the above is really a horrible changelog. It doesn't
> say how the problem is fixed. I added some verbiage to that effect.
Hello Jens,
Thanks for having fixed up the changelog and for already having picked
up this patch. I was going to repost the patch and write a proper
changelog but apparently you were faster than I :-)
BTW, since last night I can finally receive and send e-mails with a
@wdc.com suffix (thirteen months after the acquisition closed).
Bart.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 2/6] block: Introduce queue flag QUEUE_FLAG_SCSI_PASSTHROUGH
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
2017-05-31 21:43 ` [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free Bart Van Assche
@ 2017-05-31 21:43 ` Bart Van Assche
2017-05-31 21:43 ` [PATCH v2 3/6] bsg: Check queue type before attaching to a queue Bart Van Assche
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Bart Van Assche @ 2017-05-31 21:43 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Bart Van Assche, Omar Sandoval,
Don Brace
>From the context where a SCSI command is submitted it is not always
possible to figure out whether or not the queue the command is
submitted to has struct scsi_request as the first member of its
private data. Hence introduce the flag QUEUE_FLAG_SCSI_PASSTHROUGH.
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Don Brace <don.brace@microsemi.com>
---
block/bsg-lib.c | 1 +
drivers/block/cciss.c | 1 +
drivers/ide/ide-probe.c | 1 +
drivers/scsi/scsi_lib.c | 2 ++
drivers/scsi/scsi_transport_sas.c | 1 +
include/linux/blkdev.h | 3 +++
6 files changed, 9 insertions(+)
diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index 0a23dbba2d30..9b91daefcd9b 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -246,6 +246,7 @@ struct request_queue *bsg_setup_queue(struct device *dev, char *name,
q->bsg_job_size = dd_job_size;
q->bsg_job_fn = job_fn;
queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
+ queue_flag_set_unlocked(QUEUE_FLAG_SCSI_PASSTHROUGH, q);
blk_queue_softirq_done(q, bsg_softirq_done);
blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT);
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index cd375503f7b0..3761066fe89d 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1956,6 +1956,7 @@ static int cciss_add_disk(ctlr_info_t *h, struct gendisk *disk,
disk->queue->cmd_size = sizeof(struct scsi_request);
disk->queue->request_fn = do_cciss_request;
disk->queue->queue_lock = &h->lock;
+ queue_flag_set_unlocked(QUEUE_FLAG_SCSI_PASSTHROUGH, disk->queue);
if (blk_init_allocated_queue(disk->queue) < 0)
goto cleanup_queue;
diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 023562565d11..b3f85250dea9 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -773,6 +773,7 @@ static int ide_init_queue(ide_drive_t *drive)
q->request_fn = do_ide_request;
q->init_rq_fn = ide_init_rq;
q->cmd_size = sizeof(struct ide_request);
+ queue_flag_set_unlocked(QUEUE_FLAG_SCSI_PASSTHROUGH, q);
if (blk_init_allocated_queue(q) < 0) {
blk_cleanup_queue(q);
return 1;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 99e16ac479e3..884aaa84c2dd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2057,6 +2057,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
{
struct device *dev = shost->dma_dev;
+ queue_flag_set_unlocked(QUEUE_FLAG_SCSI_PASSTHROUGH, q);
+
/*
* this limit is imposed by hardware restrictions
*/
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 0ebe2f1bb908..d16414bfe2ef 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -264,6 +264,7 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
q->queuedata = shost;
queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
+ queue_flag_set_unlocked(QUEUE_FLAG_SCSI_PASSTHROUGH, q);
return 0;
out_cleanup_queue:
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ab92c4ea138b..019f18c65098 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -618,6 +618,7 @@ struct request_queue {
#define QUEUE_FLAG_STATS 27 /* track rq completion times */
#define QUEUE_FLAG_POLL_STATS 28 /* collecting stats for hybrid polling */
#define QUEUE_FLAG_REGISTERED 29 /* queue has been registered to a disk */
+#define QUEUE_FLAG_SCSI_PASSTHROUGH 30 /* queue supports SCSI commands */
#define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \
(1 << QUEUE_FLAG_STACKABLE) | \
@@ -708,6 +709,8 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
#define blk_queue_secure_erase(q) \
(test_bit(QUEUE_FLAG_SECERASE, &(q)->queue_flags))
#define blk_queue_dax(q) test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
+#define blk_queue_scsi_passthrough(q) \
+ test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
#define blk_noretry_request(rq) \
((rq)->cmd_flags & (REQ_FAILFAST_DEV|REQ_FAILFAST_TRANSPORT| \
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/6] bsg: Check queue type before attaching to a queue
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
2017-05-31 21:43 ` [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free Bart Van Assche
2017-05-31 21:43 ` [PATCH v2 2/6] block: Introduce queue flag QUEUE_FLAG_SCSI_PASSTHROUGH Bart Van Assche
@ 2017-05-31 21:43 ` Bart Van Assche
2017-05-31 21:43 ` [PATCH v2 4/6] pktcdvd: " Bart Van Assche
` (4 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Bart Van Assche @ 2017-05-31 21:43 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Bart Van Assche, Omar Sandoval
Since BSG only supports request queues for which struct scsi_request
is the first member of their private request data, refuse to register
block layer queues for which struct scsi_request is not the first
member of their private data.
References: commit bd1599d931ca ("scsi_transport_sas: fix BSG ioctl memory corruption")
References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
---
block/bsg.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/block/bsg.c b/block/bsg.c
index 6fd08544d77e..40db8ff4c618 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -750,6 +750,12 @@ static struct bsg_device *bsg_add_device(struct inode *inode,
#ifdef BSG_DEBUG
unsigned char buf[32];
#endif
+
+ if (!blk_queue_scsi_passthrough(rq)) {
+ WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
+ return ERR_PTR(-EINVAL);
+ }
+
if (!blk_get_queue(rq))
return ERR_PTR(-ENXIO);
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] pktcdvd: Check queue type before attaching to a queue
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
` (2 preceding siblings ...)
2017-05-31 21:43 ` [PATCH v2 3/6] bsg: Check queue type before attaching to a queue Bart Van Assche
@ 2017-05-31 21:43 ` Bart Van Assche
2017-12-30 21:41 ` [v2,4/6] " Maciej S. Szmigiero
2017-05-31 21:43 ` [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio Bart Van Assche
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Bart Van Assche @ 2017-05-31 21:43 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Bart Van Assche, Omar Sandoval
Since the pktcdvd driver only supports request queues for which
struct scsi_request is the first member of their private request
data, refuse to register block layer queues for which struct
scsi_request is not the first member of the private data.
References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
---
drivers/block/pktcdvd.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 205b865ebeb9..42e3c880a8a5 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -2583,6 +2583,11 @@ static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev)
bdev = bdget(dev);
if (!bdev)
return -ENOMEM;
+ if (!blk_queue_scsi_passthrough(bdev_get_queue(bdev))) {
+ WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
+ bdput(bdev);
+ return -EINVAL;
+ }
ret = blkdev_get(bdev, FMODE_READ | FMODE_NDELAY, NULL);
if (ret)
return ret;
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [v2,4/6] pktcdvd: Check queue type before attaching to a queue
2017-05-31 21:43 ` [PATCH v2 4/6] pktcdvd: " Bart Van Assche
@ 2017-12-30 21:41 ` Maciej S. Szmigiero
2017-12-31 0:53 ` Bart Van Assche
0 siblings, 1 reply; 21+ messages in thread
From: Maciej S. Szmigiero @ 2017-12-30 21:41 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Omar Sandoval
On 31.05.2017 23:43, Bart Van Assche wrote:
> Since the pktcdvd driver only supports request queues for which
> struct scsi_request is the first member of their private request
> data, refuse to register block layer queues for which struct
> scsi_request is not the first member of the private data.
>
> References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Omar Sandoval <osandov@fb.com>
> ---
> drivers/block/pktcdvd.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index 205b865ebeb9..42e3c880a8a5 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -2583,6 +2583,11 @@ static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev)
> bdev = bdget(dev);
> if (!bdev)
> return -ENOMEM;
> + if (!blk_queue_scsi_passthrough(bdev_get_queue(bdev))) {
> + WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
> + bdput(bdev);
> + return -EINVAL;
> + }
> ret = blkdev_get(bdev, FMODE_READ | FMODE_NDELAY, NULL);
> if (ret)
> return ret;
>
This commit causes a NULL pointer dereference when adding a pktcdvd
mapping.
Reproducing it is simple:
# pktsetup 1 /dev/cdrom
Specifically, the NULL dereference happens inside bdev_get_queue(bdev),
which is supposed to return bdev->bd_disk->queue, but in this case
bdev->bd_disk is NULL.
If I revert this commit the mapping is added correctly (tested on 4.14.10,
but there haven't been any changes to pktcdvd.c and bdev_get_queue() in
4.15-rc5).
Maciej
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [v2,4/6] pktcdvd: Check queue type before attaching to a queue
2017-12-30 21:41 ` [v2,4/6] " Maciej S. Szmigiero
@ 2017-12-31 0:53 ` Bart Van Assche
2017-12-31 1:23 ` Maciej S. Szmigiero
0 siblings, 1 reply; 21+ messages in thread
From: Bart Van Assche @ 2017-12-31 0:53 UTC (permalink / raw)
To: Bart Van Assche, mail; +Cc: hch, linux-block, osandov, axboe
[-- Attachment #1: Type: text/plain, Size: 458 bytes --]
On Sat, 2017-12-30 at 22:41 +0100, Maciej S. Szmigiero wrote:
> This commit causes a NULL pointer dereference when adding a pktcdvd
> mapping.
>
> Reproducing it is simple:
> # pktsetup 1 /dev/cdrom
>
> Specifically, the NULL dereference happens inside bdev_get_queue(bdev),
> which is supposed to return bdev->bd_disk->queue, but in this case
> bdev->bd_disk is NULL.
Would it be possible to test the two attached patches?
Thanks,
Bart.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-pktcdvd-Fix-a-recently-introduced-NULL-pointer-deref.patch --]
[-- Type: text/x-patch; name="0001-pktcdvd-Fix-a-recently-introduced-NULL-pointer-deref.patch", Size: 1348 bytes --]
From 8ef0308718a3f3f60c0c6983d3ff606ac8d3db8d Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@wdc.com>
Date: Sat, 30 Dec 2017 15:28:25 -0800
Subject: [PATCH 1/2] pktcdvd: Fix a recently introduced NULL pointer
dereference
Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Fixes: commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: <stable@vger.kernel.org> # v4.13
---
drivers/block/pktcdvd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 67974796c350..fc8a80ec90e5 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -2579,14 +2579,14 @@ static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev)
bdev = bdget(dev);
if (!bdev)
return -ENOMEM;
+ ret = blkdev_get(bdev, FMODE_READ | FMODE_NDELAY, NULL);
+ if (ret)
+ return ret;
if (!blk_queue_scsi_passthrough(bdev_get_queue(bdev))) {
WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
- bdput(bdev);
+ blkdev_put(bdev, FMODE_READ | FMODE_NDELAY);
return -EINVAL;
}
- ret = blkdev_get(bdev, FMODE_READ | FMODE_NDELAY, NULL);
- if (ret)
- return ret;
/* This is safe, since we have a reference from open(). */
__module_get(THIS_MODULE);
--
2.15.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-pktcdvd-Fix-pkt_setup_dev-error-path.patch --]
[-- Type: text/x-patch; name="0002-pktcdvd-Fix-pkt_setup_dev-error-path.patch", Size: 848 bytes --]
From 3192cc5f62b3ba9f866bcb245d21231a39745d8d Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@wdc.com>
Date: Sat, 30 Dec 2017 16:44:35 -0800
Subject: [PATCH 2/2] pktcdvd: Fix pkt_setup_dev() error path
Since disk_release(disk) calls blk_put_queue() if disk->queue != NULL,
clear disk->queue before calling put_disk().
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: <stable@vger.kernel.org>
---
drivers/block/pktcdvd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index fc8a80ec90e5..c5e930d23a63 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -2765,6 +2765,7 @@ static int pkt_setup_dev(dev_t dev, dev_t* pkt_dev)
out_new_dev:
blk_cleanup_queue(disk->queue);
+ disk->queue = NULL;
out_mem2:
put_disk(disk);
out_mem:
--
2.15.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [v2,4/6] pktcdvd: Check queue type before attaching to a queue
2017-12-31 0:53 ` Bart Van Assche
@ 2017-12-31 1:23 ` Maciej S. Szmigiero
0 siblings, 0 replies; 21+ messages in thread
From: Maciej S. Szmigiero @ 2017-12-31 1:23 UTC (permalink / raw)
To: Bart Van Assche; +Cc: hch, linux-block, osandov, axboe
On 31.12.2017 01:53, Bart Van Assche wrote:
> On Sat, 2017-12-30 at 22:41 +0100, Maciej S. Szmigiero wrote:
>> This commit causes a NULL pointer dereference when adding a pktcdvd
>> mapping.
>>
>> Reproducing it is simple:
>> # pktsetup 1 /dev/cdrom
>>
>> Specifically, the NULL dereference happens inside bdev_get_queue(bdev),
>> which is supposed to return bdev->bd_disk->queue, but in this case
>> bdev->bd_disk is NULL.
>
> Would it be possible to test the two attached patches?
I've tested 4.14.10 with both applied and can confirm that the NULL
pointer dereference when adding a pktcdvd mapping no longer happens
then.
> Thanks,
>
> Bart.
>
Thanks,
Maciej
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
` (3 preceding siblings ...)
2017-05-31 21:43 ` [PATCH v2 4/6] pktcdvd: " Bart Van Assche
@ 2017-05-31 21:43 ` Bart Van Assche
2017-06-01 5:50 ` Hannes Reinecke
2017-06-01 6:05 ` Christoph Hellwig
2017-05-31 21:43 ` [PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request Bart Van Assche
` (2 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Bart Van Assche @ 2017-05-31 21:43 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Bart Van Assche, Hannes Reinecke,
Omar Sandoval
The CDROMREADAUDIO ioctl uses SCSI passthrough when the .disk
pointer has been set in struct cdrom_device_info. Hence check
whether SCSI passthrough is supported before submitting a SCSI
command. Note: both the ide-cd and sr drivers set the disk
pointer in struct cdrom_device_info but neither the pcd nor
the gdrom driver sets that pointer.
References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: linux-block@vger.kernel.org
---
drivers/cdrom/cdrom.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 76c952fd9ab9..ff19cfc587f0 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2178,6 +2178,12 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
if (!q)
return -ENXIO;
+ if (!blk_queue_scsi_passthrough(q)) {
+ WARN_ONCE(true,
+ "Attempt read CDDA info through a non-SCSI queue\n");
+ return -EINVAL;
+ }
+
cdi->last_sense = 0;
while (nframes) {
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio
2017-05-31 21:43 ` [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio Bart Van Assche
@ 2017-06-01 5:50 ` Hannes Reinecke
2017-06-01 6:05 ` Christoph Hellwig
1 sibling, 0 replies; 21+ messages in thread
From: Hannes Reinecke @ 2017-06-01 5:50 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Hannes Reinecke, Omar Sandoval
On 05/31/2017 11:43 PM, Bart Van Assche wrote:
> The CDROMREADAUDIO ioctl uses SCSI passthrough when the .disk
> pointer has been set in struct cdrom_device_info. Hence check
> whether SCSI passthrough is supported before submitting a SCSI
> command. Note: both the ide-cd and sr drivers set the disk
> pointer in struct cdrom_device_info but neither the pcd nor
> the gdrom driver sets that pointer.
>
> References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: linux-block@vger.kernel.org
> ---
> drivers/cdrom/cdrom.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 76c952fd9ab9..ff19cfc587f0 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2178,6 +2178,12 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
> if (!q)
> return -ENXIO;
>
> + if (!blk_queue_scsi_passthrough(q)) {
> + WARN_ONCE(true,
> + "Attempt read CDDA info through a non-SCSI queue\n");
> + return -EINVAL;
> + }
> +
> cdi->last_sense = 0;
>
> while (nframes) {
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio
2017-05-31 21:43 ` [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio Bart Van Assche
2017-06-01 5:50 ` Hannes Reinecke
@ 2017-06-01 6:05 ` Christoph Hellwig
1 sibling, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2017-06-01 6:05 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Hannes Reinecke,
Omar Sandoval
On Wed, May 31, 2017 at 02:43:49PM -0700, Bart Van Assche wrote:
> The CDROMREADAUDIO ioctl uses SCSI passthrough when the .disk
> pointer has been set in struct cdrom_device_info. Hence check
> whether SCSI passthrough is supported before submitting a SCSI
> command. Note: both the ide-cd and sr drivers set the disk
> pointer in struct cdrom_device_info but neither the pcd nor
> the gdrom driver sets that pointer.
And I think that's exactly the point. There probably can be some
further cleanup in this area, but for now this looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
` (4 preceding siblings ...)
2017-05-31 21:43 ` [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio Bart Van Assche
@ 2017-05-31 21:43 ` Bart Van Assche
2017-06-01 13:29 ` J . Bruce Fields
2017-06-01 6:05 ` [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Christoph Hellwig
2017-06-01 19:11 ` Jens Axboe
7 siblings, 1 reply; 21+ messages in thread
From: Bart Van Assche @ 2017-05-31 21:43 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Bart Van Assche,
J . Bruce Fields, Jeff Layton, Omar Sandoval, linux-nfs
Since using scsi_req() is only allowed against request queues for
which struct scsi_request is the first member of their private
request data, refuse to submit SCSI commands against a queue for
which this is not the case.
References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: Omar Sandoval <osandov@fb.com>
Cc: linux-nfs@vger.kernel.org
---
fs/nfsd/blocklayout.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index fb5213afc854..47ed19c53f2e 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -219,6 +219,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,
u8 *buf, *d, type, assoc;
int error;
+ if (WARN_ON_ONCE(!blk_queue_scsi_passthrough(q)))
+ return -EINVAL;
+
buf = kzalloc(bufflen, GFP_KERNEL);
if (!buf)
return -ENOMEM;
--
2.12.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request
2017-05-31 21:43 ` [PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request Bart Van Assche
@ 2017-06-01 13:29 ` J . Bruce Fields
0 siblings, 0 replies; 21+ messages in thread
From: J . Bruce Fields @ 2017-06-01 13:29 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Jeff Layton,
Omar Sandoval, linux-nfs
Feel free to add
Acked-by: J. Bruce Fields <bfields@redhat.com>
if you need it.--b.
On Wed, May 31, 2017 at 02:43:50PM -0700, Bart Van Assche wrote:
> Since using scsi_req() is only allowed against request queues for
> which struct scsi_request is the first member of their private
> request data, refuse to submit SCSI commands against a queue for
> which this is not the case.
>
> References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> Cc: Jeff Layton <jlayton@poochiereds.net>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: linux-nfs@vger.kernel.org
> ---
> fs/nfsd/blocklayout.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index fb5213afc854..47ed19c53f2e 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -219,6 +219,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,
> u8 *buf, *d, type, assoc;
> int error;
>
> + if (WARN_ON_ONCE(!blk_queue_scsi_passthrough(q)))
> + return -EINVAL;
> +
> buf = kzalloc(bufflen, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
> --
> 2.12.2
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
` (5 preceding siblings ...)
2017-05-31 21:43 ` [PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request Bart Van Assche
@ 2017-06-01 6:05 ` Christoph Hellwig
2017-06-01 19:11 ` Jens Axboe
7 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2017-06-01 6:05 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig
On Wed, May 31, 2017 at 02:43:44PM -0700, Bart Van Assche wrote:
> Hello Jens,
>
> The patches in this series are a sequel of Christoph's "Split scsi passthrough
> fields out of struct request" patch series. The changes compared to v1 of this
> patch series are:
> - Renamed QUEUE_FLAG_SCSI_PDU into QUEUE_FLAG_SCSI_PASSTHROUGH and
> blk_queue_scsi_pdu() into blk_queue_scsi_passthrough().
> - In the cdrom driver, moved the SCSI passthrough support test from
> register_cdrom() to cdrom_read_cdda_bpc().
>
> Please consider these patches for kernel v4.13.
I think these should probably go into 4.12. At least the first one
should for sure.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel
2017-05-31 21:43 [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Bart Van Assche
` (6 preceding siblings ...)
2017-06-01 6:05 ` [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel Christoph Hellwig
@ 2017-06-01 19:11 ` Jens Axboe
7 siblings, 0 replies; 21+ messages in thread
From: Jens Axboe @ 2017-06-01 19:11 UTC (permalink / raw)
To: Bart Van Assche; +Cc: linux-block, Christoph Hellwig
On 05/31/2017 02:43 PM, Bart Van Assche wrote:
> Hello Jens,
>
> The patches in this series are a sequel of Christoph's "Split scsi passthrough
> fields out of struct request" patch series. The changes compared to v1 of this
> patch series are:
> - Renamed QUEUE_FLAG_SCSI_PDU into QUEUE_FLAG_SCSI_PASSTHROUGH and
> blk_queue_scsi_pdu() into blk_queue_scsi_passthrough().
> - In the cdrom driver, moved the SCSI passthrough support test from
> register_cdrom() to cdrom_read_cdda_bpc().
>
> Please consider these patches for kernel v4.13.
Applied for 4.13 (2-6).
--
Jens Axboe
^ permalink raw reply [flat|nested] 21+ messages in thread