Re: [PATCH] 2.6.0 NBD driver: remove send/recieve race for request

[Lou Langholtz <ldl@aros.net>]

Paul Clements wrote:

> . . .
>
>>Except that in the error case, the send basically didn't succeed. So no
>>need to worry about recieving a reply and no race possibility in that case.
>>    
>>
>
>As long as the request is on the queue, it is possible for nbd-client to
>die, thus freeing the request (via nbd_clear_que -> nbd_end_request),
>and leaving us with a race between the free and do_nbd_request()
>accessing the request structure.
>
>--
>Paul
>  
>
Quite right. I missed that case in this last patch (when nbd_do_it has 
returned and NBD_DO_IT is about to call nbd_clear_que [1]). Just moving 
the errors increment (near the end of nbd_send_req) to within the 
semaphore protected region would fix this particular case. An even 
larger race window exists with the request getting free'd when 
nbd-client is used to disconnect in which it calls NBD_CLEAR_QUE before 
NBD_DISCONNECT [2]. In this case, moving the errors increment doesn't 
help of course since the nbd_clear_queue in 2.6.0-test2 doesn't bother 
to check the tx_lock semaphore anyway. I believe reference counting the 
request (as you suggest) would protect against both these windows though.

It's ironic that I'd fixed both these races [1+2] a ways back in an 
earlier patch and had forgotten about these cases in this last patch I 
submitted. The earlier patch p6.2 against linux-2.5.73 looks about 
right. By that patch, the call to clear the queue before NBD_DO_IT 
returned was gone and it made sure the clear_queue functionality would 
return -EBUSY if invoked when the socket wasn't NULL (and potentially 
while nbd_send_req functionality could be called). Not that I'm arguing 
we should roll in these ealier patches again. That would re-introduce 
the compatibility break which I wouldn't want either.

Will you be working on closing the other clear-queue race also then? 
Here's the comments I shared on this in one of these earlier patches 
that didn't make it into the mainstream distro (from patch #7):

                /*

                 * Don't allow queue to be cleared while device is running!

                 * Device must be stopped (disconnected) first. Otherwise

                 * clearing is meaningless & can lock up processes: it's a

                 * race with users who may queue up more requests after the

                 * clearing is done that may then never be freed till the

                 * system reboots or clear que is run again which just

                 * opens the race yet again.

                 */