[PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[Stefan Hajnoczi]

QLIST_REMOVE() and friends leave dangling linked list pointers in the node that
was removed.  This makes it impossible to decide whether a node is currently in
a list or not.  It also makes debugging harder.

Based-on: 20200222085030.1760640-1-stefanha@redhat.com
          ("[PULL 00/31] Block patches")

Stefan Hajnoczi (2):
  qemu/queue.h: clear linked list pointers on remove
  aio-posix: remove confusing QLIST_SAFE_REMOVE()

 include/qemu/queue.h | 19 +++++++++++++++----
 util/aio-posix.c     |  2 +-
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.24.1

[PATCH 1/2] qemu/queue.h: clear linked list pointers on remove

[Stefan Hajnoczi]

Do not leave stale linked list pointers around after removal.  It's
safer to set them to NULL so that use-after-removal results in an
immediate segfault.

The RCU queue removal macros are unchanged since nodes may still be
traversed after removal.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/queue.h | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index 294db54eb1..456a5b01ee 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -142,6 +142,8 @@ struct {                                                                \
                 (elm)->field.le_next->field.le_prev =                   \
                     (elm)->field.le_prev;                               \
         *(elm)->field.le_prev = (elm)->field.le_next;                   \
+        (elm)->field.le_next = NULL;                                    \
+        (elm)->field.le_prev = NULL;                                    \
 } while (/*CONSTCOND*/0)
 
 /*
@@ -225,12 +227,15 @@ struct {                                                                \
 } while (/*CONSTCOND*/0)
 
 #define QSLIST_REMOVE_HEAD(head, field) do {                             \
-        (head)->slh_first = (head)->slh_first->field.sle_next;          \
+        typeof((head)->slh_first) elm = (head)->slh_first;               \
+        (head)->slh_first = elm->field.sle_next;                         \
+        elm->field.sle_next = NULL;                                      \
 } while (/*CONSTCOND*/0)
 
 #define QSLIST_REMOVE_AFTER(slistelm, field) do {                       \
-        (slistelm)->field.sle_next =                                    \
-            QSLIST_NEXT(QSLIST_NEXT((slistelm), field), field);         \
+        typeof(slistelm) next = (slistelm)->field.sle_next;             \
+        (slistelm)->field.sle_next = next->field.sle_next;              \
+        next->field.sle_next = NULL;                                    \
 } while (/*CONSTCOND*/0)
 
 #define QSLIST_REMOVE(head, elm, type, field) do {                      \
@@ -241,6 +246,7 @@ struct {                                                                \
         while (curelm->field.sle_next != (elm))                         \
             curelm = curelm->field.sle_next;                            \
         curelm->field.sle_next = curelm->field.sle_next->field.sle_next; \
+        (elm)->field.sle_next = NULL;                                   \
     }                                                                   \
 } while (/*CONSTCOND*/0)
 
@@ -304,8 +310,10 @@ struct {                                                                \
 } while (/*CONSTCOND*/0)
 
 #define QSIMPLEQ_REMOVE_HEAD(head, field) do {                          \
-    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
+    typeof((head)->sqh_first) elm = (head)->sqh_first;                  \
+    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
         (head)->sqh_last = &(head)->sqh_first;                          \
+    elm->field.sqe_next = NULL;                                         \
 } while (/*CONSTCOND*/0)
 
 #define QSIMPLEQ_SPLIT_AFTER(head, elm, field, removed) do {            \
@@ -329,6 +337,7 @@ struct {                                                                \
         if ((curelm->field.sqe_next =                                   \
             curelm->field.sqe_next->field.sqe_next) == NULL)            \
                 (head)->sqh_last = &(curelm)->field.sqe_next;           \
+        (elm)->field.sqe_next = NULL;                                   \
     }                                                                   \
 } while (/*CONSTCOND*/0)
 
@@ -446,6 +455,8 @@ union {                                                                 \
             (head)->tqh_circ.tql_prev = (elm)->field.tqe_circ.tql_prev; \
         (elm)->field.tqe_circ.tql_prev->tql_next = (elm)->field.tqe_next; \
         (elm)->field.tqe_circ.tql_prev = NULL;                          \
+        (elm)->field.tqe_circ.tql_next = NULL;                          \
+        (elm)->field.tqe_next = NULL;                                   \
 } while (/*CONSTCOND*/0)
 
 /* remove @left, @right and all elements in between from @head */
-- 
2.24.1

[PATCH 2/2] aio-posix: remove confusing QLIST_SAFE_REMOVE()

[Stefan Hajnoczi]

QLIST_SAFE_REMOVE() is confusing here because the node must be on the
list.  We actually just wanted to clear the linked list pointers when
removing it from the list.  QLIST_REMOVE() now does this, so switch to
it.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/aio-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 7e6c9835bb..026c1f5286 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -493,7 +493,7 @@ static bool aio_dispatch_ready_handlers(AioContext *ctx,
     AioHandler *node;
 
     while ((node = QLIST_FIRST(ready_list))) {
-        QLIST_SAFE_REMOVE(node, node_ready);
+        QLIST_REMOVE(node, node_ready);
         progress = aio_dispatch_handler(ctx, node) || progress;
     }
 
-- 
2.24.1

Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200224103406.1894923-1-stefanha@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove
Message-id: 20200224103406.1894923-1-stefanha@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
56b6529 aio-posix: remove confusing QLIST_SAFE_REMOVE()
f913b24 qemu/queue.h: clear linked list pointers on remove

=== OUTPUT BEGIN ===
1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
ERROR: do not use assignment in if condition
#65: FILE: include/qemu/queue.h:314:
+    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \

total: 1 errors, 0 warnings, 59 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 56b6529b1894 (aio-posix: remove confusing QLIST_SAFE_REMOVE())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200224103406.1894923-1-stefanha@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[Stefan Hajnoczi]

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

On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
> === OUTPUT BEGIN ===
> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
> ERROR: do not use assignment in if condition
> #65: FILE: include/qemu/queue.h:314:
> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
> 
> total: 1 errors, 0 warnings, 59 lines checked

The same pattern is used elsewhere in this file.  This code comes from
BSD and doesn't comply with QEMU's coding style.

Stefan

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

Re: [PATCH 1/2] qemu/queue.h: clear linked list pointers on remove

[Philippe Mathieu-Daudé]

On 2/24/20 11:34 AM, Stefan Hajnoczi wrote:
> Do not leave stale linked list pointers around after removal.  It's
> safer to set them to NULL so that use-after-removal results in an
> immediate segfault.
> 
> The RCU queue removal macros are unchanged since nodes may still be
> traversed after removal.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/qemu/queue.h | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> index 294db54eb1..456a5b01ee 100644
> --- a/include/qemu/queue.h
> +++ b/include/qemu/queue.h
> @@ -142,6 +142,8 @@ struct {                                                                \
>                   (elm)->field.le_next->field.le_prev =                   \
>                       (elm)->field.le_prev;                               \
>           *(elm)->field.le_prev = (elm)->field.le_next;                   \
> +        (elm)->field.le_next = NULL;                                    \
> +        (elm)->field.le_prev = NULL;                                    \
>   } while (/*CONSTCOND*/0)

OK.

>   
>   /*
> @@ -225,12 +227,15 @@ struct {                                                                \
>   } while (/*CONSTCOND*/0)
>   
>   #define QSLIST_REMOVE_HEAD(head, field) do {                             \
> -        (head)->slh_first = (head)->slh_first->field.sle_next;          \
> +        typeof((head)->slh_first) elm = (head)->slh_first;               \
> +        (head)->slh_first = elm->field.sle_next;                         \
> +        elm->field.sle_next = NULL;                                      \
>   } while (/*CONSTCOND*/0)
>   
>   #define QSLIST_REMOVE_AFTER(slistelm, field) do {                       \
> -        (slistelm)->field.sle_next =                                    \
> -            QSLIST_NEXT(QSLIST_NEXT((slistelm), field), field);         \
> +        typeof(slistelm) next = (slistelm)->field.sle_next;             \
> +        (slistelm)->field.sle_next = next->field.sle_next;              \
> +        next->field.sle_next = NULL;                                    \
>   } while (/*CONSTCOND*/0)
>   
>   #define QSLIST_REMOVE(head, elm, type, field) do {                      \
> @@ -241,6 +246,7 @@ struct {                                                                \
>           while (curelm->field.sle_next != (elm))                         \
>               curelm = curelm->field.sle_next;                            \
>           curelm->field.sle_next = curelm->field.sle_next->field.sle_next; \
> +        (elm)->field.sle_next = NULL;                                   \
>       }                                                                   \
>   } while (/*CONSTCOND*/0)
>   
> @@ -304,8 +310,10 @@ struct {                                                                \
>   } while (/*CONSTCOND*/0)
>   
>   #define QSIMPLEQ_REMOVE_HEAD(head, field) do {                          \
> -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
> +    typeof((head)->sqh_first) elm = (head)->sqh_first;                  \
> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
>           (head)->sqh_last = &(head)->sqh_first;                          \

Here you check elm for NULL ...

> +    elm->field.sqe_next = NULL;                                         \

... then you assign it.

>   } while (/*CONSTCOND*/0)
>   
>   #define QSIMPLEQ_SPLIT_AFTER(head, elm, field, removed) do {            \
> @@ -329,6 +337,7 @@ struct {                                                                \
>           if ((curelm->field.sqe_next =                                   \
>               curelm->field.sqe_next->field.sqe_next) == NULL)            \
>                   (head)->sqh_last = &(curelm)->field.sqe_next;           \
> +        (elm)->field.sqe_next = NULL;                                   \
>       }                                                                   \
>   } while (/*CONSTCOND*/0)
>   
> @@ -446,6 +455,8 @@ union {                                                                 \
>               (head)->tqh_circ.tql_prev = (elm)->field.tqe_circ.tql_prev; \
>           (elm)->field.tqe_circ.tql_prev->tql_next = (elm)->field.tqe_next; \
>           (elm)->field.tqe_circ.tql_prev = NULL;                          \
> +        (elm)->field.tqe_circ.tql_next = NULL;                          \
> +        (elm)->field.tqe_next = NULL;                                   \
>   } while (/*CONSTCOND*/0)
>   
>   /* remove @left, @right and all elements in between from @head */
> 

Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[Philippe Mathieu-Daudé]

On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
> On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
>> === OUTPUT BEGIN ===
>> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
>> ERROR: do not use assignment in if condition
>> #65: FILE: include/qemu/queue.h:314:
>> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
>>
>> total: 1 errors, 0 warnings, 59 lines checked
> 
> The same pattern is used elsewhere in this file.  This code comes from
> BSD and doesn't comply with QEMU's coding style.

Checkpatch is right, assigning out of the if statement makes the review 
easier, and we can avoid the 'elm' null deref:

#define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
-    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
+    typeof((head)->sqh_first) elm = (head)->sqh_first; \
+    (head)->sqh_first = elm->field.sqe_next; \
+    if (elm == NULL) { \
          (head)->sqh_last = &(head)->sqh_first; \
+    } else { \
+        elm->field.sqe_next = NULL; \
+    } \
  } while (/*CONSTCOND*/0)

Re: [PATCH 1/2] qemu/queue.h: clear linked list pointers on remove

[Stefan Hajnoczi]

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

On Mon, Feb 24, 2020 at 12:51:54PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/24/20 11:34 AM, Stefan Hajnoczi wrote:
> > @@ -304,8 +310,10 @@ struct {                                                                \
> >   } while (/*CONSTCOND*/0)
> >   #define QSIMPLEQ_REMOVE_HEAD(head, field) do {                          \
> > -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
> > +    typeof((head)->sqh_first) elm = (head)->sqh_first;                  \
> > +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
> >           (head)->sqh_last = &(head)->sqh_first;                          \
> 
> Here you check elm for NULL ...
> 
> > +    elm->field.sqe_next = NULL;                                         \
> 
> ... then you assign it.

The sqe_next field is copied into the head.  If this was the last
element in the list then the head's sqh_last needs to be fixed up.

Finally we clear the linked list sqe_next pointer inside the element
itself (which is no longer in the list).

Is there an issue?

Stefan

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

Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[Stefan Hajnoczi]

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

On Mon, Feb 24, 2020 at 12:54:37PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
> > On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
> > > === OUTPUT BEGIN ===
> > > 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
> > > ERROR: do not use assignment in if condition
> > > #65: FILE: include/qemu/queue.h:314:
> > > +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
> > > 
> > > total: 1 errors, 0 warnings, 59 lines checked
> > 
> > The same pattern is used elsewhere in this file.  This code comes from
> > BSD and doesn't comply with QEMU's coding style.
> 
> Checkpatch is right, assigning out of the if statement makes the review
> easier, and we can avoid the 'elm' null deref:

The rest of the file uses if ((a = b) == NULL), so making it
inconsistent in this one instance isn't very satisfying.

> #define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
> -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
> +    typeof((head)->sqh_first) elm = (head)->sqh_first; \
> +    (head)->sqh_first = elm->field.sqe_next; \
> +    if (elm == NULL) { \

The previous line would have segfaulted if elm was NULL so this check
doesn't make sense.

This macro assumes there is at least one element in the list.

The point of the check is to fix up the sqh_last pointer in the head
when the final element is removed from the list.

>          (head)->sqh_last = &(head)->sqh_first; \
> +    } else { \
> +        elm->field.sqe_next = NULL; \
> +    } \
>  } while (/*CONSTCOND*/0)

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

Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[Philippe Mathieu-Daudé]

On 2/25/20 10:05 AM, Stefan Hajnoczi wrote:
> On Mon, Feb 24, 2020 at 12:54:37PM +0100, Philippe Mathieu-Daudé wrote:
>> On 2/24/20 12:39 PM, Stefan Hajnoczi wrote:
>>> On Mon, Feb 24, 2020 at 02:55:33AM -0800, no-reply@patchew.org wrote:
>>>> === OUTPUT BEGIN ===
>>>> 1/2 Checking commit f913b2430ad3 (qemu/queue.h: clear linked list pointers on remove)
>>>> ERROR: do not use assignment in if condition
>>>> #65: FILE: include/qemu/queue.h:314:
>>>> +    if (((head)->sqh_first = elm->field.sqe_next) == NULL)              \
>>>>
>>>> total: 1 errors, 0 warnings, 59 lines checked
>>>
>>> The same pattern is used elsewhere in this file.  This code comes from
>>> BSD and doesn't comply with QEMU's coding style.
>>
>> Checkpatch is right, assigning out of the if statement makes the review
>> easier, and we can avoid the 'elm' null deref:
> 
> The rest of the file uses if ((a = b) == NULL), so making it
> inconsistent in this one instance isn't very satisfying.
> 
>> #define QSIMPLEQ_REMOVE_HEAD(head, field) do { \
>> -    if (((head)->sqh_first = (head)->sqh_first->field.sqe_next) == NULL)\
>> +    typeof((head)->sqh_first) elm = (head)->sqh_first; \
>> +    (head)->sqh_first = elm->field.sqe_next; \
>> +    if (elm == NULL) { \
> 
> The previous line would have segfaulted if elm was NULL so this check
> doesn't make sense.
> 
> This macro assumes there is at least one element in the list.

Ah good point, thanks.

> 
> The point of the check is to fix up the sqh_last pointer in the head
> when the final element is removed from the list.
> 
>>           (head)->sqh_last = &(head)->sqh_first; \
>> +    } else { \
>> +        elm->field.sqe_next = NULL; \
>> +    } \
>>   } while (/*CONSTCOND*/0)

Re: [PATCH 0/2] qemu/queue.h: clear linked list pointers on remove

[Stefan Hajnoczi]

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

On Mon, Feb 24, 2020 at 10:34:04AM +0000, Stefan Hajnoczi wrote:
> QLIST_REMOVE() and friends leave dangling linked list pointers in the node that
> was removed.  This makes it impossible to decide whether a node is currently in
> a list or not.  It also makes debugging harder.
> 
> Based-on: 20200222085030.1760640-1-stefanha@redhat.com
>           ("[PULL 00/31] Block patches")
> 
> Stefan Hajnoczi (2):
>   qemu/queue.h: clear linked list pointers on remove
>   aio-posix: remove confusing QLIST_SAFE_REMOVE()
> 
>  include/qemu/queue.h | 19 +++++++++++++++----
>  util/aio-posix.c     |  2 +-
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> -- 
> 2.24.1
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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