From bd40b17ca49d7d110adf456e647701ce74de2241 Mon Sep 17 00:00:00 2001 From: "Matthew Wilcox (Oracle)" Date: Fri, 31 Jan 2020 05:07:55 -0500 Subject: [PATCH 1/7] XArray: Fix xa_find_next for large multi-index entries Coverity pointed out that xas_sibling() was shifting xa_offset without promoting it to an unsigned long first, so the shift could cause an overflow and we'd get the wrong answer. The fix is obvious, and the new test-case provokes UBSAN to report an error: runtime error: shift exponent 60 is too large for 32-bit type 'int' Fixes: 19c30f4dd092 ("XArray: Fix xa_find_after with multi-index entries") Reported-by: Bjorn Helgaas Reported-by: Kees Cook Signed-off-by: Matthew Wilcox (Oracle) Cc: stable@vger.kernel.org --- lib/test_xarray.c | 18 ++++++++++++++++++ lib/xarray.c | 3 ++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/test_xarray.c b/lib/test_xarray.c index 55c14e8c8859..8c7d7a8468b8 100644 --- a/lib/test_xarray.c +++ b/lib/test_xarray.c @@ -12,6 +12,9 @@ static unsigned int tests_run; static unsigned int tests_passed; +static const unsigned int order_limit = + IS_ENABLED(CONFIG_XARRAY_MULTI) ? BITS_PER_LONG : 1; + #ifndef XA_DEBUG # ifdef __KERNEL__ void xa_dump(const struct xarray *xa) { } @@ -959,6 +962,20 @@ static noinline void check_multi_find_2(struct xarray *xa) } } +static noinline void check_multi_find_3(struct xarray *xa) +{ + unsigned int order; + + for (order = 5; order < order_limit; order++) { + unsigned long index = 1UL << (order - 5); + + XA_BUG_ON(xa, !xa_empty(xa)); + xa_store_order(xa, 0, order - 4, xa_mk_index(0), GFP_KERNEL); + XA_BUG_ON(xa, xa_find_after(xa, &index, ULONG_MAX, XA_PRESENT)); + xa_erase_index(xa, 0); + } +} + static noinline void check_find_1(struct xarray *xa) { unsigned long i, j, k; @@ -1081,6 +1098,7 @@ static noinline void check_find(struct xarray *xa) for (i = 2; i < 10; i++) check_multi_find_1(xa, i); check_multi_find_2(xa); + check_multi_find_3(xa); } /* See find_swap_entry() in mm/shmem.c */ diff --git a/lib/xarray.c b/lib/xarray.c index 1d9fab7db8da..acd1fad2e862 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1839,7 +1839,8 @@ static bool xas_sibling(struct xa_state *xas) if (!node) return false; mask = (XA_CHUNK_SIZE << node->shift) - 1; - return (xas->xa_index & mask) > (xas->xa_offset << node->shift); + return (xas->xa_index & mask) > + ((unsigned long)xas->xa_offset << node->shift); } /** From c36d451ad386b34f452fc3c8621ff14b9eaa31a6 Mon Sep 17 00:00:00 2001 From: "Matthew Wilcox (Oracle)" Date: Fri, 31 Jan 2020 06:17:09 -0500 Subject: [PATCH 2/7] XArray: Fix xas_pause for large multi-index entries Inspired by the recent Coverity report, I looked for other places where the offset wasn't being converted to an unsigned long before being shifted, and I found one in xas_pause() when the entry being paused is of order >32. Fixes: b803b42823d0 ("xarray: Add XArray iterators") Signed-off-by: Matthew Wilcox (Oracle) Cc: stable@vger.kernel.org --- lib/test_xarray.c | 37 +++++++++++++++++++++++++++++++++++++ lib/xarray.c | 2 +- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/lib/test_xarray.c b/lib/test_xarray.c index 8c7d7a8468b8..d4f97925dbd8 100644 --- a/lib/test_xarray.c +++ b/lib/test_xarray.c @@ -1156,6 +1156,42 @@ static noinline void check_find_entry(struct xarray *xa) XA_BUG_ON(xa, !xa_empty(xa)); } +static noinline void check_pause(struct xarray *xa) +{ + XA_STATE(xas, xa, 0); + void *entry; + unsigned int order; + unsigned long index = 1; + unsigned int count = 0; + + for (order = 0; order < order_limit; order++) { + XA_BUG_ON(xa, xa_store_order(xa, index, order, + xa_mk_index(index), GFP_KERNEL)); + index += 1UL << order; + } + + rcu_read_lock(); + xas_for_each(&xas, entry, ULONG_MAX) { + XA_BUG_ON(xa, entry != xa_mk_index(1UL << count)); + count++; + } + rcu_read_unlock(); + XA_BUG_ON(xa, count != order_limit); + + count = 0; + xas_set(&xas, 0); + rcu_read_lock(); + xas_for_each(&xas, entry, ULONG_MAX) { + XA_BUG_ON(xa, entry != xa_mk_index(1UL << count)); + count++; + xas_pause(&xas); + } + rcu_read_unlock(); + XA_BUG_ON(xa, count != order_limit); + + xa_destroy(xa); +} + static noinline void check_move_tiny(struct xarray *xa) { XA_STATE(xas, xa, 0); @@ -1664,6 +1700,7 @@ static int xarray_checks(void) check_xa_alloc(); check_find(&array); check_find_entry(&array); + check_pause(&array); check_account(&array); check_destroy(&array); check_move(&array); diff --git a/lib/xarray.c b/lib/xarray.c index acd1fad2e862..05324cf571f4 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -970,7 +970,7 @@ void xas_pause(struct xa_state *xas) xas->xa_node = XAS_RESTART; if (node) { - unsigned int offset = xas->xa_offset; + unsigned long offset = xas->xa_offset; while (++offset < XA_CHUNK_SIZE) { if (!xa_is_sibling(xa_entry(xas->xa, node, offset))) break; From 24a448b165253b6f2ab1e0bcdba9a733007681d6 Mon Sep 17 00:00:00 2001 From: Chengguang Xu Date: Fri, 6 Dec 2019 18:29:03 +0800 Subject: [PATCH 3/7] XArray: Fix incorrect comment in header file 256 means Retry entry and 257 means Zero entry, so fix it. Signed-off-by: Chengguang Xu Signed-off-by: Matthew Wilcox (Oracle) --- include/linux/xarray.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/xarray.h b/include/linux/xarray.h index f73e1775ded0..a491653d8882 100644 --- a/include/linux/xarray.h +++ b/include/linux/xarray.h @@ -32,8 +32,8 @@ * The following internal entries have a special meaning: * * 0-62: Sibling entries - * 256: Zero entry - * 257: Retry entry + * 256: Retry entry + * 257: Zero entry * * Errors are also represented as internal entries, but use the negative * space (-4094 to -2). They're never stored in the slots array; only From 3a00e7c47c382b30524e78b36ab047c16b8fcfef Mon Sep 17 00:00:00 2001 From: Alex Shi Date: Tue, 21 Jan 2020 16:34:05 +0800 Subject: [PATCH 4/7] ida: remove abandoned macros 3 IDA_ started macros aren't used from commit f32f004cddf8 ("ida: Convert to XArray"). so better to remove them. Signed-off-by: Alex Shi Signed-off-by: Matthew Wilcox (Oracle) --- lib/radix-tree.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/radix-tree.c b/lib/radix-tree.c index c8fa1d274530..2ee6ae3b0ade 100644 --- a/lib/radix-tree.c +++ b/lib/radix-tree.c @@ -55,14 +55,6 @@ struct kmem_cache *radix_tree_node_cachep; RADIX_TREE_MAP_SHIFT)) #define IDR_PRELOAD_SIZE (IDR_MAX_PATH * 2 - 1) -/* - * The IDA is even shorter since it uses a bitmap at the last level. - */ -#define IDA_INDEX_BITS (8 * sizeof(int) - 1 - ilog2(IDA_BITMAP_BITS)) -#define IDA_MAX_PATH (DIV_ROUND_UP(IDA_INDEX_BITS, \ - RADIX_TREE_MAP_SHIFT)) -#define IDA_PRELOAD_SIZE (IDA_MAX_PATH * 2 - 1) - /* * Per-cpu pool of preloaded nodes */ From d8e93e3f22d9fd2e6a3ccae3623c3af8789ccfc0 Mon Sep 17 00:00:00 2001 From: "Matthew Wilcox (Oracle)" Date: Thu, 27 Feb 2020 07:37:40 -0500 Subject: [PATCH 5/7] XArray: Optimise xas_sibling() if !CONFIG_XARRAY_MULTI If CONFIG_XARRAY_MULTI is disabled, then xas_sibling() must be false. Reported-by: JaeJoon Jung Signed-off-by: Matthew Wilcox (Oracle) --- lib/xarray.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/xarray.c b/lib/xarray.c index 05324cf571f4..f448bcd263ac 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1836,7 +1836,7 @@ static bool xas_sibling(struct xa_state *xas) struct xa_node *node = xas->xa_node; unsigned long mask; - if (!node) + if (!IS_ENABLED(CONFIG_XARRAY_MULTI) || !node) return false; mask = (XA_CHUNK_SIZE << node->shift) - 1; return (xas->xa_index & mask) > From 34eee836a9dd3e1987c10ed6afc7ece4131a993d Mon Sep 17 00:00:00 2001 From: "Matthew Wilcox (Oracle)" Date: Thu, 27 Feb 2020 12:25:47 -0500 Subject: [PATCH 6/7] radix tree test suite: Support kmem_cache alignment The radix tree doesn't use alignment, so the argument was ignored. The maple tree needs its nodes to be aligned, so we need to pay attention to the alignment argument. Also change the types of 'size' and 'align' to unsigned int to match commit f4957d5bd0916. Signed-off-by: Matthew Wilcox (Oracle) --- tools/testing/radix-tree/linux.c | 32 +++++++++++++++++---------- tools/testing/radix-tree/linux/slab.h | 6 ++--- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/tools/testing/radix-tree/linux.c b/tools/testing/radix-tree/linux.c index 44a0d1ad4408..2d9c59df60de 100644 --- a/tools/testing/radix-tree/linux.c +++ b/tools/testing/radix-tree/linux.c @@ -19,37 +19,44 @@ int test_verbose; struct kmem_cache { pthread_mutex_t lock; - int size; + unsigned int size; + unsigned int align; int nr_objs; void *objs; void (*ctor)(void *); }; -void *kmem_cache_alloc(struct kmem_cache *cachep, int flags) +void *kmem_cache_alloc(struct kmem_cache *cachep, int gfp) { - struct radix_tree_node *node; + void *p; - if (!(flags & __GFP_DIRECT_RECLAIM)) + if (!(gfp & __GFP_DIRECT_RECLAIM)) return NULL; pthread_mutex_lock(&cachep->lock); if (cachep->nr_objs) { + struct radix_tree_node *node = cachep->objs; cachep->nr_objs--; - node = cachep->objs; cachep->objs = node->parent; pthread_mutex_unlock(&cachep->lock); node->parent = NULL; + p = node; } else { pthread_mutex_unlock(&cachep->lock); - node = malloc(cachep->size); + if (cachep->align) + posix_memalign(&p, cachep->align, cachep->size); + else + p = malloc(cachep->size); if (cachep->ctor) - cachep->ctor(node); + cachep->ctor(p); + else if (gfp & __GFP_ZERO) + memset(p, 0, cachep->size); } uatomic_inc(&nr_allocated); if (kmalloc_verbose) - printf("Allocating %p from slab\n", node); - return node; + printf("Allocating %p from slab\n", p); + return p; } void kmem_cache_free(struct kmem_cache *cachep, void *objp) @@ -59,7 +66,7 @@ void kmem_cache_free(struct kmem_cache *cachep, void *objp) if (kmalloc_verbose) printf("Freeing %p to slab\n", objp); pthread_mutex_lock(&cachep->lock); - if (cachep->nr_objs > 10) { + if (cachep->nr_objs > 10 || cachep->align) { memset(objp, POISON_FREE, cachep->size); free(objp); } else { @@ -98,13 +105,14 @@ void kfree(void *p) } struct kmem_cache * -kmem_cache_create(const char *name, size_t size, size_t offset, - unsigned long flags, void (*ctor)(void *)) +kmem_cache_create(const char *name, unsigned int size, unsigned int align, + unsigned int flags, void (*ctor)(void *)) { struct kmem_cache *ret = malloc(sizeof(*ret)); pthread_mutex_init(&ret->lock, NULL); ret->size = size; + ret->align = align; ret->nr_objs = 0; ret->objs = NULL; ret->ctor = ctor; diff --git a/tools/testing/radix-tree/linux/slab.h b/tools/testing/radix-tree/linux/slab.h index a037def0dec6..2958830ce4d7 100644 --- a/tools/testing/radix-tree/linux/slab.h +++ b/tools/testing/radix-tree/linux/slab.h @@ -20,8 +20,8 @@ static inline void *kzalloc(size_t size, gfp_t gfp) void *kmem_cache_alloc(struct kmem_cache *cachep, int flags); void kmem_cache_free(struct kmem_cache *cachep, void *objp); -struct kmem_cache * -kmem_cache_create(const char *name, size_t size, size_t offset, - unsigned long flags, void (*ctor)(void *)); +struct kmem_cache *kmem_cache_create(const char *name, unsigned int size, + unsigned int align, unsigned int flags, + void (*ctor)(void *)); #endif /* SLAB_H */ From 7e934cf5ace1dceeb804f7493fa28bb697ed3c52 Mon Sep 17 00:00:00 2001 From: "Matthew Wilcox (Oracle)" Date: Thu, 12 Mar 2020 17:29:11 -0400 Subject: [PATCH 7/7] xarray: Fix early termination of xas_for_each_marked xas_for_each_marked() is using entry == NULL as a termination condition of the iteration. When xas_for_each_marked() is used protected only by RCU, this can however race with xas_store(xas, NULL) in the following way: TASK1 TASK2 page_cache_delete() find_get_pages_range_tag() xas_for_each_marked() xas_find_marked() off = xas_find_chunk() xas_store(&xas, NULL) xas_init_marks(&xas); ... rcu_assign_pointer(*slot, NULL); entry = xa_entry(off); And thus xas_for_each_marked() terminates prematurely possibly leading to missed entries in the iteration (translating to missing writeback of some pages or a similar problem). If we find a NULL entry that has been marked, skip it (unless we're trying to allocate an entry). Reported-by: Jan Kara CC: stable@vger.kernel.org Fixes: ef8e5717db01 ("page cache: Convert delete_batch to XArray") Signed-off-by: Matthew Wilcox (Oracle) --- include/linux/xarray.h | 6 +- lib/xarray.c | 2 + tools/testing/radix-tree/Makefile | 4 +- tools/testing/radix-tree/iteration_check_2.c | 87 ++++++++++++++++++++ tools/testing/radix-tree/main.c | 1 + tools/testing/radix-tree/test.h | 1 + 6 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 tools/testing/radix-tree/iteration_check_2.c diff --git a/include/linux/xarray.h b/include/linux/xarray.h index a491653d8882..14c893433139 100644 --- a/include/linux/xarray.h +++ b/include/linux/xarray.h @@ -1648,6 +1648,7 @@ static inline void *xas_next_marked(struct xa_state *xas, unsigned long max, xa_mark_t mark) { struct xa_node *node = xas->xa_node; + void *entry; unsigned int offset; if (unlikely(xas_not_node(node) || node->shift)) @@ -1659,7 +1660,10 @@ static inline void *xas_next_marked(struct xa_state *xas, unsigned long max, return NULL; if (offset == XA_CHUNK_SIZE) return xas_find_marked(xas, max, mark); - return xa_entry(xas->xa, node, offset); + entry = xa_entry(xas->xa, node, offset); + if (!entry) + return xas_find_marked(xas, max, mark); + return entry; } /* diff --git a/lib/xarray.c b/lib/xarray.c index f448bcd263ac..e9e641d3c0c3 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1208,6 +1208,8 @@ void *xas_find_marked(struct xa_state *xas, unsigned long max, xa_mark_t mark) } entry = xa_entry(xas->xa, xas->xa_node, xas->xa_offset); + if (!entry && !(xa_track_free(xas->xa) && mark == XA_FREE_MARK)) + continue; if (!xa_is_node(entry)) return entry; xas->xa_node = xa_to_node(entry); diff --git a/tools/testing/radix-tree/Makefile b/tools/testing/radix-tree/Makefile index 397d6b612502..aa6abfe0749c 100644 --- a/tools/testing/radix-tree/Makefile +++ b/tools/testing/radix-tree/Makefile @@ -7,8 +7,8 @@ LDLIBS+= -lpthread -lurcu TARGETS = main idr-test multiorder xarray CORE_OFILES := xarray.o radix-tree.o idr.o linux.o test.o find_bit.o bitmap.o OFILES = main.o $(CORE_OFILES) regression1.o regression2.o regression3.o \ - regression4.o \ - tag_check.o multiorder.o idr-test.o iteration_check.o benchmark.o + regression4.o tag_check.o multiorder.o idr-test.o iteration_check.o \ + iteration_check_2.o benchmark.o ifndef SHIFT SHIFT=3 diff --git a/tools/testing/radix-tree/iteration_check_2.c b/tools/testing/radix-tree/iteration_check_2.c new file mode 100644 index 000000000000..aac5c50a3674 --- /dev/null +++ b/tools/testing/radix-tree/iteration_check_2.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * iteration_check_2.c: Check that deleting a tagged entry doesn't cause + * an RCU walker to finish early. + * Copyright (c) 2020 Oracle + * Author: Matthew Wilcox + */ +#include +#include "test.h" + +static volatile bool test_complete; + +static void *iterator(void *arg) +{ + XA_STATE(xas, arg, 0); + void *entry; + + rcu_register_thread(); + + while (!test_complete) { + xas_set(&xas, 0); + rcu_read_lock(); + xas_for_each_marked(&xas, entry, ULONG_MAX, XA_MARK_0) + ; + rcu_read_unlock(); + assert(xas.xa_index >= 100); + } + + rcu_unregister_thread(); + return NULL; +} + +static void *throbber(void *arg) +{ + struct xarray *xa = arg; + + rcu_register_thread(); + + while (!test_complete) { + int i; + + for (i = 0; i < 100; i++) { + xa_store(xa, i, xa_mk_value(i), GFP_KERNEL); + xa_set_mark(xa, i, XA_MARK_0); + } + for (i = 0; i < 100; i++) + xa_erase(xa, i); + } + + rcu_unregister_thread(); + return NULL; +} + +void iteration_test2(unsigned test_duration) +{ + pthread_t threads[2]; + DEFINE_XARRAY(array); + int i; + + printv(1, "Running iteration test 2 for %d seconds\n", test_duration); + + test_complete = false; + + xa_store(&array, 100, xa_mk_value(100), GFP_KERNEL); + xa_set_mark(&array, 100, XA_MARK_0); + + if (pthread_create(&threads[0], NULL, iterator, &array)) { + perror("create iterator thread"); + exit(1); + } + if (pthread_create(&threads[1], NULL, throbber, &array)) { + perror("create throbber thread"); + exit(1); + } + + sleep(test_duration); + test_complete = true; + + for (i = 0; i < 2; i++) { + if (pthread_join(threads[i], NULL)) { + perror("pthread_join"); + exit(1); + } + } + + xa_destroy(&array); +} diff --git a/tools/testing/radix-tree/main.c b/tools/testing/radix-tree/main.c index 7a22d6e3732e..f2cbc8e5b97c 100644 --- a/tools/testing/radix-tree/main.c +++ b/tools/testing/radix-tree/main.c @@ -311,6 +311,7 @@ int main(int argc, char **argv) regression4_test(); iteration_test(0, 10 + 90 * long_run); iteration_test(7, 10 + 90 * long_run); + iteration_test2(10 + 90 * long_run); single_thread_tests(long_run); /* Free any remaining preallocated nodes */ diff --git a/tools/testing/radix-tree/test.h b/tools/testing/radix-tree/test.h index 1ee4b2c0ad10..34dab4d18744 100644 --- a/tools/testing/radix-tree/test.h +++ b/tools/testing/radix-tree/test.h @@ -34,6 +34,7 @@ void xarray_tests(void); void tag_check(void); void multiorder_checks(void); void iteration_test(unsigned order, unsigned duration); +void iteration_test2(unsigned duration); void benchmark(void); void idr_checks(void); void ida_tests(void);