Lib fixes for 7.3
Subsystems (8)
Liam R. Howlett (Google)
maintainercover Lib fixes for 7.3
Two bugs found by an LLM review, and one problem Lorenzo Stoakes reported off list.
An RCU reader that restarts after its node dies can dereference NULL if the tree was emptied, or holds a single entry in the root pointer. Patch 1 fixes it and patch 2 adds a test.
rhashtable can deadlock when an insert in atomic context grows the
table: the rehash may call vmalloc(), which takes locks that are not safe against interrupts. Patch 3 keeps non-blocking callers on kmalloc() or a nested table.
With CONFIG_DEBUG_MAPLE_TREE, MT_WARN_ON() and the other WARN macros print the tree and call dump_stack() rather than WARN(), so syzbot never sees them. Patch 4 makes them WARN once per call site, after the dump, and makes the BUG macros BUG() after theirs.
Chris Mason (1):
rhashtable: fix vmalloc() deadlock in atomic rehash
Liam R. Howlett (Google) (3):
maple_tree: Fix rcu reader NULL dereference after the tree is emptied
maple_tree: Test emptying tree while RCU reading
maple_tree: Update internal warning and bug_on code
include/linux/maple_tree.h | 54 +++++++++------- lib/maple_tree.c | 110 +++++++++++++++++++++++-------- lib/rhashtable.c | 18 +++++- lib/test_maple_tree.c | 129 +++++++++++++++++++++++++++++++++++++ 4 files changed, 257 insertions(+), 54 deletions(-)
Signature · 1 line
2.47.3
Liam R. Howlett (Google)
maintainerPATCH 1/4 maple_tree: Fix rcu reader NULL dereference after the tree is emptied
RCU readers may hit a NULL dereference if the tree is emptied while the walker is restarted from a dead node. Avoid the NULL dereference by checking for an emptied/pointer tree (0->ptr, 1-ULONG_MAX -> NULL) and set the maple state limits correctly.
Initial fix was generated by LLM and sent by Chris Mason in private.
--- lib/maple_tree.c | 110 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 83 insertions(+), 27 deletions(-)
lib/maple_tree.c +83 -27
@@ -3891,14 +3891,54 @@ static __always_inline void mas_rewalk(struct ma_state *mas, unsigned long index goto retry; } -static __always_inline bool mas_rewalk_if_dead(struct ma_state *mas, -struct maple_node *node,const unsigned long index) +/* +* mas_next_no_node() - Finish a next walk that restarted into a tree without +* nodes. +* @mas: The inactive maple state (ma_none or ma_root) +* +* The arrival here is either because there is a root value or nothing. Either +* way, the next entry is nothing. So set the limits and overwrite the status. +* +* Return: NULL +*/ +static inline void *mas_next_no_node (struct ma_state *mas) { -if (unlikely (ma_dead_node (node))) { -mas_rewalk (mas, index); -return true; +if (mas_is_ptr (mas)) + mas->index =1 ; +else + mas->index =0 ; + + mas->last =ULONG_MAX ; + mas->status = ma_none; +return NULL ; +} + +/* +* mas_prev_no_node() - Finish a prev walk that restarted into a tree without +* nodes. +* @mas: The inactive maple state (ma_none or ma_root) +* @min: The lowest index to return +* +* The arrival here is either because there is a root value or nothing. With +* nothing, leave the limits mas_walk() sets for an empty tree. A root value +* sits at 0, before the saved index: stop on it as mas_prev_setup() does. +* +* Return: The root entry if @min allows it, NULL otherwise. A node if the +* root changed again since the walk; the caller restarts. +*/ +static inline void *mas_prev_no_node (struct ma_state *mas,unsigned long min) +{ + mas->index =0 ; +if (mas_is_none (mas)) { + mas->last =ULONG_MAX ; +return NULL ; } -return false; + + mas->last =0 ; +if (min) +return NULL ; + +return mas_root (mas); }/*
@@ -4006,14 +4046,14 @@ static void *mas_prev_slot(struct ma_state *mas, unsigned long min, bool empty) node =mas_mn (mas); type =mte_node_type (mas->node); pivots =ma_pivots (node, type); -if (unlikely (mas_rewalk_if_dead (mas, node, save_point))) - goto retry; +if (unlikely (ma_dead_node (node))) + goto dead;if (mas->min <= min) { pivot =mas_safe_min (mas, pivots, mas->offset); -if (unlikely (mas_rewalk_if_dead (mas, node, save_point))) - goto retry; +if (unlikely (ma_dead_node (node))) + goto dead;if (pivot <= min) goto underflow;
@@ -4028,10 +4068,8 @@ static void *mas_prev_slot(struct ma_state *mas, unsigned long min, bool empty)if (mas->index <= min) goto underflow; -if (mas_prev_node (mas, min)) { -mas_rewalk (mas, save_point); - goto retry; - } +if (mas_prev_node (mas, min)) + goto dead;if (WARN_ON_ONCE (mas_is_underflow (mas)))return NULL ;
@@ -4045,8 +4083,8 @@ static void *mas_prev_slot(struct ma_state *mas, unsigned long min, bool empty) slots =ma_slots (node, type); entry =mas_slot (mas, slots, mas->offset); -if (unlikely (mas_rewalk_if_dead (mas, node, save_point))) - goto retry; +if (unlikely (ma_dead_node (node))) + goto dead;if (likely (entry))return entry;
@@ -4063,6 +4101,17 @@ static void *mas_prev_slot(struct ma_state *mas, unsigned long min, bool empty) underflow: mas->status = ma_underflow;return NULL ; + +dead: +mas_rewalk (mas, save_point); +if (likely (mas_is_active (mas))) + goto retry; + + entry =mas_prev_no_node (mas, min); +if (unlikely (xa_is_node (entry)))/* The root changed again */ + goto dead; + +return entry; }/*
@@ -4176,8 +4225,8 @@ static void *mas_next_slot(struct ma_state *mas, unsigned long max, bool empty) node =mas_mn (mas); type =mte_node_type (mas->node); pivots =ma_pivots (node, type); -if (unlikely (mas_rewalk_if_dead (mas, node, save_point))) - goto retry; +if (unlikely (ma_dead_node (node))) + goto dead;if (mas->max >= max) {if (likely (mas->offset < mas->end))
@@ -4185,8 +4234,8 @@ static void *mas_next_slot(struct ma_state *mas, unsigned long max, bool empty)else pivot = mas->max; -if (unlikely (mas_rewalk_if_dead (mas, node, save_point))) - goto retry; +if (unlikely (ma_dead_node (node))) + goto dead;if (pivot >= max) {/* Was at the limit, next will extend beyond */ mas->status = ma_overflow;
@@ -4208,10 +4257,8 @@ static void *mas_next_slot(struct ma_state *mas, unsigned long max, bool empty)return NULL ; } -if (mas_next_node (mas, node, max)) { -mas_rewalk (mas, save_point); - goto retry; - } +if (mas_next_node (mas, node, max)) + goto dead;if (WARN_ON_ONCE (mas_is_overflow (mas)))return NULL ;
@@ -4226,8 +4273,8 @@ static void *mas_next_slot(struct ma_state *mas, unsigned long max, bool empty) slots =ma_slots (node, type); entry =mt_slot (mas->tree, slots, mas->offset); -if (unlikely (mas_rewalk_if_dead (mas, node, save_point))) - goto retry; +if (unlikely (ma_dead_node (node))) + goto dead;if (entry)return entry;
@@ -4244,6 +4291,13 @@ static void *mas_next_slot(struct ma_state *mas, unsigned long max, bool empty) }return entry; + +dead: +mas_rewalk (mas, save_point); +if (likely (mas_is_active (mas))) + goto retry; + +return mas_next_no_node (mas); }/*
@@ -5450,7 +5504,9 @@ void *mas_find(struct ma_state *mas, unsigned long max)/* Retries on dead nodes handled by mas_next_slot */ entry =mas_next_slot (mas, max, false);/* Ignore overflow */ - mas->status = ma_active; +if (mas_is_overflow (mas)) + mas->status = ma_active; +return entry; }EXPORT_SYMBOL_GPL (mas_find);
Signature · 1 line
2.47.3
Liam R. Howlett (Google)
maintainerPATCH 2/4 maple_tree: Test emptying tree while RCU reading
In reply to Liam R. Howlett (Google)Add a test case for the rcu reader being restarted by emptying the tree.
--- lib/test_maple_tree.c | 129 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+)
lib/test_maple_tree.c +129 -0
@@ -3740,6 +3740,134 @@ static noinline void __init check_range64_in_rcu(struct maple_tree *mt)rcu_barrier (); } +/* +* A reader that restarts after its leaf died may find a tree without nodes: +* emptied, or emptied and then given a single entry at 0. Each case starts +* from a tree with nodes, built by check_seq(). The stores run while the +* reader holds rcu_read_lock, so they use GFP_ATOMIC. +*/ +static noinline void __init check_restart_no_node(struct maple_tree *mt) +{ +void *ptr =xa_mk_value (1234 ); +void *entry; +MA_STATE (mas, mt,0 ,0 ); + +MT_BUG_ON (mt, !mtree_empty (mt)); +mt_set_non_kernel (99999 ); + +/* find: the tree is emptied */ +mt_init_flags (mt,MT_FLAGS_USE_RCU ); +check_seq (mt,100 , false); +rcu_read_lock (); +mas_set (&mas,0 ); + entry =mas_find (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=xa_mk_value (0 )); +MT_BUG_ON (mt,mtree_store_range (mt,0 ,ULONG_MAX ,NULL ,GFP_ATOMIC )); + entry =mas_find (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_none); +MT_BUG_ON (mt, mas.index !=0 ); +MT_BUG_ON (mt, mas.last !=ULONG_MAX ); + entry =mas_find (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_none); +rcu_read_unlock (); +mtree_destroy (mt); +rcu_barrier (); + +/* next: the tree is emptied */ +mt_init_flags (mt,MT_FLAGS_USE_RCU ); +check_seq (mt,100 , false); +rcu_read_lock (); +mas_set (&mas,0 ); + entry =mas_find (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=xa_mk_value (0 )); +MT_BUG_ON (mt,mtree_store_range (mt,0 ,ULONG_MAX ,NULL ,GFP_ATOMIC )); + entry =mas_next (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_none); +MT_BUG_ON (mt, mas.index !=0 ); +MT_BUG_ON (mt, mas.last !=ULONG_MAX ); +rcu_read_unlock (); +mtree_destroy (mt); +rcu_barrier (); + +/* find: the tree is emptied, then holds one entry at 0 */ +mt_init_flags (mt,MT_FLAGS_USE_RCU ); +check_seq (mt,100 , false); +rcu_read_lock (); +mas_set (&mas,0 ); + entry =mas_find (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=xa_mk_value (0 )); +MT_BUG_ON (mt,mtree_store_range (mt,0 ,ULONG_MAX ,NULL ,GFP_ATOMIC )); +MT_BUG_ON (mt,mtree_store (mt,0 , ptr,GFP_ATOMIC )); + entry =mas_find (&mas,ULONG_MAX ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_none); +MT_BUG_ON (mt, mas.index !=1 ); +MT_BUG_ON (mt, mas.last !=ULONG_MAX ); +rcu_read_unlock (); +mtree_destroy (mt); +rcu_barrier (); + +/* find_rev: the tree is emptied */ +mt_init_flags (mt,MT_FLAGS_USE_RCU ); +check_seq (mt,100 , false); +rcu_read_lock (); +mas_set (&mas,ULONG_MAX ); + entry =mas_find_rev (&mas,0 ); +MT_BUG_ON (mt, entry !=xa_mk_value (100 )); +MT_BUG_ON (mt,mtree_store_range (mt,0 ,ULONG_MAX ,NULL ,GFP_ATOMIC )); + entry =mas_find_rev (&mas,0 ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_none); +MT_BUG_ON (mt, mas.index !=0 ); +MT_BUG_ON (mt, mas.last !=ULONG_MAX ); + entry =mas_find_rev (&mas,0 ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_none); +rcu_read_unlock (); +mtree_destroy (mt); +rcu_barrier (); + +/* prev: the tree is emptied, then holds one entry at 0 */ +mt_init_flags (mt,MT_FLAGS_USE_RCU ); +check_seq (mt,100 , false); +rcu_read_lock (); +mas_set (&mas,ULONG_MAX ); + entry =mas_find_rev (&mas,0 ); +MT_BUG_ON (mt, entry !=xa_mk_value (100 )); +MT_BUG_ON (mt,mtree_store_range (mt,0 ,ULONG_MAX ,NULL ,GFP_ATOMIC )); +MT_BUG_ON (mt,mtree_store (mt,0 , ptr,GFP_ATOMIC )); + entry =mas_prev (&mas,0 ); +MT_BUG_ON (mt, entry != ptr); +MT_BUG_ON (mt, mas.status != ma_root); +MT_BUG_ON (mt, mas.index !=0 ); +MT_BUG_ON (mt, mas.last !=0 ); +rcu_read_unlock (); +mtree_destroy (mt); +rcu_barrier (); + +/* find_rev: the entry at 0 is below the minimum */ +mt_init_flags (mt,MT_FLAGS_USE_RCU ); +check_seq (mt,100 , false); +rcu_read_lock (); +mas_set (&mas,ULONG_MAX ); + entry =mas_find_rev (&mas,1 ); +MT_BUG_ON (mt, entry !=xa_mk_value (100 )); +MT_BUG_ON (mt,mtree_store_range (mt,0 ,ULONG_MAX ,NULL ,GFP_ATOMIC )); +MT_BUG_ON (mt,mtree_store (mt,0 , ptr,GFP_ATOMIC )); + entry =mas_find_rev (&mas,1 ); +MT_BUG_ON (mt, entry !=NULL ); +MT_BUG_ON (mt, mas.status != ma_root); +MT_BUG_ON (mt, mas.index !=0 ); +MT_BUG_ON (mt, mas.last !=0 ); +rcu_read_unlock (); +mtree_destroy (mt); +rcu_barrier (); +mt_set_non_kernel (0 ); +} +static DEFINE_MTREE (tree);static int __init maple_tree_seed (void ) {
@@ -4031,6 +4159,7 @@ static int __init maple_tree_seed(void)check_range64_in_rcu (&tree); +check_restart_no_node (&tree);#if defined(BENCH ) skip:
Signature · 1 line
2.47.3
Liam R. Howlett (Google)
maintainerPATCH 3/4 maple_tree: Update internal warning and bug_on code
In reply to Liam R. Howlett (Google)When CONFIG_DEBUG_MAPLE_TREE is set, call the WARN_ON_ONCE() after the custom information is dumped about the tree. Without using the kernel's WARN_ON calls, syzbot will not catch issues detected by this code.
Update the non-debug code to call WARN_ON_ONCE() as well.
--- include/linux/maple_tree.h | 54 +++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 24 deletions(-)
include/linux/maple_tree.h +30 -24
@@ -680,7 +680,7 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \
@@ -696,7 +696,7 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \
@@ -713,62 +713,68 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \ } while (0)#define MT_WARN_ON (__tree, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mt_dump(__tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })#define MAS_WARN_ON (__mas, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mas_dump(__mas); \ mt_dump((__mas)->tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })#define MAS_WR_WARN_ON (__wrmas, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mas_wr_dump(__wrmas); \ mas_dump((__wrmas)->mas); \ mt_dump((__wrmas)->mas->tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })
@@ -776,9 +782,9 @@ void mt_cache_shrink(void);#define MT_BUG_ON (__tree, __x) BUG_ON(__x)#define MAS_BUG_ON (__mas, __x) BUG_ON(__x)#define MAS_WR_BUG_ON (__mas, __x) BUG_ON(__x) -#define MT_WARN_ON (__tree, __x) WARN_ON(__x) -#define MAS_WARN_ON (__mas, __x) WARN_ON(__x) -#define MAS_WR_WARN_ON (__mas, __x) WARN_ON(__x) +#define MT_WARN_ON (__tree, __x) WARN_ON_ONCE(__x) +#define MAS_WARN_ON (__mas, __x) WARN_ON_ONCE(__x) +#define MAS_WR_WARN_ON (__mas, __x) WARN_ON_ONCE(__x)#endif /* CONFIG_DEBUG_MAPLE_TREE */ /**
Signature · 1 line
2.47.3
Re: [PATCH 3/4] maple_tree: Update internal warning and bug_on code
ma
Quoted 3 lines · Liam R. Howlett (Google)
When
CONFIG_DEBUG_MAPLE_TREEis set, call theWARN_ON_ONCE()after the custom information is dumped about the tree. Without using the kernel'sWARN_ONcalls, syzbot will not catch issues detected by this code.
Mega nit but commit message doesn't mention the BUG_ON() bits :)
Quoted 6 lines
Update the non-debug code to call
WARN_ON_ONCE()as well.Cc <[email protected]>Reported-by Lorenzo Stoakes <[email protected]>Fixes bf857ddd21d0b ("maple_tree: move debug check to __mas_set_range()")
LLM overlord says bf857ddd21d0b only moved it, and f0a1f866aba1 actually implemented it.
Quoted 1 line
Signed-off-by Liam R. Howlett (Google) <[email protected]>
Looks sensible to me from kernel perspective, but LLM overlord tells me this changes the userland tests from asserting on dump_stack() to just printing something out on WARN_ON().
(BUG_ON() is OK, it asserts already).
However we really do need this for the kernel so CONFIG_DEBUG_MAPLE_TREE triggers syzbot and such, so maybe the userland bit can be a follow-up?
On that basis:
Or... maybe just respin with something actually, can fix it by updating the shared maple tree testing header to make warn do something like this (schlopped, apologies):
tools/testing/shared/maple-shared.h +23 -0
@@ -10,6 +10,30 @@#include <time.h> #include "linux/init.h" +/* +* The tools/include/asm/bug.h WARN*() family only prints a message and +* carries on. Nothing in the maple tree or VMA tests expects a warning, so +* treat one as a failure and abort at the site. +*/ +#undef WARN +#undef WARN_ON +#undef WARN_ON_ONCE +#undef WARN_ONCE + +#define WARN(condition, format...) ({ \ + int __ret_warn_on = !!(condition); \ + if (unlikely(__ret_warn_on)) { \ + fprintf(stderr, "WARN at %s:%d: ", __FILE__, __LINE__); \ + fprintf(stderr, format); \ + abort(); \ + } \ + unlikely(__ret_warn_on); \ +}) + +#define WARN_ON (condition) WARN(condition, "%s\n", #condition) +#define WARN_ON_ONCE (condition) WARN_ON(condition) +#define WARN_ONCE(condition, format...) WARN(condition, format)
Quoted 128 lines
--- include/linux/maple_tree.h | 54 +++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 24 deletions(-)include/linux/maple_tree.h +30
-25@@ -680,7 +680,7 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \@@ -696,7 +696,7 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \@@ -713,62 +713,68 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \ } while (0)#define MT_WARN_ON (__tree, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mt_dump(__tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })#define MAS_WARN_ON (__mas, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mas_dump(__mas); \ mt_dump((__mas)->tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })#define MAS_WR_WARN_ON (__wrmas, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mas_wr_dump(__wrmas); \ mas_dump((__wrmas)->mas); \ mt_dump((__wrmas)->mas->tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })@@ -776,9 +782,9 @@ void mt_cache_shrink(void);#define MT_BUG_ON (__tree, __x) BUG_ON(__x)#define MAS_BUG_ON (__mas, __x) BUG_ON(__x)#define MAS_WR_BUG_ON (__mas, __x) BUG_ON(__x) -#define MT_WARN_ON (__tree, __x) WARN_ON(__x) -#define MAS_WARN_ON (__mas, __x) WARN_ON(__x) -#define MAS_WR_WARN_ON (__mas, __x) WARN_ON(__x) +#define MT_WARN_ON (__tree, __x) WARN_ON_ONCE(__x) +#define MAS_WARN_ON (__mas, __x) WARN_ON_ONCE(__x) +#define MAS_WR_WARN_ON (__mas, __x) WARN_ON_ONCE(__x)#endif /* CONFIG_DEBUG_MAPLE_TREE */ /** --
Quoted 2 lines
2.47.3
--
Cheers, Lorenzo
Liam R. Howlett (Google)
maintainerRe: [PATCH 3/4] maple_tree: Update internal warning and bug_on code
Quoted 20 lines · Lorenzo Stoakes (ARM)
ma
Quoted 3 lines · Liam R. Howlett (Google)
When
CONFIG_DEBUG_MAPLE_TREEis set, call theWARN_ON_ONCE()after the custom information is dumped about the tree. Without using the kernel'sWARN_ONcalls, syzbot will not catch issues detected by this code.Mega nit but commit message doesn't mention the
BUG_ON()bits :)Quoted 6 lines
Update the non-debug code to call
WARN_ON_ONCE()as well.Cc <[email protected]>Reported-by Lorenzo Stoakes <[email protected]>Fixes bf857ddd21d0b ("maple_tree: move debug check to __mas_set_range()")LLM overlord says
bf857ddd21d0bonly moved it, andf0a1f866aba1actually implemented it.Quoted 1 line
Signed-off-by Liam R. Howlett (Google) <[email protected]>
Quoted 8 lines
Looks sensible to me from kernel perspective, but LLM overlord tells me this changes the userland tests from asserting on
dump_stack()to just printing something out onWARN_ON().(
BUG_ON()is OK, it asserts already).However we really do need this for the kernel so
CONFIG_DEBUG_MAPLE_TREEtriggers syzbot and such, so maybe the userland bit can be a follow-up?
Wow, so any WARN() before... I'm trying not to think about it.
The change below is good to be squashed into the fix I sent. It also makes vma warnings fatal - which should also happen.
Andrew, can you take care of this or would you rather a respin?
Quoted 174 lines
On that basis:
Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>Or... maybe just respin with something actually, can fix it by updating the shared maple tree testing header to make warn do something like this (schlopped, apologies):
tools/testing/shared/maple-shared.h +23
-0@@ -10,6 +10,30 @@#include <time.h> #include "linux/init.h" +/* +* The tools/include/asm/bug.h WARN*() family only prints a message and +* carries on. Nothing in the maple tree or VMA tests expects a warning, so +* treat one as a failure and abort at the site. +*/ +#undef WARN +#undef WARN_ON +#undef WARN_ON_ONCE +#undef WARN_ONCE + +#define WARN(condition, format...) ({ \ + int __ret_warn_on = !!(condition); \ + if (unlikely(__ret_warn_on)) { \ + fprintf(stderr, "WARN at %s:%d: ", __FILE__, __LINE__); \ + fprintf(stderr, format); \ + abort(); \ + } \ + unlikely(__ret_warn_on); \ +}) + +#define WARN_ON (condition) WARN(condition, "%s\n", #condition) +#define WARN_ON_ONCE (condition) WARN_ON(condition) +#define WARN_ONCE(condition, format...) WARN(condition, format)Quoted 128 lines
--- include/linux/maple_tree.h | 54 +++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 24 deletions(-)include/linux/maple_tree.h +30
-25@@ -680,7 +680,7 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \@@ -696,7 +696,7 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \@@ -713,62 +713,68 @@ void mt_cache_shrink(void);pr_info ("Pass: %u Run:%u\n" , \atomic_read (&maple_tree_tests_passed), \atomic_read (&maple_tree_tests_run)); \ -dump_stack (); \ +BUG_ON (1 ); \ }else { \atomic_inc (&maple_tree_tests_passed); \ } \ } while (0)#define MT_WARN_ON (__tree, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mt_dump(__tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })#define MAS_WARN_ON (__mas, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mas_dump(__mas); \ mt_dump((__mas)->tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })#define MAS_WR_WARN_ON (__wrmas, __x) ({ \ + static bool __warned; \ int ret = !!(__x); \ atomic_inc(&maple_tree_tests_run); \ - if (ret) { \ - pr_info("WARN at %s:%d (%u)\n", \ - __func__, __LINE__, __x); \ + if (!ret) { \ + atomic_inc(&maple_tree_tests_passed); \ + } else if (!__warned) { \ + __warned = true; \ + pr_info("WARN at %s:%d\n", \ + __func__, __LINE__); \ mas_wr_dump(__wrmas); \ mas_dump((__wrmas)->mas); \ mt_dump((__wrmas)->mas->tree, mt_dump_hex); \ pr_info("Pass: %u Run:%u\n", \ atomic_read(&maple_tree_tests_passed), \ atomic_read(&maple_tree_tests_run)); \ - dump_stack(); \ - } else { \ - atomic_inc(&maple_tree_tests_passed); \ + WARN_ON(1); \ } \ unlikely(ret); \ })@@ -776,9 +782,9 @@ void mt_cache_shrink(void);#define MT_BUG_ON (__tree, __x) BUG_ON(__x)#define MAS_BUG_ON (__mas, __x) BUG_ON(__x)#define MAS_WR_BUG_ON (__mas, __x) BUG_ON(__x) -#define MT_WARN_ON (__tree, __x) WARN_ON(__x) -#define MAS_WARN_ON (__mas, __x) WARN_ON(__x) -#define MAS_WR_WARN_ON (__mas, __x) WARN_ON(__x) +#define MT_WARN_ON (__tree, __x) WARN_ON_ONCE(__x) +#define MAS_WARN_ON (__mas, __x) WARN_ON_ONCE(__x) +#define MAS_WR_WARN_ON (__mas, __x) WARN_ON_ONCE(__x)#endif /* CONFIG_DEBUG_MAPLE_TREE */ /** -- Quoted 2 lines
2.47.3
--
Cheers, Lorenzo
Liam R. Howlett (Google)
maintainerPATCH 4/4 rhashtable: fix vmalloc() deadlock in atomic rehash
In reply to Liam R. Howlett (Google)From: Chris Mason <[email protected]>
An unprivileged user can deadlock two CPUs by filling the forwarding database (FDB) of a bridge in their own network namespace. A peer that floods a bridge port with frames from new source addresses can fill the FDB too. The flood alone was never seen to cause a deadlock. Together with local tasks that call vmalloc() in a loop on the CPU that receives the frames, it can deadlock that CPU. Other CPUs that call vmalloc() or vfree() after that can hang too.
The FDB holds the MAC addresses that the bridge knows. It is an rhashtable, a hash table that grows as it fills.
An rhashtable normally grows from a worker. If an insert finds the table too full before the worker has run, rhashtable_insert_rehash() allocates the larger table immediately, with GFP_ATOMIC, in the context of the insert. bucket_table_alloc() uses kvmalloc() for that.
kvmalloc() tries kmalloc() first. Since v6.19, if kmalloc() fails, it falls back to vmalloc() for non-blocking GFP flags too, and kmalloc() always fails above KMALLOC_MAX_SIZE.
vmalloc() and vfree() take their spinlocks with bottom halves enabled. Such a lock must not be taken in a softirq, or while holding a lock that a softirq takes. In the bridge, an insert that falls back to vmalloc() breaks the second rule: the bridge inserts into its FDB under br->hash_lock, and its receive softirq takes br->hash_lock as well.
Below, one CPU adds an FDB entry for an RTM_NEWNEIGH netlink request, while another CPU is inside vmalloc() or vfree():
RTM_NEWNEIGH vmalloc() or vfree()
============ ====================
spin_lock_bh(&br->hash_lock)
spin_lock(&vn->busy.lock)
rhashtable_insert_rehash()
kvmalloc(GFP_ATOMIC)
vmalloc()
spin_lock(&vn->busy.lock)
waits for the other CPU
softirq: a frame for the bridge
spin_lock(&br->hash_lock)
waits for RTM_NEWNEIGHThe receive softirq also inserts into the FDB itself: on a learning port, br_fdb_update() adds the source address of a frame if the FDB does not have it yet. If that insert falls back to vmalloc(), it breaks the first rule. The CPU then deadlocks with itself if the softirq has interrupted a task that holds the same vmalloc lock.
Reproduced on x86-64 under emulation, with three things going on at the same time:
- RTM_NEWNEIGH requests filled an FDB past 262,144 entries, where the next table is too large for kmalloc() on x86-64.
- Frames arrived on a veth port.
- Tasks called vmalloc() in a loop.
Two CPUs ended up spinning on each other's lock.
The softirq path was reproduced too, on x86-64 under emulation and with RPS set by root. Repeated floods of 280,000 source addresses on a veth port, while tasks called vmalloc() in a loop on the CPU that received the frames, ended in a soft lockup in alloc_vmap_area() in 6 of 6 boots. Without these tasks, on a kernel with no debug options, none of 33 floods reached vmalloc().
Fix this by calling kmalloc() instead of kvmalloc() in bucket_table_alloc() if the GFP flags do not allow blocking.
Known gaps:
- By code inspection, bpf_iter_tcp_realloc_batch() and the dm-bufio clients that set no_sleep also call a non-blocking vmalloc() under spin_lock_bh(), and can still deadlock in the same way. The later commit "mm/vmalloc: fix deadlock in softirq and BH-off callers" fixes them.
- If kmalloc() fails, the atomic rehash falls back to a nested table, as before v6.19. A nested table is built from single pages, so it needs no large allocation. An insert then fails in two cases. If the nested table cannot get a page, the insert fails with -ENOMEM or -EBUSY. If the new table has 512 buckets, that is too few for a nested table, and the insert fails with -ENOMEM.
- A lookup in a nested table follows up to three more pointers until the worker has rehashed once more.
Changes from the LLM patch:
Dropped the __GFP_NOWARN from the kmalloc_noprof() when gfpflags_allow_blocking() is false since the only non-blocking caller already sets the flag.
Tried to make the alloc_hooks_tag() multi-line call more readable (whitespace changes only).
--- lib/rhashtable.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
lib/rhashtable.c +15 -3
@@ -193,9 +193,21 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,int i;static struct lock_class_key __key; - tbl =alloc_hooks_tag (ht->alloc_tag, -kvmalloc_node_align_noprof (struct_size (tbl, buckets, nbuckets), -1 , gfp|__GFP_ZERO,NUMA_NO_NODE )); +/* +* vmalloc() takes locks that are not safe against interrupts, and the +* emergency rehash runs in whatever context the insert runs in. So a +* caller that cannot block gets slab memory, or else a nested table. +*/ +if (gfpflags_allow_blocking (gfp)) + tbl =alloc_hooks_tag (ht->alloc_tag, +kvmalloc_node_align_noprof ( +struct_size (tbl, buckets, nbuckets), +1 , gfp | __GFP_ZERO,NUMA_NO_NODE )); +else + tbl =alloc_hooks_tag (ht->alloc_tag, +kmalloc_noprof ( +struct_size (tbl, buckets, nbuckets), + gfp | __GFP_ZERO)); size = nbuckets;
Signature · 1 line
2.47.3
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 4 lines · Liam R. Howlett (Google)
Below, one CPU adds an FDB entry for an
RTM_NEWNEIGHnetlink request, while another CPU is insidevmalloc()orvfree():
Quoted 12 lines
RTM_NEWNEIGH vmalloc() or vfree()
============ ====================
spin_lock_bh(&br->hash_lock)
spin_lock(&vn->busy.lock)
rhashtable_insert_rehash()
kvmalloc(GFP_ATOMIC)
vmalloc()
spin_lock(&vn->busy.lock)
waits for the other CPU
softirq: a frame for the bridge
spin_lock(&br->hash_lock)
waits for RTM_NEWNEIGHI think this is a fundamental bug in the kvmalloc non-blocking support patch. How can it be used at all if it is susceptible to such dead-locks?
And if you can't use it safely why don't we just get rid of the broken kvmalloc non-block path and have it always go to kmalloc like it did before?
Quoted 1 line
Fixes c6307674ed82 ("mm: kvmalloc: add non-blocking support for vmalloc")
BTW, rhashtable patches should be sent to linux-crypto.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 18 lines · Herbert Xu
Quoted 4 lines · Liam R. Howlett (Google)
Below, one CPU adds an FDB entry for an
RTM_NEWNEIGHnetlink request, while another CPU is insidevmalloc()orvfree():Quoted 12 lines
RTM_NEWNEIGH vmalloc() or vfree() ============ ==================== spin_lock_bh(&br->hash_lock) spin_lock(&vn->busy.lock) rhashtable_insert_rehash() kvmalloc(GFP_ATOMIC) vmalloc() spin_lock(&vn->busy.lock) waits for the other CPU softirq: a frame for the bridge spin_lock(&br->hash_lock) waits for RTM_NEWNEIGH
Quoted 8 lines
I think this is a fundamental bug in the kvmalloc non-blocking support patch. How can it be used at all if it is susceptible to such dead-locks?
And if you can't use it safely why don't we just get rid of the broken kvmalloc non-block path and have it always go to kmalloc like it did before?
If we have a look at this as we did not have vmalloc() here:
RTM_NEWNEIGH foo() or ...
============ ====================
lock_bh(&br->hash_lock)
foo()
lock(B)
rhashtable_insert_rehash()
foo()
lock(B)
waits for the other CPU
softirq: a frame for the bridge
lock(&br->hash_lock)
waits for RTM_NEWNEIGHSame issue?
I have a question:
GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee not hitting ABBA dead-locks or they are not interrupted?
--
Uladzislau Rezki
Liam R. Howlett (Google)
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Sorry for missing the ML in my initial email, I missed it from get_maintainers. I should have really clued in with such a small cc list ;)
Quoted 43 lines · Uladzislau Rezki
Quoted 18 lines · Herbert Xu
Quoted 4 lines · Liam R. Howlett (Google)
Below, one CPU adds an FDB entry for an
RTM_NEWNEIGHnetlink request, while another CPU is insidevmalloc()orvfree():Quoted 12 lines
RTM_NEWNEIGH vmalloc() or vfree() ============ ==================== spin_lock_bh(&br->hash_lock) spin_lock(&vn->busy.lock) rhashtable_insert_rehash() kvmalloc(GFP_ATOMIC) vmalloc() spin_lock(&vn->busy.lock) waits for the other CPU softirq: a frame for the bridge spin_lock(&br->hash_lock) waits for RTM_NEWNEIGHQuoted 8 lines
I think this is a fundamental bug in the kvmalloc non-blocking support patch. How can it be used at all if it is susceptible to such dead-locks?
And if you can't use it safely why don't we just get rid of the broken kvmalloc non-block path and have it always go to kmalloc like it did before?
If we have a look at this as we did not have
vmalloc()here:RTM_NEWNEIGH foo() or ... ============ ==================== lock_bh(&br->hash_lock) foo() lock(B) rhashtable_insert_rehash() foo() lock(B) waits for the other CPU softirq: a frame for the bridge lock(&br->hash_lock) waits for RTM_NEWNEIGH
Quoted 1 line
Same issue?
Looks like it to me.
Quoted 5 lines
I have a question:
GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee not hitting ABBA dead-locks or they are not interrupted?
GFP_ATOMIC and GFP_NOWAIT can be interrupted. It's more about not waiting for reclaim (kswapd might still run) and GFP_ATOMIC might dip into reserves.
The issue is that kvmalloc() now falls back to vmalloc() for non-blocking flags (I am restating this to make sure I'm not missing something here, please correct me).
So this changes what is called internal to rhastable so it doesn't reach vmalloc(), like before, if the caller cannot block - they go to kmalloc() with the old fall back.
I *think* the question is if the change should be here in the caller, internal to mm, or a new call to opt-in.
Considering this is the only kvmalloc() caller that forwards non-blocking gfp from BH lock + softirq context, I think that it makes sense to do it in the caller.
Given where we are in the release, this fixes a real issue that can happen so maybe we take this now and settle the general handling later?
Thanks,
Liam
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 59 lines · Liam R. Howlett (Google)
Sorry for missing the ML in my initial email, I missed it from
get_maintainers. I should have really clued in with such a small cc list ;)Quoted 43 lines · Uladzislau Rezki
Quoted 18 lines · Herbert Xu
Quoted 4 lines · Liam R. Howlett (Google)
Below, one CPU adds an FDB entry for an
RTM_NEWNEIGHnetlink request, while another CPU is insidevmalloc()orvfree():Quoted 12 lines
RTM_NEWNEIGH vmalloc() or vfree() ============ ==================== spin_lock_bh(&br->hash_lock) spin_lock(&vn->busy.lock) rhashtable_insert_rehash() kvmalloc(GFP_ATOMIC) vmalloc() spin_lock(&vn->busy.lock) waits for the other CPU softirq: a frame for the bridge spin_lock(&br->hash_lock) waits for RTM_NEWNEIGHQuoted 8 lines
I think this is a fundamental bug in the kvmalloc non-blocking support patch. How can it be used at all if it is susceptible to such dead-locks?
And if you can't use it safely why don't we just get rid of the broken kvmalloc non-block path and have it always go to kmalloc like it did before?
If we have a look at this as we did not have
vmalloc()here:RTM_NEWNEIGH foo() or ... ============ ==================== lock_bh(&br->hash_lock) foo() lock(B) rhashtable_insert_rehash() foo() lock(B) waits for the other CPU softirq: a frame for the bridge lock(&br->hash_lock) waits for RTM_NEWNEIGHQuoted 1 line
Same issue?
Looks like it to me.
Quoted 5 lines
I have a question:
GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee not hitting ABBA dead-locks or they are not interrupted?
Quoted 7 lines
GFP_ATOMICandGFP_NOWAITcan be interrupted. It's more about not waiting for reclaim (kswapd might still run) andGFP_ATOMICmight dip into reserves.The issue is that
kvmalloc()now falls back tovmalloc()for non-blocking flags (I am restating this to make sure I'm not missing something here, please correct me).
Isn't the real problem that vmalloc should use irqsave version of the spinlock to preserve the locking constrains of the caller?
Signature · 2 lines
Michal Hocko SUSE Labs
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
In reply to Liam R. Howlett (Google)Quoted 3 lines · Liam R. Howlett (Google)
Given where we are in the release, this fixes a real issue that can happen so maybe we take this now and settle the general handling later?
There is no rush at all since the bug that you found has existed since 2025:
commit c6307674ed82c0c57d6e1e3408e84ac449ab8e94
Author: Uladzislau Rezki (Sony) <[email protected]> Date: Tue Oct 7 14:20:35 2025 +0200
mm: kvmalloc: add non-blocking support for vmalloc
Let's take the time to fix this properly.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 8 lines · Liam R. Howlett (Google)
Quoted 3 lines
Given where we are in the release, this fixes a real issue that can happen so maybe we take this now and settle the general handling later?
There is no rush at all since the bug that you found has existed since 2025:
Quoted 8 lines
commit
c6307674ed82c0c57d6e1e3408e84ac449ab8e94Author: Uladzislau Rezki (Sony) <[email protected]> Date: Tue Oct 7 14:20:35 2025 +0200mm: kvmalloc: add non-blocking support for vmalloc
Let's take the time to fix this properly.
The problem with spin_lock_irqsave() is that vmalloc() path can take time to alloc and map pages. I am not sure this is worth to switch to irq-save locking primitive. So this is the "heaviest" allocator in the kernel. Callers have to be careful how the call vmalloc().
IMO, this caller has to be fixed.
--
Uladzislau Rezki
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 6 lines · Uladzislau Rezki
The problem with
spin_lock_irqsave()is thatvmalloc()path can take time to alloc and map pages. I am not sure this is worth to switch to irq-save locking primitive. So this is the "heaviest" allocator in the kernel. Callers have to be careful how the callvmalloc().IMO, this caller has to be fixed.
Which caller, though? Are you saying rhashtable_insert_rehash() needs to call kmalloc() instead of kvmalloc()?
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Hello, Matthew!
Quoted 11 lines · Uladzislau Rezki
Quoted 6 lines
The problem with
spin_lock_irqsave()is thatvmalloc()path can take time to alloc and map pages. I am not sure this is worth to switch to irq-save locking primitive. So this is the "heaviest" allocator in the kernel. Callers have to be careful how the callvmalloc().IMO, this caller has to be fixed.
Which caller, though? Are you saying
rhashtable_insert_rehash()needs to callkmalloc()instead ofkvmalloc()?
The problem is that we used to BUG_ON() in vmalloc if soft-irq or BH is disabled, i.e. for vmalloc it is incorrect state.
See https://lore.kernel.org/all/[email protected]/
Two patches to address this:
1. https://lore.kernel.org/all/[email protected]/
2. https://lore.kernel.org/all/[email protected]/
second can be updated and resent as it checks if the context does soft irq processing whereas it allows:
bh-disable;vmalloc();
forbidding this will address the rhashtable deadlock(discussed here) but this will just fail allocation because it is incorrect state for vmalloc and calling kvmalloc() for rhashtable will not succeed.
i.e. rhashtable context should be fixed also. This is what i meant by
saying: this caller has to be fixed.
--
Uladzislau Rezki
Liam R. Howlett (Google)
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
In reply to Liam R. Howlett (Google)Quoted 74 lines · Liam R. Howlett (Google)
Sorry for missing the ML in my initial email, I missed it from
get_maintainers. I should have really clued in with such a small cc list ;)Quoted 43 lines · Uladzislau Rezki
Quoted 18 lines · Herbert Xu
Quoted 4 lines · Liam R. Howlett (Google)
Below, one CPU adds an FDB entry for an
RTM_NEWNEIGHnetlink request, while another CPU is insidevmalloc()orvfree():Quoted 12 lines
RTM_NEWNEIGH vmalloc() or vfree() ============ ==================== spin_lock_bh(&br->hash_lock) spin_lock(&vn->busy.lock) rhashtable_insert_rehash() kvmalloc(GFP_ATOMIC) vmalloc() spin_lock(&vn->busy.lock) waits for the other CPU softirq: a frame for the bridge spin_lock(&br->hash_lock) waits for RTM_NEWNEIGHQuoted 8 lines
I think this is a fundamental bug in the kvmalloc non-blocking support patch. How can it be used at all if it is susceptible to such dead-locks?
And if you can't use it safely why don't we just get rid of the broken kvmalloc non-block path and have it always go to kmalloc like it did before?
If we have a look at this as we did not have
vmalloc()here:RTM_NEWNEIGH foo() or ... ============ ==================== lock_bh(&br->hash_lock) foo() lock(B) rhashtable_insert_rehash() foo() lock(B) waits for the other CPU softirq: a frame for the bridge lock(&br->hash_lock) waits for RTM_NEWNEIGHQuoted 1 line
Same issue?
Looks like it to me.
Quoted 5 lines
I have a question:
GFP_ATOMIC/GFP_NOWAIT guarantee they do not sleep. Do they guarantee not hitting ABBA dead-locks or they are not interrupted?
GFP_ATOMICandGFP_NOWAITcan be interrupted. It's more about not waiting for reclaim (kswapd might still run) andGFP_ATOMICmight dip into reserves.The issue is that
kvmalloc()now falls back tovmalloc()for non-blocking flags (I am restating this to make sure I'm not missing something here, please correct me).So this changes what is called internal to rhastable so it doesn't reach
vmalloc(), like before, if the caller cannot block - they go tokmalloc()with the old fall back.I *think* the question is if the change should be here in the caller, internal to mm, or a new call to opt-in.
Quoted 6 lines
Considering this is the only
kvmalloc()caller that forwards non-blocking gfp from BH lock + softirq context, I think that it makes sense to do it in the caller.Given where we are in the release, this fixes a real issue that can happen so maybe we take this now and settle the general handling later?
Hubert, I have another version of this patch set to send out, but this particular patch remains the same.
Are you okay with keeping it the way it is for this release?
I don't want to force your hand but I also don't want to keep a valid deadlock scenario in-tree.
Thanks,
Liam
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 8 lines · Liam R. Howlett (Google)
Hubert, I have another version of this patch set to send out, but this particular patch remains the same.
Are you okay with keeping it the way it is for this release?
I don't want to force your hand but I also don't want to keep a valid deadlock scenario in-tree.
Instead of your patch, please revert the original change that added kvmalloc to rhashtable:
commit 93f976b5190df32793908d49165f78e67fcb66cf
Author: Davidlohr Bueso <[email protected]> Date: Tue Aug 21 22:01:45 2018 -0700
lib/rhashtable: simplify bucket_table_alloc()
The newly added alloc_hooks_tag call should go after the if statement.
This patch should also be split out of your series and posted to linux-crypto.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Liam R. Howlett (Google)
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 10 lines · Herbert Xu
Quoted 8 lines · Liam R. Howlett (Google)
Hubert, I have another version of this patch set to send out, but this particular patch remains the same.
Are you okay with keeping it the way it is for this release?
I don't want to force your hand but I also don't want to keep a valid deadlock scenario in-tree.
Quoted 9 lines
Instead of your patch, please revert the original change that added kvmalloc to rhashtable:
commit
93f976b5190df32793908d49165f78e67fcb66cfAuthor: Davidlohr Bueso <[email protected]> Date: Tue Aug 21 22:01:45 2018 -0700lib/rhashtable: simplify
bucket_table_alloc()
I don't follow. Reverting that patch gives the same functional flow as the patch I already sent; kzalloc() for one branch and kvzalloc() for the other.
We will, however, restore the useless __GFP_NORETRY flag to one branch, and a broken check.
- if (gfp != GFP_KERNEL)
- tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);
gfp != GFP_KERNEL is supposed to just send GFP_ATOMIC in here, but instead you are sending GFP_KERNEL | __GFP_NOFAIL as well. gfpflags_allow_blocking() should be used here, right?
The patch you are saying to revert was correct when it went in.
Quoted 1 line
The newly added
alloc_hooks_tagcall should go after the if statement.
You are saying to add it to both branches of the if statement, right?
Quoted 3 lines
This patch should also be split out of your series and posted to linux-crypto.
Sounds good.
The fixes tag should still point to c6307674ed82, since that's when the issue became visible.
Thanks,
Liam
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 4 lines · Liam R. Howlett (Google)
I don't follow. Reverting that patch gives the same functional flow as the patch I already sent;
kzalloc()for one branch andkvzalloc()for the other.
I'm not talking about a literal revert. But that patch is what introduced the use of an unconditional kvzalloc for the !GFP_KERNEL case which is what we are trying to undo.
So it should at least be mentioned prominently in the commit description.
Quoted 9 lines
We will, however, restore the useless
__GFP_NORETRYflag to one branch, and a broken check.(quoted diff) +0
-2- if (gfp != GFP_KERNEL) - tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY);gfp !=
GFP_KERNELis supposed to just sendGFP_ATOMICin here, but instead you are sendingGFP_KERNEL|__GFP_NOFAILas well.gfpflags_allow_blocking()should be used here, right?
That made me dig deeper into the original change and why it was made. It was actually Linus who suggested merging the two calls. It's a shame that we have to bring this pattern back.
https://lore.kernel.org/all/CA+55aFxnrwFniJq6nCFn4pWRHLH-Wac9bVFKieBC-TixTsFg=Q@mail.gmail.com/
Let's add him to the discussion and see if he can offer any insights on how we can avoid undoing this change.
To recap, kvmalloc was modified in 2025 such that it can use vmalloc even when called with GFP_ATOMIC (commit c6307674ed82). Unfortunately, it does so in a way where it has to wait for other CPUs to do certain work. As a result, if you call kvmalloc with GFP_ATOMIC under an external lock, this easily leads to a dead-lock when other CPUs end up blocking on the same external lock.
Hence rhashtable is being asked to undo the change and reintroduce this pattern to avoid the dead-lock:
if (!gfpflags_allow_blocking(gfp)) tbl = kzalloc(size, gfp | __GFP_NOWARN | __GFP_NORETRY); else tbl = kvzalloc(size, gfp);
My feeling is that the new kvmalloc is simply broken but its author disagrees.
Quoted 1 line
You are saying to add it to both branches of the if statement, right?
Nevermind, I didn't look at how alloc_hooks_tag was implemented. This is simply disgusting.
But instead of
if (gfpflags_allow_blocking(gfp)) tbl = alloc_hooks_tag(ht->alloc_tag, kvmalloc_node_align_noprof( struct_size(tbl, buckets, nbuckets), 1, gfp | __GFP_ZERO, NUMA_NO_NODE)); else tbl = alloc_hooks_tag(ht->alloc_tag, kmalloc_noprof( struct_size(tbl, buckets, nbuckets), gfp | __GFP_ZERO));
, plesae do
if (gfpflags_allow_blocking(gfp)) tbl = alloc_hooks_tag(ht->alloc_tag, kvmalloc_noprof( struct_size(tbl, buckets, nbuckets), gfp | __GFP_ZERO)); else tbl = alloc_hooks_tag(ht->alloc_tag, kzalloc_noprof( struct_size(tbl, buckets, nbuckets), gfp));
It would be even nicer if you could add a kvzalloc_noprof helper but it's not a deal-breaker.
Quoted 2 lines
The fixes tag should still point to
c6307674ed82, since that's when the issue became visible.
Sure that sounds good.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 7 lines · Herbert Xu
To recap, kvmalloc was modified in 2025 such that it can use vmalloc even when called with
GFP_ATOMIC(commitc6307674ed82). Unfortunately, it does so in a way where it has to wait for other CPUs to do certain work. As a result, if you call kvmalloc withGFP_ATOMICunder an external lock, this easily leads to a dead-lock when other CPUs end up blocking on the same external lock.
Ok, so it sounds like that change was clearly complete garbage and needs fixing.
The only reason for GFP_ATOMIC is that you're in a context where you can't block. That's the *point*. So if GFP_ATOMIC waits for something, it's just fundamentally buggy.
Sadly, the commit you point to doesn't talk about *why* that braindamage was done. Looking at the original thread it seems like it was for kmsan.
I didn't actually find the kmsan use case, but I didn't search very hard. I absolutely *DETEST* it when "debug infrastructure" causes actual bugs and mis-designs. That's the opposite of debugging.
IOW, I guess we do have to split those allocation cases up, and we damn well should remove GFP_ATOMIC support from vmalloc() again since it's clearly buggered beyond belief.
Linus
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 4 lines · Linus Torvalds
Sadly, the commit you point to doesn't talk about *why* that braindamage was done. Looking at the original thread it seems like it was for kmsan.
Here is the link to the whole series that was applied (v4):
https://lore.kernel.org/all/[email protected]/T/#m50cf87c3f12015a56abe5a6359525019851f1b97
This is the original RFC:
https://lore.kernel.org/all/[email protected]/
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 8 lines · Herbert Xu
Quoted 4 lines · Linus Torvalds
Sadly, the commit you point to doesn't talk about *why* that braindamage was done. Looking at the original thread it seems like it was for kmsan.
Here is the link to the whole series that was applied (v4):
None of that says why. It only repeats what I already said: it seems to be due to kmsan doing bad things.
Linus
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 3 lines · Linus Torvalds
None of that says why. It only repeats what I already said: it seems to be due to kmsan doing bad things.
Fair enough.
When I was told that folks needed vmalloc under GFP_ATOMIC I didn't question it any further:
https://lore.kernel.org/all/aeiHkG7X_dieMdw5@milan/
Looking back, it does look like it was never explained who these folks are.
Ulazislau, so who needs vmalloc GFP_ATOMIC support in the kernel? Or is it only used by out-of-kernel code?
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Hello, Herbert!
Quoted 9 lines
Quoted 3 lines
None of that says why. It only repeats what I already said: it seems to be due to kmsan doing bad things.
Fair enough.
When I was told that folks needed vmalloc under
GFP_ATOMICI didn't question it any further:
Quoted 8 lines
https://lore.kernel.org/all/aeiHkG7X_dieMdw5@milan/
Looking back, it does look like it was never explained who these folks are.
Ulazislau, so who needs vmalloc
GFP_ATOMICsupport in the kernel? Or is it only used by out-of-kernel code?
There are users in kernel. For example:
drivers/GPU
/* Allocate the file in vmalloc memory, it's likely to be big */
iter.start = __vmalloc(file_size, GFP_NOWAIT);AMD-GPU:
kvzalloc(size, GFP_NOWAIT);
kvzalloc_objs(struct amdgpu_coredump_ring, GFP_NOWAIT);
kvzalloc(ring->ring_size, GFP_NOWAIT);Virtio ring-testing:
(alloc_ring(0)), gfpflags_allow_blocking(0) does not allow blocking.
net/ipv4/udp.c:
err = bpf_iter_udp_realloc_batch(iter, batch_sks, GFP_NOWAIT);
etc...
Apart of above, the obvious user of GFP_ATOMIC is the bridge forwarding database. The br_fdb_add_local() holds br->hash_lock with bottom halves disabled, on rehash event it invokes rhashtable_insert_rehash() to allocate a new bucket table with GFP_ATOMIC | __GFP_NOWARN. In that scenario kvmalloc() can fallback to vmalloc().
This path runs and exists because syzbot hit the old in_interrupt() check there, leading to 04aa71da5f3 ("mm/vmalloc: do not trigger BUG() on BH disabled context").
The vmalloc() fallback improves the chance of completing the insert on the spot to minimize -ENOMEM hit rate thus minimize queuing the irq_work_queue(&ht->run_irq_work) to retry allocation in process context.
According to XFS code and comments it wants non-blocking support for kvmalloc().
But indeed the v4 cover letter didn't name a consumer. The bridge report came later.
--
Uladzislau Rezki
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 2 lines · Uladzislau Rezki
There are users in kernel. For example:
Thanks for the list.
Quoted 8 lines
drivers/GPU/* Allocate the file in vmalloc memory, it's likely to be big */ iter.start = __vmalloc(file_size, GFP_NOWAIT);AMD-GPU:
kvzalloc(size, GFP_NOWAIT); kvzalloc_objs(struct amdgpu_coredump_ring, GFP_NOWAIT); kvzalloc(ring->ring_size, GFP_NOWAIT);
These appear to be executing out of process context without spin locks so they're not really GFP_ATOMIC.
Quoted 2 lines
Virtio ring-testing:
(alloc_ring(0)),gfpflags_allow_blocking(0)does not allow blocking.
I didn't really look into this as it's just test code.
Quoted 2 lines
net/ipv4/udp.c:err = bpf_iter_udp_realloc_batch(iter, batch_sks, GFP_NOWAIT);
This looks buggy in exactly the same way as rhashtable. It's called under spin_lock_bh, and the same lock can be taken on the softirq path on another CPU.
So are there any safe users of kvmalloc with GFP_ATOMIC? If not I suggest that we change kvmalloc to disallow vmalloc when called with GFP_ATOMIC.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 11 lines · Herbert Xu
Quoted 2 lines · Uladzislau Rezki
There are users in kernel. For example:
Thanks for the list.
Quoted 4 lines
drivers/GPU/* Allocate the file in vmalloc memory, it's likely to be big */ iter.start = __vmalloc(file_size, GFP_NOWAIT);
Quoted 7 lines
Quoted 4 lines
AMD-GPU:
kvzalloc(size, GFP_NOWAIT); kvzalloc_objs(struct amdgpu_coredump_ring, GFP_NOWAIT); kvzalloc(ring->ring_size, GFP_NOWAIT);These appear to be executing out of process context without spin locks so they're not really
GFP_ATOMIC.
Historically, GFP_ATOMIC and GFP_NOWAIT have been largely the same except for GFP_ATOMIC eating into reserves.
But I wonder if we might be able to soften that a bit, or add a new form, because it looks like the AMD-GPU case at least doesn't really care about any kind of "don't sleep", what it really just wants is an "optimistic allocation" - try to get the memory, but don't spend effort on freeing things if there is not enough memort.
The AMD code in question literally has a comment that says
/* Dump ring content if memory allocation succeeds. */
so while it's possible that it has some atomicity requirements, it doesn't look that way to me from a very quick look.
The etnaviv case looks to be the exact same thing: it is definitely called in normal process context (that code takes a mutex lock around it all).
So those two cases really don't looke like they are about "atomicity" at all.
Quoted 6 lines
Quoted 2 lines
net/ipv4/udp.c:err = bpf_iter_udp_realloc_batch(iter, batch_sks, GFP_NOWAIT);This looks buggy in exactly the same way as rhashtable. It's called under
spin_lock_bh, and the same lock can be taken on the softirq path on another CPU.
Yeah, that case is definitely actually atomic and holds a lock. Not great.
Is there any way to limit the vmalloc() case to the actuall atomic one and just fail when it needs to busy-wait?
Linus
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 45 lines · Linus Torvalds
Quoted 11 lines · Herbert Xu
Quoted 2 lines · Uladzislau Rezki
There are users in kernel. For example:
Thanks for the list.
Quoted 4 lines
drivers/GPU/* Allocate the file in vmalloc memory, it's likely to be big */ iter.start = __vmalloc(file_size, GFP_NOWAIT);Quoted 7 lines
Quoted 4 lines
AMD-GPU:
kvzalloc(size, GFP_NOWAIT); kvzalloc_objs(struct amdgpu_coredump_ring, GFP_NOWAIT); kvzalloc(ring->ring_size, GFP_NOWAIT);These appear to be executing out of process context without spin locks so they're not really
GFP_ATOMIC.Historically,
GFP_ATOMICandGFP_NOWAIThave been largely the same except forGFP_ATOMICeating into reserves.But I wonder if we might be able to soften that a bit, or add a new form, because it looks like the AMD-GPU case at least doesn't really care about any kind of "don't sleep", what it really just wants is an "optimistic allocation" - try to get the memory, but don't spend effort on freeing things if there is not enough memort.
The AMD code in question literally has a comment that says
/* Dump ring content if memory allocation succeeds. */so while it's possible that it has some atomicity requirements, it doesn't look that way to me from a very quick look.
The etnaviv case looks to be the exact same thing: it is definitely called in normal process context (that code takes a mutex lock around it all).
So those two cases really don't looke like they are about "atomicity" at all.
Quoted 3 lines
Quoted 2 lines
net/ipv4/udp.c:err = bpf_iter_udp_realloc_batch(iter, batch_sks, GFP_NOWAIT);
Quoted 9 lines
Quoted 3 lines
This looks buggy in exactly the same way as rhashtable. It's called under
spin_lock_bh, and the same lock can be taken on the softirq path on another CPU.Yeah, that case is definitely actually atomic and holds a lock. Not great.
Is there any way to limit the
vmalloc()case to the actuall atomic one and just fail when it needs to busy-wait?
Probably it is not easy. vmalloc() takes several locks including during page-table setup.
Probably the simplest option is to return NULL for non-blocking vmalloc() allocations when BH are disabled. That would address the reported rhashtable deadlock.
--
Uladzislau Rezki
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
In reply to Herbert XuQuoted 26 lines · Herbert Xu
Quoted 2 lines · Uladzislau Rezki
There are users in kernel. For example:
Thanks for the list.
Quoted 8 lines
drivers/GPU/* Allocate the file in vmalloc memory, it's likely to be big */ iter.start = __vmalloc(file_size, GFP_NOWAIT);AMD-GPU:
kvzalloc(size, GFP_NOWAIT); kvzalloc_objs(struct amdgpu_coredump_ring, GFP_NOWAIT); kvzalloc(ring->ring_size, GFP_NOWAIT);These appear to be executing out of process context without spin locks so they're not really
GFP_ATOMIC.Quoted 2 lines
Virtio ring-testing:
(alloc_ring(0)),gfpflags_allow_blocking(0)does not allow blocking.I didn't really look into this as it's just test code.
Quoted 2 lines
net/ipv4/udp.c:err = bpf_iter_udp_realloc_batch(iter, batch_sks, GFP_NOWAIT);
Quoted 8 lines
This looks buggy in exactly the same way as rhashtable. It's called under
spin_lock_bh, and the same lock can be taken on the softirq path on another CPU.So are there any safe users of kvmalloc with
GFP_ATOMIC? If not I suggest that we change kvmalloc to disallow vmalloc when called withGFP_ATOMIC.
kvmalloc() disallow and vmalloc() allow?
The deadlock looks like from below spin_lock():
<snip>
net/bridge/br_fdb.c +7 -1
@@ -1025,7 +1025,13 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, } } }else { -spin_lock (&br->hash_lock); +if (in_serving_softirq ()) { +if (!spin_trylock (&br->hash_lock)) +return ; + }else { +spin_lock (&br->hash_lock); + } + fdb =fdb_create (br, source, addr, vid, flags);if (fdb) {trace_br_fdb_update (br, source, addr, vid, flags);
<snip>
is that applicable for bridge update code?
--
Uladzislau Rezki
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
net/bridge/br_fdb.c void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, · line 1029
Quoted 8 lines · Uladzislau Rezki
net/bridge/br_fdb.c +2
-1@@ -1025,7 +1025,13 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, } } }else { -spin_lock (&br->hash_lock); +if (in_serving_softirq ()) { +if (!spin_trylock (&br->hash_lock))
I don't think that in_serving_softirq() makes any sense.
If I read this code correctly - and it's possible I didn't - this is always called under local_bh_disable().
And local_bh_disable() will have increased the preempt-count exactly so that it looks like it's in_serving_softirq().
Maybe I got lost somewhere, but it basically looks like that test will always be true and is pointless (and misleading).
Linus
Herbert Xu
maintainerRe: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
Quoted 11 lines · Linus Torvalds
I don't think that
in_serving_softirq()makes any sense.If I read this code correctly - and it's possible I didn't - this is always called under
local_bh_disable().And
local_bh_disable()will have increased the preempt-count exactly so that it looks like it'sin_serving_softirq().Maybe I got lost somewhere, but it basically looks like that test will always be true and is pointless (and misleading).
You're absolutely right.
Even if there was a way to tell whether you're in a real softirq context, it's crazy to change how locking is done in br_fdb when the real issue is caused by a kvmalloc call that is made through rhashtables.
Now if we did the same test within kvmalloc and simply fell back to kmalloc it might work. But then again, why are we trying to support GFP_ATOMIC vmalloc when there appears to be zero users of it, a whole year after it was introduced?
Cheers,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
In reply to Linus TorvaldsQuoted 12 lines · Linus Torvalds
net/bridge/br_fdb.c void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, · line 1029
Quoted 8 lines · Uladzislau Rezki
net/bridge/br_fdb.c +2
-1@@ -1025,7 +1025,13 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, } } }else { -spin_lock (&br->hash_lock); +if (in_serving_softirq ()) { +if (!spin_trylock (&br->hash_lock))I don't think that
in_serving_softirq()makes any sense.
Quoted 6 lines
If I read this code correctly - and it's possible I didn't - this is always called under
local_bh_disable().And
local_bh_disable()will have increased the preempt-count exactly so that it looks like it'sin_serving_softirq().
Sorry, probably i am missing something here. My understanding is that local_bh_disable() adds SOFTIRQ_DISABLE_OFFSET and in_serving_softirq() checks another SOFTIRQ_OFFSET bit.
--
Uladzislau Rezki
Re: [PATCH 4/4] rhashtable: fix vmalloc() deadlock in atomic rehash
In reply to Liam R. Howlett (Google)Quoted 15 lines · Liam R. Howlett (Google)
Below, one CPU adds an FDB entry for an
RTM_NEWNEIGHnetlink request, while another CPU is insidevmalloc()orvfree():RTM_NEWNEIGH vmalloc() or vfree() ============ ==================== spin_lock_bh(&br->hash_lock) spin_lock(&vn->busy.lock) rhashtable_insert_rehash() kvmalloc(GFP_ATOMIC) vmalloc() spin_lock(&vn->busy.lock) waits for the other CPU softirq: a frame for the bridge spin_lock(&br->hash_lock) waits for RTM_NEWNEIGH
This seems like the kind of deadlock that lockdep can catch. Why hasn't it? Do we need to add a fake lock for RTM_NEWNEIGH?