Function instance_set() expects to enable event 'sched_switch', so we
should set 1 to its 'enable' file.
Testcase passed after this patch:
# ./ftracetest test.d/instances/instance-event.tc
=== Ftrace unit tests ===
[1] Test creation and deletion of trace instances while setting an event
[PASS]
# of passed: 1
# of failed: 0
# of unresolved: 0
# of untested: 0
# of unsupported: 0
# of xfailed: 0
# of undefined(test bug): 0
Signed-off-by: Zheng Yejian <zhengyejian1(a)huawei.com>
---
.../testing/selftests/ftrace/test.d/instances/instance-event.tc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
index 0eb47fbb3f44..42422e425107 100644
--- a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
+++ b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
@@ -39,7 +39,7 @@ instance_read() {
instance_set() {
while :; do
- echo 1 > foo/events/sched/sched_switch
+ echo 1 > foo/events/sched/sched_switch/enable
done 2> /dev/null
}
--
2.25.1
Hi Rae,
Em Thu, 13 Jul 2023 17:31:19 -0400
Rae Moar <rmoar(a)google.com> escreveu:
> On Wed, Jul 12, 2023 at 10:29 AM Mauro Carvalho Chehab <mchehab(a)kernel.org>
> wrote:
>
> > As an example for the new documentation tool, add a documentation
> > for drm_buddy_test.
> >
> > I opted to place this on a completely different directory, in order
> > to make easier to test the feature with:
> >
> > $ make SPHINXDIRS="tests" htmldocs
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab(a)kernel.org>
> > ---
> >
> > To avoid mailbombing on a large number of people, only mailing lists were
> > C/C on the cover.
> > See [PATCH RFC 0/2] at:
> > https://lore.kernel.org/all/cover.1689171160.git.mchehab@kernel.org/
> >
> > Documentation/index.rst | 2 +-
> > Documentation/tests/index.rst | 6 ++++++
> > Documentation/tests/kunit.rst | 5 +++++
> > drivers/gpu/drm/tests/drm_buddy_test.c | 12 ++++++++++++
> > 4 files changed, 24 insertions(+), 1 deletion(-)
> > create mode 100644 Documentation/tests/index.rst
> > create mode 100644 Documentation/tests/kunit.rst
> >
> > diff --git a/Documentation/index.rst b/Documentation/index.rst
> > index 9dfdc826618c..80a6ce14a61a 100644
> > --- a/Documentation/index.rst
> > +++ b/Documentation/index.rst
> > @@ -60,7 +60,7 @@ Various other manuals with useful information for all
> > kernel developers.
> > fault-injection/index
> > livepatch/index
> > rust/index
> > -
> > + test/index
> >
> > User-oriented documentation
> > ===========================
> > diff --git a/Documentation/tests/index.rst b/Documentation/tests/index.rst
> > new file mode 100644
> > index 000000000000..bfc39eb5c0aa
> > --- /dev/null
> > +++ b/Documentation/tests/index.rst
> > @@ -0,0 +1,6 @@
> > +========================
> > +Kunit documentation test
> > +========================
> > +
> > +.. toctree::
> > + kunit
> > diff --git a/Documentation/tests/kunit.rst b/Documentation/tests/kunit.rst
> > new file mode 100644
> > index 000000000000..6ffc151988a0
> > --- /dev/null
> > +++ b/Documentation/tests/kunit.rst
> > @@ -0,0 +1,5 @@
> > +Kunit tests
> > +-----------
> > +
> > +.. include-test:: drivers/gpu/drm/tests/drm_buddy_test.c
> > +
> > diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c
> > b/drivers/gpu/drm/tests/drm_buddy_test.c
> > index 09ee6f6af896..dd6c5afd6cd6 100644
> > --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> > +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> > @@ -737,6 +737,18 @@ static int drm_buddy_suite_init(struct kunit_suite
> > *suite)
> > return 0;
> > }
> >
> > +/**
> > + * KTEST_SUITE: set of tests for drm buddy alloc
> > + * Scope: drm subsystem
> > + * Mega feature: drm
> > + * Feature: buddy_alloc
> > + *
> > + * KTEST_TEST: drm_test_buddy_alloc_%s
> > + * Description: Run DRM buddy allocation %arg[1] test
> > + *
> > + * arg[1].values: limit, range, optimistic, smoke, pathological
> > + */
>
>
> Hi!
>
> This is such a cool patch series. I just have a few comments related to the
> output.
Thank you for your comments! Sorry for not answering earlier. I took some
vacations and this series ended sleeping over other tasks on my
todo list.
Also, before sending another version, I wanted to have the test_list.py
changes to make it generic enough to be merged on IGT, to avoid having
a fork of it. Those got merged today.
> In the html output the tests are listed as:
> ktest@drm_buddy_test@…
>
> I wonder if instead of using the file name of “drm_buddy_test” this could
> possibly be the suite name, “drm_buddy”, as this is what users will call
> when using kunit.py to run the tests. Although "drm_buddy_test" is also the
> module name so I don't mind it too much. But in the future the file name
> and module name are not guaranteed to be the same for other tests.
>
> Most preferably, there would be a reference to the kunit suite name, file
> name, and the module name.
I guess it shouldn't be hard to do such change in a way that it won't
affect its usage on IGT. We need to define what would be the best
pattern. As this can be used for both kunit and selftests, I would
place kunit at the beginning.
Currently, we're using:
kunit@<base file name without .c>@<test_name>
Some possible patterns would be:
kunit@<base file name without .c>@<suite name>@<test_name>
kunit@<subsystem>@<base file name without .c>@<suite name>@<test_name>
kunit@<subsystem>@<suite name>@<test_name>
Would do you think it would work best?
> This may be difficult to implement as these can all differ. I am currently
> working on the KUnit Attribute framework which saves the module name and I
> am thinking about also saving the file path as a future attribute. This
> could be a helpful framework for the KUnit tests specifically.
>
> I am not sure how easy it would be to access c objects/functions using this
> system.
I would prefer not. C language allows lots of flexibility with macros,
making it hard to write a parser to read those C objects from the source.
We have this at kernel-doc. As one of the people that did some work there,
I can say that that several tricks are needed to keep this working.
On the other hand, it should be easy to use the TestList class from
test_list.py at kunit.py.
So, kunit.py could use the data that came from the documentation
directly.
> Finally, I was wondering if it is the intention to put a list of all kunit
> tests that use this new feature into tests/kunit.rst or would this be
> broken up in some way
IMO, it makes sense to break this per subsystem, and have an auto-generated
index.rst pointing to the entire set of documents.
We're already storing the subsystem at the documentation macros, so, IMO,
it should shouldn't be hard to implement it.
Regards,
Mauro
On Wed, Sep 06, 2023 at 08:39:22AM -0700, Beau Belgrave wrote:
> What is the expected behavior of self tests if run without their
> feature enabled? Is it expected these should skip vs fail in that case?
Yes, tests should skip if preconditions for running them aren't met.
On Tue, Sep 05, 2023 at 03:42:58PM +0200, Thomas Hellström wrote:
> Hi, Maxime
>
> On 9/5/23 15:16, Maxime Ripard wrote:
> > On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote:
> > > Hi,
> > >
> > > On 9/5/23 14:05, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
> > > > > Check that object freeing from within drm_exec_fini() works as expected
> > > > > and doesn't generate any warnings.
> > > > >
> > > > > Cc: Christian König <christian.koenig(a)amd.com>
> > > > > Cc: dri-devel(a)lists.freedesktop.org
> > > > > Signed-off-by: Thomas Hellström <thomas.hellstrom(a)linux.intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
> > > > > 1 file changed, 47 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> > > > > index 563949d777dd..294c25f49cc7 100644
> > > > > --- a/drivers/gpu/drm/tests/drm_exec_test.c
> > > > > +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> > > > > @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
> > > > > drm_gem_private_object_fini(&gobj2);
> > > > > }
> > > > > +static const struct drm_gem_object_funcs put_funcs = {
> > > > > + .free = (void *)kfree,
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Check that freeing objects from within drm_exec_fini()
> > > > > + * behaves as expected.
> > > > > + */
> > > > > +static void test_early_put(struct kunit *test)
> > > > > +{
> > > > > + struct drm_exec_priv *priv = test->priv;
> > > > > + struct drm_gem_object *gobj1;
> > > > > + struct drm_gem_object *gobj2;
> > > > > + struct drm_gem_object *array[2];
> > > > > + struct drm_exec exec;
> > > > > + int ret;
> > > > > +
> > > > > + gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> > > > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
> > > > > + if (!gobj1)
> > > > > + return;
> > > > > +
> > > > > + gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> > > > > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
> > > > > + if (!gobj2) {
> > > > > + kfree(gobj1);
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + gobj1->funcs = &put_funcs;
> > > > > + gobj2->funcs = &put_funcs;
> > > > > + array[0] = gobj1;
> > > > > + array[1] = gobj2;
> > > > > + drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> > > > > + drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> > > > > +
> > > > > + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> > > > > + drm_exec_until_all_locked(&exec)
> > > > > + ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
> > > > > + 1);
> > > > > + KUNIT_EXPECT_EQ(test, ret, 0);
> > > > > + drm_gem_object_put(gobj1);
> > > > > + drm_gem_object_put(gobj2);
> > > > > + drm_exec_fini(&exec);
> > > > It doesn't look like you actually check that "freeing objects from
> > > > within drm_exec_fini() behaves as expected." What is the expectation
> > > > here, and how is it checked?
> > > Hm. Good question, I've been manually checking dmesg for lockdep splats. Is
> > > there a way to automate that?
> > I'm not familiar with the drm_exec API, but judging by the code I'd
> > assume you want to check that gobj1 and gobj2 are actually freed using
> > kfree?
>
> Actually not. What's important here is that the call to drm_exec_fini(),
> which puts the last references to gobj1 and gobj2 doesn't trigger any
> lockdep splats, like the one in the commit message of patch 3/3. So to make
> more sense, the test could perhaps be conditioned on
> CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of dmesg()
> after being run.
I'm not aware of something to check on lockdep's status when running a
kunit test, but I'm not sure anyone is expected to look at the dmesg
trace when running kunit to find out whether the test succeeded or not.
It looks like there was an attempt at some point to fail the test if
there was a lockdep error:
https://lore.kernel.org/all/20200814205527.1833459-1-urielguajardojr@gmail.…
It doesn't look like it's been merged though. David, Brendan, do you
know why it wasn't merged or if there is a good option for us there?
At the very least, I think a comment after the call to drm_exec_fini to
make it clear that the error would be in the kernel logs, and a better
one on the test definition to explicitly say what you want to make sure
of, and how one can check it's been done would be great.
Maxime
From: Benjamin Berg <benjamin.berg(a)intel.com>
The existing KUNIT_ARRAY_PARAM macro requires a separate function to
get the description. However, in a lot of cases the description can
just be copied directly from the array. Add a second macro that
avoids having to write a static function just for a single strscpy.
Signed-off-by: Benjamin Berg <benjamin.berg(a)intel.com>
---
Documentation/dev-tools/kunit/usage.rst | 7 ++++---
include/kunit/test.h | 19 +++++++++++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index c27e1646ecd9..fe8c28d66dfe 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a
{
strcpy(desc, t->str);
}
- // Creates `sha1_gen_params()` to iterate over `cases`.
- KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
+ // Creates `sha1_gen_params()` to iterate over `cases` while using
+ // the struct member `str` for the case description.
+ KUNIT_ARRAY_PARAM_DESC(sha1, cases, str);
// Looks no different from a normal test.
static void sha1_test(struct kunit *test)
@@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a
}
// Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass in the
- // function declared by KUNIT_ARRAY_PARAM.
+ // function declared by KUNIT_ARRAY_PARAM or KUNIT_ARRAY_PARAM_DESC.
static struct kunit_case sha1_test_cases[] = {
KUNIT_CASE_PARAM(sha1_test, sha1_gen_params),
{}
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 68ff01aee244..f60d11e41855 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -1516,6 +1516,25 @@ do { \
return NULL; \
}
+/**
+ * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from an array.
+ * @name: prefix for the test parameter generator function.
+ * @array: array of test parameters.
+ * @desc_member: structure member from array element to use as description
+ *
+ * Define function @name_gen_params which uses @array to generate parameters.
+ */
+#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \
+ static const void *name##_gen_params(const void *prev, char *desc) \
+ { \
+ typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \
+ if (__next - (array) < ARRAY_SIZE((array))) { \
+ strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE); \
+ return __next; \
+ } \
+ return NULL; \
+ }
+
// TODO(dlatypov(a)google.com): consider eventually migrating users to explicitly
// include resource.h themselves if they need it.
#include <kunit/resource.h>
--
2.41.0
The most critical issue with vm.memfd_noexec=2 (the fact that passing
MFD_EXEC would bypass it entirely[1]) has been fixed in Andrew's
tree[2], but there are still some outstanding issues that need to be
addressed:
* vm.memfd_noexec=2 shouldn't reject old-style memfd_create(2) syscalls
because it will make it far to difficult to ever migrate. Instead it
should imply MFD_EXEC.
* The dmesg warnings are pr_warn_once(), which on most systems means
that they will be used up by systemd or some other boot process and
userspace developers will never see it.
- For the !(flags & (MFD_EXEC | MFD_NOEXEC_SEAL)) case, outputting a
rate-limited message to the kernel log is necessary to tell
userspace that they should add the new flags.
Arguably the most ideal way to deal with the spam concern[3,4]
while still prompting userspace to switch to the new flags would be
to only log the warning once per task or something similar.
However, adding something to task_struct for tracking this would be
needless bloat for a single pr_warn_ratelimited().
So just switch to pr_info_ratelimited() to avoid spamming the log
with something that isn't a real warning. There's lots of
info-level stuff in dmesg, it seems really unlikely that this
should be an actual problem. Most programs are already switching to
the new flags anyway.
- For the vm.memfd_noexec=2 case, we need to log a warning for every
failure because otherwise userspace will have no idea why their
previously working program started returning -EACCES (previously
-EINVAL) from memfd_create(2). pr_warn_once() is simply wrong here.
* The racheting mechanism for vm.memfd_noexec makes it incredibly
unappealing for most users to enable the sysctl because enabling it
on &init_pid_ns means you need a system reboot to unset it. Given the
actual security threat being protected against, CAP_SYS_ADMIN users
being restricted in this way makes little sense.
The argument for this ratcheting by the original author was that it
allows you to have a hierarchical setting that cannot be unset by
child pidnses, but this is not accurate -- changing the parent
pidns's vm.memfd_noexec setting to be more restrictive didn't affect
children.
Instead, switch the vm.memfd_noexec sysctl to be properly
hierarchical and allow CAP_SYS_ADMIN users (in the pidns's owning
userns) to lower the setting as long as it is not lower than the
parent's effective setting. This change also makes it so that
changing a parent pidns's vm.memfd_noexec will affect all
descendants, providing a properly hierarchical setting. The
performance impact of this is incredibly minimal since the maximum
depth of pidns is 32 and it is only checked during memfd_create(2)
and unshare(CLONE_NEWPID).
* The memfd selftests would not exit with a non-zero error code when
certain tests that ran in a forked process (specifically the ones
related to MFD_EXEC and MFD_NOEXEC_SEAL) failed.
[1]: https://lore.kernel.org/all/ZJwcsU0vI-nzgOB_@codewreck.org/
[2]: https://lore.kernel.org/all/20230705063315.3680666-1-jeffxu@google.com/
[3]: https://lore.kernel.org/Y5yS8wCnuYGLHMj4@x1n/
[4]: https://lore.kernel.org/f185bb42-b29c-977e-312e-3349eea15383@linuxfoundatio…
Signed-off-by: Aleksa Sarai <cyphar(a)cyphar.com>
---
Changes in v2:
- Make vm.memfd_noexec restrictions properly hierarchical.
- Allow vm.memfd_noexec setting to be lowered by CAP_SYS_ADMIN as long
as it is not lower than the parent's effective setting.
- Fix the logging behaviour related to the new flags and
vm.memfd_noexec=2.
- Add more thorough tests for vm.memfd_noexec in selftests.
- v1: <https://lore.kernel.org/r/20230713143406.14342-1-cyphar@cyphar.com>
---
Aleksa Sarai (5):
selftests: memfd: error out test process when child test fails
memfd: do not -EACCES old memfd_create() users with vm.memfd_noexec=2
memfd: improve userspace warnings for missing exec-related flags
memfd: replace ratcheting feature from vm.memfd_noexec with hierarchy
selftests: improve vm.memfd_noexec sysctl tests
include/linux/pid_namespace.h | 39 ++--
kernel/pid.c | 3 +
kernel/pid_namespace.c | 6 +-
kernel/pid_sysctl.h | 28 ++-
mm/memfd.c | 33 ++-
tools/testing/selftests/memfd/memfd_test.c | 332 +++++++++++++++++++++++------
6 files changed, 322 insertions(+), 119 deletions(-)
---
base-commit: 3ff995246e801ea4de0a30860a1d8da4aeb538e7
change-id: 20230803-memfd-vm-noexec-uapi-fixes-ace725c67b0f
Best regards,
--
Aleksa Sarai <cyphar(a)cyphar.com>