| Message ID | 20200224103406.1894923-2-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu/queue.h: clear linked list pointers on remove | expand |
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
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 */
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(-)