[Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups

[Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups

[Paolo Bonzini]

The first fixes a NULL-pointer dereference that was reported by
Coverity (so definitely for 2.8).  The second is a small simplification.

Paolo Bonzini (2):
  aio-posix: avoid NULL pointer dereference in aio_epoll_update
  aio-posix: simplify aio_epoll_update

 aio-posix.c | 56 ++++++++++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update

[Paolo Bonzini]

aio_epoll_update dereferences parameter "node", but it could have been NULL
if deleting an fd handler that was not registered in the first place.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 aio-posix.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index 4ef34dd..ec908f7 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -217,21 +217,24 @@ void aio_set_fd_handler(AioContext *ctx,
 
     /* Are we deleting the fd handler? */
     if (!io_read && !io_write) {
-        if (node) {
-            g_source_remove_poll(&ctx->source, &node->pfd);
-
-            /* If the lock is held, just mark the node as deleted */
-            if (ctx->walking_handlers) {
-                node->deleted = 1;
-                node->pfd.revents = 0;
-            } else {
-                /* Otherwise, delete it for real.  We can't just mark it as
-                 * deleted because deleted nodes are only cleaned up after
-                 * releasing the walking_handlers lock.
-                 */
-                QLIST_REMOVE(node, node);
-                deleted = true;
-            }
+        if (node == NULL) {
+            return;
+        }
+
+        node->pfd.events = 0;
+        g_source_remove_poll(&ctx->source, &node->pfd);
+
+        /* If the lock is held, just mark the node as deleted */
+        if (ctx->walking_handlers) {
+            node->deleted = 1;
+            node->pfd.revents = 0;
+        } else {
+            /* Otherwise, delete it for real.  We can't just mark it as
+             * deleted because deleted nodes are only cleaned up after
+             * releasing the walking_handlers lock.
+             */
+            QLIST_REMOVE(node, node);
+            deleted = true;
         }
     } else {
         if (node == NULL) {
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

On Mon, Nov 07, 2016 at 10:33:33AM +0100, Paolo Bonzini wrote:
> aio_epoll_update dereferences parameter "node", but it could have been NULL
> if deleting an fd handler that was not registered in the first place.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  aio-posix.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index 4ef34dd..ec908f7 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -217,21 +217,24 @@ void aio_set_fd_handler(AioContext *ctx,
>  
>      /* Are we deleting the fd handler? */
>      if (!io_read && !io_write) {
> -        if (node) {
> -            g_source_remove_poll(&ctx->source, &node->pfd);
> -
> -            /* If the lock is held, just mark the node as deleted */
> -            if (ctx->walking_handlers) {
> -                node->deleted = 1;
> -                node->pfd.revents = 0;
> -            } else {
> -                /* Otherwise, delete it for real.  We can't just mark it as
> -                 * deleted because deleted nodes are only cleaned up after
> -                 * releasing the walking_handlers lock.
> -                 */
> -                QLIST_REMOVE(node, node);
> -                deleted = true;
> -            }
> +        if (node == NULL) {
> +            return;
> +        }
> +
> +        node->pfd.events = 0;

^--- is this left over from debugging...

> +        g_source_remove_poll(&ctx->source, &node->pfd);
> +
> +        /* If the lock is held, just mark the node as deleted */
> +        if (ctx->walking_handlers) {
> +            node->deleted = 1;
> +            node->pfd.revents = 0;

...the original code clears revents here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update

[Paolo Bonzini]

On 07/11/2016 15:58, Stefan Hajnoczi wrote:
> On Mon, Nov 07, 2016 at 10:33:33AM +0100, Paolo Bonzini wrote:
>> aio_epoll_update dereferences parameter "node", but it could have been NULL
>> if deleting an fd handler that was not registered in the first place.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  aio-posix.c | 33 ++++++++++++++++++---------------
>>  1 file changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/aio-posix.c b/aio-posix.c
>> index 4ef34dd..ec908f7 100644
>> --- a/aio-posix.c
>> +++ b/aio-posix.c
>> @@ -217,21 +217,24 @@ void aio_set_fd_handler(AioContext *ctx,
>>  
>>      /* Are we deleting the fd handler? */
>>      if (!io_read && !io_write) {
>> -        if (node) {
>> -            g_source_remove_poll(&ctx->source, &node->pfd);
>> -
>> -            /* If the lock is held, just mark the node as deleted */
>> -            if (ctx->walking_handlers) {
>> -                node->deleted = 1;
>> -                node->pfd.revents = 0;
>> -            } else {
>> -                /* Otherwise, delete it for real.  We can't just mark it as
>> -                 * deleted because deleted nodes are only cleaned up after
>> -                 * releasing the walking_handlers lock.
>> -                 */
>> -                QLIST_REMOVE(node, node);
>> -                deleted = true;
>> -            }
>> +        if (node == NULL) {
>> +            return;
>> +        }
>> +
>> +        node->pfd.events = 0;
> 
> ^--- is this left over from debugging...

No, it's left over from solving conflicts.  This is an old patch that
got lost.  Will send v2, thanks for the review!

Paolo

>> +        g_source_remove_poll(&ctx->source, &node->pfd);
>> +
>> +        /* If the lock is held, just mark the node as deleted */
>> +        if (ctx->walking_handlers) {
>> +            node->deleted = 1;
>> +            node->pfd.revents = 0;
> 
> ...the original code clears revents here?
> 

[Qemu-devel] [PATCH 2/2] aio-posix: simplify aio_epoll_update

[Paolo Bonzini]

Extract common code out of the "if".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 aio-posix.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index ec908f7..d54553d 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -81,29 +81,22 @@ static void aio_epoll_update(AioContext *ctx, AioHandler *node, bool is_new)
 {
     struct epoll_event event;
     int r;
+    int ctl;
 
     if (!ctx->epoll_enabled) {
         return;
     }
     if (!node->pfd.events) {
-        r = epoll_ctl(ctx->epollfd, EPOLL_CTL_DEL, node->pfd.fd, &event);
-        if (r) {
-            aio_epoll_disable(ctx);
-        }
+        ctl = EPOLL_CTL_DEL;
     } else {
         event.data.ptr = node;
         event.events = epoll_events_from_pfd(node->pfd.events);
-        if (is_new) {
-            r = epoll_ctl(ctx->epollfd, EPOLL_CTL_ADD, node->pfd.fd, &event);
-            if (r) {
-                aio_epoll_disable(ctx);
-            }
-        } else {
-            r = epoll_ctl(ctx->epollfd, EPOLL_CTL_MOD, node->pfd.fd, &event);
-            if (r) {
-                aio_epoll_disable(ctx);
-            }
-        }
+        ctl = is_new ? EPOLL_CTL_ADD : EPOLL_CTL_MOD;
+    }
+
+    r = epoll_ctl(ctx->epollfd, ctl, node->pfd.fd, &event);
+    if (r) {
+        aio_epoll_disable(ctx);
     }
 }
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] aio-posix: simplify aio_epoll_update

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

On Mon, Nov 07, 2016 at 10:33:34AM +0100, Paolo Bonzini wrote:
> Extract common code out of the "if".
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  aio-posix.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups

[Fam Zheng]

On Mon, 11/07 10:33, Paolo Bonzini wrote:
> The first fixes a NULL-pointer dereference that was reported by
> Coverity (so definitely for 2.8).  The second is a small simplification.
> 
> Paolo Bonzini (2):
>   aio-posix: avoid NULL pointer dereference in aio_epoll_update
>   aio-posix: simplify aio_epoll_update

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH for-2.8 v2 0/2] aio-posix: epoll cleanups

[Paolo Bonzini]

The first fixes a NULL-pointer dereference that was reported by
Coverity (so definitely for 2.8).  The second is a small simplification.

Paolo Bonzini (2):
  aio-posix: avoid NULL pointer dereference in aio_epoll_update
  aio-posix: simplify aio_epoll_update

 aio-posix.c | 55 +++++++++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 30 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update

[Paolo Bonzini]

aio_epoll_update dereferences parameter "node", but it could have been NULL
if deleting an fd handler that was not registered in the first place.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        Remove unnecessary assignment to node->pfd.revents.

 aio-posix.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/aio-posix.c b/aio-posix.c
index 4ef34dd..ec908f7 100644
--- a/aio-posix.c
+++ b/aio-posix.c
@@ -217,21 +217,23 @@ void aio_set_fd_handler(AioContext *ctx,
 
     /* Are we deleting the fd handler? */
     if (!io_read && !io_write) {
-        if (node) {
-            g_source_remove_poll(&ctx->source, &node->pfd);
-
-            /* If the lock is held, just mark the node as deleted */
-            if (ctx->walking_handlers) {
-                node->deleted = 1;
-                node->pfd.revents = 0;
-            } else {
-                /* Otherwise, delete it for real.  We can't just mark it as
-                 * deleted because deleted nodes are only cleaned up after
-                 * releasing the walking_handlers lock.
-                 */
-                QLIST_REMOVE(node, node);
-                deleted = true;
-            }
+        if (node == NULL) {
+            return;
+        }
+
+        g_source_remove_poll(&ctx->source, &node->pfd);
+
+        /* If the lock is held, just mark the node as deleted */
+        if (ctx->walking_handlers) {
+            node->deleted = 1;
+            node->pfd.revents = 0;
+        } else {
+            /* Otherwise, delete it for real.  We can't just mark it as
+             * deleted because deleted nodes are only cleaned up after
+             * releasing the walking_handlers lock.
+             */
+            QLIST_REMOVE(node, node);
+            deleted = true;
         }
     } else {
         if (node == NULL) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update

[Fam Zheng]

On Tue, 11/08 14:55, Paolo Bonzini wrote:
> aio_epoll_update dereferences parameter "node", but it could have been NULL
> if deleting an fd handler that was not registered in the first place.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>         Remove unnecessary assignment to node->pfd.revents.
> 
>  aio-posix.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index 4ef34dd..ec908f7 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -217,21 +217,23 @@ void aio_set_fd_handler(AioContext *ctx,
>  
>      /* Are we deleting the fd handler? */
>      if (!io_read && !io_write) {
> -        if (node) {
> -            g_source_remove_poll(&ctx->source, &node->pfd);
> -
> -            /* If the lock is held, just mark the node as deleted */
> -            if (ctx->walking_handlers) {
> -                node->deleted = 1;
> -                node->pfd.revents = 0;
> -            } else {
> -                /* Otherwise, delete it for real.  We can't just mark it as
> -                 * deleted because deleted nodes are only cleaned up after
> -                 * releasing the walking_handlers lock.
> -                 */
> -                QLIST_REMOVE(node, node);
> -                deleted = true;
> -            }
> +        if (node == NULL) {
> +            return;
> +        }
> +
> +        g_source_remove_poll(&ctx->source, &node->pfd);
> +
> +        /* If the lock is held, just mark the node as deleted */
> +        if (ctx->walking_handlers) {
> +            node->deleted = 1;
> +            node->pfd.revents = 0;
> +        } else {
> +            /* Otherwise, delete it for real.  We can't just mark it as
> +             * deleted because deleted nodes are only cleaned up after
> +             * releasing the walking_handlers lock.
> +             */
> +            QLIST_REMOVE(node, node);
> +            deleted = true;
>          }
>      } else {
>          if (node == NULL) {
> -- 
> 2.7.4
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>