* [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups
@ 2016-11-07 9:33 Paolo Bonzini
2016-11-07 9:33 ` [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Paolo Bonzini @ 2016-11-07 9:33 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, famz, kwolf
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update
2016-11-07 9:33 [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Paolo Bonzini
@ 2016-11-07 9:33 ` Paolo Bonzini
2016-11-07 14:58 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-11-07 9:33 ` [Qemu-devel] [PATCH 2/2] aio-posix: simplify aio_epoll_update Paolo Bonzini
2016-11-07 10:08 ` [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Fam Zheng
2 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2016-11-07 9:33 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, famz, kwolf
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] aio-posix: simplify aio_epoll_update
2016-11-07 9:33 [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Paolo Bonzini
2016-11-07 9:33 ` [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update Paolo Bonzini
@ 2016-11-07 9:33 ` Paolo Bonzini
2016-11-07 15:01 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-11-07 10:08 ` [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Fam Zheng
2 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2016-11-07 9:33 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, famz, kwolf
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups
2016-11-07 9:33 [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Paolo Bonzini
2016-11-07 9:33 ` [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update Paolo Bonzini
2016-11-07 9:33 ` [Qemu-devel] [PATCH 2/2] aio-posix: simplify aio_epoll_update Paolo Bonzini
@ 2016-11-07 10:08 ` Fam Zheng
2 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2016-11-07 10:08 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, kwolf, qemu-block
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update
2016-11-07 9:33 ` [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update Paolo Bonzini
@ 2016-11-07 14:58 ` Stefan Hajnoczi
2016-11-07 15:35 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2016-11-07 14:58 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, kwolf, famz, qemu-block
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] aio-posix: simplify aio_epoll_update
2016-11-07 9:33 ` [Qemu-devel] [PATCH 2/2] aio-posix: simplify aio_epoll_update Paolo Bonzini
@ 2016-11-07 15:01 ` Stefan Hajnoczi
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2016-11-07 15:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, kwolf, famz, qemu-block
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update
2016-11-07 14:58 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2016-11-07 15:35 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2016-11-07 15:35 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, kwolf, famz, qemu-block
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?
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-07 15:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-07 9:33 [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Paolo Bonzini
2016-11-07 9:33 ` [Qemu-devel] [PATCH 1/2] aio-posix: avoid NULL pointer dereference in aio_epoll_update Paolo Bonzini
2016-11-07 14:58 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-11-07 15:35 ` Paolo Bonzini
2016-11-07 9:33 ` [Qemu-devel] [PATCH 2/2] aio-posix: simplify aio_epoll_update Paolo Bonzini
2016-11-07 15:01 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-11-07 10:08 ` [Qemu-devel] [PATCH for-2.8 0/2] aio-posix: epoll cleanups Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.