On 2022/11/7 01:36, Dmitry Vyukov wrote:
On Sat, 5 Nov 2022 at 05:16, Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> wrote:
On 2022/11/5 02:21, Dmitry Vyukov wrote:
On Fri, 4 Nov 2022 at 11:03, Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
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. :)
I think the interface design question is mostly to Akinobu as fault
injection maintainer.
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);
should_fail/should_fail_gfp duplicate some code + gfp is slab-specific
(while clumsy to use for other fault injection types + we won't be
able to pass any runtime flags that are not present in gfp). So I
would go either with:
bool should_fail(struct fault_attr *attr, ssize_t size, bool nowarn);
or even more extensible interface would be:
enum fault_flags {
fault_nowarn = 1 << 0,
};
bool should_fail(struct fault_attr *attr, ssize_t size, fault_flags flags);
And if we don't want to change all callers to avoid code duplication:
bool should_fail_ex(struct fault_attr *attr, ssize_t size, fault_flags flags);
bool should_fail(struct fault_attr *attr, ssize_t size) {
return should_fail_ex(attr, size, 0);
}
This looks better to me, I will revise and resend this fix patch later.
Thanks,
Qi
#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
--
You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@xxxxxxxxxxxxxxxx.
To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/be6a67b0-479f-db0a-fa69-764713135d70%40bytedance.com.
--
Thanks,
Qi
