BUG/MEDIUM: lists: Lock the element while we check if it is in a list.

In MT_LIST_TRY_ADDQ() and MT_LIST_TRY_ADD() we can't just check if the
element is already in a list, because there's a small race condition, it
could be added  between the time we checked, and the time we actually set
its next and prev, so we have to lock it first.

This is required to address issue #958.

This should be backported to 2.3, 2.2 and 2.1.

(cherry picked from commit 1f05324cbe92a7dde71f44dc740eb8240539746f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c4f3013d9452707d4efbda455360c8d2af022411)
[wt: fix has been in 2.3 for 3 versions now]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 4aebb4e88ed360003efacc7f75a48c1d39bda6bb)
[wt: updated the two macros to match the equivalent upstream patch
 because during the reorg they were cleaned up by trimming the unused
 do{}while(0), making the patch impossible to apply that way]
Signed-off-by: Willy Tarreau <w@1wt.eu>

diff --git a/include/common/mini-clist.h b/include/common/mini-clist.h
index bdb9212..0bd5622 100644 (file)
--- a/include/common/mini-clist.h
+++ b/include/common/mini-clist.h
@@ -229,36 +229,53 @@ struct cond_wordlist {
      ({                                                                    \
         int _ret = 0;                                                      \
        struct mt_list *lh = (_lh), *el = (_el);                           \
-       do {                                                               \
-               while (1) {                                                \
-                       struct mt_list *n;                                 \
-                       struct mt_list *p;                                 \
-                       n = _HA_ATOMIC_XCHG(&(lh)->next, MT_LIST_BUSY);    \
-                       if (n == MT_LIST_BUSY)                             \
-                               continue;                                  \
-                       p = _HA_ATOMIC_XCHG(&n->prev, MT_LIST_BUSY);       \
-                       if (p == MT_LIST_BUSY) {                           \
-                               (lh)->next = n;                            \
-                               __ha_barrier_store();                      \
-                               continue;                                  \
-                       }                                                  \
-                       if ((el)->next != (el) || (el)->prev != (el)) {    \
-                               (n)->prev = p;                             \
-                               (lh)->next = n;                            \
-                               __ha_barrier_store();                      \
-                               break;                                     \
-                       }                                                  \
-                       (el)->next = n;                                    \
-                       (el)->prev = p;                                    \
+       while (1) {                                                        \
+               struct mt_list *n, *n2;                                    \
+               struct mt_list *p, *p2;                                    \
+               n = _HA_ATOMIC_XCHG(&(lh)->next, MT_LIST_BUSY);            \
+               if (n == MT_LIST_BUSY)                                     \
+                       continue;                                          \
+               p = _HA_ATOMIC_XCHG(&n->prev, MT_LIST_BUSY);               \
+               if (p == MT_LIST_BUSY) {                                   \
+                       (lh)->next = n;                                    \
                        __ha_barrier_store();                              \
-                       n->prev = (el);                                    \
+                       continue;                                          \
+               }                                                          \
+               n2 = _HA_ATOMIC_XCHG(&el->next, MT_LIST_BUSY);             \
+               if (n2 != el) { /* element already linked */               \
+                       if (n2 != MT_LIST_BUSY)                            \
+                               el->next = n2;                             \
+                       n->prev = p;                                       \
                        __ha_barrier_store();                              \
-                       p->next = (el);                                    \
+                       lh->next = n;                                      \
                        __ha_barrier_store();                              \
-                       _ret = 1;                                          \
+                       if (n2 == MT_LIST_BUSY)                            \
+                               continue;                                  \
                        break;                                             \
                }                                                          \
-       } while (0);                                                       \
+               p2 = _HA_ATOMIC_XCHG(&el->prev, MT_LIST_BUSY);             \
+               if (p2 != el) {                                            \
+                       if (p2 != MT_LIST_BUSY)                            \
+                               el->prev = p2;                             \
+                       n->prev = p;                                       \
+                       el->next = el;                                     \
+                       __ha_barrier_store();                              \
+                       lh->next = n;                                      \
+                       __ha_barrier_store();                              \
+                       if (p2 == MT_LIST_BUSY)                            \
+                               continue;                                  \
+                       break;                                             \
+               }                                                          \
+               (el)->next = n;                                            \
+               (el)->prev = p;                                            \
+               __ha_barrier_store();                                      \
+               n->prev = (el);                                            \
+               __ha_barrier_store();                                      \
+               p->next = (el);                                            \
+               __ha_barrier_store();                                      \
+               _ret = 1;                                                  \
+               break;                                                     \
+       }                                                                  \
        (_ret);                                                            \
      })
 
@@ -269,38 +286,55 @@ struct cond_wordlist {
  */
 #define MT_LIST_ADDQ(_lh, _el)                                             \
     ({                                                                     \
-           int _ret = 0;                                                  \
-           struct mt_list *lh = (_lh), *el = (_el);                       \
-       do {                                                               \
-               while (1) {                                                \
-                       struct mt_list *n;                                 \
-                       struct mt_list *p;                                 \
-                       p = _HA_ATOMIC_XCHG(&(lh)->prev, MT_LIST_BUSY);    \
-                       if (p == MT_LIST_BUSY)                             \
-                               continue;                                  \
-                       n = _HA_ATOMIC_XCHG(&p->next, MT_LIST_BUSY);       \
-                       if (n == MT_LIST_BUSY) {                           \
-                               (lh)->prev = p;                            \
-                               __ha_barrier_store();                      \
-                               continue;                                  \
-                       }                                                  \
-                       if ((el)->next != (el) || (el)->prev != (el)) {    \
-                               p->next = n;                               \
-                               (lh)->prev = p;                            \
-                               __ha_barrier_store();                      \
-                               break;                                     \
-                       }                                                  \
-                       (el)->next = n;                                    \
-                       (el)->prev = p;                                    \
+       int _ret = 0;                                                      \
+       struct mt_list *lh = (_lh), *el = (_el);                           \
+       while (1) {                                                        \
+               struct mt_list *n, *n2;                                    \
+               struct mt_list *p, *p2;                                    \
+               p = _HA_ATOMIC_XCHG(&(lh)->prev, MT_LIST_BUSY);            \
+               if (p == MT_LIST_BUSY)                                     \
+                       continue;                                          \
+               n = _HA_ATOMIC_XCHG(&p->next, MT_LIST_BUSY);               \
+               if (n == MT_LIST_BUSY) {                                   \
+                       (lh)->prev = p;                                    \
                        __ha_barrier_store();                              \
-                       p->next = (el);                                    \
+                       continue;                                          \
+               }                                                          \
+               n2 = _HA_ATOMIC_XCHG(&el->next, MT_LIST_BUSY);             \
+               if (n2 != el) { /* element already linked */               \
+                       if (n2 != MT_LIST_BUSY)                            \
+                               el->next = n2;                             \
+                       p->next = n;                                       \
                        __ha_barrier_store();                              \
-                       n->prev = (el);                                    \
+                       lh->prev = p;                                      \
                        __ha_barrier_store();                              \
-                       _ret = 1;                                          \
+                       if (n2 == MT_LIST_BUSY)                            \
+                               continue;                                  \
                        break;                                             \
                }                                                          \
-       } while (0);                                                       \
+               p2 = _HA_ATOMIC_XCHG(&el->prev, MT_LIST_BUSY);             \
+               if (p2 != el) {                                            \
+                       if (p2 != MT_LIST_BUSY)                            \
+                               el->prev = p2;                             \
+                       p->next = n;                                       \
+                       el->next = el;                                     \
+                       __ha_barrier_store();                              \
+                       lh->prev = p;                                      \
+                       __ha_barrier_store();                              \
+                       if (p2 == MT_LIST_BUSY)                            \
+                               continue;                                  \
+                       break;                                             \
+               }                                                          \
+               (el)->next = n;                                            \
+               (el)->prev = p;                                            \
+               __ha_barrier_store();                                      \
+               p->next = (el);                                            \
+               __ha_barrier_store();                                      \
+               n->prev = (el);                                            \
+               __ha_barrier_store();                                      \
+               _ret = 1;                                                  \
+               break;                                                     \
+       }                                                                  \
        (_ret);                                                            \
     })