From: Luis Chamberlain <mcgrof@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: tj@kernel.org, gregkh@linuxfoundation.org,
akpm@linux-foundation.org, minchan@kernel.org, jeyu@kernel.org,
shuah@kernel.org, bvanassche@acm.org, dan.j.williams@intel.com,
joe@perches.com, tglx@linutronix.de, rostedt@goodmis.org,
linux-spdx@vger.kernel.org, linux-doc@vger.kernel.org,
linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 04/12] kernfs: add initial failure injection support
Date: Mon, 11 Oct 2021 13:44:19 -0700 [thread overview]
Message-ID: <YWSiIwr/8/JQE9qW@bombadil.infradead.org> (raw)
In-Reply-To: <202110051225.419CD64@keescook>
On Tue, Oct 05, 2021 at 12:47:22PM -0700, Kees Cook wrote:
> On Mon, Sep 27, 2021 at 09:37:57AM -0700, Luis Chamberlain wrote:
> > This adds initial failure injection support to kernfs. We start
> > off with debug knobs which when enabled allow test drivers, such as
> > test_sysfs, to then make use of these to try to force certain
> > difficult races to take place with a high degree of certainty.
> >
> > This only adds runtime code *iff* the new bool CONFIG_FAIL_KERNFS_KNOBS is
> > enabled in your kernel. If you don't have this enabled this provides
> > no new functional. When CONFIG_FAIL_KERNFS_KNOBS is disabled the new
> > routine kernfs_debug_should_wait() ends up being transformed to if
> > (false), and so the compiler should optimize these out as dead code
> > producing no new effective binary changes.
> >
> > We start off with enabling failure injections in kernfs by allowing us to
> > alter the way kernfs_fop_write_iter() behaves. We allow for the routine
> > kernfs_fop_write_iter() to wait for a certain condition in the kernel to
> > occur, after which it will sleep a predefined amount of time. This lets
> > kernfs users to time exactly when it want kernfs_fop_write_iter() to
> > complete, allowing for developing race conditions and test for correctness
> > in kernfs.
> >
> > You'd boot with this enabled on your kernel command line:
> >
> > fail_kernfs_fop_write_iter=1,100,0,1
> >
> > The values are <interval,probability,size,times>, we don't care for
> > size, so for now we ignore it. The above ensures a failure will trigger
> > only once.
> >
> > *How* we allow for this routine to change behaviour is left to knobs we
> > expose under debugfs:
> >
> > # ls -1 /sys/kernel/debug/kernfs/config_fail_kernfs_fop_write_iter/
>
> I'd expect this to live under /sys/kernel/debug/fail_kernfs, like the
> other fault injectors.
Yes I see, thanks will fix up!
> > diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
> > index 4a25c5eb6f07..d4d34b082f47 100644
> > --- a/Documentation/fault-injection/fault-injection.rst
> > +++ b/Documentation/fault-injection/fault-injection.rst
> > @@ -28,6 +28,28 @@ Available fault injection capabilities
> >
> > injects kernel RPC client and server failures.
> >
> > +- fail_kernfs_fop_write_iter
> > +
> > + Allows for failures to be enabled inside kernfs_fop_write_iter(). Enabling
> > + this does not immediately enable any errors to occur. You must configure
> > + how you want this routine to fail or change behaviour by using the debugfs
> > + knobs for it:
> > +
> > + # ls -1 /sys/kernel/debug/kernfs/config_fail_kernfs_fop_write_iter/
> > + wait_after_active
> > + wait_after_mutex
> > + wait_at_start
> > + wait_before_mutex
>
> This should be split up and detailed in the "debugfs entries" section
> below here.
Done!
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1b4cefcb064c..fadfd961ad80 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10384,7 +10384,7 @@ M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > M: Tejun Heo <tj@kernel.org>
> > S: Supported
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> > -F: fs/kernfs/
> > +F: fs/kernfs/*
> > F: include/linux/kernfs.h
> >
> > KEXEC
> > diff --git a/fs/kernfs/Makefile b/fs/kernfs/Makefile
> > index 4ca54ff54c98..bc5b32ca39f9 100644
> > --- a/fs/kernfs/Makefile
> > +++ b/fs/kernfs/Makefile
> > @@ -4,3 +4,4 @@
> > #
> >
> > obj-y := mount.o inode.o dir.o file.o symlink.o
> > +obj-$(CONFIG_FAIL_KERNFS_KNOBS) += failure-injection.o
> > diff --git a/fs/kernfs/failure-injection.c b/fs/kernfs/failure-injection.c
> > new file mode 100644
> > index 000000000000..4130d202c13b
> > --- /dev/null
> > +++ b/fs/kernfs/failure-injection.c
>
> I'd name this fault_inject.c, which matches the more common case:
>
> $ find . -type f -name '*fault*inject*.c'
> ./fs/nfsd/fault_inject.c
> ./drivers/nvme/host/fault_inject.c
> ./drivers/scsi/ufs/ufs-fault-injection.c
> ./lib/fault-inject.c
> ./lib/fault-inject-usercopy.c
Sure, done.
> > +int __kernfs_debug_should_wait_kernfs_fop_write_iter(bool evaluate)
> > +{
> > + if (!evaluate)
> > + return 0;
> > +
> > + return should_fail(&fail_kernfs_fop_write_iter, 0);
> > +}
>
> Every caller ends up doing the wait, so how about just including that
> here instead? It should make things much less intrusive and more readable.
>
> And for the naming, other fault injectors use "should_fail_$topic", so
> maybe better here would be something like may_wait_kernfs(...).
In case anyone is reading Hail Mary by Andy Weir: "Yes yes yes!"
Indeed, that's a great idea. Changed!
> > +
> > +DECLARE_COMPLETION(kernfs_debug_wait_completion);
> > +EXPORT_SYMBOL_NS_GPL(kernfs_debug_wait_completion, KERNFS_DEBUG_PRIVATE);
> > +
> > +void kernfs_debug_wait(void)
> > +{
> > + unsigned long timeout;
> > +
> > + timeout = wait_for_completion_timeout(&kernfs_debug_wait_completion,
> > + msecs_to_jiffies(3000));
> > + if (!timeout)
> > + pr_info("%s waiting for kernfs_debug_wait_completion timed out\n",
> > + __func__);
> > + else
> > + pr_info("%s received completion with time left on timeout %u ms\n",
> > + __func__, jiffies_to_msecs(timeout));
> > +
> > + /**
> > + * The goal is wait for an event, and *then* once we have
> > + * reached it, the other side will try to do something which
> > + * it thinks will break. So we must give it some time to do
> > + * that. The amount of time is configurable.
> > + */
> > + msleep(kernfs_config_fail.sleep_after_wait_ms);
> > + pr_info("%s ended\n", __func__);
> > +}
>
> All the uses of "__func__" here seems redundant; I would drop them.
Alright, and I also added the pr_fmt define which I forgot.
> > diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> > index 60e2a86c535e..4479c6580333 100644
> > --- a/fs/kernfs/file.c
> > +++ b/fs/kernfs/file.c
> > @@ -259,6 +259,9 @@ static ssize_t kernfs_fop_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> > const struct kernfs_ops *ops;
> > char *buf;
> >
> > + if (kernfs_debug_should_wait(kernfs_fop_write_iter, at_start))
> > + kernfs_debug_wait();
>
> So this could just be:
>
> may_wait_kernfs(kernfs_fop_write_iter, at_start);
Yup! Thanks!
> > diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> > index f9cc912c31e1..9e3abf597e2d 100644
> > --- a/fs/kernfs/kernfs-internal.h
> > +++ b/fs/kernfs/kernfs-internal.h
> > +#define __kernfs_config_wait_var(func, when) \
> > + (kernfs_config_fail. func ## _fail.wait_ ## when)
> ^^ ^ ^
> nit: needless spaces
Trimmed.
Luis
next prev parent reply other threads:[~2021-10-11 20:44 UTC|newest]
Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-27 16:37 [PATCH v8 00/12] syfs: generic deadlock fix with module removal Luis Chamberlain
2021-09-27 16:37 ` [PATCH v8 01/12] LICENSES: Add the copyleft-next-0.3.1 license Luis Chamberlain
[not found] ` <202110050907.35FBD2A1@keescook>
[not found] ` <YWR2ZrtzChamY1y4@bombadil.infradead.org>
2021-10-11 17:57 ` Kees Cook
2021-09-27 16:37 ` [PATCH v8 02/12] testing: use the copyleft-next-0.3.1 SPDX tag Luis Chamberlain
2021-10-05 16:11 ` Kees Cook
2021-09-27 16:37 ` [PATCH v8 03/12] selftests: add tests_sysfs module Luis Chamberlain
2021-10-05 14:16 ` Greg KH
2021-10-05 16:57 ` Tim.Bird
2021-10-11 17:40 ` Luis Chamberlain
2021-10-11 17:38 ` Luis Chamberlain
2021-10-07 14:23 ` Miroslav Benes
2021-10-11 19:11 ` Luis Chamberlain
[not found] ` <202110050912.3DF681ED@keescook>
2021-10-11 19:03 ` Luis Chamberlain
2021-09-27 16:37 ` [PATCH v8 04/12] kernfs: add initial failure injection support Luis Chamberlain
2021-10-05 19:47 ` Kees Cook
2021-10-11 20:44 ` Luis Chamberlain [this message]
2021-09-27 16:37 ` [PATCH v8 05/12] test_sysfs: add support to use kernfs failure injection Luis Chamberlain
2021-10-05 19:51 ` Kees Cook
2021-10-11 20:56 ` Luis Chamberlain
2021-09-27 16:37 ` [PATCH v8 06/12] kernel/module: add documentation for try_module_get() Luis Chamberlain
2021-10-05 19:58 ` Kees Cook
2021-10-11 21:16 ` Luis Chamberlain
2021-09-27 16:38 ` [PATCH v8 07/12] fs/kernfs/symlink.c: replace S_IRWXUGO with 0777 on kernfs_create_link() Luis Chamberlain
2021-10-05 19:59 ` Kees Cook
2021-09-27 16:38 ` [PATCH v8 08/12] fs/sysfs/dir.c: replace S_IRWXU|S_IRUGO|S_IXUGO with 0755 sysfs_create_dir_ns() Luis Chamberlain
2021-10-05 16:05 ` Kees Cook
2021-09-27 16:38 ` [PATCH v8 09/12] sysfs: fix deadlock race with module removal Luis Chamberlain
2021-10-05 9:24 ` Ming Lei
2021-10-11 21:25 ` Luis Chamberlain
2021-10-12 0:20 ` Ming Lei
2021-10-12 21:18 ` Luis Chamberlain
2021-10-13 1:07 ` Ming Lei
2021-10-13 12:35 ` Luis Chamberlain
2021-10-13 15:04 ` Ming Lei
2021-10-13 21:16 ` Luis Chamberlain
2021-10-05 20:50 ` Kees Cook
2021-10-11 22:26 ` Luis Chamberlain
2021-10-13 12:41 ` Luis Chamberlain
2021-09-27 16:38 ` [PATCH v8 10/12] test_sysfs: enable deadlock tests by default Luis Chamberlain
2021-09-27 16:38 ` [PATCH v8 11/12] zram: fix crashes with cpu hotplug multistate Luis Chamberlain
2021-10-05 20:55 ` Kees Cook
2021-10-11 18:27 ` Luis Chamberlain
2021-10-14 1:55 ` Ming Lei
2021-10-14 2:11 ` Ming Lei
2021-10-14 20:24 ` Luis Chamberlain
2021-10-14 23:52 ` Ming Lei
2021-10-15 0:22 ` Luis Chamberlain
2021-10-15 8:36 ` Ming Lei
2021-10-15 8:52 ` Greg KH
2021-10-15 17:31 ` Luis Chamberlain
2021-10-16 11:28 ` Ming Lei
2021-10-18 19:32 ` Luis Chamberlain
2021-10-19 2:34 ` Ming Lei
2021-10-19 6:23 ` Miroslav Benes
2021-10-19 9:23 ` Ming Lei
2021-10-20 6:43 ` Miroslav Benes
2021-10-20 7:49 ` Ming Lei
2021-10-20 8:19 ` Miroslav Benes
2021-10-20 8:28 ` Greg KH
2021-10-25 9:58 ` Miroslav Benes
2021-10-20 10:09 ` Ming Lei
2021-10-26 8:48 ` Petr Mladek
2021-10-26 15:37 ` Ming Lei
2021-10-26 17:01 ` Luis Chamberlain
2021-10-27 11:57 ` Miroslav Benes
2021-10-27 14:27 ` Luis Chamberlain
2021-11-02 15:24 ` Petr Mladek
2021-11-02 16:25 ` Luis Chamberlain
2021-11-03 0:01 ` Ming Lei
2021-11-03 12:44 ` Luis Chamberlain
2021-10-27 11:42 ` Miroslav Benes
2021-11-02 14:15 ` Petr Mladek
2021-11-02 14:51 ` Petr Mladek
2021-11-02 15:17 ` Ming Lei
2021-11-02 14:56 ` Ming Lei
2021-10-19 15:28 ` Luis Chamberlain
2021-10-19 16:29 ` Ming Lei
2021-10-19 19:36 ` Luis Chamberlain
2021-10-20 1:15 ` Ming Lei
2021-10-20 15:48 ` Luis Chamberlain
2021-10-21 0:39 ` Ming Lei
2021-10-21 17:18 ` Luis Chamberlain
2021-10-22 0:05 ` Ming Lei
2021-10-19 15:50 ` Luis Chamberlain
2021-10-19 16:25 ` Greg KH
2021-10-19 16:30 ` Luis Chamberlain
2021-10-19 17:28 ` Greg KH
2021-10-19 19:46 ` Luis Chamberlain
2021-10-19 16:39 ` Ming Lei
2021-10-19 19:38 ` Luis Chamberlain
2021-10-20 0:55 ` Ming Lei
2021-09-27 16:38 ` [PATCH v8 12/12] zram: use ATTRIBUTE_GROUPS to fix sysfs deadlock module removal Luis Chamberlain
2021-10-05 20:57 ` Kees Cook
2021-10-11 18:28 ` Luis Chamberlain
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=YWSiIwr/8/JQE9qW@bombadil.infradead.org \
--to=mcgrof@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=bvanassche@acm.org \
--cc=dan.j.williams@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jeyu@kernel.org \
--cc=joe@perches.com \
--cc=keescook@chromium.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-spdx@vger.kernel.org \
--cc=minchan@kernel.org \
--cc=rostedt@goodmis.org \
--cc=shuah@kernel.org \
--cc=tglx@linutronix.de \
--cc=tj@kernel.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).