Re: [PATCH 2/2] moderr: add module error injection tool

[Luis Chamberlain <mcgrof@kernel.org>]

On Wed, Jan 22, 2025 at 09:02:19AM -0800, Alexei Starovoitov wrote:
> On Wed, Jan 22, 2025 at 5:12 AM Daniel Gomez <da.gomez@samsung.com> wrote:
> >
> > Add support for a module error injection tool. The tool
> > can inject errors in the annotated module kernel functions
> > such as complete_formation(), do_init_module() and
> > module_enable_rodata_after_init(). Module name and module function are
> > required parameters to have control over the error injection.
> >
> > Example: Inject error -22 to module_enable_rodata_ro_after_init for
> > brd module:
> >
> > sudo moderr --modname=brd --modfunc=module_enable_rodata_ro_after_init \
> > --error=-22 --trace
> > Monitoring module error injection... Hit Ctrl-C to end.
> > MODULE     ERROR FUNCTION
> > brd        -22   module_enable_rodata_after_init()
> >
> > Kernel messages:
> > [   89.463690] brd: module loaded
> > [   89.463855] brd: module_enable_rodata_ro_after_init() returned -22,
> > ro_after_init data might still be writable
> >
> > Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> > ---
> >  tools/bpf/Makefile            |  13 ++-
> >  tools/bpf/moderr/.gitignore   |   2 +
> >  tools/bpf/moderr/Makefile     |  95 +++++++++++++++++
> >  tools/bpf/moderr/moderr.bpf.c | 127 +++++++++++++++++++++++
> >  tools/bpf/moderr/moderr.c     | 236 ++++++++++++++++++++++++++++++++++++++++++
> >  tools/bpf/moderr/moderr.h     |  40 +++++++
> >  6 files changed, 510 insertions(+), 3 deletions(-)
> 
> The tool looks useful, but we don't add tools to the kernel repo.
> It has to stay out of tree.

For selftests we do add random tools.

> The value of error injection is not clear to me.

It is of great value, since it deals with corner cases which are
otherwise hard to reproduce in places which a real error can be
catostrophic.

> Other places in the kernel use it to test paths in the kernel
> that are difficult to do otherwise.

Right.

> These 3 functions don't seem to be in this category.

That's the key here we should focus on. The problem is when a maintainer
*does* agree that adding an error injection entry is useful for testing,
and we have a developer willing to do the work to help test / validate
it. In this case, this error case is rare but we do want to strive to
test this as we ramp up and extend our modules selftests.

Then there is the aspect of how to mitigate how instrusive code changes
to allow error injection are. In 2021 we evaluated the prospect of error
injection in-kernel long ago for other areas like the block layer for
add_disk() failures [0] but the minimal interface to enable this from
userspace with debugfs was considered just too intrusive.

This effort tried to evaluate what this could look like with eBPF to
mitigate the required in-kernel code, and I believe the light weight
nature of it by just requiring a sprinkle with ALLOW_ERROR_INJECTION()
suffices to my taste.

So, perhaps the tools aspect can just go in:

tools/testing/selftests/module/

[0] https://www.spinics.net/lists/linux-block/msg68159.html

  Luis