* [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
@ 2014-07-02 19:35 Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 1/2] block,scsi: verify return pointer from blk_get_request Joe Lawrence
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-07-02 19:35 UTC (permalink / raw)
To: linux-kernel
Cc: Jens Axboe, Jiri Kosina, Jeff Moyer, Boaz Harrosh, Joe Lawrence
v2->v3: rebase to 3.16-rc2, consider return values from the
blk_mq_alloc_request leg of the blk_get_request callchain
(noted by Jeff), noted in the second patch changelog.
blk_mq_queue_enter may return 0 or errno, which
blk_mq_alloc_request can propogate out via ERR_PTR.
__blk_mq_alloc_request doesn't include any blk_queue_dying
checks, so I'm assuming that its failures can be attributed
to -EWOULDBLOCK under !GFP_WAIT conditions.
v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
tags.
Joe Lawrence (2):
block,scsi: verify return pointer from blk_get_request
block,scsi: fixup blk_get_request dead queue scenarios
block/blk-core.c | 34 +++++++++++++--------------
block/blk-mq.c | 8 +++++--
block/bsg.c | 8 +++----
block/scsi_ioctl.c | 13 +++++++---
drivers/block/paride/pd.c | 2 ++
drivers/block/pktcdvd.c | 2 ++
drivers/block/sx8.c | 2 +-
drivers/cdrom/cdrom.c | 4 ++--
drivers/ide/ide-park.c | 2 +-
drivers/scsi/device_handler/scsi_dh_alua.c | 2 +-
drivers/scsi/device_handler/scsi_dh_emc.c | 2 +-
drivers/scsi/device_handler/scsi_dh_hp_sw.c | 4 ++--
drivers/scsi/device_handler/scsi_dh_rdac.c | 2 +-
drivers/scsi/osd/osd_initiator.c | 4 ++--
drivers/scsi/osst.c | 2 +-
drivers/scsi/scsi_error.c | 2 ++
drivers/scsi/scsi_lib.c | 2 +-
drivers/scsi/scsi_tgt_lib.c | 2 +-
drivers/scsi/sg.c | 4 ++--
drivers/scsi/st.c | 2 +-
drivers/target/target_core_pscsi.c | 2 +-
21 files changed, 61 insertions(+), 44 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] block,scsi: verify return pointer from blk_get_request
2014-07-02 19:35 [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
@ 2014-07-02 19:35 ` Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 2/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-07-02 19:35 UTC (permalink / raw)
To: linux-kernel
Cc: Jens Axboe, Jiri Kosina, Jeff Moyer, Boaz Harrosh, Joe Lawrence
The blk-core dead queue checks introduce an error scenario to
blk_get_request that returns NULL if the request queue has been
shutdown. This affects the behavior for __GFP_WAIT callers, who should
verify the return value before dereferencing.
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Acked-by: Jiri Kosina <jkosina@suse.cz> [for pktdvd]
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
---
block/scsi_ioctl.c | 9 ++++++++-
drivers/block/paride/pd.c | 2 ++
drivers/block/pktcdvd.c | 2 ++
drivers/scsi/scsi_error.c | 2 ++
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 14695c6..3e1d80c 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -438,6 +438,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
}
rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT);
+ if (!rq) {
+ err = -ENODEV;
+ goto error_free_buffer;
+ }
cmdlen = COMMAND_SIZE(opcode);
@@ -510,8 +514,9 @@ out:
}
error:
- kfree(buffer);
blk_put_request(rq);
+error_free_buffer:
+ kfree(buffer);
return err;
}
EXPORT_SYMBOL_GPL(sg_scsi_ioctl);
@@ -524,6 +529,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
int err;
rq = blk_get_request(q, WRITE, __GFP_WAIT);
+ if (!rq)
+ return -ENODEV;
blk_rq_set_block_pc(rq);
rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
rq->cmd[0] = cmd;
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index fea7e76..ca831f7 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -722,6 +722,8 @@ static int pd_special_command(struct pd_unit *disk,
int err = 0;
rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
+ if (!rq)
+ return -ENODEV;
rq->cmd_type = REQ_TYPE_SPECIAL;
rq->special = func;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 758ac44..7fa8c80 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -704,6 +704,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
WRITE : READ, __GFP_WAIT);
+ if (!rq)
+ return -ENODEV;
blk_rq_set_block_pc(rq);
if (cgc->buflen) {
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cbe38e5..b403b04 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1951,6 +1951,8 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
* request becomes available
*/
req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
+ if (!req)
+ return;
blk_rq_set_block_pc(req);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-07-02 19:35 [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 1/2] block,scsi: verify return pointer from blk_get_request Joe Lawrence
@ 2014-07-02 19:35 ` Joe Lawrence
2014-07-02 20:26 ` [PATCH v3 0/2] " Joe Lawrence
2014-08-26 17:24 ` Jeff Moyer
3 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-07-02 19:35 UTC (permalink / raw)
To: linux-kernel
Cc: Jens Axboe, Jiri Kosina, Jeff Moyer, Boaz Harrosh, Joe Lawrence
The blk_get_request function may fail in low-memory conditions or during
device removal (even if __GFP_WAIT is set). To distinguish between these
errors, modify the blk_get_request call stack to return the appropriate
ERR_PTR. Verify that all callers check the return status and consider
IS_ERR instead of a simple NULL pointer check.
For consistency, make a similar change to the blk_mq_alloc_request leg
of blk_get_request. It may fail if the queue is dead, or the caller was
unwilling to wait.
Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Acked-by: Jiri Kosina <jkosina@suse.cz> [for pktdvd]
Acked-by: Boaz Harrosh <bharrosh@panasas.com> [for osd]
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
---
block/blk-core.c | 34 +++++++++++++--------------
block/blk-mq.c | 8 +++++--
block/bsg.c | 8 +++----
block/scsi_ioctl.c | 12 +++++-----
drivers/block/paride/pd.c | 4 ++--
drivers/block/pktcdvd.c | 4 ++--
drivers/block/sx8.c | 2 +-
drivers/cdrom/cdrom.c | 4 ++--
drivers/ide/ide-park.c | 2 +-
drivers/scsi/device_handler/scsi_dh_alua.c | 2 +-
drivers/scsi/device_handler/scsi_dh_emc.c | 2 +-
drivers/scsi/device_handler/scsi_dh_hp_sw.c | 4 ++--
drivers/scsi/device_handler/scsi_dh_rdac.c | 2 +-
drivers/scsi/osd/osd_initiator.c | 4 ++--
drivers/scsi/osst.c | 2 +-
drivers/scsi/scsi_error.c | 2 +-
drivers/scsi/scsi_lib.c | 2 +-
drivers/scsi/scsi_tgt_lib.c | 2 +-
drivers/scsi/sg.c | 4 ++--
drivers/scsi/st.c | 2 +-
drivers/target/target_core_pscsi.c | 2 +-
21 files changed, 56 insertions(+), 52 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 6f8dba1..7dd4692 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -930,9 +930,9 @@ static struct io_context *rq_ioc(struct bio *bio)
* Get a free request from @q. This function may fail under memory
* pressure or if @q is dead.
*
- * Must be callled with @q->queue_lock held and,
- * Returns %NULL on failure, with @q->queue_lock held.
- * Returns !%NULL on success, with @q->queue_lock *not held*.
+ * Must be called with @q->queue_lock held and,
+ * Returns ERR_PTR on failure, with @q->queue_lock held.
+ * Returns request pointer on success, with @q->queue_lock *not held*.
*/
static struct request *__get_request(struct request_list *rl, int rw_flags,
struct bio *bio, gfp_t gfp_mask)
@@ -946,7 +946,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
int may_queue;
if (unlikely(blk_queue_dying(q)))
- return NULL;
+ return ERR_PTR(-ENODEV);
may_queue = elv_may_queue(q, rw_flags);
if (may_queue == ELV_MQUEUE_NO)
@@ -971,7 +971,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
* process is not a "batcher", and not
* exempted by the IO scheduler
*/
- return NULL;
+ return ERR_PTR(-ENOMEM);
}
}
}
@@ -989,7 +989,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
* allocated with any setting of ->nr_requests
*/
if (rl->count[is_sync] >= (3 * q->nr_requests / 2))
- return NULL;
+ return ERR_PTR(-ENOMEM);
q->nr_rqs[is_sync]++;
rl->count[is_sync]++;
@@ -1094,7 +1094,7 @@ fail_alloc:
rq_starved:
if (unlikely(rl->count[is_sync] == 0))
rl->starved[is_sync] = 1;
- return NULL;
+ return ERR_PTR(-ENOMEM);
}
/**
@@ -1107,9 +1107,9 @@ rq_starved:
* Get a free request from @q. If %__GFP_WAIT is set in @gfp_mask, this
* function keeps retrying under memory pressure and fails iff @q is dead.
*
- * Must be callled with @q->queue_lock held and,
- * Returns %NULL on failure, with @q->queue_lock held.
- * Returns !%NULL on success, with @q->queue_lock *not held*.
+ * Must be called with @q->queue_lock held and,
+ * Returns ERR_PTR on failure, with @q->queue_lock held.
+ * Returns request pointer on success, with @q->queue_lock *not held*.
*/
static struct request *get_request(struct request_queue *q, int rw_flags,
struct bio *bio, gfp_t gfp_mask)
@@ -1122,12 +1122,12 @@ static struct request *get_request(struct request_queue *q, int rw_flags,
rl = blk_get_rl(q, bio); /* transferred to @rq on success */
retry:
rq = __get_request(rl, rw_flags, bio, gfp_mask);
- if (rq)
+ if (!IS_ERR(rq))
return rq;
if (!(gfp_mask & __GFP_WAIT) || unlikely(blk_queue_dying(q))) {
blk_put_rl(rl);
- return NULL;
+ return rq;
}
/* wait on @rl and retry */
@@ -1164,7 +1164,7 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
spin_lock_irq(q->queue_lock);
rq = get_request(q, rw, NULL, gfp_mask);
- if (!rq)
+ if (IS_ERR(rq))
spin_unlock_irq(q->queue_lock);
/* q->queue_lock is unlocked at this point */
@@ -1216,8 +1216,8 @@ struct request *blk_make_request(struct request_queue *q, struct bio *bio,
{
struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
- if (unlikely(!rq))
- return ERR_PTR(-ENOMEM);
+ if (IS_ERR(rq))
+ return rq;
blk_rq_set_block_pc(rq);
@@ -1612,8 +1612,8 @@ get_rq:
* Returns with the queue unlocked.
*/
req = get_request(q, rw_flags, bio, GFP_NOIO);
- if (unlikely(!req)) {
- bio_endio(bio, -ENODEV); /* @q is dead */
+ if (IS_ERR(req)) {
+ bio_endio(bio, PTR_ERR(req)); /* @q is dead */
goto out_unlock;
}
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0ef2dc7..eaec14b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -240,9 +240,11 @@ struct request *blk_mq_alloc_request(struct request_queue *q, int rw, gfp_t gfp,
struct blk_mq_hw_ctx *hctx;
struct request *rq;
struct blk_mq_alloc_data alloc_data;
+ int ret;
- if (blk_mq_queue_enter(q))
- return NULL;
+ ret = blk_mq_queue_enter(q);
+ if (ret)
+ return ERR_PTR(ret);
ctx = blk_mq_get_ctx(q);
hctx = q->mq_ops->map_queue(q, ctx->cpu);
@@ -262,6 +264,8 @@ struct request *blk_mq_alloc_request(struct request_queue *q, int rw, gfp_t gfp,
ctx = alloc_data.ctx;
}
blk_mq_put_ctx(ctx);
+ if (!rq)
+ return ERR_PTR(-EWOULDBLOCK);
return rq;
}
EXPORT_SYMBOL(blk_mq_alloc_request);
diff --git a/block/bsg.c b/block/bsg.c
index ff46add..73c78fd 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -270,8 +270,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
* map scatter-gather elements separately and string them to request
*/
rq = blk_get_request(q, rw, GFP_KERNEL);
- if (!rq)
- return ERR_PTR(-ENOMEM);
+ if (IS_ERR(rq))
+ return rq;
blk_rq_set_block_pc(rq);
ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, bd, has_write_perm);
@@ -285,8 +285,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
}
next_rq = blk_get_request(q, READ, GFP_KERNEL);
- if (!next_rq) {
- ret = -ENOMEM;
+ if (IS_ERR(next_rq)) {
+ ret = PTR_ERR(next_rq);
goto out;
}
rq->next_rq = next_rq;
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 3e1d80c..531cbe5 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -308,8 +308,8 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
}
rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
- if (!rq)
- return -ENOMEM;
+ if (IS_ERR(rq))
+ return PTR_ERR(rq);
blk_rq_set_block_pc(rq);
if (blk_fill_sghdr_rq(q, rq, hdr, mode)) {
@@ -438,8 +438,8 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
}
rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT);
- if (!rq) {
- err = -ENODEV;
+ if (IS_ERR(rq)) {
+ err = PTR_ERR(rq);
goto error_free_buffer;
}
@@ -529,8 +529,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
int err;
rq = blk_get_request(q, WRITE, __GFP_WAIT);
- if (!rq)
- return -ENODEV;
+ if (IS_ERR(rq))
+ return PTR_ERR(rq);
blk_rq_set_block_pc(rq);
rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
rq->cmd[0] = cmd;
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index ca831f7..d48715b 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -722,8 +722,8 @@ static int pd_special_command(struct pd_unit *disk,
int err = 0;
rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
- if (!rq)
- return -ENODEV;
+ if (IS_ERR(rq))
+ return PTR_ERR(rq);
rq->cmd_type = REQ_TYPE_SPECIAL;
rq->special = func;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 7fa8c80..09e628da 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -704,8 +704,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
WRITE : READ, __GFP_WAIT);
- if (!rq)
- return -ENODEV;
+ if (IS_ERR(rq))
+ return PTR_ERR(rq);
blk_rq_set_block_pc(rq);
if (cgc->buflen) {
diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index d5e2d12..5d55285 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -568,7 +568,7 @@ static struct carm_request *carm_get_special(struct carm_host *host)
return NULL;
rq = blk_get_request(host->oob_q, WRITE /* bogus */, GFP_KERNEL);
- if (!rq) {
+ if (IS_ERR(rq)) {
spin_lock_irqsave(&host->lock, flags);
carm_put_request(host, crq);
spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 898b84b..5d28a45 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2180,8 +2180,8 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
len = nr * CD_FRAMESIZE_RAW;
rq = blk_get_request(q, READ, GFP_KERNEL);
- if (!rq) {
- ret = -ENOMEM;
+ if (IS_ERR(rq)) {
+ ret = PTR_ERR(rq);
break;
}
blk_rq_set_block_pc(rq);
diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
index f41558a..ca95860 100644
--- a/drivers/ide/ide-park.c
+++ b/drivers/ide/ide-park.c
@@ -46,7 +46,7 @@ static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
* timeout has expired, so power management will be reenabled.
*/
rq = blk_get_request(q, READ, GFP_NOWAIT);
- if (unlikely(!rq))
+ if (IS_ERR(rq))
goto out;
rq->cmd[0] = REQ_UNPARK_HEADS;
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 7bcf67e..e99507e 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -115,7 +115,7 @@ static struct request *get_alua_req(struct scsi_device *sdev,
rq = blk_get_request(q, rw, GFP_NOIO);
- if (!rq) {
+ if (IS_ERR(rq)) {
sdev_printk(KERN_INFO, sdev,
"%s: blk_get_request failed\n", __func__);
return NULL;
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 6f07f7f..8476538 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -275,7 +275,7 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
rq = blk_get_request(sdev->request_queue,
(cmd != INQUIRY) ? WRITE : READ, GFP_NOIO);
- if (!rq) {
+ if (IS_ERR(rq)) {
sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
return NULL;
}
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index e9d9fea..4ee2759 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -117,7 +117,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
retry:
req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
- if (!req)
+ if (IS_ERR(req))
return SCSI_DH_RES_TEMP_UNAVAIL;
blk_rq_set_block_pc(req);
@@ -247,7 +247,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
struct request *req;
req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC);
- if (!req)
+ if (IS_ERR(req))
return SCSI_DH_RES_TEMP_UNAVAIL;
blk_rq_set_block_pc(req);
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 826069d..1b5bc92 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -274,7 +274,7 @@ static struct request *get_rdac_req(struct scsi_device *sdev,
rq = blk_get_request(q, rw, GFP_NOIO);
- if (!rq) {
+ if (IS_ERR(rq)) {
sdev_printk(KERN_INFO, sdev,
"get_rdac_req: blk_get_request failed.\n");
return NULL;
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 5f4cbf0..fd19fd8 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1567,8 +1567,8 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
struct request *req;
req = blk_get_request(q, has_write ? WRITE : READ, flags);
- if (unlikely(!req))
- return ERR_PTR(-ENOMEM);
+ if (IS_ERR(req))
+ return req;
blk_rq_set_block_pc(req);
return req;
diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 0727ea7..dff37a25 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -362,7 +362,7 @@ static int osst_execute(struct osst_request *SRpnt, const unsigned char *cmd,
int write = (data_direction == DMA_TO_DEVICE);
req = blk_get_request(SRpnt->stp->device->request_queue, write, GFP_KERNEL);
- if (!req)
+ if (IS_ERR(req))
return DRIVER_ERROR << 24;
blk_rq_set_block_pc(req);
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index b403b04..641ffde 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1951,7 +1951,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
* request becomes available
*/
req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
- if (!req)
+ if (IS_ERR(req))
return;
blk_rq_set_block_pc(req);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f7e3163..b64bba3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -193,7 +193,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int ret = DRIVER_ERROR << 24;
req = blk_get_request(sdev->request_queue, write, __GFP_WAIT);
- if (!req)
+ if (IS_ERR(req))
return ret;
blk_rq_set_block_pc(req);
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index e51add0..b14c15b 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -97,7 +97,7 @@ struct scsi_cmnd *scsi_host_get_command(struct Scsi_Host *shost,
* we are in target mode we want the opposite.
*/
rq = blk_get_request(shost->uspace_req_q, !write, gfp_mask);
- if (!rq)
+ if (IS_ERR(rq))
goto free_tcmd;
cmd = __scsi_get_command(shost, gfp_mask);
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 53268aa..dec00f6 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1650,8 +1650,8 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
dxfer_len));
rq = blk_get_request(q, rw, GFP_ATOMIC);
- if (!rq)
- return -ENOMEM;
+ if (IS_ERR(rq))
+ return PTR_ERR(rq);
blk_rq_set_block_pc(rq);
memcpy(rq->cmd, cmd, hp->cmd_len);
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 14eb4b2..54f74b0 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -481,7 +481,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd,
req = blk_get_request(SRpnt->stp->device->request_queue, write,
GFP_KERNEL);
- if (!req)
+ if (IS_ERR(req))
return DRIVER_ERROR << 24;
blk_rq_set_block_pc(req);
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 94d00df..b2bdced 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1050,7 +1050,7 @@ pscsi_execute_cmd(struct se_cmd *cmd)
req = blk_get_request(pdv->pdv_sd->request_queue,
(data_direction == DMA_TO_DEVICE),
GFP_KERNEL);
- if (!req) {
+ if (IS_ERR(req)) {
pr_err("PSCSI: blk_get_request() failed\n");
ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
goto fail;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-07-02 19:35 [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 1/2] block,scsi: verify return pointer from blk_get_request Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 2/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
@ 2014-07-02 20:26 ` Joe Lawrence
2014-08-26 17:24 ` Jeff Moyer
3 siblings, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-07-02 20:26 UTC (permalink / raw)
To: Joe Lawrence
Cc: linux-kernel, Jens Axboe, Jiri Kosina, Jeff Moyer, Boaz Harrosh
FWIW, I spent some time looking at blk_get_request callers and as far as
I can tell, most should be able to gracefully handle additional errno
values of -ENODEV and -EWOULDBLOCK. I didn't chase down the pktcdvd or
osd paths, however Jiri and Boaz ack'd the earlier patch version that
added -ENODEV.
Here are my wide-screen notes from the cscope goose-chase...
Functions calling this function: blk_get_request
File Function Line Return value handling
0 blk-core.c blk_make_request 1217 struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask); ERR_PTR propagated, see callers below, TODO
1 bsg.c bsg_map_hdr 272 rq = blk_get_request(q, rw, GFP_KERNEL); ERR_PTR propagated, see callers below
2 bsg.c bsg_map_hdr 287 next_rq = blk_get_request(q, READ, GFP_KERNEL); ERR_PTR propagated, see callers below
3 scsi_ioctl.c sg_io 310 rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL); PTR_ERR propagated, see callers below
4 scsi_ioctl.c sg_scsi_ioctl 440 rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT); PTR_ERR propagated, see callers below
5 scsi_ioctl.c __blk_send_generic 526 rq = blk_get_request(q, WRITE, __GFP_WAIT); PTR_ERR propagated, see callers below
6 pd.c action 724 rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT); caller is actually "pd_special_command", PTR_ERR propagated, see callers below
7 pktcdvd.c pkt_generic_packet 705 rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ? PTR_ERR propagated, see callers below, TODO
8 sx8.c carm_get_special 570 rq = blk_get_request(host->oob_q, WRITE , GFP_KERNEL); return code is NULL
9 cdrom.c cdrom_read_cdda_bpc 2182 rq = blk_get_request(q, READ, GFP_KERNEL); filters out -EIO, PTR_ERR propagated, see callers below
a ide-atapi.c ide_queue_pc_tail 95 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
b ide-cd.c ide_cd_queue_pc 444 rq = blk_get_request(drive->queue, write, __GFP_WAIT); don't care, deprecated driver
c ide-cd_ioctl.c ide_cdrom_reset 306 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
d ide-devsets.c ide_devset_execute 168 rq = blk_get_request(q, READ, __GFP_WAIT); don't care, deprecated driver
e ide-disk.c set_multcount 480 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
f ide-ioctls.c ide_cmd_ioctl 128 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
g ide-ioctls.c generic_drive_reset 224 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
h ide-park.c issue_park_cmd 34 rq = blk_get_request(q, READ, __GFP_WAIT); caller is void
i ide-park.c issue_park_cmd 48 rq = blk_get_request(q, READ, GFP_NOWAIT); don't care, deprecated driver
j ide-pm.c generic_ide_suspend 21 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
k ide-pm.c generic_ide_resume 61 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
l ide-tape.c idetape_queue_rw_tail 855 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver
m ide-taskfile.c ide_raw_taskfile 433 rq = blk_get_request(drive->queue, rw, __GFP_WAIT); don't care, deprecated driver
n scsi_dh_alua.c get_alua_req 116 rq = blk_get_request(q, rw, GFP_NOIO); return code is NULL
o scsi_dh_emc.c get_req 276 rq = blk_get_request(sdev->request_queue, return code is NULL
p scsi_dh_hp_sw.c hp_sw_tur 119 req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO); return code is driver/core specific
q scsi_dh_hp_sw.c hp_sw_start_stop 249 req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC); return code is driver/core specific
r scsi_dh_rdac.c get_rdac_req 275 rq = blk_get_request(q, rw, GFP_NOIO); return code is NULL
s osd_initiator.c _make_request 1569 req = blk_get_request(q, has_write ? WRITE : READ, flags); ERR_PTR propagated, see callers below, TODO
t osst.c osst_execute 364 req = blk_get_request(SRpnt->stp->device->request_queue, write, GFP_KERNEL); return code is driver/core specific
u scsi_error.c scsi_eh_lock_door 1953 req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL); caller is void
v scsi_lib.c scsi_execute 195 req = blk_get_request(sdev->request_queue, write, __GFP_WAIT); return code is driver/core specific
w scsi_tgt_lib.c scsi_host_get_command 99 rq = blk_get_request(shost->uspace_req_q, !write, gfp_mask); return code is NULL
x sg.c sg_start_req 1652 rq = blk_get_request(q, rw, GFP_ATOMIC); PTR_ERR propagated, see callers below
y st.c st_scsi_execute 482 req = blk_get_request(SRpnt->stp->device->request_queue, write, return code is driver/core specific
z target_core_pscsi.c pscsi_execute_cmd 1050 req = blk_get_request(pdv->pdv_sd->request_queue, return code is driver/core specific
Functions calling this function: blk_make_request
File Function Line
0 virtio_blk.c virtblk_get_id 241 req = blk_make_request(vblk->disk->queue, bio, GFP_KERNEL); PTR_ERR propagated, see callers below
1 osd_initiator.c _make_request 1565 return blk_make_request(q, oii->bio, flags); ERR_PTR propagated, see callers below, TODO
2 target_core_pscsi.c pscsi_execute_cmd 1067 req = blk_make_request(pdv->pdv_sd->request_queue, hbio, return code is driver/core specific
Functions calling this function: virtblk_get_id
File Function Line
0 virtio_blk.c virtblk_serial_show 318 err = virtblk_get_id(disk, buf); Filters out -EIO, others propagated, caller is a DEVICE_ATTR
Functions calling this function: _make_request
File Function Line
0 osd_initiator.c _init_blk_request 1587 req = _make_request(q, has_out, has_out ? &or->out : &or->in, flags); PTR_ERR propagated, see callers below, TDOO
1 osd_initiator.c _init_blk_request 1605 req = _make_request(q, false, &or->in, flags); PTR_ERR propagated, see callers below, TODO
Functions calling this function: _init_blk_request
File Function Line
0 osd_initiator.c osd_finalize_request 1654 ret = _init_blk_request(or, has_in, has_out); PTR_ERR propagated, see callers below
Functions calling this function: osd_finalize_request
File Function Line
0 osdblk.c osd_sync_op 151 ret = osd_finalize_request(or, 0, credential, NULL); PTR_ERR propagated, see callers below, TODO
1 osdblk.c osd_async_op 169 ret = osd_finalize_request(or, 0, cred, NULL); PTR_ERR propagated, see callers below, TODO
2 osd_initiator.c _osd_get_print_system_info 109 ret = osd_finalize_request(or, 0, caps, NULL); PTR_ERR propagated, see callers below, TODO
3 ore.c ore_io_execute 376 ret = osd_finalize_request(or, 0, _ios_cred(ios, i), NULL); PTR_ERR propagated, see callers below, TODO
4 super.c exofs_read_kern 242 ret = osd_finalize_request(or, 0, cred, NULL); PTR_ERR propagated, see callers below, TODO
Functions calling this function: bsg_map_hdr
File Function Line
0 bsg.c __bsg_write 649 rq = bsg_map_hdr(bd, &bc->hdr, has_write_perm, bc->sense); PTR_ERR propagated, see callers below
1 bsg.c bsg_ioctl 937 rq = bsg_map_hdr(bd, &hdr, file->f_mode & FMODE_WRITE, sense); PTR_ERR propagated, caller is a file_operations.unlocked_ioctl method
Functions calling this function: __bsg_write
File Function Line
0 bsg.c bsg_write 682 ret = __bsg_write(bd, buf, count, &bytes_written, PTR_ERR propagated, caller is a file_operations.write method
Functions calling this function: sg_io
File Function Line
0 scsi_ioctl.c scsi_cmd_ioctl 586 err = sg_io(q, bd_disk, &hdr, mode); If !EFAULT, calls copy_to_user and sets -EFAULT
1 scsi_ioctl.c scsi_cmd_ioctl 634 err = sg_io(q, bd_disk, &hdr, mode);
Functions calling this function: sg_scsi_ioctl
File Function Line
0 scsi_ioctl.c scsi_cmd_ioctl 658 err = sg_scsi_ioctl(q, bd_disk, mode, arg); PTR_ERR propagated, see callers below
1 scsi_ioctl.c scsi_ioctl 247 return sg_scsi_ioctl(sdev->request_queue, NULL, 0, arg); PTR_ERR propagated, see callers below
2 sg.c sg_ioctl 1083 return sg_scsi_ioctl(sdp->device->request_queue, NULL, filp->f_mode, p); PTR_ERR propagated, caller is a file_operations .unlocked_ioctl method
3 blkdev.h sg_ioctl 820 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t, not a function
Functions calling this function: scsi_cmd_ioctl
File Function Line
0 bsg.c bsg_ioctl 925 return scsi_cmd_ioctl(bd->queue, NULL, file->f_mode, cmd, uarg); PTR_ERR propagated, see callers above
1 scsi_ioctl.c scsi_cmd_blk_ioctl 724 return scsi_cmd_ioctl(bd->bd_disk->queue, bd->bd_disk, mode, cmd, arg); PTR_ERR propagated, see callers below
2 skd_main.c skd_bdev_ioctl 1277 rc = scsi_cmd_ioctl(disk->queue, disk, mode, cmd_in, p); PTR_ERR propagated, caller is a block_device_operations .compat_ioctl method
3 st.c st_ioctl 3662 i = scsi_cmd_ioctl(STp->disk->queue, STp->disk, PTR_ERR propagated, caller is a file_operations .compat_ioctl method
4 blkdev.h st_ioctl 818 extern int scsi_cmd_ioctl(struct request_queue *, struct gendisk *, fmode_t, not a function
Functions calling this function: scsi_cmd_blk_ioctl
File Function Line
0 cciss.c cciss_ioctl 1753 return scsi_cmd_blk_ioctl(bdev, mode, cmd, argp); PTR_ERR propagated, see callers below
1 virtio_blk.c virtblk_ioctl 266 return scsi_cmd_blk_ioctl(bdev, mode, cmd, PTR_ERR propagated, caller is a block_device_operations .ioctl method
2 cdrom.c cdrom_ioctl 3314 ret = scsi_cmd_blk_ioctl(bdev, mode, cmd, argp); PTR_ERR propagated, see callers below
3 ide-floppy_ioctl.c ide_floppy_ioctl 295 err = scsi_cmd_blk_ioctl(bdev, mode, cmd, argp); PTR_ERR propagated, caller is a ide_disk_ops .compat_ioctl method
4 sd.c sd_ioctl 1304 error = scsi_cmd_blk_ioctl(bdev, mode, cmd, p); PTR_ERR propagated, caller is a block_device_operations .compat_ioctl method
5 blkdev.h sd_ioctl 816 extern int scsi_cmd_blk_ioctl(struct block_device *, fmode_t, not a function
Functions calling this function: cciss_ioctl
File Function Line
0 cciss.c cciss_compat_ioctl 1170 return cciss_ioctl(bdev, mode, cmd, arg); PTR_ERR propagated, caller is a block_device_operations .compat_ioctl method
1 cciss.c cciss_ioctl32_passthru 1211 err = cciss_ioctl(bdev, mode, CCISS_PASSTHRU, (unsigned long )p); PTR_ERR propagated, see callers below
2 cciss.c cciss_ioctl32_big_passthru 1253 err = cciss_ioctl(bdev, mode, CCISS_BIG_PASSTHRU, (unsigned long )p); PTR_ERR propagated, see callers below
Functions calling this function: cciss_ioctl32_passthru
File Function Line
0 cciss.c cciss_compat_ioctl 1173 return cciss_ioctl32_passthru(bdev, mode, cmd, arg); PTR_ERR propagated, see callers above
Functions calling this function: cciss_ioctl32_big_passthru
File Function Line
0 cciss.c cciss_compat_ioctl 1175 return cciss_ioctl32_big_passthru(bdev, mode, cmd, arg); PTR_ERR propagated, see callers above
Functions calling this function: cdrom_ioctl
File Function Line
0 pcd.c pcd_block_ioctl 254 ret = cdrom_ioctl(&cd->info, bdev, mode, cmd, arg); PTR_ERR propagated, caller is a block_device_operations .ioctl method
1 gdrom.c gdrom_bdops_ioctl 525 ret = cdrom_ioctl(gd.cd_info, bdev, mode, cmd, arg); PTR_ERR propagated, caller is a block_device_operations .ioctl method
2 ide-cd.c idecd_locked_ioctl 1676 err = cdrom_ioctl(&info->devinfo, bdev, mode, cmd, arg); PTR_ERR propagated, see callers below
3 sr.c sr_block_ioctl 564 ret = cdrom_ioctl(&cd->cdi, bdev, mode, cmd, arg); PTR_ERR propagated, caller is a block_device_operations .ioctl method
Functions calling this function: idecd_locked_ioctl
File Function Line
0 ide-cd.c idecd_ioctl 1687 ret = idecd_locked_ioctl(bdev, mode, cmd, arg); PTR_ERR propagated, caller is a block_device_operations .ioctl method
Functions calling this function: scsi_ioctl
File Function Line
0 ch.c ch_ioctl 842 return scsi_ioctl(ch->device, cmd, argp); PTR_ERR propagated, see callers below
1 osst.c do_door_lock 3334 retval = scsi_ioctl(STp->device, cmd, NULL); PTR_ERR propagated, see callers below
2 osst.c osst_ioctl 5264 retval = scsi_ioctl(STp->device, cmd_in, p); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
3 sd.c sd_ioctl 1301 error = scsi_ioctl(sdp, cmd, p); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
4 sd.c sd_ioctl 1307 error = scsi_ioctl(sdp, cmd, p); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
5 sg.c sg_ioctl 1096 return scsi_ioctl(sdp->device, cmd_in, p); PTR_ERR propagated, caller is a file_operations .unlocked_ioctl method
6 sg.c sg_ioctl 1115 return scsi_ioctl(sdp->device, cmd_in, p); PTR_ERR propagated, caller is a file_operations .unlocked_ioctl method
7 sr.c sr_block_ioctl 560 ret = scsi_ioctl(sdev, cmd, argp); PTR_ERR propagated, caller is a block_device_operations .ioctl method
8 sr.c sr_block_ioctl 578 ret = scsi_ioctl(sdev, cmd, argp); PTR_ERR propagated, caller is a block_device_operations .ioctl method
9 st.c do_door_lock 853 retval = scsi_ioctl(STp->device, cmd, NULL); PTR_ERR propagated, see callers below
a st.c st_ioctl 3668 retval = scsi_ioctl(STp->device, cmd_in, p); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
Functions calling this function: ch_ioctl
File Function Line
0 ch.c ch_ioctl_compat 870 return ch_ioctl(file, cmd, arg); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
Functions calling this function: do_door_lock
File Function Line
0 osst.c osst_write 3447 if (STp->do_auto_lock && STp->door_locked == ST_UNLOCKED && !do_door_lock(STp, 1)) return value checked, but not propagated
1 osst.c osst_read 3740 if (STp->do_auto_lock && STp->door_locked == ST_UNLOCKED && !do_door_lock(STp, 1)) return value checked, but not propagated
2 osst.c __os_scsi_tape_open 4640 if (do_door_lock(STp, 1)) return value checked, but not propagated
3 osst.c __os_scsi_tape_open 4783 if (do_door_lock(STp, 1)) return value checked, but not propagated
4 osst.c os_scsi_tape_close 4920 do_door_lock(STp, 0); return value ignored
5 osst.c osst_ioctl 5097 do_door_lock(STp, 0); return value ignored
6 osst.c osst_ioctl 5149 retval = do_door_lock(STp, (mtc.mt_op == MTLOCK)); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
7 st.c st_release 1396 do_door_lock(STp, 0); return value ignored
8 st.c rw_checks 1468 !do_door_lock(STp, 1)) return value checked, but not propagated
9 st.c st_ioctl 3466 do_door_lock(STp, 0); return value ignored
a st.c st_ioctl 3535 retval = do_door_lock(STp, (mtc.mt_op == MTLOCK)); PTR_ERR propagated, caller is a file_operations .compat_ioctl method
Functions calling this function: __blk_send_generic
File Function Line
0 scsi_ioctl.c blk_send_start_stop 541 return __blk_send_generic(q, bd_disk, GPCMD_START_STOP_UNIT, data); PTR_ERR propagated, see callers below
Functions calling this function: blk_send_start_stop
File Function Line
0 scsi_ioctl.c scsi_cmd_ioctl 661 err = blk_send_start_stop(q, bd_disk, 0x03); PTR_ERR propagated, see callers above
1 scsi_ioctl.c scsi_cmd_ioctl 664 err = blk_send_start_stop(q, bd_disk, 0x02); PTR_ERR propagated, see callers above
Functions calling this function: pd_special_command
File Function Line
0 pd.c pd_open 745 pd_special_command(disk, pd_media_check); return value ignored
1 pd.c pd_open 746 pd_special_command(disk, pd_door_lock); return value ignored
2 pd.c pd_ioctl 778 pd_special_command(disk, pd_eject); return value ignored
3 pd.c pd_release 792 pd_special_command(disk, pd_door_unlock); return value ignored
4 pd.c pd_check_events 802 pd_special_command(disk, pd_media_check); return value ignored
5 pd.c pd_revalidate 811 if (pd_special_command(disk, pd_identify) == 0) return value checked, but not propagated
6 pd.c pd_probe_drive 845 if (pd_special_command(disk, pd_identify) == 0) return value checked, but not propagated
7 pd.c pd_probe_drive 847 } else if (pd_special_command(disk, pd_identify) == 0) return value checked, but not propagated
Functions calling this function: pkt_generic_packet
File Function Line
0 pktcdvd.c pkt_flush_cache 778 return pkt_generic_packet(pd, &cgc);
1 pktcdvd.c pkt_set_speed 799 if ((ret = pkt_generic_packet(pd, &cgc))) PTR_ERR propagated, see callers below TODO
2 pktcdvd.c pkt_mode_sense 1573 return pkt_generic_packet(pd, cgc); PTR_ERR propagated, see callers below TODO
3 pktcdvd.c pkt_mode_select 1585 return pkt_generic_packet(pd, cgc); PTR_ERR propagated, see callers below TODO
4 pktcdvd.c pkt_get_disc_info 1599 if ((ret = pkt_generic_packet(pd, &cgc))) PTR_ERR propagated, see callers below TODO
5 pktcdvd.c pkt_get_disc_info 1612 return pkt_generic_packet(pd, &cgc); PTR_ERR propagated, see callers below TODO
6 pktcdvd.c pkt_get_track_info 1628 if ((ret = pkt_generic_packet(pd, &cgc))) PTR_ERR propagated, see callers below TODO
7 pktcdvd.c pkt_get_track_info 1638 return pkt_generic_packet(pd, &cgc); PTR_ERR propagated, see callers below TODO
8 pktcdvd.c pkt_probe_settings 1850 ret = pkt_generic_packet(pd, &cgc); PTR_ERR propagated, see callers below TODO
9 pktcdvd.c pkt_lock_door 1977 return pkt_generic_packet(pd, &cgc); PTR_ERR propagated, see callers below TODO
a pktcdvd.c pkt_media_speed 2059 ret = pkt_generic_packet(pd, &cgc); PTR_ERR propagated, see callers below TODO
b pktcdvd.c pkt_media_speed 2074 ret = pkt_generic_packet(pd, &cgc); PTR_ERR propagated, see callers below TODO
c pktcdvd.c pkt_perform_opc 2130 if ((ret = pkt_generic_packet(pd, &cgc))) PTR_ERR propagated, see callers below TODO
Functions calling this function: cdrom_read_cdda_bpc
File Function Line
0 cdrom.c cdrom_read_cdda 2243 ret = cdrom_read_cdda_bpc(cdi, ubuf, lba, nframes); PTR_ERR propagated, see callers below
Functions calling this function: cdrom_read_cdda
File Function Line
0 cdrom.c mmc_ioctl_cdrom_read_audio 3009 return cdrom_read_cdda(cdi, ra.buf, lba, ra.nframes); PTR_ERR propagated, see callers below
Functions calling this function: mmc_ioctl_cdrom_read_audio
File Function Line
0 cdrom.c mmc_ioctl 3271 return mmc_ioctl_cdrom_read_audio(cdi, userptr); PTR_ERR propagated, see callers below
Functions calling this function: mmc_ioctl
File Function Line
0 cdrom.c cdrom_ioctl 3362 ret = mmc_ioctl(cdi, cmd, arg); filters out -ENOTTY, PTR_ERR propagated, see callers above
Functions calling this function: sg_start_req
File Function Line
0 sg.c sg_common_write 761 k = sg_start_req(srp, cmnd); PTR_ERR propagated, see callers below
Functions calling this function: sg_common_write
File Function Line
0 sg.c sg_write 666 k = sg_common_write(sfp, srp, cmnd, sfp->timeout, blocking); PTR_ERR propagated, caller is a file_operations .write method
1 sg.c sg_new_write 734 k = sg_common_write(sfp, srp, cmnd, timeout, blocking); PTR_ERR propagated, see callers below
Functions calling this function: sg_new_write
File Function Line
0 sg.c sg_write 589 return sg_new_write(sfp, filp, buf, count, PTR_ERR propagated, caller is a file_operations .write method
1 sg.c sg_ioctl 835 result = sg_new_write(sfp, filp, p, SZ_SG_IO_HDR, PTR_ERR propagated, caller is a file_operations .unlocked_ioctl method
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-07-02 19:35 [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
` (2 preceding siblings ...)
2014-07-02 20:26 ` [PATCH v3 0/2] " Joe Lawrence
@ 2014-08-26 17:24 ` Jeff Moyer
2014-08-26 21:19 ` Jens Axboe
3 siblings, 1 reply; 16+ messages in thread
From: Jeff Moyer @ 2014-08-26 17:24 UTC (permalink / raw)
To: Jens Axboe, Joe Lawrence; +Cc: linux-kernel, Jiri Kosina, Boaz Harrosh
Joe Lawrence <joe.lawrence@stratus.com> writes:
> v2->v3: rebase to 3.16-rc2, consider return values from the
> blk_mq_alloc_request leg of the blk_get_request callchain
> (noted by Jeff), noted in the second patch changelog.
>
> blk_mq_queue_enter may return 0 or errno, which
> blk_mq_alloc_request can propogate out via ERR_PTR.
> __blk_mq_alloc_request doesn't include any blk_queue_dying
> checks, so I'm assuming that its failures can be attributed
> to -EWOULDBLOCK under !GFP_WAIT conditions.
>
> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
> tags.
>
> Joe Lawrence (2):
> block,scsi: verify return pointer from blk_get_request
> block,scsi: fixup blk_get_request dead queue scenarios
Jens,
Did this patch set fall through the cracks again?
Cheers,
Jeff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 17:24 ` Jeff Moyer
@ 2014-08-26 21:19 ` Jens Axboe
2014-08-26 21:24 ` Jens Axboe
2014-08-26 21:27 ` Jeff Moyer
0 siblings, 2 replies; 16+ messages in thread
From: Jens Axboe @ 2014-08-26 21:19 UTC (permalink / raw)
To: Jeff Moyer, Joe Lawrence; +Cc: linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/26/2014 11:24 AM, Jeff Moyer wrote:
> Joe Lawrence <joe.lawrence@stratus.com> writes:
>
>> v2->v3: rebase to 3.16-rc2, consider return values from the
>> blk_mq_alloc_request leg of the blk_get_request callchain
>> (noted by Jeff), noted in the second patch changelog.
>>
>> blk_mq_queue_enter may return 0 or errno, which
>> blk_mq_alloc_request can propogate out via ERR_PTR.
>> __blk_mq_alloc_request doesn't include any blk_queue_dying
>> checks, so I'm assuming that its failures can be attributed
>> to -EWOULDBLOCK under !GFP_WAIT conditions.
>>
>> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
>> tags.
>>
>> Joe Lawrence (2):
>> block,scsi: verify return pointer from blk_get_request
>> block,scsi: fixup blk_get_request dead queue scenarios
>
> Jens,
>
> Did this patch set fall through the cracks again?
Falling through the cracks implies that I meant to apply it and did not,
which was not the case. But I think we're at the point now where I'm
finally comfortable with applying it. So, Joe, could you ensure that it
applies to 3.17-rc2, then I will roll it in to the updates for 3.18.
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 21:19 ` Jens Axboe
@ 2014-08-26 21:24 ` Jens Axboe
2014-08-26 21:27 ` Jeff Moyer
1 sibling, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2014-08-26 21:24 UTC (permalink / raw)
To: Jeff Moyer, Joe Lawrence; +Cc: linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/26/2014 03:19 PM, Jens Axboe wrote:
> On 08/26/2014 11:24 AM, Jeff Moyer wrote:
>> Joe Lawrence <joe.lawrence@stratus.com> writes:
>>
>>> v2->v3: rebase to 3.16-rc2, consider return values from the
>>> blk_mq_alloc_request leg of the blk_get_request callchain
>>> (noted by Jeff), noted in the second patch changelog.
>>>
>>> blk_mq_queue_enter may return 0 or errno, which
>>> blk_mq_alloc_request can propogate out via ERR_PTR.
>>> __blk_mq_alloc_request doesn't include any blk_queue_dying
>>> checks, so I'm assuming that its failures can be attributed
>>> to -EWOULDBLOCK under !GFP_WAIT conditions.
>>>
>>> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
>>> tags.
>>>
>>> Joe Lawrence (2):
>>> block,scsi: verify return pointer from blk_get_request
>>> block,scsi: fixup blk_get_request dead queue scenarios
>>
>> Jens,
>>
>> Did this patch set fall through the cracks again?
>
> Falling through the cracks implies that I meant to apply it and did not,
> which was not the case. But I think we're at the point now where I'm
> finally comfortable with applying it. So, Joe, could you ensure that it
> applies to 3.17-rc2, then I will roll it in to the updates for 3.18.
Actually, just audited a few of them, and conversions like this:
- if (!rq)
+ if (IS_ERR(rq))
will break spectacularly if rq == NULL is returned. Should all these be
IS_ERR_OR_NULL?
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 21:19 ` Jens Axboe
2014-08-26 21:24 ` Jens Axboe
@ 2014-08-26 21:27 ` Jeff Moyer
2014-08-26 21:33 ` Jens Axboe
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Moyer @ 2014-08-26 21:27 UTC (permalink / raw)
To: Jens Axboe; +Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
Jens Axboe <axboe@kernel.dk> writes:
> On 08/26/2014 11:24 AM, Jeff Moyer wrote:
>> Joe Lawrence <joe.lawrence@stratus.com> writes:
>>
>>> v2->v3: rebase to 3.16-rc2, consider return values from the
>>> blk_mq_alloc_request leg of the blk_get_request callchain
>>> (noted by Jeff), noted in the second patch changelog.
>>>
>>> blk_mq_queue_enter may return 0 or errno, which
>>> blk_mq_alloc_request can propogate out via ERR_PTR.
>>> __blk_mq_alloc_request doesn't include any blk_queue_dying
>>> checks, so I'm assuming that its failures can be attributed
>>> to -EWOULDBLOCK under !GFP_WAIT conditions.
>>>
>>> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
>>> tags.
>>>
>>> Joe Lawrence (2):
>>> block,scsi: verify return pointer from blk_get_request
>>> block,scsi: fixup blk_get_request dead queue scenarios
>>
>> Jens,
>>
>> Did this patch set fall through the cracks again?
>
> Falling through the cracks implies that I meant to apply it and did not,
> which was not the case.
Sorry, I was mislead by our earlier conversation on this (mail inline
below).
> But I think we're at the point now where I'm finally comfortable with
> applying it. So, Joe, could you ensure that it applies to 3.17-rc2,
> then I will roll it in to the updates for 3.18.
Joe, you will have one hunk to modify for sure, in scsi_ioctl.c. A
previous patch added a check for null, but ended up returning the wrong
value (ENOMEM instead of ENODEV).
Cheers,
Jeff
---old mail below---
From: Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH v2 0/2] block,scsi: fixup blk_get_request dead queue scenarios
To: Jeff Moyer <jmoyer@redhat.com>, Joe Lawrence <joe.lawrence@stratus.com>
CC: linux-kernel@vger.kernel.org
Date: Thu, 26 Jun 2014 10:11:09 -0600 (8 weeks, 5 days, 5 hours ago)
On 2014-06-26 10:08, Jeff Moyer wrote:
> Joe Lawrence <joe.lawrence@stratus.com> writes:
>
>> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
>> tags.
>>
>> Joe Lawrence (2):
>> block,scsi: verify return pointer from blk_get_request
>> block,scsi: convert and handle ERR_PTR from blk_get_request
>
> Jens,
>
> Can you pick this series up? It actually fixes a NULL pointer
> dereference that can happen if (for example) a usb optical drive is
> removed while an application is using it.
Yeah I'll pick it up, opening up the 3.17 branches soon.
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 21:27 ` Jeff Moyer
@ 2014-08-26 21:33 ` Jens Axboe
2014-08-26 21:37 ` Jens Axboe
0 siblings, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2014-08-26 21:33 UTC (permalink / raw)
To: Jeff Moyer; +Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/26/2014 03:27 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
>> On 08/26/2014 11:24 AM, Jeff Moyer wrote:
>>> Joe Lawrence <joe.lawrence@stratus.com> writes:
>>>
>>>> v2->v3: rebase to 3.16-rc2, consider return values from the
>>>> blk_mq_alloc_request leg of the blk_get_request callchain
>>>> (noted by Jeff), noted in the second patch changelog.
>>>>
>>>> blk_mq_queue_enter may return 0 or errno, which
>>>> blk_mq_alloc_request can propogate out via ERR_PTR.
>>>> __blk_mq_alloc_request doesn't include any blk_queue_dying
>>>> checks, so I'm assuming that its failures can be attributed
>>>> to -EWOULDBLOCK under !GFP_WAIT conditions.
>>>>
>>>> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
>>>> tags.
>>>>
>>>> Joe Lawrence (2):
>>>> block,scsi: verify return pointer from blk_get_request
>>>> block,scsi: fixup blk_get_request dead queue scenarios
>>>
>>> Jens,
>>>
>>> Did this patch set fall through the cracks again?
>>
>> Falling through the cracks implies that I meant to apply it and did not,
>> which was not the case.
>
> Sorry, I was mislead by our earlier conversation on this (mail inline
> below).
I changed my mind, it didn't feel fully baked to me.
>> But I think we're at the point now where I'm finally comfortable with
>> applying it. So, Joe, could you ensure that it applies to 3.17-rc2,
>> then I will roll it in to the updates for 3.18.
>
> Joe, you will have one hunk to modify for sure, in scsi_ioctl.c. A
> previous patch added a check for null, but ended up returning the wrong
> value (ENOMEM instead of ENODEV).
I have applied the first one, will look over the second one and hand
apply it. Seems the NULL return was completely removed, so we _should_
be ok on the IS_ERR() conversion, though that sort of thing always
worries me a little bit. A NULL return can quickly show up again, and
then they would all fail.
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 21:33 ` Jens Axboe
@ 2014-08-26 21:37 ` Jens Axboe
2014-08-26 22:01 ` Jeff Moyer
0 siblings, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2014-08-26 21:37 UTC (permalink / raw)
To: Jeff Moyer; +Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/26/2014 03:33 PM, Jens Axboe wrote:
> On 08/26/2014 03:27 PM, Jeff Moyer wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
>>
>>> On 08/26/2014 11:24 AM, Jeff Moyer wrote:
>>>> Joe Lawrence <joe.lawrence@stratus.com> writes:
>>>>
>>>>> v2->v3: rebase to 3.16-rc2, consider return values from the
>>>>> blk_mq_alloc_request leg of the blk_get_request callchain
>>>>> (noted by Jeff), noted in the second patch changelog.
>>>>>
>>>>> blk_mq_queue_enter may return 0 or errno, which
>>>>> blk_mq_alloc_request can propogate out via ERR_PTR.
>>>>> __blk_mq_alloc_request doesn't include any blk_queue_dying
>>>>> checks, so I'm assuming that its failures can be attributed
>>>>> to -EWOULDBLOCK under !GFP_WAIT conditions.
>>>>>
>>>>> v1->v2: incorporate Jeff's feedback in bsg_map_hdr() and Reviewed-by
>>>>> tags.
>>>>>
>>>>> Joe Lawrence (2):
>>>>> block,scsi: verify return pointer from blk_get_request
>>>>> block,scsi: fixup blk_get_request dead queue scenarios
>>>>
>>>> Jens,
>>>>
>>>> Did this patch set fall through the cracks again?
>>>
>>> Falling through the cracks implies that I meant to apply it and did not,
>>> which was not the case.
>>
>> Sorry, I was mislead by our earlier conversation on this (mail inline
>> below).
>
> I changed my mind, it didn't feel fully baked to me.
>
>>> But I think we're at the point now where I'm finally comfortable with
>>> applying it. So, Joe, could you ensure that it applies to 3.17-rc2,
>>> then I will roll it in to the updates for 3.18.
>>
>> Joe, you will have one hunk to modify for sure, in scsi_ioctl.c. A
>> previous patch added a check for null, but ended up returning the wrong
>> value (ENOMEM instead of ENODEV).
>
> I have applied the first one, will look over the second one and hand
> apply it. Seems the NULL return was completely removed, so we _should_
> be ok on the IS_ERR() conversion, though that sort of thing always
> worries me a little bit. A NULL return can quickly show up again, and
> then they would all fail.
Additionally, there's still quite a few places that call
blk_get_request() and don't check the error return if __GFP_WAIT is set.
Since most of the point of this is to fix segfaulting on queue dead
scenarios, why aren't they all converted?
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 21:37 ` Jens Axboe
@ 2014-08-26 22:01 ` Jeff Moyer
2014-08-27 14:07 ` Jens Axboe
2014-08-27 14:13 ` Joe Lawrence
0 siblings, 2 replies; 16+ messages in thread
From: Jeff Moyer @ 2014-08-26 22:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
Jens Axboe <axboe@kernel.dk> writes:
>> I have applied the first one, will look over the second one and hand
>> apply it. Seems the NULL return was completely removed, so we _should_
>> be ok on the IS_ERR() conversion, though that sort of thing always
>> worries me a little bit. A NULL return can quickly show up again, and
>> then they would all fail.
Well, we could guard against that with a BUG_ON in blk_get_request,
right?
> Additionally, there's still quite a few places that call
> blk_get_request() and don't check the error return if __GFP_WAIT is set.
> Since most of the point of this is to fix segfaulting on queue dead
> scenarios, why aren't they all converted?
Odd, I thought they all were converted last I checked. They definitely
should be.
-Jeff
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 22:01 ` Jeff Moyer
@ 2014-08-27 14:07 ` Jens Axboe
2014-08-27 15:33 ` Joe Lawrence
2014-08-27 15:50 ` Boaz Harrosh
2014-08-27 14:13 ` Joe Lawrence
1 sibling, 2 replies; 16+ messages in thread
From: Jens Axboe @ 2014-08-27 14:07 UTC (permalink / raw)
To: Jeff Moyer; +Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/26/2014 04:01 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
>>> I have applied the first one, will look over the second one and hand
>>> apply it. Seems the NULL return was completely removed, so we _should_
>>> be ok on the IS_ERR() conversion, though that sort of thing always
>>> worries me a little bit. A NULL return can quickly show up again, and
>>> then they would all fail.
>
> Well, we could guard against that with a BUG_ON in blk_get_request,
> right?
Yes, that might be a good idea.
>> Additionally, there's still quite a few places that call
>> blk_get_request() and don't check the error return if __GFP_WAIT is set.
>> Since most of the point of this is to fix segfaulting on queue dead
>> scenarios, why aren't they all converted?
>
> Odd, I thought they all were converted last I checked. They definitely
> should be.
drivers/ide/ide-park:issue_park_cmd() (patch oddly converts just the one?!)
drivers/ide/ide-pm.c:generic_ide_suspend()
drivers/ide/ide-pm.c:generic_ide_resume()
drivers/ide/ide-cd.c:ide_cd_queue_pc()
drivers/ide/ide-atapi.c:ide_queue_pc_tail()
drivers/ide/ide-ioctls.c:ide_cmd_ioctl()
drivers/ide/ide-ioctls.c:generic_drive_reset()
drivers/ide/ide-taskfile.c:ide_raw_taskfile()
drivers/ide/ide-tape.c:idetape_queue_rw_tail()
drivers/ide/ide-cd_ioctls.c:ide_cdrom_reset()
drivers/ide/ide-disk.c:set_multcount()
drivers/ide/ide-devsets.c:ide_devset_execute()
Why only one location in ide-park.c was converted and the rest of IDE
left untouched, I don't know. But there are definitely lots of them left
in there.
There's also a bug in osd_initiator.c, _init_blk_request(). We jump to
'out' for IS_ERR(req), which attempts to print or->request, which hasn't
been assigned yet. This is my primary concern with this patch, basically
every single of these call sites must be verified or it will do more
harm than good. Have they been?
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-26 22:01 ` Jeff Moyer
2014-08-27 14:07 ` Jens Axboe
@ 2014-08-27 14:13 ` Joe Lawrence
1 sibling, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-08-27 14:13 UTC (permalink / raw)
To: Jeff Moyer; +Cc: Jens Axboe, linux-kernel, Jiri Kosina, Boaz Harrosh
On Tue, 26 Aug 2014 18:01:23 -0400
Jeff Moyer <jmoyer@redhat.com> wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
> >> I have applied the first one, will look over the second one and hand
> >> apply it. Seems the NULL return was completely removed, so we _should_
> >> be ok on the IS_ERR() conversion, though that sort of thing always
> >> worries me a little bit. A NULL return can quickly show up again, and
> >> then they would all fail.
>
> Well, we could guard against that with a BUG_ON in blk_get_request,
> right?
Since the two error cases (ENOMEM and ENODEV) are rare exceptions,
could the reintroduction of a NULL return slip by a quick bench test?
> > Additionally, there's still quite a few places that call
> > blk_get_request() and don't check the error return if __GFP_WAIT is set.
> > Since most of the point of this is to fix segfaulting on queue dead
> > scenarios, why aren't they all converted?
>
> Odd, I thought they all were converted last I checked. They definitely
> should be.
I largely left the ide-*.c files alone. The only guy who bothered
checking the blk_get_request return was ide-park, which I updated with
IS_ERR. If the others should be hardened (I assumed these were mostly
deprecated drivers), I can add that code in a v3.
Jens, were there other callers that were missed? I'm using cscope to
find them, so perhaps I inadvertently filtered a file out of the search.
Regards,
-- Joe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-27 14:07 ` Jens Axboe
@ 2014-08-27 15:33 ` Joe Lawrence
2014-08-27 15:50 ` Boaz Harrosh
1 sibling, 0 replies; 16+ messages in thread
From: Joe Lawrence @ 2014-08-27 15:33 UTC (permalink / raw)
To: Jens Axboe; +Cc: Jeff Moyer, linux-kernel, Jiri Kosina, Boaz Harrosh
On Wed, 27 Aug 2014 08:07:29 -0600
Jens Axboe <axboe@kernel.dk> wrote:
> On 08/26/2014 04:01 PM, Jeff Moyer wrote:
> >> Additionally, there's still quite a few places that call
> >> blk_get_request() and don't check the error return if __GFP_WAIT is set.
> >> Since most of the point of this is to fix segfaulting on queue dead
> >> scenarios, why aren't they all converted?
> >
> > Odd, I thought they all were converted last I checked. They definitely
> > should be.
>
> drivers/ide/ide-park:issue_park_cmd() (patch oddly converts just the one?!)
> drivers/ide/ide-pm.c:generic_ide_suspend()
> drivers/ide/ide-pm.c:generic_ide_resume()
> drivers/ide/ide-cd.c:ide_cd_queue_pc()
> drivers/ide/ide-atapi.c:ide_queue_pc_tail()
> drivers/ide/ide-ioctls.c:ide_cmd_ioctl()
> drivers/ide/ide-ioctls.c:generic_drive_reset()
> drivers/ide/ide-taskfile.c:ide_raw_taskfile()
> drivers/ide/ide-tape.c:idetape_queue_rw_tail()
> drivers/ide/ide-cd_ioctls.c:ide_cdrom_reset()
> drivers/ide/ide-disk.c:set_multcount()
> drivers/ide/ide-devsets.c:ide_devset_execute()
>
> Why only one location in ide-park.c was converted and the rest of IDE
> left untouched, I don't know. But there are definitely lots of them left
> in there.
These files didn't seem to have much recent development going on, so my
thinking was that if one were to bother checking the return from
blk_get_request, I would update it. If the code didn't include such
check in the first place, then I let it be.
> There's also a bug in osd_initiator.c, _init_blk_request(). We jump to
> 'out' for IS_ERR(req), which attempts to print or->request, which hasn't
> been assigned yet. This is my primary concern with this patch, basically
> every single of these call sites must be verified or it will do more
> harm than good. Have they been?
So the _init_blk_request bug has been there since c29b70f6 when the
_make_request wrapper was introduced -- I missed that when inspecting
the surrounding areas that the patch modified.
Given the scope of the changes, I agree that the probability of
introducing another bug is real. I think either you or James suggested
splitting this fix into two parts: the first patch avoiding the crash I
originally saw, the second modifying ABI to propagate out additional
information required to discern why blk_get_request failed.
As I mentioned in [1], the call chain into blk_get_request is pretty
wide. I did my best trying to hunt down the callers callers callers,
etc. to figure out how the returns are handled. Without testing every
single site, I can't be 100% sure. In the end, I'd be happy with patch
1 to avoid the original crash report. Patch 2 was the for
truth-and-beauty approach. Whether you think it's worth the risk is a
judgment call on your part.
[1] http://www.spinics.net/lists/kernel/msg1776335.html
-- Joe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-27 14:07 ` Jens Axboe
2014-08-27 15:33 ` Joe Lawrence
@ 2014-08-27 15:50 ` Boaz Harrosh
2014-08-27 16:05 ` Jens Axboe
1 sibling, 1 reply; 16+ messages in thread
From: Boaz Harrosh @ 2014-08-27 15:50 UTC (permalink / raw)
To: Jens Axboe, Jeff Moyer
Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/27/2014 05:07 PM, Jens Axboe wrote:
> On 08/26/2014 04:01 PM, Jeff Moyer wrote:
>> Jens Axboe <axboe@kernel.dk> writes:
<>
> There's also a bug in osd_initiator.c, _init_blk_request(). We jump to
> 'out' for IS_ERR(req), which attempts to print or->request, which hasn't
> been assigned yet.
You mean this code:
req = _make_request(q, has_out, has_out ? &or->out : &or->in, flags);
if (IS_ERR(req)) {
ret = PTR_ERR(req);
goto out;
}
or->request = req;
But _make_request used to already return -ENOMEM as a pointer in req
So if this is a bug it was not introduced by this patch.
And it is not a bug at all the print prints the pointer. The all of
this code assumes osd_request was allocated ZERO set. the print of %p
is fine with NULLs (and any number for that matter)
> This is my primary concern with this patch, basically
> every single of these call sites must be verified or it will do more
> harm than good. Have they been?
>
I have reviewed this patch for osd part, it is fine
Thanks
Boaz
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
2014-08-27 15:50 ` Boaz Harrosh
@ 2014-08-27 16:05 ` Jens Axboe
0 siblings, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2014-08-27 16:05 UTC (permalink / raw)
To: Boaz Harrosh, Jeff Moyer
Cc: Joe Lawrence, linux-kernel, Jiri Kosina, Boaz Harrosh
On 08/27/2014 09:50 AM, Boaz Harrosh wrote:
> On 08/27/2014 05:07 PM, Jens Axboe wrote:
>> On 08/26/2014 04:01 PM, Jeff Moyer wrote:
>>> Jens Axboe <axboe@kernel.dk> writes:
> <>
>> There's also a bug in osd_initiator.c, _init_blk_request(). We jump to
>> 'out' for IS_ERR(req), which attempts to print or->request, which hasn't
>> been assigned yet.
>
> You mean this code:
> req = _make_request(q, has_out, has_out ? &or->out : &or->in, flags);
> if (IS_ERR(req)) {
> ret = PTR_ERR(req);
> goto out;
> }
>
> or->request = req;
>
> But _make_request used to already return -ENOMEM as a pointer in req
> So if this is a bug it was not introduced by this patch.
Joe brought this up too, I guess it's been there before this patch.
> And it is not a bug at all the print prints the pointer. The all of
> this code assumes osd_request was allocated ZERO set. the print of %p
> is fine with NULLs (and any number for that matter)
If or->request is NULL initialized, it's fine. I didn't track the path
to check if it was garbage or NULL.
> I have reviewed this patch for osd part, it is fine
Thanks!
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-08-27 16:05 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-02 19:35 [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 1/2] block,scsi: verify return pointer from blk_get_request Joe Lawrence
2014-07-02 19:35 ` [PATCH v3 2/2] block,scsi: fixup blk_get_request dead queue scenarios Joe Lawrence
2014-07-02 20:26 ` [PATCH v3 0/2] " Joe Lawrence
2014-08-26 17:24 ` Jeff Moyer
2014-08-26 21:19 ` Jens Axboe
2014-08-26 21:24 ` Jens Axboe
2014-08-26 21:27 ` Jeff Moyer
2014-08-26 21:33 ` Jens Axboe
2014-08-26 21:37 ` Jens Axboe
2014-08-26 22:01 ` Jeff Moyer
2014-08-27 14:07 ` Jens Axboe
2014-08-27 15:33 ` Joe Lawrence
2014-08-27 15:50 ` Boaz Harrosh
2014-08-27 16:05 ` Jens Axboe
2014-08-27 14:13 ` Joe Lawrence
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).