Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
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 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 ? >out : >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/27/2014 05:07 PM, Jens Axboe wrote:
> On 08/26/2014 04:01 PM, Jeff Moyer wrote:
>> Jens Axboe 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 ? >out : >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On Wed, 27 Aug 2014 08:07:29 -0600 Jens Axboe 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On Tue, 26 Aug 2014 18:01:23 -0400 Jeff Moyer wrote: > Jens Axboe 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 04:01 PM, Jeff Moyer wrote: > Jens Axboe 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 04:01 PM, Jeff Moyer wrote: Jens Axboe ax...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On Tue, 26 Aug 2014 18:01:23 -0400 Jeff Moyer jmo...@redhat.com wrote: Jens Axboe ax...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On Wed, 27 Aug 2014 08:07:29 -0600 Jens Axboe ax...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/27/2014 05:07 PM, Jens Axboe wrote:
On 08/26/2014 04:01 PM, Jeff Moyer wrote:
Jens Axboe ax...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
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 ax...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
Jens Axboe 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 03:33 PM, Jens Axboe wrote: > On 08/26/2014 03:27 PM, Jeff Moyer wrote: >> Jens Axboe writes: >> >>> On 08/26/2014 11:24 AM, Jeff Moyer wrote: Joe Lawrence 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 03:27 PM, Jeff Moyer wrote: > Jens Axboe writes: > >> On 08/26/2014 11:24 AM, Jeff Moyer wrote: >>> Joe Lawrence 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
Jens Axboe writes: > On 08/26/2014 11:24 AM, Jeff Moyer wrote: >> Joe Lawrence 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 Subject: Re: [PATCH v2 0/2] block,scsi: fixup blk_get_request dead queue scenarios To: Jeff Moyer , Joe Lawrence 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 03:19 PM, Jens Axboe wrote: > On 08/26/2014 11:24 AM, Jeff Moyer wrote: >> Joe Lawrence 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 11:24 AM, Jeff Moyer wrote: > Joe Lawrence 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
Joe Lawrence 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
Joe Lawrence joe.lawre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 11:24 AM, Jeff Moyer wrote: Joe Lawrence joe.lawre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 03:19 PM, Jens Axboe wrote: On 08/26/2014 11:24 AM, Jeff Moyer wrote: Joe Lawrence joe.lawre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
Jens Axboe ax...@kernel.dk writes: On 08/26/2014 11:24 AM, Jeff Moyer wrote: Joe Lawrence joe.lawre...@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 ax...@kernel.dk Subject: Re: [PATCH v2 0/2] block,scsi: fixup blk_get_request dead queue scenarios To: Jeff Moyer jmo...@redhat.com, Joe Lawrence joe.lawre...@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.lawre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 03:27 PM, Jeff Moyer wrote: Jens Axboe ax...@kernel.dk writes: On 08/26/2014 11:24 AM, Jeff Moyer wrote: Joe Lawrence joe.lawre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
On 08/26/2014 03:33 PM, Jens Axboe wrote: On 08/26/2014 03:27 PM, Jeff Moyer wrote: Jens Axboe ax...@kernel.dk writes: On 08/26/2014 11:24 AM, Jeff Moyer wrote: Joe Lawrence joe.lawre...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
Jens Axboe ax...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
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 FileFunction 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_hdr272 rq = blk_get_request(q, rw, GFP_KERNEL);ERR_PTR propagated, see callers below 2 bsg.c bsg_map_hdr287 next_rq = blk_get_request(q, READ, GFP_KERNEL); ERR_PTR propagated, see callers below 3 scsi_ioctl.csg_io 310 rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);PTR_ERR propagated, see callers below 4 scsi_ioctl.csg_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.caction 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.cide_cd_queue_pc444 rq = blk_get_request(drive->queue, write, __GFP_WAIT); don't care, deprecated driver c ide-cd_ioctl.c ide_cdrom_reset306 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.cide_cmd_ioctl 128 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver g ide-ioctls.cgeneric_drive_reset224 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.cgeneric_ide_suspend 21 rq = blk_get_request(drive->queue, READ, __GFP_WAIT); don't care, deprecated driver k ide-pm.cgeneric_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_req276 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
[PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 0/2] block,scsi: fixup blk_get_request dead queue scenarios
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 FileFunction 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_hdr272 rq = blk_get_request(q, rw, GFP_KERNEL);ERR_PTR propagated, see callers below 2 bsg.c bsg_map_hdr287 next_rq = blk_get_request(q, READ, GFP_KERNEL); ERR_PTR propagated, see callers below 3 scsi_ioctl.csg_io 310 rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);PTR_ERR propagated, see callers below 4 scsi_ioctl.csg_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.caction 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.cide_cd_queue_pc444 rq = blk_get_request(drive-queue, write, __GFP_WAIT); don't care, deprecated driver c ide-cd_ioctl.c ide_cdrom_reset306 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.cide_cmd_ioctl 128 rq = blk_get_request(drive-queue, READ, __GFP_WAIT); don't care, deprecated driver g ide-ioctls.cgeneric_drive_reset224 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.cgeneric_ide_suspend 21 rq = blk_get_request(drive-queue, READ, __GFP_WAIT); don't care, deprecated driver k ide-pm.cgeneric_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_req276 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
