From: Qi Zheng <zhengqi.arch@bytedance.com>
To: Dmitry Vyukov <dvyukov@google.com>, Jason Gunthorpe <jgg@nvidia.com>
Cc: Matthew Wilcox <willy@infradead.org>,
Akinobu Mita <akinobu.mita@gmail.com>,
linux-fsdevel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
syzkaller <syzkaller@googlegroups.com>
Subject: Re: xarray, fault injection and syzkaller
Date: Sat, 5 Nov 2022 20:16:35 +0800 [thread overview]
Message-ID: <be6a67b0-479f-db0a-fa69-764713135d70@bytedance.com> (raw)
In-Reply-To: <6e33dd02-99b0-0899-aed5-07f770340a74@bytedance.com>
On 2022/11/5 06:43, Qi Zheng wrote:
>
>
> On 2022/11/5 02:21, Dmitry Vyukov wrote:
>> On Fri, 4 Nov 2022 at 11:03, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>
>>> On Fri, Nov 04, 2022 at 10:47:17AM -0700, Dmitry Vyukov wrote:
>>>>>> Do we know how common/useful such an allocation pattern is?
>>>>>
>>>>> I have coded something like this a few times, in my cases it is
>>>>> usually something like: try to allocate a big chunk of memory hoping
>>>>> for a huge page, then fall back to a smaller allocation
>>>>>
>>>>> Most likely the key consideration is that the callsites are using
>>>>> GFP_NOWARN, so perhaps we can just avoid decrementing the nth on a
>>>>> NOWARN case assuming that another allocation attempt will closely
>>>>> follow?
>>>>
>>>> GFP_NOWARN is also extensively used for allocations with
>>>> user-controlled size, e.g.:
>>>> https://elixir.bootlin.com/linux/v6.1-rc3/source/net/unix/af_unix.c#L3451
>>>>
>>>> That's different and these allocations are usually not repeated.
>>>> So looking at GFP_NOWARN does not look like the right thing to do.
>>>
>>> This may be the best option then, arguably perhaps even more
>>> "realistic" than normal fail_nth as in a real system if this stuff
>>> starts failing there is a good chance things from then on will fail
>>> too during the error cleanup.
>>>
>>>>> However, this would also have to fix the obnoxious behavior of fail
>>>>> nth where it fails its own copy_from_user on its write system call -
>>>>> meaning there would be no way to turn it off.
>>>>
>>>> Oh, interesting. We added failing of copy_from/to_user later and did
>>>> not consider such interaction.
>>>> Filed https://bugzilla.kernel.org/show_bug.cgi?id=216660 for this.
>>>
>>> Oh, I will tell you the other two bugish things I noticed
>>>
>>> __should_failslab() has this:
>>>
>>> if (gfpflags & __GFP_NOWARN)
>>> failslab.attr.no_warn = true;
>>>
>>> return should_fail(&failslab.attr, s->object_size);
>>>
>>> Which always permanently turns off no_warn for slab during early
>>> boot. This is why syzkaller reports are so confusing. They trigger a
>>> slab fault injection, which in all other cases gives a notification
>>> backtrace, but in slab cases there is no hint about the fault
>>> injection in the log.
>>
>> Ouch, this looks like a bug in:
>>
>> commit 3f913fc5f9745613088d3c569778c9813ab9c129
>> Author: Qi Zheng <zhengqi.arch@bytedance.com>
>> Date: Thu May 19 14:08:55 2022 -0700
>> mm: fix missing handler for __GFP_NOWARN
>>
>> +Qi could you please fix it?
>>
>> At the very least the local gfpflags should not alter the global
>> failslab.attr that is persistent and shared by all tasks.
>
> Oh, It indeed shouldn't alter the global failslab.attr, I'll fix it.
How about the following changes? If it's ok, I will send this fix patch.
Thanks. :)
diff --git a/include/linux/fault-inject.h b/include/linux/fault-inject.h
index 9f6e25467844..b61a3fb7a2a3 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;
@@ -40,12 +39,12 @@ 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(struct fault_attr *attr, ssize_t size);
+bool should_fail_gfp(struct fault_attr *attr, ssize_t size, gfp_t
gfpflags);
#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 4b8fafce415c..95af50832770 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, "
@@ -98,12 +95,7 @@ static inline bool fail_stacktrace(struct fault_attr
*attr)
#endif /* CONFIG_FAULT_INJECTION_STACKTRACE_FILTER */
-/*
- * This code is stolen from failmalloc-1.0
- * http://www.nongnu.org/failmalloc/
- */
-
-bool should_fail(struct fault_attr *attr, ssize_t size)
+bool should_fail_check(struct fault_attr *attr, ssize_t size)
{
bool stack_checked = false;
@@ -118,7 +110,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size)
fail_nth--;
WRITE_ONCE(current->fail_nth, fail_nth);
if (!fail_nth)
- goto fail;
+ return true;
return false;
}
@@ -151,7 +143,19 @@ bool should_fail(struct fault_attr *attr, ssize_t size)
if (attr->probability <= get_random_u32_below(100))
return false;
-fail:
+ return true;
+}
+
+/*
+ * This code is stolen from failmalloc-1.0
+ * http://www.nongnu.org/failmalloc/
+ */
+
+bool should_fail(struct fault_attr *attr, ssize_t size)
+{
+ if (!should_fail_check(attr, size))
+ return false;
+
fail_dump(attr);
if (atomic_read(&attr->times) != -1)
@@ -161,6 +165,21 @@ bool should_fail(struct fault_attr *attr, ssize_t size)
}
EXPORT_SYMBOL_GPL(should_fail);
+bool should_fail_gfp(struct fault_attr *attr, ssize_t size, gfp_t gfpflags)
+{
+ if (!should_fail_check(attr, size))
+ return false;
+
+ if (!(gfpflags & __GFP_NOWARN))
+ fail_dump(attr);
+
+ if (atomic_read(&attr->times) != -1)
+ atomic_dec_not_zero(&attr->times);
+
+ return true;
+}
+EXPORT_SYMBOL_GPL(should_fail_gfp);
+
#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
static int debugfs_ul_set(void *data, u64 val)
diff --git a/mm/failslab.c b/mm/failslab.c
index 58df9789f1d2..21338b256791 100644
--- a/mm/failslab.c
+++ b/mm/failslab.c
@@ -30,10 +30,7 @@ bool __should_failslab(struct kmem_cache *s, gfp_t
gfpflags)
if (failslab.cache_filter && !(s->flags & SLAB_FAILSLAB))
return false;
- if (gfpflags & __GFP_NOWARN)
- failslab.attr.no_warn = true;
-
- return should_fail(&failslab.attr, s->object_size);
+ return should_fail_gfp(&failslab.attr, s->object_size, gfpflags);
}
static int __init setup_failslab(char *str)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 7192ded44ad0..4e70b5599ada 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3912,10 +3912,7 @@ static bool __should_fail_alloc_page(gfp_t
gfp_mask, unsigned int order)
(gfp_mask & __GFP_DIRECT_RECLAIM))
return false;
- if (gfp_mask & __GFP_NOWARN)
- fail_page_alloc.attr.no_warn = true;
-
- return should_fail(&fail_page_alloc.attr, 1 << order);
+ return should_fail_gfp(&fail_page_alloc.attr, 1 << order, gfp_mask);
}
#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
>
> But a warning should not be printed for callers that currently specify
> __GFP_NOWARN, because that could lead to deadlocks, such as the deadlock
> case mentioned in commit 6b9dbedbe349 ("tty: fix deadlock caused by
> calling printk() under tty_port->lock").
>
> Thanks,
> Qi
>
>>
>> But I am not sure if we really don't want to issue the fault injection
>> stack in this case. It's not a WARNING, it's merely an information
>> message. It looks useful in all cases, even with GFP_NOWARN. Why
>> should it be suppressed?
>>
>>
>>> Once that is fixed we can quickly explain why the socketpair() example
>>> in the docs shows success ret codes in the middle of the sweep when
>>> run on syzkaller kernels
>>>
>>> fail_nth interacts badly with other kernel features typically enabled
>>> in syzkaller kernels. Eg it fails in hidden kmemleak instrumentation:
>>>
>>> [ 18.499559] FAULT_INJECTION: forcing a failure.
>>> [ 18.499559] name failslab, interval 1, probability 0, space 0,
>>> times 0
>>> [ 18.499720] CPU: 10 PID: 386 Comm: iommufd_fail_nt Not tainted
>>> 6.1.0-rc3+ #34
>>> [ 18.499826] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
>>> BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
>>> [ 18.499971] Call Trace:
>>> [ 18.500010] <TASK>
>>> [ 18.500048] show_stack+0x3d/0x3f
>>> [ 18.500114] dump_stack_lvl+0x92/0xbd
>>> [ 18.500171] dump_stack+0x15/0x17
>>> [ 18.500232] should_fail.cold+0x5/0xa
>>> [ 18.500291] __should_failslab+0xb6/0x100
>>> [ 18.500349] should_failslab+0x9/0x20
>>> [ 18.500416] kmem_cache_alloc+0x64/0x4e0
>>> [ 18.500477] ? __create_object+0x40/0xc50
>>> [ 18.500539] __create_object+0x40/0xc50
>>> [ 18.500620] ? kasan_poison+0x3a/0x50
>>> [ 18.500690] ? kasan_unpoison+0x28/0x50
>>> [***18.500753] kmemleak_alloc+0x24/0x30
>>> [ 18.500816] __kmem_cache_alloc_node+0x1de/0x400
>>> [ 18.500900] ? iopt_alloc_area_pages+0x95/0x560 [iommufd]
>>> [ 18.500993] kmalloc_trace+0x26/0x110
>>> [ 18.501059] iopt_alloc_area_pages+0x95/0x560 [iommufd]
>>>
>>> Which has the consequence of syzkaller wasting half its fail_nth
>>> effort because it is triggering failures in hidden instrumentation
>>> that has no impact on the main code path.
>>>
>>> Maybe a kmem_cache_alloc_no_fault_inject() would be helpful for a few
>>> cases.
>>>
>>> Jason
>
--
Thanks,
Qi
next prev parent reply other threads:[~2022-11-05 12:16 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-03 19:09 xarray, fault injection and syzkaller Jason Gunthorpe
2022-11-03 20:00 ` Matthew Wilcox
2022-11-03 20:07 ` Jason Gunthorpe
2022-11-04 0:11 ` Dmitry Vyukov
2022-11-04 0:21 ` Jason Gunthorpe
2022-11-04 17:47 ` Dmitry Vyukov
2022-11-04 18:03 ` Jason Gunthorpe
2022-11-04 18:21 ` Dmitry Vyukov
2022-11-04 18:30 ` Jason Gunthorpe
2022-11-04 18:38 ` Dmitry Vyukov
2022-11-04 18:45 ` Jason Gunthorpe
2022-11-04 22:43 ` Qi Zheng
2022-11-05 12:16 ` Qi Zheng [this message]
2022-11-06 17:36 ` Dmitry Vyukov
2022-11-07 2:13 ` Qi Zheng
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
2022-11-04 18:42 ` xarray, fault injection and syzkaller Dmitry Vyukov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=be6a67b0-479f-db0a-fa69-764713135d70@bytedance.com \
--to=zhengqi.arch@bytedance.com \
--cc=akinobu.mita@gmail.com \
--cc=dvyukov@google.com \
--cc=jgg@nvidia.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=syzkaller-bugs@googlegroups.com \
--cc=syzkaller@googlegroups.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).