[Qemu-devel] [PATCH] Fix race in POSIX AIO emulation

[Qemu-devel] [PATCH] Fix race in POSIX AIO emulation

[Jan Kiszka]

When we cancel an AIO request that is already being processed by
aio_thread, qemu_paio_cancel should return QEMU_PAIO_NOTCANCELED as long
as aio_thread isn't done with this request. But as the latter currently
updates aiocb->ret after every block of the request, we may report
QEMU_PAIO_ALLDONE too early.

Futhermore, in case some zero-length request should have been queued,
aiocb->ret is never set to != -EINPROGRESS and callers like
raw_aio_cancel could get stuck in an endless loop.

Fix those issues by updating aiocb->ret _after_ the request has been
fully processed. This also simplifies the locking.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 posix-aio-compat.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 92ec234..c919e3b 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -81,21 +81,16 @@ static void *aio_thread(void *unused)
             if (len == -1 && errno == EINTR)
                 continue;
             else if (len == -1) {
-                pthread_mutex_lock(&lock);
-                aiocb->ret = -errno;
-                pthread_mutex_unlock(&lock);
+                offset = -errno;
                 break;
             } else if (len == 0)
                 break;
 
             offset += len;
-
-            pthread_mutex_lock(&lock);
-            aiocb->ret = offset;
-            pthread_mutex_unlock(&lock);
         }
 
         pthread_mutex_lock(&lock);
+        aiocb->ret = offset;
         idle_threads++;
         pthread_mutex_unlock(&lock);
 

[Qemu-devel] Re: [PATCH] Fix race in POSIX AIO emulation

[Jan Kiszka]

Jan Kiszka wrote:
> When we cancel an AIO request that is already being processed by
> aio_thread, qemu_paio_cancel should return QEMU_PAIO_NOTCANCELED as long
> as aio_thread isn't done with this request. But as the latter currently
> updates aiocb->ret after every block of the request, we may report
> QEMU_PAIO_ALLDONE too early.
> 
> Futhermore, in case some zero-length request should have been queued,
> aiocb->ret is never set to != -EINPROGRESS and callers like
> raw_aio_cancel could get stuck in an endless loop.
> 
> Fix those issues by updating aiocb->ret _after_ the request has been
> fully processed. This also simplifies the locking.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  posix-aio-compat.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index 92ec234..c919e3b 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -81,21 +81,16 @@ static void *aio_thread(void *unused)
>              if (len == -1 && errno == EINTR)
>                  continue;
>              else if (len == -1) {
> -                pthread_mutex_lock(&lock);
> -                aiocb->ret = -errno;
> -                pthread_mutex_unlock(&lock);
> +                offset = -errno;
>                  break;
>              } else if (len == 0)
>                  break;
>  
>              offset += len;
> -
> -            pthread_mutex_lock(&lock);
> -            aiocb->ret = offset;
> -            pthread_mutex_unlock(&lock);
>          }
>  
>          pthread_mutex_lock(&lock);
> +        aiocb->ret = offset;
>          idle_threads++;
>          pthread_mutex_unlock(&lock);
>  
> 

Problem still exists, patch still applies - but no feedback yet.
Forgotten under the Christmas tree?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH] Fix race in POSIX AIO emulation

[Anthony Liguori]

Jan Kiszka wrote:
> Problem still exists, patch still applies - but no feedback yet.
> Forgotten under the Christmas tree?
>   

Always wonderful to find an extra present :-)

Applied.  Thanks.

Regards,

Anthony Liguori

> Jan
>
>