* [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr [not found] <CACT4Y+Zc21Aj+5KjeTEsvOysJGHRYDSKgu_+_xN1LUYfG_H0sg@mail.gmail.com> @ 2022-11-07 3:31 ` Qi Zheng 2022-11-07 12:42 ` Jason Gunthorpe 0 siblings, 1 reply; 14+ messages in thread From: Qi Zheng @ 2022-11-07 3:31 UTC (permalink / raw) To: dvyukov, jgg, willy, akinobu.mita Cc: akpm, linux-kernel, linux-mm, linux-fsdevel, Qi Zheng, stable When we specify __GFP_NOWARN, we only expect that no warnings will be issued for current caller. But in the __should_failslab() and __should_fail_alloc_page(), the local GFP flags alter the global {failslab|fail_page_alloc}.attr, which is persistent and shared by all tasks. This is not what we expected, let's fix it. Cc: stable@vger.kernel.org Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") Reported-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- include/linux/fault-inject.h | 7 +++++-- lib/fault-inject.c | 14 +++++++++----- mm/failslab.c | 6 ++++-- mm/page_alloc.c | 6 ++++-- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h index 9f6e25467844..444236dadcf0 100644 --- a/include/linux/fault-inject.h +++ b/include/linux/fault-inject.h @@ -20,7 +20,6 @@ struct fault_attr { atomic_t space; unsigned long verbose; bool task_filter; - bool no_warn; unsigned long stacktrace_depth; unsigned long require_start; unsigned long require_end; @@ -32,6 +31,10 @@ struct fault_attr { struct dentry *dname; }; +enum fault_flags { + FAULT_NOWARN = 1 << 0, +}; + #define FAULT_ATTR_INITIALIZER { \ .interval = 1, \ .times = ATOMIC_INIT(1), \ @@ -40,11 +43,11 @@ struct fault_attr { .ratelimit_state = RATELIMIT_STATE_INIT_DISABLED, \ .verbose = 2, \ .dname = NULL, \ - .no_warn = false, \ } #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER int setup_fault_attr(struct fault_attr *attr, char *str); +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags); bool should_fail(struct fault_attr *attr, ssize_t size); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS diff --git a/lib/fault-inject.c b/lib/fault-inject.c index 4b8fafce415c..5971f7c3e49e 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -41,9 +41,6 @@ EXPORT_SYMBOL_GPL(setup_fault_attr); static void fail_dump(struct fault_attr *attr) { - if (attr->no_warn) - return; - if (attr->verbose > 0 && __ratelimit(&attr->ratelimit_state)) { printk(KERN_NOTICE "FAULT_INJECTION: forcing a failure.\n" "name %pd, interval %lu, probability %lu, " @@ -103,7 +100,7 @@ static inline bool fail_stacktrace(struct fault_attr *attr) * http://www.nongnu.org/failmalloc/ */ -bool should_fail(struct fault_attr *attr, ssize_t size) +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags) { bool stack_checked = false; @@ -152,13 +149,20 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return false; fail: - fail_dump(attr); + if (!(flags & FAULT_NOWARN)) + fail_dump(attr); if (atomic_read(&attr->times) != -1) atomic_dec_not_zero(&attr->times); return true; } +EXPORT_SYMBOL_GPL(should_fail_ex); + +bool should_fail(struct fault_attr *attr, ssize_t size) +{ + return should_fail_ex(attr, size, 0); +} EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS diff --git a/mm/failslab.c b/mm/failslab.c index 58df9789f1d2..fc046f26606c 100644 --- a/mm/failslab.c +++ b/mm/failslab.c @@ -16,6 +16,8 @@ static struct { bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) { + int flags = 0; + /* No fault-injection for bootstrap cache */ if (unlikely(s == kmem_cache)) return false; @@ -31,9 +33,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) return false; if (gfpflags & __GFP_NOWARN) - failslab.attr.no_warn = true; + flags |= FAULT_NOWARN; - return should_fail(&failslab.attr, s->object_size); + return should_fail_ex(&failslab.attr, s->object_size, flags); } static int __init setup_failslab(char *str) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 7192ded44ad0..e537d3a950a4 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3902,6 +3902,8 @@ __setup("fail_page_alloc=", setup_fail_page_alloc); static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) { + int flags = 0; + if (order < fail_page_alloc.min_order) return false; if (gfp_mask & __GFP_NOFAIL) @@ -3913,9 +3915,9 @@ static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) return false; if (gfp_mask & __GFP_NOWARN) - fail_page_alloc.attr.no_warn = true; + flags |= FAULT_NOWARN; - return should_fail(&fail_page_alloc.attr, 1 << order); + return should_fail_ex(&fail_page_alloc.attr, 1 << order, flags); } #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS -- 2.20.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-07 3:31 ` [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr Qi Zheng @ 2022-11-07 12:42 ` Jason Gunthorpe 2022-11-07 15:05 ` Qi Zheng 0 siblings, 1 reply; 14+ messages in thread From: Jason Gunthorpe @ 2022-11-07 12:42 UTC (permalink / raw) To: Qi Zheng Cc: dvyukov, willy, akinobu.mita, akpm, linux-kernel, linux-mm, linux-fsdevel, stable On Mon, Nov 07, 2022 at 11:31:09AM +0800, Qi Zheng wrote: > @@ -31,9 +33,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > return false; > > if (gfpflags & __GFP_NOWARN) > - failslab.attr.no_warn = true; > + flags |= FAULT_NOWARN; You should add a comment here about why this is required, to avoid deadlocking printk Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-07 12:42 ` Jason Gunthorpe @ 2022-11-07 15:05 ` Qi Zheng 2022-11-07 16:26 ` Jason Gunthorpe 0 siblings, 1 reply; 14+ messages in thread From: Qi Zheng @ 2022-11-07 15:05 UTC (permalink / raw) To: Jason Gunthorpe Cc: dvyukov, willy, akinobu.mita, akpm, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/7 20:42, Jason Gunthorpe wrote: > On Mon, Nov 07, 2022 at 11:31:09AM +0800, Qi Zheng wrote: > >> @@ -31,9 +33,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >> return false; >> >> if (gfpflags & __GFP_NOWARN) >> - failslab.attr.no_warn = true; >> + flags |= FAULT_NOWARN; > > You should add a comment here about why this is required, to avoid > deadlocking printk I think this comment should be placed where __GFP_NOWARN is specified instead of here. What do you think? :) Thanks, Qi > > Jason -- Thanks, Qi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-07 15:05 ` Qi Zheng @ 2022-11-07 16:26 ` Jason Gunthorpe 2022-11-08 2:47 ` Qi Zheng 2022-11-08 3:52 ` [PATCH v2] " Qi Zheng 0 siblings, 2 replies; 14+ messages in thread From: Jason Gunthorpe @ 2022-11-07 16:26 UTC (permalink / raw) To: Qi Zheng Cc: dvyukov, willy, akinobu.mita, akpm, linux-kernel, linux-mm, linux-fsdevel, stable On Mon, Nov 07, 2022 at 11:05:42PM +0800, Qi Zheng wrote: > > > On 2022/11/7 20:42, Jason Gunthorpe wrote: > > On Mon, Nov 07, 2022 at 11:31:09AM +0800, Qi Zheng wrote: > > > > > @@ -31,9 +33,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > > > return false; > > > if (gfpflags & __GFP_NOWARN) > > > - failslab.attr.no_warn = true; > > > + flags |= FAULT_NOWARN; > > > > You should add a comment here about why this is required, to avoid > > deadlocking printk > > I think this comment should be placed where __GFP_NOWARN is specified > instead of here. What do you think? :) NOWARN is clear what it does, it is this specifically that is very subtle about avoiding deadlock aginst allocations triggered by printk/etc code. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-07 16:26 ` Jason Gunthorpe @ 2022-11-08 2:47 ` Qi Zheng 2022-11-08 3:52 ` [PATCH v2] " Qi Zheng 1 sibling, 0 replies; 14+ messages in thread From: Qi Zheng @ 2022-11-08 2:47 UTC (permalink / raw) To: Jason Gunthorpe Cc: dvyukov, willy, akinobu.mita, akpm, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/8 00:26, Jason Gunthorpe wrote: > On Mon, Nov 07, 2022 at 11:05:42PM +0800, Qi Zheng wrote: >> >> >> On 2022/11/7 20:42, Jason Gunthorpe wrote: >>> On Mon, Nov 07, 2022 at 11:31:09AM +0800, Qi Zheng wrote: >>> >>>> @@ -31,9 +33,9 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >>>> return false; >>>> if (gfpflags & __GFP_NOWARN) >>>> - failslab.attr.no_warn = true; >>>> + flags |= FAULT_NOWARN; >>> >>> You should add a comment here about why this is required, to avoid >>> deadlocking printk >> >> I think this comment should be placed where __GFP_NOWARN is specified >> instead of here. What do you think? :) > > NOWARN is clear what it does, it is this specifically that is very > subtle about avoiding deadlock aginst allocations triggered by > printk/etc code. Oh, maybe I understand your concern. Some people may think that this is just a print of fault injection information, not a warning. I'll add a comment explaining why in some cases there must be no printing. Thanks, Qi > > Jason -- Thanks, Qi ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-07 16:26 ` Jason Gunthorpe 2022-11-08 2:47 ` Qi Zheng @ 2022-11-08 3:52 ` Qi Zheng 2022-11-08 8:44 ` Wei Yongjun 2022-11-08 17:36 ` Akinobu Mita 1 sibling, 2 replies; 14+ messages in thread From: Qi Zheng @ 2022-11-08 3:52 UTC (permalink / raw) To: dvyukov, jgg, willy, akinobu.mita Cc: akpm, linux-kernel, linux-mm, linux-fsdevel, Qi Zheng, stable When we specify __GFP_NOWARN, we only expect that no warnings will be issued for current caller. But in the __should_failslab() and __should_fail_alloc_page(), the local GFP flags alter the global {failslab|fail_page_alloc}.attr, which is persistent and shared by all tasks. This is not what we expected, let's fix it. Cc: stable@vger.kernel.org Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") Reported-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ Changelog in v1 -> v2: - add comment for __should_failslab() and __should_fail_alloc_page() (suggested by Jason) include/linux/fault-inject.h | 7 +++++-- lib/fault-inject.c | 14 +++++++++----- mm/failslab.c | 12 ++++++++++-- mm/page_alloc.c | 7 +++++-- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h index 9f6e25467844..444236dadcf0 100644 --- a/include/linux/fault-inject.h +++ b/include/linux/fault-inject.h @@ -20,7 +20,6 @@ struct fault_attr { atomic_t space; unsigned long verbose; bool task_filter; - bool no_warn; unsigned long stacktrace_depth; unsigned long require_start; unsigned long require_end; @@ -32,6 +31,10 @@ struct fault_attr { struct dentry *dname; }; +enum fault_flags { + FAULT_NOWARN = 1 << 0, +}; + #define FAULT_ATTR_INITIALIZER { \ .interval = 1, \ .times = ATOMIC_INIT(1), \ @@ -40,11 +43,11 @@ struct fault_attr { .ratelimit_state = RATELIMIT_STATE_INIT_DISABLED, \ .verbose = 2, \ .dname = NULL, \ - .no_warn = false, \ } #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER int setup_fault_attr(struct fault_attr *attr, char *str); +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags); bool should_fail(struct fault_attr *attr, ssize_t size); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS diff --git a/lib/fault-inject.c b/lib/fault-inject.c index 4b8fafce415c..5971f7c3e49e 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -41,9 +41,6 @@ EXPORT_SYMBOL_GPL(setup_fault_attr); static void fail_dump(struct fault_attr *attr) { - if (attr->no_warn) - return; - if (attr->verbose > 0 && __ratelimit(&attr->ratelimit_state)) { printk(KERN_NOTICE "FAULT_INJECTION: forcing a failure.\n" "name %pd, interval %lu, probability %lu, " @@ -103,7 +100,7 @@ static inline bool fail_stacktrace(struct fault_attr *attr) * http://www.nongnu.org/failmalloc/ */ -bool should_fail(struct fault_attr *attr, ssize_t size) +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags) { bool stack_checked = false; @@ -152,13 +149,20 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return false; fail: - fail_dump(attr); + if (!(flags & FAULT_NOWARN)) + fail_dump(attr); if (atomic_read(&attr->times) != -1) atomic_dec_not_zero(&attr->times); return true; } +EXPORT_SYMBOL_GPL(should_fail_ex); + +bool should_fail(struct fault_attr *attr, ssize_t size) +{ + return should_fail_ex(attr, size, 0); +} EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS diff --git a/mm/failslab.c b/mm/failslab.c index 58df9789f1d2..ffc420c0e767 100644 --- a/mm/failslab.c +++ b/mm/failslab.c @@ -16,6 +16,8 @@ static struct { bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) { + int flags = 0; + /* No fault-injection for bootstrap cache */ if (unlikely(s == kmem_cache)) return false; @@ -30,10 +32,16 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) return false; + /* + * In some cases, it expects to specify __GFP_NOWARN + * to avoid printing any information(not just a warning), + * thus avoiding deadlocks. See commit 6b9dbedbe349 for + * details. + */ if (gfpflags & __GFP_NOWARN) - failslab.attr.no_warn = true; + flags |= FAULT_NOWARN; - return should_fail(&failslab.attr, s->object_size); + return should_fail_ex(&failslab.attr, s->object_size, flags); } static int __init setup_failslab(char *str) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 7192ded44ad0..cb6fe715d983 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3902,6 +3902,8 @@ __setup("fail_page_alloc=", setup_fail_page_alloc); static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) { + int flags = 0; + if (order < fail_page_alloc.min_order) return false; if (gfp_mask & __GFP_NOFAIL) @@ -3912,10 +3914,11 @@ static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) (gfp_mask & __GFP_DIRECT_RECLAIM)) return false; + /* See comment in __should_failslab() */ if (gfp_mask & __GFP_NOWARN) - fail_page_alloc.attr.no_warn = true; + flags |= FAULT_NOWARN; - return should_fail(&fail_page_alloc.attr, 1 << order); + return should_fail_ex(&fail_page_alloc.attr, 1 << order, flags); } #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS -- 2.20.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 3:52 ` [PATCH v2] " Qi Zheng @ 2022-11-08 8:44 ` Wei Yongjun 2022-11-08 8:58 ` Qi Zheng 2022-11-08 17:36 ` Akinobu Mita 1 sibling, 1 reply; 14+ messages in thread From: Wei Yongjun @ 2022-11-08 8:44 UTC (permalink / raw) To: Qi Zheng, dvyukov, jgg, willy, akinobu.mita Cc: akpm, linux-kernel, linux-mm, linux-fsdevel, stable Hi Zheng Qi, On 2022/11/8 11:52, Qi Zheng wrote: > When we specify __GFP_NOWARN, we only expect that no warnings > will be issued for current caller. But in the __should_failslab() > and __should_fail_alloc_page(), the local GFP flags alter the > global {failslab|fail_page_alloc}.attr, which is persistent and > shared by all tasks. This is not what we expected, let's fix it. > > Cc: stable@vger.kernel.org > Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ > > Changelog in v1 -> v2: > - add comment for __should_failslab() and __should_fail_alloc_page() > (suggested by Jason) > > include/linux/fault-inject.h | 7 +++++-- > lib/fault-inject.c | 14 +++++++++----- > mm/failslab.c | 12 ++++++++++-- > mm/page_alloc.c | 7 +++++-- > 4 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h > index 9f6e25467844..444236dadcf0 100644 > --- a/include/linux/fault-inject.h > +++ b/include/linux/fault-inject.h > @@ -20,7 +20,6 @@ struct fault_attr { > atomic_t space; > unsigned long verbose; > bool task_filter; > - bool no_warn; > unsigned long stacktrace_depth; > unsigned long require_start; > unsigned long require_end; > @@ -32,6 +31,10 @@ struct fault_attr { > struct dentry *dname; > }; > > +enum fault_flags { > + FAULT_NOWARN = 1 << 0, > +}; > + > #define FAULT_ATTR_INITIALIZER { \ > .interval = 1, \ > .times = ATOMIC_INIT(1), \ > @@ -40,11 +43,11 @@ struct fault_attr { > .ratelimit_state = RATELIMIT_STATE_INIT_DISABLED, \ > .verbose = 2, \ > .dname = NULL, \ > - .no_warn = false, \ How about keep no_warn attr as it be, and export it to user? When testing with fault injection, and each fault will print an backtrace. but not all of the testsuit can tell us which one is fault injection message or other is a real warning/crash like syzkaller do. In my case, to make things simple, we usually used a regex to detect whether wanring/error happend. So we disabled the slab/page fault warning message by default, and only enable it when debug real issue. Regards, > } > > #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER > int setup_fault_attr(struct fault_attr *attr, char *str); > +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags); > bool should_fail(struct fault_attr *attr, ssize_t size); > > #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS > diff --git a/lib/fault-inject.c b/lib/fault-inject.c > index 4b8fafce415c..5971f7c3e49e 100644 > --- a/lib/fault-inject.c > +++ b/lib/fault-inject.c > @@ -41,9 +41,6 @@ EXPORT_SYMBOL_GPL(setup_fault_attr); > > static void fail_dump(struct fault_attr *attr) > { > - if (attr->no_warn) > - return; > - > if (attr->verbose > 0 && __ratelimit(&attr->ratelimit_state)) { > printk(KERN_NOTICE "FAULT_INJECTION: forcing a failure.\n" > "name %pd, interval %lu, probability %lu, " > @@ -103,7 +100,7 @@ static inline bool fail_stacktrace(struct fault_attr *attr) > * http://www.nongnu.org/failmalloc/ > */ > > -bool should_fail(struct fault_attr *attr, ssize_t size) > +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags) > { > bool stack_checked = false; > > @@ -152,13 +149,20 @@ bool should_fail(struct fault_attr *attr, ssize_t size) > return false; > > fail: > - fail_dump(attr); > + if (!(flags & FAULT_NOWARN)) > + fail_dump(attr); > > if (atomic_read(&attr->times) != -1) > atomic_dec_not_zero(&attr->times); > > return true; > } > +EXPORT_SYMBOL_GPL(should_fail_ex); > + > +bool should_fail(struct fault_attr *attr, ssize_t size) > +{ > + return should_fail_ex(attr, size, 0); > +} > EXPORT_SYMBOL_GPL(should_fail); > > #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS > diff --git a/mm/failslab.c b/mm/failslab.c > index 58df9789f1d2..ffc420c0e767 100644 > --- a/mm/failslab.c > +++ b/mm/failslab.c > @@ -16,6 +16,8 @@ static struct { > > bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > { > + int flags = 0; > + > /* No fault-injection for bootstrap cache */ > if (unlikely(s == kmem_cache)) > return false; > @@ -30,10 +32,16 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) > if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) > return false; > > + /* > + * In some cases, it expects to specify __GFP_NOWARN > + * to avoid printing any information(not just a warning), > + * thus avoiding deadlocks. See commit 6b9dbedbe349 for > + * details. > + */ > if (gfpflags & __GFP_NOWARN) > - failslab.attr.no_warn = true; > + flags |= FAULT_NOWARN; > > - return should_fail(&failslab.attr, s->object_size); > + return should_fail_ex(&failslab.attr, s->object_size, flags); > } > > static int __init setup_failslab(char *str) > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 7192ded44ad0..cb6fe715d983 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3902,6 +3902,8 @@ __setup("fail_page_alloc=", setup_fail_page_alloc); > > static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) > { > + int flags = 0; > + > if (order < fail_page_alloc.min_order) > return false; > if (gfp_mask & __GFP_NOFAIL) > @@ -3912,10 +3914,11 @@ static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) > (gfp_mask & __GFP_DIRECT_RECLAIM)) > return false; > > + /* See comment in __should_failslab() */ > if (gfp_mask & __GFP_NOWARN) > - fail_page_alloc.attr.no_warn = true; > + flags |= FAULT_NOWARN; > > - return should_fail(&fail_page_alloc.attr, 1 << order); > + return should_fail_ex(&fail_page_alloc.attr, 1 << order, flags); > } > > #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 8:44 ` Wei Yongjun @ 2022-11-08 8:58 ` Qi Zheng 2022-11-08 9:32 ` Wei Yongjun 0 siblings, 1 reply; 14+ messages in thread From: Qi Zheng @ 2022-11-08 8:58 UTC (permalink / raw) To: Wei Yongjun, dvyukov, jgg, willy, akinobu.mita Cc: akpm, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/8 16:44, Wei Yongjun wrote: > Hi Zheng Qi, > > On 2022/11/8 11:52, Qi Zheng wrote: >> When we specify __GFP_NOWARN, we only expect that no warnings >> will be issued for current caller. But in the __should_failslab() >> and __should_fail_alloc_page(), the local GFP flags alter the >> global {failslab|fail_page_alloc}.attr, which is persistent and >> shared by all tasks. This is not what we expected, let's fix it. >> >> Cc: stable@vger.kernel.org >> Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ >> >> Changelog in v1 -> v2: >> - add comment for __should_failslab() and __should_fail_alloc_page() >> (suggested by Jason) >> >> include/linux/fault-inject.h | 7 +++++-- >> lib/fault-inject.c | 14 +++++++++----- >> mm/failslab.c | 12 ++++++++++-- >> mm/page_alloc.c | 7 +++++-- >> 4 files changed, 29 insertions(+), 11 deletions(-) >> >> diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h >> index 9f6e25467844..444236dadcf0 100644 >> --- a/include/linux/fault-inject.h >> +++ b/include/linux/fault-inject.h >> @@ -20,7 +20,6 @@ struct fault_attr { >> atomic_t space; >> unsigned long verbose; >> bool task_filter; >> - bool no_warn; >> unsigned long stacktrace_depth; >> unsigned long require_start; >> unsigned long require_end; >> @@ -32,6 +31,10 @@ struct fault_attr { >> struct dentry *dname; >> }; >> >> +enum fault_flags { >> + FAULT_NOWARN = 1 << 0, >> +}; >> + >> #define FAULT_ATTR_INITIALIZER { \ >> .interval = 1, \ >> .times = ATOMIC_INIT(1), \ >> @@ -40,11 +43,11 @@ struct fault_attr { >> .ratelimit_state = RATELIMIT_STATE_INIT_DISABLED, \ >> .verbose = 2, \ >> .dname = NULL, \ >> - .no_warn = false, \ > > How about keep no_warn attr as it be, and export it to user? > > When testing with fault injection, and each fault will print an backtrace. > but not all of the testsuit can tell us which one is fault injection > message or other is a real warning/crash like syzkaller do. > > In my case, to make things simple, we usually used a regex to detect whether > wanring/error happend. So we disabled the slab/page fault warning message by > default, and only enable it when debug real issue. So you want to set/clear this no_warn attr through the procfs or sysfs interface, so that you can easily disable/enable the slab/page fault warning message from the user mode. Right? Seems reasonable to me. Anyone else has an opinion on this? If it is really needed, I can do it later. Thanks, Qi > > Regards, > > >> } >> >> #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER >> int setup_fault_attr(struct fault_attr *attr, char *str); >> +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags); >> bool should_fail(struct fault_attr *attr, ssize_t size); >> >> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >> diff --git a/lib/fault-inject.c b/lib/fault-inject.c >> index 4b8fafce415c..5971f7c3e49e 100644 >> --- a/lib/fault-inject.c >> +++ b/lib/fault-inject.c >> @@ -41,9 +41,6 @@ EXPORT_SYMBOL_GPL(setup_fault_attr); >> >> static void fail_dump(struct fault_attr *attr) >> { >> - if (attr->no_warn) >> - return; >> - >> if (attr->verbose > 0 && __ratelimit(&attr->ratelimit_state)) { >> printk(KERN_NOTICE "FAULT_INJECTION: forcing a failure.\n" >> "name %pd, interval %lu, probability %lu, " >> @@ -103,7 +100,7 @@ static inline bool fail_stacktrace(struct fault_attr *attr) >> * http://www.nongnu.org/failmalloc/ >> */ >> >> -bool should_fail(struct fault_attr *attr, ssize_t size) >> +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags) >> { >> bool stack_checked = false; >> >> @@ -152,13 +149,20 @@ bool should_fail(struct fault_attr *attr, ssize_t size) >> return false; >> >> fail: >> - fail_dump(attr); >> + if (!(flags & FAULT_NOWARN)) >> + fail_dump(attr); >> >> if (atomic_read(&attr->times) != -1) >> atomic_dec_not_zero(&attr->times); >> >> return true; >> } >> +EXPORT_SYMBOL_GPL(should_fail_ex); >> + >> +bool should_fail(struct fault_attr *attr, ssize_t size) >> +{ >> + return should_fail_ex(attr, size, 0); >> +} >> EXPORT_SYMBOL_GPL(should_fail); >> >> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >> diff --git a/mm/failslab.c b/mm/failslab.c >> index 58df9789f1d2..ffc420c0e767 100644 >> --- a/mm/failslab.c >> +++ b/mm/failslab.c >> @@ -16,6 +16,8 @@ static struct { >> >> bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >> { >> + int flags = 0; >> + >> /* No fault-injection for bootstrap cache */ >> if (unlikely(s == kmem_cache)) >> return false; >> @@ -30,10 +32,16 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >> if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) >> return false; >> >> + /* >> + * In some cases, it expects to specify __GFP_NOWARN >> + * to avoid printing any information(not just a warning), >> + * thus avoiding deadlocks. See commit 6b9dbedbe349 for >> + * details. >> + */ >> if (gfpflags & __GFP_NOWARN) >> - failslab.attr.no_warn = true; >> + flags |= FAULT_NOWARN; >> >> - return should_fail(&failslab.attr, s->object_size); >> + return should_fail_ex(&failslab.attr, s->object_size, flags); >> } >> >> static int __init setup_failslab(char *str) >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 7192ded44ad0..cb6fe715d983 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -3902,6 +3902,8 @@ __setup("fail_page_alloc=", setup_fail_page_alloc); >> >> static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) >> { >> + int flags = 0; >> + >> if (order < fail_page_alloc.min_order) >> return false; >> if (gfp_mask & __GFP_NOFAIL) >> @@ -3912,10 +3914,11 @@ static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) >> (gfp_mask & __GFP_DIRECT_RECLAIM)) >> return false; >> >> + /* See comment in __should_failslab() */ >> if (gfp_mask & __GFP_NOWARN) >> - fail_page_alloc.attr.no_warn = true; >> + flags |= FAULT_NOWARN; >> >> - return should_fail(&fail_page_alloc.attr, 1 << order); >> + return should_fail_ex(&fail_page_alloc.attr, 1 << order, flags); >> } >> >> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS -- Thanks, Qi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 8:58 ` Qi Zheng @ 2022-11-08 9:32 ` Wei Yongjun 2022-11-08 9:45 ` Qi Zheng 2022-11-08 12:04 ` Jason Gunthorpe 0 siblings, 2 replies; 14+ messages in thread From: Wei Yongjun @ 2022-11-08 9:32 UTC (permalink / raw) To: Qi Zheng, dvyukov, jgg, willy, akinobu.mita Cc: akpm, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/8 16:58, Qi Zheng wrote: > > > On 2022/11/8 16:44, Wei Yongjun wrote: >> Hi Zheng Qi, >> >> On 2022/11/8 11:52, Qi Zheng wrote: >>> When we specify __GFP_NOWARN, we only expect that no warnings >>> will be issued for current caller. But in the __should_failslab() >>> and __should_fail_alloc_page(), the local GFP flags alter the >>> global {failslab|fail_page_alloc}.attr, which is persistent and >>> shared by all tasks. This is not what we expected, let's fix it. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") >>> Reported-by: Dmitry Vyukov <dvyukov@google.com> >>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >>> --- >>> v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ >>> >>> Changelog in v1 -> v2: >>> - add comment for __should_failslab() and __should_fail_alloc_page() >>> (suggested by Jason) >>> >>> include/linux/fault-inject.h | 7 +++++-- >>> lib/fault-inject.c | 14 +++++++++----- >>> mm/failslab.c | 12 ++++++++++-- >>> mm/page_alloc.c | 7 +++++-- >>> 4 files changed, 29 insertions(+), 11 deletions(-) >>> >>> diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h >>> index 9f6e25467844..444236dadcf0 100644 >>> --- a/include/linux/fault-inject.h >>> +++ b/include/linux/fault-inject.h >>> @@ -20,7 +20,6 @@ struct fault_attr { >>> atomic_t space; >>> unsigned long verbose; >>> bool task_filter; >>> - bool no_warn; >>> unsigned long stacktrace_depth; >>> unsigned long require_start; >>> unsigned long require_end; >>> @@ -32,6 +31,10 @@ struct fault_attr { >>> struct dentry *dname; >>> }; >>> +enum fault_flags { >>> + FAULT_NOWARN = 1 << 0, >>> +}; >>> + >>> #define FAULT_ATTR_INITIALIZER { \ >>> .interval = 1, \ >>> .times = ATOMIC_INIT(1), \ >>> @@ -40,11 +43,11 @@ struct fault_attr { >>> .ratelimit_state = RATELIMIT_STATE_INIT_DISABLED, \ >>> .verbose = 2, \ >>> .dname = NULL, \ >>> - .no_warn = false, \ >> >> How about keep no_warn attr as it be, and export it to user? >> >> When testing with fault injection, and each fault will print an backtrace. >> but not all of the testsuit can tell us which one is fault injection >> message or other is a real warning/crash like syzkaller do. >> >> In my case, to make things simple, we usually used a regex to detect whether >> wanring/error happend. So we disabled the slab/page fault warning message by >> default, and only enable it when debug real issue. > > So you want to set/clear this no_warn attr through the procfs or sysfs > interface, so that you can easily disable/enable the slab/page fault > warning message from the user mode. Right? Yes, just like: echo 1 > /sys/kernel/debug/failslab/no_warn #disable message echo 0 > /sys/kernel/debug/failslab/no_warn #enable message Regards Wei Yongjun > > Seems reasonable to me. Anyone else has an opinion on this? If it is > really needed, I can do it later. > > Thanks, > Qi > >> >> Regards, >> >> >>> } >>> #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER >>> int setup_fault_attr(struct fault_attr *attr, char *str); >>> +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags); >>> bool should_fail(struct fault_attr *attr, ssize_t size); >>> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >>> diff --git a/lib/fault-inject.c b/lib/fault-inject.c >>> index 4b8fafce415c..5971f7c3e49e 100644 >>> --- a/lib/fault-inject.c >>> +++ b/lib/fault-inject.c >>> @@ -41,9 +41,6 @@ EXPORT_SYMBOL_GPL(setup_fault_attr); >>> static void fail_dump(struct fault_attr *attr) >>> { >>> - if (attr->no_warn) >>> - return; >>> - >>> if (attr->verbose > 0 && __ratelimit(&attr->ratelimit_state)) { >>> printk(KERN_NOTICE "FAULT_INJECTION: forcing a failure.\n" >>> "name %pd, interval %lu, probability %lu, " >>> @@ -103,7 +100,7 @@ static inline bool fail_stacktrace(struct fault_attr *attr) >>> * http://www.nongnu.org/failmalloc/ >>> */ >>> -bool should_fail(struct fault_attr *attr, ssize_t size) >>> +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags) >>> { >>> bool stack_checked = false; >>> @@ -152,13 +149,20 @@ bool should_fail(struct fault_attr *attr, ssize_t size) >>> return false; >>> fail: >>> - fail_dump(attr); >>> + if (!(flags & FAULT_NOWARN)) >>> + fail_dump(attr); >>> if (atomic_read(&attr->times) != -1) >>> atomic_dec_not_zero(&attr->times); >>> return true; >>> } >>> +EXPORT_SYMBOL_GPL(should_fail_ex); >>> + >>> +bool should_fail(struct fault_attr *attr, ssize_t size) >>> +{ >>> + return should_fail_ex(attr, size, 0); >>> +} >>> EXPORT_SYMBOL_GPL(should_fail); >>> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >>> diff --git a/mm/failslab.c b/mm/failslab.c >>> index 58df9789f1d2..ffc420c0e767 100644 >>> --- a/mm/failslab.c >>> +++ b/mm/failslab.c >>> @@ -16,6 +16,8 @@ static struct { >>> bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >>> { >>> + int flags = 0; >>> + >>> /* No fault-injection for bootstrap cache */ >>> if (unlikely(s == kmem_cache)) >>> return false; >>> @@ -30,10 +32,16 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >>> if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) >>> return false; >>> + /* >>> + * In some cases, it expects to specify __GFP_NOWARN >>> + * to avoid printing any information(not just a warning), >>> + * thus avoiding deadlocks. See commit 6b9dbedbe349 for >>> + * details. >>> + */ >>> if (gfpflags & __GFP_NOWARN) >>> - failslab.attr.no_warn = true; >>> + flags |= FAULT_NOWARN; >>> - return should_fail(&failslab.attr, s->object_size); >>> + return should_fail_ex(&failslab.attr, s->object_size, flags); >>> } >>> static int __init setup_failslab(char *str) >>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>> index 7192ded44ad0..cb6fe715d983 100644 >>> --- a/mm/page_alloc.c >>> +++ b/mm/page_alloc.c >>> @@ -3902,6 +3902,8 @@ __setup("fail_page_alloc=", setup_fail_page_alloc); >>> static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) >>> { >>> + int flags = 0; >>> + >>> if (order < fail_page_alloc.min_order) >>> return false; >>> if (gfp_mask & __GFP_NOFAIL) >>> @@ -3912,10 +3914,11 @@ static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) >>> (gfp_mask & __GFP_DIRECT_RECLAIM)) >>> return false; >>> + /* See comment in __should_failslab() */ >>> if (gfp_mask & __GFP_NOWARN) >>> - fail_page_alloc.attr.no_warn = true; >>> + flags |= FAULT_NOWARN; >>> - return should_fail(&fail_page_alloc.attr, 1 << order); >>> + return should_fail_ex(&fail_page_alloc.attr, 1 << order, flags); >>> } >>> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 9:32 ` Wei Yongjun @ 2022-11-08 9:45 ` Qi Zheng 2022-11-08 12:04 ` Jason Gunthorpe 1 sibling, 0 replies; 14+ messages in thread From: Qi Zheng @ 2022-11-08 9:45 UTC (permalink / raw) To: Wei Yongjun, dvyukov, jgg, willy, akinobu.mita Cc: akpm, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/8 17:32, Wei Yongjun wrote: > > > On 2022/11/8 16:58, Qi Zheng wrote: >> >> >> On 2022/11/8 16:44, Wei Yongjun wrote: >>> Hi Zheng Qi, >>> >>> On 2022/11/8 11:52, Qi Zheng wrote: >>>> When we specify __GFP_NOWARN, we only expect that no warnings >>>> will be issued for current caller. But in the __should_failslab() >>>> and __should_fail_alloc_page(), the local GFP flags alter the >>>> global {failslab|fail_page_alloc}.attr, which is persistent and >>>> shared by all tasks. This is not what we expected, let's fix it. >>>> >>>> Cc: stable@vger.kernel.org >>>> Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") >>>> Reported-by: Dmitry Vyukov <dvyukov@google.com> >>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >>>> --- >>>> v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ >>>> >>>> Changelog in v1 -> v2: >>>> - add comment for __should_failslab() and __should_fail_alloc_page() >>>> (suggested by Jason) >>>> >>>> include/linux/fault-inject.h | 7 +++++-- >>>> lib/fault-inject.c | 14 +++++++++----- >>>> mm/failslab.c | 12 ++++++++++-- >>>> mm/page_alloc.c | 7 +++++-- >>>> 4 files changed, 29 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h >>>> index 9f6e25467844..444236dadcf0 100644 >>>> --- a/include/linux/fault-inject.h >>>> +++ b/include/linux/fault-inject.h >>>> @@ -20,7 +20,6 @@ struct fault_attr { >>>> atomic_t space; >>>> unsigned long verbose; >>>> bool task_filter; >>>> - bool no_warn; >>>> unsigned long stacktrace_depth; >>>> unsigned long require_start; >>>> unsigned long require_end; >>>> @@ -32,6 +31,10 @@ struct fault_attr { >>>> struct dentry *dname; >>>> }; >>>> +enum fault_flags { >>>> + FAULT_NOWARN = 1 << 0, >>>> +}; >>>> + >>>> #define FAULT_ATTR_INITIALIZER { \ >>>> .interval = 1, \ >>>> .times = ATOMIC_INIT(1), \ >>>> @@ -40,11 +43,11 @@ struct fault_attr { >>>> .ratelimit_state = RATELIMIT_STATE_INIT_DISABLED, \ >>>> .verbose = 2, \ >>>> .dname = NULL, \ >>>> - .no_warn = false, \ >>> >>> How about keep no_warn attr as it be, and export it to user? >>> >>> When testing with fault injection, and each fault will print an backtrace. >>> but not all of the testsuit can tell us which one is fault injection >>> message or other is a real warning/crash like syzkaller do. >>> >>> In my case, to make things simple, we usually used a regex to detect whether >>> wanring/error happend. So we disabled the slab/page fault warning message by >>> default, and only enable it when debug real issue. >> >> So you want to set/clear this no_warn attr through the procfs or sysfs >> interface, so that you can easily disable/enable the slab/page fault >> warning message from the user mode. Right? > > Yes, just like: > > echo 1 > /sys/kernel/debug/failslab/no_warn #disable message > echo 0 > /sys/kernel/debug/failslab/no_warn #enable message Got it. Let's wait for the other people's comments and suggestions. :) > > Regards > Wei Yongjun > >> >> Seems reasonable to me. Anyone else has an opinion on this? If it is >> really needed, I can do it later. >> >> Thanks, >> Qi >> >>> >>> Regards, >>> >>> >>>> } >>>> #define DECLARE_FAULT_ATTR(name) struct fault_attr name = FAULT_ATTR_INITIALIZER >>>> int setup_fault_attr(struct fault_attr *attr, char *str); >>>> +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags); >>>> bool should_fail(struct fault_attr *attr, ssize_t size); >>>> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >>>> diff --git a/lib/fault-inject.c b/lib/fault-inject.c >>>> index 4b8fafce415c..5971f7c3e49e 100644 >>>> --- a/lib/fault-inject.c >>>> +++ b/lib/fault-inject.c >>>> @@ -41,9 +41,6 @@ EXPORT_SYMBOL_GPL(setup_fault_attr); >>>> static void fail_dump(struct fault_attr *attr) >>>> { >>>> - if (attr->no_warn) >>>> - return; >>>> - >>>> if (attr->verbose > 0 && __ratelimit(&attr->ratelimit_state)) { >>>> printk(KERN_NOTICE "FAULT_INJECTION: forcing a failure.\n" >>>> "name %pd, interval %lu, probability %lu, " >>>> @@ -103,7 +100,7 @@ static inline bool fail_stacktrace(struct fault_attr *attr) >>>> * http://www.nongnu.org/failmalloc/ >>>> */ >>>> -bool should_fail(struct fault_attr *attr, ssize_t size) >>>> +bool should_fail_ex(struct fault_attr *attr, ssize_t size, int flags) >>>> { >>>> bool stack_checked = false; >>>> @@ -152,13 +149,20 @@ bool should_fail(struct fault_attr *attr, ssize_t size) >>>> return false; >>>> fail: >>>> - fail_dump(attr); >>>> + if (!(flags & FAULT_NOWARN)) >>>> + fail_dump(attr); >>>> if (atomic_read(&attr->times) != -1) >>>> atomic_dec_not_zero(&attr->times); >>>> return true; >>>> } >>>> +EXPORT_SYMBOL_GPL(should_fail_ex); >>>> + >>>> +bool should_fail(struct fault_attr *attr, ssize_t size) >>>> +{ >>>> + return should_fail_ex(attr, size, 0); >>>> +} >>>> EXPORT_SYMBOL_GPL(should_fail); >>>> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >>>> diff --git a/mm/failslab.c b/mm/failslab.c >>>> index 58df9789f1d2..ffc420c0e767 100644 >>>> --- a/mm/failslab.c >>>> +++ b/mm/failslab.c >>>> @@ -16,6 +16,8 @@ static struct { >>>> bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >>>> { >>>> + int flags = 0; >>>> + >>>> /* No fault-injection for bootstrap cache */ >>>> if (unlikely(s == kmem_cache)) >>>> return false; >>>> @@ -30,10 +32,16 @@ bool __should_failslab(struct kmem_cache *s, gfp_t gfpflags) >>>> if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB)) >>>> return false; >>>> + /* >>>> + * In some cases, it expects to specify __GFP_NOWARN >>>> + * to avoid printing any information(not just a warning), >>>> + * thus avoiding deadlocks. See commit 6b9dbedbe349 for >>>> + * details. >>>> + */ >>>> if (gfpflags & __GFP_NOWARN) >>>> - failslab.attr.no_warn = true; >>>> + flags |= FAULT_NOWARN; >>>> - return should_fail(&failslab.attr, s->object_size); >>>> + return should_fail_ex(&failslab.attr, s->object_size, flags); >>>> } >>>> static int __init setup_failslab(char *str) >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index 7192ded44ad0..cb6fe715d983 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -3902,6 +3902,8 @@ __setup("fail_page_alloc=", setup_fail_page_alloc); >>>> static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) >>>> { >>>> + int flags = 0; >>>> + >>>> if (order < fail_page_alloc.min_order) >>>> return false; >>>> if (gfp_mask & __GFP_NOFAIL) >>>> @@ -3912,10 +3914,11 @@ static bool __should_fail_alloc_page(gfp_t gfp_mask, unsigned int order) >>>> (gfp_mask & __GFP_DIRECT_RECLAIM)) >>>> return false; >>>> + /* See comment in __should_failslab() */ >>>> if (gfp_mask & __GFP_NOWARN) >>>> - fail_page_alloc.attr.no_warn = true; >>>> + flags |= FAULT_NOWARN; >>>> - return should_fail(&fail_page_alloc.attr, 1 << order); >>>> + return should_fail_ex(&fail_page_alloc.attr, 1 << order, flags); >>>> } >>>> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS >> -- Thanks, Qi ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 9:32 ` Wei Yongjun 2022-11-08 9:45 ` Qi Zheng @ 2022-11-08 12:04 ` Jason Gunthorpe 2022-11-09 3:57 ` Wei Yongjun 1 sibling, 1 reply; 14+ messages in thread From: Jason Gunthorpe @ 2022-11-08 12:04 UTC (permalink / raw) To: Wei Yongjun Cc: Qi Zheng, dvyukov, willy, akinobu.mita, akpm, linux-kernel, linux-mm, linux-fsdevel, stable On Tue, Nov 08, 2022 at 05:32:52PM +0800, Wei Yongjun wrote: > > So you want to set/clear this no_warn attr through the procfs or sysfs > > interface, so that you can easily disable/enable the slab/page fault > > warning message from the user mode. Right? > > Yes, just like: > > echo 1 > /sys/kernel/debug/failslab/no_warn #disable message > echo 0 > /sys/kernel/debug/failslab/no_warn #enable message You can already do that: echo 0 > /sys/kernel/debug/failslab/verbose #disable message Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 12:04 ` Jason Gunthorpe @ 2022-11-09 3:57 ` Wei Yongjun 0 siblings, 0 replies; 14+ messages in thread From: Wei Yongjun @ 2022-11-09 3:57 UTC (permalink / raw) To: Jason Gunthorpe Cc: Qi Zheng, dvyukov, willy, akinobu.mita, akpm, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/8 20:04, Jason Gunthorpe wrote: > On Tue, Nov 08, 2022 at 05:32:52PM +0800, Wei Yongjun wrote: >>> So you want to set/clear this no_warn attr through the procfs or sysfs >>> interface, so that you can easily disable/enable the slab/page fault >>> warning message from the user mode. Right? >> >> Yes, just like: >> >> echo 1 > /sys/kernel/debug/failslab/no_warn #disable message >> echo 0 > /sys/kernel/debug/failslab/no_warn #enable message > > You can already do that: > > echo 0 > /sys/kernel/debug/failslab/verbose #disable message Got it, thanks. Wei Yongjun ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 3:52 ` [PATCH v2] " Qi Zheng 2022-11-08 8:44 ` Wei Yongjun @ 2022-11-08 17:36 ` Akinobu Mita 2022-11-14 3:59 ` Qi Zheng 1 sibling, 1 reply; 14+ messages in thread From: Akinobu Mita @ 2022-11-08 17:36 UTC (permalink / raw) To: Qi Zheng Cc: dvyukov, jgg, willy, akpm, linux-kernel, linux-mm, linux-fsdevel, stable 2022年11月8日(火) 12:52 Qi Zheng <zhengqi.arch@bytedance.com>: > > When we specify __GFP_NOWARN, we only expect that no warnings > will be issued for current caller. But in the __should_failslab() > and __should_fail_alloc_page(), the local GFP flags alter the > global {failslab|fail_page_alloc}.attr, which is persistent and > shared by all tasks. This is not what we expected, let's fix it. > > Cc: stable@vger.kernel.org > Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ > > Changelog in v1 -> v2: > - add comment for __should_failslab() and __should_fail_alloc_page() > (suggested by Jason) Looks good. Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr 2022-11-08 17:36 ` Akinobu Mita @ 2022-11-14 3:59 ` Qi Zheng 0 siblings, 0 replies; 14+ messages in thread From: Qi Zheng @ 2022-11-14 3:59 UTC (permalink / raw) To: Akinobu Mita, Andrew Morton Cc: dvyukov, jgg, willy, linux-kernel, linux-mm, linux-fsdevel, stable On 2022/11/9 01:36, Akinobu Mita wrote: > 2022年11月8日(火) 12:52 Qi Zheng <zhengqi.arch@bytedance.com>: >> >> When we specify __GFP_NOWARN, we only expect that no warnings >> will be issued for current caller. But in the __should_failslab() >> and __should_fail_alloc_page(), the local GFP flags alter the >> global {failslab|fail_page_alloc}.attr, which is persistent and >> shared by all tasks. This is not what we expected, let's fix it. >> >> Cc: stable@vger.kernel.org >> Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN") >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@bytedance.com/ >> >> Changelog in v1 -> v2: >> - add comment for __should_failslab() and __should_fail_alloc_page() >> (suggested by Jason) > > Looks good. > > Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com> Thanks. And hi Andrew, seems no action left for me, can this patch be applied to mm-unstable tree for testing? :) -- Thanks, Qi ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-11-14 3:59 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CACT4Y+Zc21Aj+5KjeTEsvOysJGHRYDSKgu_+_xN1LUYfG_H0sg@mail.gmail.com>
2022-11-07 3:31 ` [PATCH] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr Qi Zheng
2022-11-07 12:42 ` Jason Gunthorpe
2022-11-07 15:05 ` Qi Zheng
2022-11-07 16:26 ` Jason Gunthorpe
2022-11-08 2:47 ` Qi Zheng
2022-11-08 3:52 ` [PATCH v2] " Qi Zheng
2022-11-08 8:44 ` Wei Yongjun
2022-11-08 8:58 ` Qi Zheng
2022-11-08 9:32 ` Wei Yongjun
2022-11-08 9:45 ` Qi Zheng
2022-11-08 12:04 ` Jason Gunthorpe
2022-11-09 3:57 ` Wei Yongjun
2022-11-08 17:36 ` Akinobu Mita
2022-11-14 3:59 ` Qi Zheng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox