Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race

["Roger Pau Monné" <roger.pau@citrix.com>]

On 04/02/14 09:02, Jan Beulich wrote:
>>>> On 03.02.14 at 17:58, Roger Pau Monné<roger.pau@citrix.com> wrote:
>> On 29/01/14 09:52, Jan Beulich wrote:
>>>>>> On 28.01.14 at 18:43, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>>> +		free_req(blkif, pending_req);
>>>> +		/*
>>>> +		 * Make sure the request is freed before releasing blkif,
>>>> +		 * or there could be a race between free_req and the
>>>> +		 * cleanup done in xen_blkif_free during shutdown.
>>>> +		 *
>>>> +		 * NB: The fact that we might try to wake up pending_free_wq
>>>> +		 * before drain_complete (in case there's a drain going on)
>>>> +		 * it's not a problem with our current implementation
>>>> +		 * because we can assure there's no thread waiting on
>>>> +		 * pending_free_wq if there's a drain going on, but it has
>>>> +		 * to be taken into account if the current model is changed.
>>>> +		 */
>>>> +		xen_blkif_put(blkif);
>>>> +		if (atomic_read(&blkif->refcnt) <= 2) {
>>>> +			if (atomic_read(&blkif->drain))
>>>> +				complete(&blkif->drain_complete);
>>>>  		}
>>>> -		free_req(pending_req->blkif, pending_req);
>>>>  	}
>>>>  }
>>>
>>> The put is still too early imo - you're explicitly accessing field in the
>>> structure immediately afterwards. This may not be an issue at
>>> present, but I think it's at least a latent one.
>>>
>>> Apart from that, the two if()s would - at least to me - be more
>>> clear if combined into one.
>>
>> In order to get rid of the race I had to introduce yet another atomic_t 
>> in xen_blkif struct, which is something I don't really like, but I 
>> could not see any other way to solve this. If that's fine I will resend 
>> the series, here is the reworked patch:
> 
> Mind explaining why you can't simply move the xen_blkif_put()
> down between the if() and the free_ref().

You mean doing something like:

if (atomic_read(&blkif->refcnt) <= 3) {
	if (atomic_read(&blkif->drain))
		complete(&blkif->drain_complete);
}
xen_blkif_put(blkif);
free_req(blkif, pending_req);

This would not be safe, because we are comparing refcnt before
decrementing it, and also we are not doing it atomically (the dec and
compare). If for example we have two inflight requests, both could
perform the atomic_read of refcnt, see there's still another inflight
request, and then both decrement, without anyone calling complete.

There's also the issue that with this approach as we are freeing the
request after we have put blkif, which is a race with the cleanup being
done in xen_blkif_free.

Roger.