linux-kernel - Re: [PATCH v3 0/2] block,scsi: fixup blk_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53FDE621.2050009@kernel.dk>
Date:	Wed, 27 Aug 2014 08:07:29 -0600
From:	Jens Axboe <axboe@...nel.dk>
To:	Jeff Moyer <jmoyer@...hat.com>
CC:	Joe Lawrence <joe.lawrence@...atus.com>,
	linux-kernel@...r.kernel.org, Jiri Kosina <jkosina@...e.cz>,
	Boaz Harrosh <bharrosh@...asas.com>
Subject: 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 <axboe@...nel.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/