Skip to contentSkip to conversation

Fix race in POSIX AIO emulation

PATCH Applied Jan Kiszka 2 participants 3 messages Dec 19, 2008 – Jan 13, 2009

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 Fix race in POSIX AIO emulation

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.

---

 posix-aio-compat.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)
posix-aio-compat.c +2 -7
index 92ec234..c919e3b 100644
@@ -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);
 
author
#1 lore raw
Quoted 38 lines · 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.

---

 posix-aio-compat.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)
posix-aio-compat.c +1 -4
index 92ec234..c919e3b 100644
@@ -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;
-

posix-aio-compat.c static void *aio_thread(void *unused) · line 96

Quoted 10 lines
posix-aio-compat.c +1 -3
-            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

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux
#2 lore raw
Quoted 3 lines · Jan Kiszka

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

Quoted 3 lines

Jan