XArray: Fix xa_release in allocating arrays

xa_cmpxchg() was a little too magic in turning ZERO entries into NULL,
and would leave the entry set to the ZERO entry instead of releasing
it for future use.  After careful review of existing users of
xa_cmpxchg(), change the semantics so that it does not translate either
incoming argument from NULL into ZERO entries.

Add several tests to the test-suite to make sure this problem doesn't
come back.

Reported-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Matthew Wilcox <willy@infradead.org>

diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index 687c150071a543756b37c100fc8a1b330a3c6d5a..588733abd19d7457b3b9075ed174b0f0e780caf9 100644 (file)
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -131,6 +131,12 @@ static inline unsigned int xa_pointer_tag(void *entry)
  * xa_mk_internal() - Create an internal entry.
  * @v: Value to turn into an internal entry.
  *
+ * Internal entries are used for a number of purposes.  Entries 0-255 are
+ * used for sibling entries (only 0-62 are used by the current code).  256
+ * is used for the retry entry.  257 is used for the reserved / zero entry.
+ * Negative internal entries are used to represent errnos.  Node pointers
+ * are also tagged as internal entries in some situations.
+ *
  * Context: Any context.
  * Return: An XArray internal entry corresponding to this value.
  */
@@ -163,6 +169,22 @@ static inline bool xa_is_internal(const void *entry)
        return ((unsigned long)entry & 3) == 2;
 }
 
+#define XA_ZERO_ENTRY          xa_mk_internal(257)
+
+/**
+ * xa_is_zero() - Is the entry a zero entry?
+ * @entry: Entry retrieved from the XArray
+ *
+ * The normal API will return NULL as the contents of a slot containing
+ * a zero entry.  You can only see zero entries by using the advanced API.
+ *
+ * Return: %true if the entry is a zero entry.
+ */
+static inline bool xa_is_zero(const void *entry)
+{
+       return unlikely(entry == XA_ZERO_ENTRY);
+}
+
 /**
  * xa_is_err() - Report whether an XArray operation returned an error
  * @entry: Result from calling an XArray function
@@ -1050,7 +1072,7 @@ int xa_reserve_irq(struct xarray *xa, unsigned long index, gfp_t gfp)
  */
 static inline void xa_release(struct xarray *xa, unsigned long index)
 {
-       xa_cmpxchg(xa, index, NULL, NULL, 0);
+       xa_cmpxchg(xa, index, XA_ZERO_ENTRY, NULL, 0);
 }
 
 /* Everything below here is the Advanced API.  Proceed with caution. */
@@ -1210,18 +1232,6 @@ static inline bool xa_is_sibling(const void *entry)
 }
 
 #define XA_RETRY_ENTRY         xa_mk_internal(256)
-#define XA_ZERO_ENTRY          xa_mk_internal(257)
-
-/**
- * xa_is_zero() - Is the entry a zero entry?
- * @entry: Entry retrieved from the XArray
- *
- * Return: %true if the entry is a zero entry.
- */
-static inline bool xa_is_zero(const void *entry)
-{
-       return unlikely(entry == XA_ZERO_ENTRY);
-}
 
 /**
  * xa_is_retry() - Is the entry a retry entry?

diff --git a/lib/test_xarray.c b/lib/test_xarray.c
index 3eaa40ddc390878ef68fa0914d1c379e4a7757cf..52f8ecff8c0cc3306e4785141161acc81313477a 100644 (file)
--- a/lib/test_xarray.c
+++ b/lib/test_xarray.c
@@ -361,6 +361,7 @@ static noinline void check_reserve(struct xarray *xa)
 {
        void *entry;
        unsigned long index;
+       int count;
 
        /* An array with a reserved entry is not empty */
        XA_BUG_ON(xa, !xa_empty(xa));
@@ -377,15 +378,15 @@ static noinline void check_reserve(struct xarray *xa)
        xa_erase_index(xa, 12345678);
        XA_BUG_ON(xa, !xa_empty(xa));
 
-       /* cmpxchg sees a reserved entry as NULL */
+       /* cmpxchg sees a reserved entry as ZERO */
        XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0);
-       XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, NULL, xa_mk_value(12345678),
-                               GFP_NOWAIT) != NULL);
+       XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, XA_ZERO_ENTRY,
+                               xa_mk_value(12345678), GFP_NOWAIT) != NULL);
        xa_release(xa, 12345678);
        xa_erase_index(xa, 12345678);
        XA_BUG_ON(xa, !xa_empty(xa));
 
-       /* But xa_insert does not */
+       /* xa_insert treats it as busy */
        XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0);
        XA_BUG_ON(xa, xa_insert(xa, 12345678, xa_mk_value(12345678), 0) !=
                        -EBUSY);
@@ -398,9 +399,27 @@ static noinline void check_reserve(struct xarray *xa)
        XA_BUG_ON(xa, xa_reserve(xa, 6, GFP_KERNEL) != 0);
        xa_store_index(xa, 7, GFP_KERNEL);
 
+       count = 0;
        xa_for_each(xa, index, entry) {
                XA_BUG_ON(xa, index != 5 && index != 7);
+               count++;
+       }
+       XA_BUG_ON(xa, count != 2);
+
+       /* If we free a reserved entry, we should be able to allocate it */
+       if (xa->xa_flags & XA_FLAGS_ALLOC) {
+               u32 id;
+
+               XA_BUG_ON(xa, xa_alloc(xa, &id, xa_mk_value(8),
+                                       XA_LIMIT(5, 10), GFP_KERNEL) != 0);
+               XA_BUG_ON(xa, id != 8);
+
+               xa_release(xa, 6);
+               XA_BUG_ON(xa, xa_alloc(xa, &id, xa_mk_value(6),
+                                       XA_LIMIT(5, 10), GFP_KERNEL) != 0);
+               XA_BUG_ON(xa, id != 6);
        }
+
        xa_destroy(xa);
 }
 
@@ -1486,6 +1505,7 @@ static int xarray_checks(void)
        check_xas_erase(&array);
        check_cmpxchg(&array);
        check_reserve(&array);
+       check_reserve(&xa0);
        check_multi_store(&array);
        check_xa_alloc();
        check_find(&array);

diff --git a/lib/xarray.c b/lib/xarray.c
index 89e37ac50850df3cec844d1c0e6342387c3ce4c0..b9a6cf42feeea293c2dca33cefc8e14f8f2bf31c 100644 (file)
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -1429,16 +1429,12 @@ void *__xa_cmpxchg(struct xarray *xa, unsigned long index,
 
        if (WARN_ON_ONCE(xa_is_advanced(entry)))
                return XA_ERROR(-EINVAL);
-       if (xa_track_free(xa) && !entry)
-               entry = XA_ZERO_ENTRY;
 
        do {
                curr = xas_load(&xas);
-               if (curr == XA_ZERO_ENTRY)
-                       curr = NULL;
                if (curr == old) {
                        xas_store(&xas, entry);
-                       if (xa_track_free(xa))
+                       if (xa_track_free(xa) && entry && !curr)
                                xas_clear_mark(&xas, XA_FREE_MARK);
                }
        } while (__xas_nomem(&xas, gfp));