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
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 */
>
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
© 2016 - 2025 Red Hat, Inc.