[PATCH] eal: fix alarm cancel list walk

[Stephen Hemminger <stephen@networkplumber.org>]

All three implementations of rte_eal_alarm_cancel() free entries while
walking the alarm list with LIST_FOREACH, which leaves the iterator
pointing into freed memory.

Linux and FreeBSD use two loops: one draining matches from the head of
the list, then a LIST_FOREACH over the rest that frees the current
entry and assigns the saved ap_prev to ap so iteration resumes from the
predecessor.  ap_prev is only refreshed to a live entry by an iteration
that does not remove, and the head loop leaves it NULL when it empties
the list.  A removal in the second loop then sets ap to NULL or to an
already freed entry, and the LIST_FOREACH increment dereferences it.
GCC -fanalyzer reports the freed case:

  lib/eal/linux/eal_alarm.c:224:44: warning: use after 'free' of 'ap'
	[CWE-416] [-Wanalyzer-use-after-free]

Windows has no such dance: it calls alarm_remove_unsafe() straight from
the loop body, so the increment reads freed memory on every removal but
the last.

Replace all of these with LIST_FOREACH_SAFE.  FreeBSD sys/queue.h and
the bundled Windows sys/queue.h already provide it; glibc does not, so
define it locally as is already done in several drivers.

Fixes: af75078fece3 ("first public release")
Fixes: f4cbdbc7fbd2 ("eal/windows: implement alarm API")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/freebsd/eal_alarm.c | 41 +++++++------------------------------
 lib/eal/linux/eal_alarm.c   | 40 ++++++++++++------------------------
 lib/eal/windows/eal_alarm.c |  4 ++--
 3 files changed, 22 insertions(+), 63 deletions(-)

diff --git a/lib/eal/freebsd/eal_alarm.c b/lib/eal/freebsd/eal_alarm.c
index c03e281e67..1585a651e9 100644
--- a/lib/eal/freebsd/eal_alarm.c
+++ b/lib/eal/freebsd/eal_alarm.c
@@ -264,7 +264,7 @@ RTE_EXPORT_SYMBOL(rte_eal_alarm_cancel)
 int
 rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 {
-	struct alarm_entry *ap, *ap_prev;
+	struct alarm_entry *ap, *ap_next;
 	int count = 0;
 	int err = 0;
 	int executing;
@@ -277,15 +277,12 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 	do {
 		executing = 0;
 		rte_spinlock_lock(&alarm_list_lk);
-		/* remove any matches at the start of the list */
-		while (1) {
-			ap = LIST_FIRST(&alarm_list);
-			if (ap == NULL)
-				break;
-			if (cb_fn != ap->cb_fn)
-				break;
-			if (cb_arg != ap->cb_arg && cb_arg != (void *) -1)
-				break;
+
+		LIST_FOREACH_SAFE(ap, &alarm_list, next, ap_next) {
+			if (cb_fn != ap->cb_fn ||
+					(cb_arg != (void *)-1 && cb_arg != ap->cb_arg))
+				continue;
+
 			if (ap->executing == 0) {
 				LIST_REMOVE(ap, next);
 				free(ap);
@@ -301,31 +298,7 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 					executing++;
 				else
 					err = EINPROGRESS;
-
-				break;
-			}
-		}
-		ap_prev = ap;
-
-		/* now go through list, removing entries not at start */
-		LIST_FOREACH(ap, &alarm_list, next) {
-			/* this won't be true first time through */
-			if (cb_fn == ap->cb_fn &&
-					(cb_arg == (void *)-1 ||
-					 cb_arg == ap->cb_arg)) {
-				if (ap->executing == 0) {
-					LIST_REMOVE(ap, next);
-					free(ap);
-					count++;
-					ap = ap_prev;
-				} else if (pthread_equal(ap->executing_id,
-							 pthread_self()) == 0) {
-					executing++;
-				} else {
-					err = EINPROGRESS;
-				}
 			}
-			ap_prev = ap;
 		}
 
 		rte_spinlock_unlock(&alarm_list_lk);
diff --git a/lib/eal/linux/eal_alarm.c b/lib/eal/linux/eal_alarm.c
index a1433eb867..eb41064851 100644
--- a/lib/eal/linux/eal_alarm.c
+++ b/lib/eal/linux/eal_alarm.c
@@ -25,6 +25,13 @@
 #define	TFD_NONBLOCK	O_NONBLOCK
 #endif
 
+#ifndef LIST_FOREACH_SAFE
+#define LIST_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = LIST_FIRST((head));				\
+	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
 #define NS_PER_US 1000
 #define US_PER_MS 1000
 #define MS_PER_S 1000
@@ -206,7 +213,7 @@ RTE_EXPORT_SYMBOL(rte_eal_alarm_cancel)
 int
 rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 {
-	struct alarm_entry *ap, *ap_prev;
+	struct alarm_entry *ap, *ap_next;
 	int count = 0;
 	int err = 0;
 	int executing;
@@ -219,10 +226,11 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 	do {
 		executing = 0;
 		rte_spinlock_lock(&alarm_list_lk);
-		/* remove any matches at the start of the list */
-		while ((ap = LIST_FIRST(&alarm_list)) != NULL &&
-				cb_fn == ap->cb_fn &&
-				(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {
+
+		LIST_FOREACH_SAFE(ap, &alarm_list, next, ap_next) {
+			if (cb_fn != ap->cb_fn ||
+					(cb_arg != (void *)-1 && cb_arg != ap->cb_arg))
+				continue;
 
 			if (ap->executing == 0) {
 				LIST_REMOVE(ap, next);
@@ -236,29 +244,7 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 					executing++;
 				else
 					err = EINPROGRESS;
-
-				break;
-			}
-		}
-		ap_prev = ap;
-
-		/* now go through list, removing entries not at start */
-		LIST_FOREACH(ap, &alarm_list, next) {
-			/* this won't be true first time through */
-			if (cb_fn == ap->cb_fn &&
-					(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {
-
-				if (ap->executing == 0) {
-					LIST_REMOVE(ap, next);
-					free(ap);
-					count++;
-					ap = ap_prev;
-				} else if (pthread_equal(ap->executing_id, pthread_self()) == 0)
-					executing++;
-				else
-					err = EINPROGRESS;
 			}
-			ap_prev = ap;
 		}
 
 		rte_spinlock_unlock(&alarm_list_lk);
diff --git a/lib/eal/windows/eal_alarm.c b/lib/eal/windows/eal_alarm.c
index 0b11d331dc..ed6e7f2245 100644
--- a/lib/eal/windows/eal_alarm.c
+++ b/lib/eal/windows/eal_alarm.c
@@ -190,7 +190,7 @@ RTE_EXPORT_SYMBOL(rte_eal_alarm_cancel)
 int
 rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 {
-	struct alarm_entry *ap;
+	struct alarm_entry *ap, *ap_next;
 	unsigned int state;
 	int removed;
 	bool executing;
@@ -207,7 +207,7 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)
 
 		rte_spinlock_lock(&alarm_lock);
 
-		LIST_FOREACH(ap, &alarm_list, next) {
+		LIST_FOREACH_SAFE(ap, &alarm_list, next, ap_next) {
 			if (!alarm_matches(ap, cb_fn, cb_arg))
 				continue;
 
-- 
2.53.0