linux-kernel - Re: [PATCH] Fix race in 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]

Date:	Fri, 08 Aug 2014 08:24:28 -0600
From:	Jens Axboe <axboe@...nel.dk>
To:	Jörn Engel <joern@...fs.org>,
	oren@...estorage.com
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix race in get_request()

On 08/07/2014 06:54 PM, Jörn Engel wrote:
> Hello Jens!
> 
> I came across the below while investigating some other problem.
> Something here doesn't seem right.  This looks like an obvious bug and
> something roughly along the lines of my patch would fix it.  But I
> must be in the wrong decade to find such a bug in the block layer.
> 
> Is this for real?  Or if not, what am I missing?
> 
> Jörn
> 
> --
> 
> If __get_request() returns NULL, get_request will call
> prepare_to_wait_exclusive() followed by io_schedule().  Not rechecking
> the sleep condition after prepare_to_wait_exclusive() leaves a race
> where the condition changes before prepare_to_wait_exclusive(), but
> not after and accordingly this thread never gets woken up.
> 
> The race must be exceedingly hard to hit, otherwise I cannot explain how
> such a classic race could outlive the last millenium.

I think that is a genuine bug, it's just extremely hard to hit in real
life. It has probably only potentially ever triggered in the cases where
we are so out of memory that a blocking ~300b alloc fails, and Linux
generally shits itself pretty hard when it gets to that stage anyway...
And for the bug to be critical, you'd need this to happen for a device
that otherwise has no IO pending, since you'd get woken up by the next
completed request anyway.

> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3275353957f0..00aa6c7abe5a 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1068,6 +1068,11 @@ retry:
>  
>  	trace_block_sleeprq(q, bio, rw_flags & 1);
>  
> +	rq = __get_request(rl, rw_flags, bio, gfp_mask);
> +	if (rq) {
> +		finish_wait(&rl->wait[is_sync], &wait);
> +		return rq;
> +	}

The extra __get_request() call should go before the
trace_block_sleeprq(), however. I'll rejuggle that when applying.

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