[Qemu-devel] [PATCH] nbd: wait all read/write requests finished when shutdowning nbd socket

[Qemu-devel] [PATCH] nbd: wait all read/write requests finished when shutdowning nbd socket

[Wen Congyang]

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);
@@ -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;
+    }
+
     switch (command) {
     case NBD_CMD_READ:
         TRACE("Request type is READ");
-- 
2.4.3

Re: [Qemu-devel] [PATCH] nbd: wait all read/write requests finished when shutdowning nbd socket

[Paolo Bonzini]

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");
> 

Re: [Qemu-devel] [PATCH] nbd: wait all read/write requests finished when shutdowning nbd socket

[Wen Congyang]

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");
>>
> .
> 

Re: [Qemu-devel] [PATCH] nbd: wait all read/write requests finished when shutdowning nbd socket

[Paolo Bonzini]

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