Re: [PATCH v2] mm: fix unexpected changes to {failslab|fail_page_alloc}.attr

Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> · Tue, 8 Nov 2022 17:45:30 +0800

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@xxxxxxxxxxxxxxx
Fixes: 3f913fc5f974 ("mm: fix missing handler for __GFP_NOWARN")
Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
---
   v1: https://lore.kernel.org/lkml/20221107033109.59709-1-zhengqi.arch@xxxxxxxxxxxxx/

   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






