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/
