nbd: wait all read/write requests finished when shutdowning nbd socket

If the socket fd is shutdown, there may be some data which is received before
shutdown. We will read the data and do read/write in nbd_trip(). But the exp's
blk is NULL, and it will cause qemu crashed.

Reported-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 nbd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On 16/09/2015 09:16, Wen Congyang wrote:
> If the socket fd is shutdown, there may be some data which is received before
> shutdown. We will read the data and do read/write in nbd_trip(). But the exp's
> blk is NULL, and it will cause qemu crashed.
> 
> Reported-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  nbd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/nbd.c b/nbd.c
> index 06b501b..d8586a1 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -1132,6 +1132,7 @@ void nbd_export_close(NBDExport *exp)
>      nbd_export_set_name(exp, NULL);
>      nbd_export_put(exp);
>      if (exp->blk) {
> +        bdrv_drain(blk_bs(exp->blk));
>          blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
>                                          blk_aio_detach, exp);
>          blk_unref(exp->blk);

I think a better fix is to move the whole "if" to nbd_export_put.  In
fact, nbd_export_close is wrong because exp can be freed by
nbd_export_close's call to nbd_export_put.

> @@ -1305,6 +1306,14 @@ static void nbd_trip(void *opaque)
>          goto invalid_request;
>      }
>  
> +    if (client->closing) {
> +        /*
> +         * The client may be closed when we are blocked in
> +         * nbd_co_receive_request()
> +         */
> +        goto done;
> +    }

This is okay.

Paolo

>      switch (command) {
>      case NBD_CMD_READ:
>          TRACE("Request type is READ");
>

On 09/16/2015 04:17 PM, Paolo Bonzini wrote:
> 
> 
> On 16/09/2015 09:16, Wen Congyang wrote:
>> If the socket fd is shutdown, there may be some data which is received before
>> shutdown. We will read the data and do read/write in nbd_trip(). But the exp's
>> blk is NULL, and it will cause qemu crashed.
>>
>> Reported-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>  nbd.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/nbd.c b/nbd.c
>> index 06b501b..d8586a1 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -1132,6 +1132,7 @@ void nbd_export_close(NBDExport *exp)
>>      nbd_export_set_name(exp, NULL);
>>      nbd_export_put(exp);
>>      if (exp->blk) {
>> +        bdrv_drain(blk_bs(exp->blk));
>>          blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
>>                                          blk_aio_detach, exp);
>>          blk_unref(exp->blk);
> 
> I think a better fix is to move the whole "if" to nbd_export_put.  In
> fact, nbd_export_close is wrong because exp can be freed by
> nbd_export_close's call to nbd_export_put.

OK, I will fix it in the next version.

Thanks
Wen Congyang


> 
>> @@ -1305,6 +1306,14 @@ static void nbd_trip(void *opaque)
>>          goto invalid_request;
>>      }
>>  
>> +    if (client->closing) {
>> +        /*
>> +         * The client may be closed when we are blocked in
>> +         * nbd_co_receive_request()
>> +         */
>> +        goto done;
>> +    }
> 
> This is okay.
> 
> Paolo
> 
>>      switch (command) {
>>      case NBD_CMD_READ:
>>          TRACE("Request type is READ");
>>
> .
>

On 16/09/2015 10:22, Wen Congyang wrote:
>>> >>      if (exp->blk) {
>>> >> +        bdrv_drain(blk_bs(exp->blk));
>>> >>          blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
>>> >>                                          blk_aio_detach, exp);
>>> >>          blk_unref(exp->blk);
>> > 
>> > I think a better fix is to move the whole "if" to nbd_export_put.  In
>> > fact, nbd_export_close is wrong because exp can be freed by
>> > nbd_export_close's call to nbd_export_put.
> OK, I will fix it in the next version.

To be clear, the bug is pre-existing (commit 38b54b6, "nbd: use
BlockDriverState refcnt", 2013-08-23).

Paolo