[PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel

[PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel

[Bart Van Assche]

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.

Thanks,

Bart.

Bart Van Assche (6):
  block: Avoid that blk_exit_rl() triggers a use-after-free
  block: Introduce queue flag QUEUE_FLAG_SCSI_PASSTHROUGH
  bsg: Check queue type before attaching to a queue
  pktcdvd: Check queue type before attaching to a queue
  cdrom: Check SCSI passthrough support before reading audio
  nfsd: Check queue type before submitting a SCSI request

 block/blk-cgroup.c                |  2 +-
 block/blk-core.c                  | 10 ++++++++--
 block/blk-sysfs.c                 |  2 +-
 block/blk.h                       |  2 +-
 block/bsg-lib.c                   |  1 +
 block/bsg.c                       |  6 ++++++
 drivers/block/cciss.c             |  1 +
 drivers/block/pktcdvd.c           |  5 +++++
 drivers/cdrom/cdrom.c             |  6 ++++++
 drivers/ide/ide-probe.c           |  1 +
 drivers/scsi/scsi_lib.c           |  2 ++
 drivers/scsi/scsi_transport_sas.c |  1 +
 fs/nfsd/blocklayout.c             |  3 +++
 include/linux/blkdev.h            |  3 +++
 14 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.12.2

[PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Bart Van Assche]

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

[PATCH v2 2/6] block: Introduce queue flag QUEUE_FLAG_SCSI_PASSTHROUGH

[Bart Van Assche]

>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

[PATCH v2 3/6] bsg: Check queue type before attaching to a queue

[Bart Van Assche]

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

[PATCH v2 4/6] pktcdvd: Check queue type before attaching to a queue

[Bart Van Assche]

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

[PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio

[Bart Van Assche]

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

[PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request

[Bart Van Assche]

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

Re: [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio

[Hannes Reinecke]

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)

Re: [PATCH v2 5/6] cdrom: Check SCSI passthrough support before reading audio

[Christoph Hellwig]

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>

Re: [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel

[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.

Re: [PATCH v2 6/6] nfsd: Check queue type before submitting a SCSI request

[J . Bruce Fields]

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

Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Jens Axboe]

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

Re: [PATCH v2 0/6] Split scsi passthrough fields out of struct request sequel

[Jens Axboe]

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

Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Ross Zwisler]

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

Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Bart Van Assche]

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

Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Ross Zwisler]

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>

Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Jens Axboe]

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

Re: [PATCH v2 1/6] block: Avoid that blk_exit_rl() triggers a use-after-free

[Bart Van Assche]

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.

Re: [v2,4/6] pktcdvd: Check queue type before attaching to a queue

[Maciej S. Szmigiero]

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

Re: [v2,4/6] pktcdvd: Check queue type before attaching to a queue

[Bart Van Assche]

[-- 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

Re: [v2,4/6] pktcdvd: Check queue type before attaching to a queue

[Maciej S. Szmigiero]

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