turning off Smatch for kunit

[Dan Carpenter]

Smatch is mostly used for error paths (bugs in the success paths tend to
get caught in testing). But kunit code tends to not have cleanup.
I think the only practical way to handle this is just to disable
checking on a function if it has a KUNIT_ASSERT_ macro.

For example:
drivers/base/regmap/regmap-kunit.c:108 basic_read_write()
warn: 'map' is not an error pointer

drivers/base/regmap/regmap-kunit.c
95 static void basic_read_write(struct kunit *test)
96 {
97 struct regcache_types *t = (struct regcache_types *)test->param_value;
98 struct regmap *map;
99 struct regmap_config config;
100 struct regmap_ram_data *data;
101 unsigned int val, rval;
102
103 config = test_regmap_config;
104 config.cache_type = t->type;
105
106 map = gen_regmap(&config, &data);
107 KUNIT_ASSERT_FALSE(test, IS_ERR(map));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This will abort the test if map is an error.

108 if (IS_ERR(map))
^^^^^^^^^^^
So this test will trigger a Smatch warning message.

109 return;
110

No one runs kunit on production systems so I think just disabling
Smatch for these functions is okay.

regards,
dan carpenter

[David Gow]

As you note, these IS_ERR() calls are totally redundant. So there's a
fair argument that this is a bug (albeit a harmless one) in the test,
which smatch has successfully caught.

That being said, I could definitely see there being situations where
this sort of check would result in a false positive with KUnit tests,
particularly if the function being tested checks its arguments.

>
> 109 return;
> 110
>
> No one runs kunit on production systems so I think just disabling
> Smatch for these functions is okay.
>

While "no one runs KUnit on production systems" may prove slightly
optimistic (there are definitely quite a few people who _build_ with
KUnit enabled on production systems, though they go to some effort to
not actually run it), I think this is probably an appropriate
tradeoff. Certainly, I think it makes sense to disable this IS_ERR
warning, and anything similar.

I suspect most of the problems we'd encounter are with the
KUNIT_ASSERT_* macros, given those are the only ones which trigger
serious control-flow changes. We do also have the kunit_add_action()
API now[1], which allows cleanup to be more easily scheduled to occur
when a test exits. I don't know if that's also likely to trip smatch
up...

Either way, I'm not too worried about disabling smatch in these cases
where false positives are likely: we're pretty careful to do things
like taint the kernel when tests are run, so that should keep people
from doing anything too silly in production (or at least warn them).

Cheers,
-- David

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=b9dce8a1ed3efe0f5c0957f4605140f204226a0f

[Dan Carpenter]

I sent this email and then I realized that actually Linaro does a lot of
kunit testing so it's valuable for me to have static analysis on this...

What would have helped is if the kunit_abort() were pulled out of
match_kunit_do_failed_assertion() so the function either returns or it
doesn't. Then it would have been parsed automatically. Smatch doesn't
inline the match_kunit_do_failed_assertion() function.

Smatch tries to use various heuristics to separate returns into
something concise but meaningful. For example, zero vs negative or NULL
vs non-NULL. But that fails for match_kunit_do_failed_assertion().

Anyway, I've created a special function to look at the caller and if we
pass KUNIT_ASSERTION then the function doesn't return.

820 static void match_kunit_do_failed_assertion(const char *fn, struct expression *call, void *unused)
821 {
822 struct expression *arg;
823 sval_t sval;
824
825 arg = get_argument_from_call_expr(call->args, 2);
826 if (get_implied_value(arg, &sval) &&
827 sval.value == 0)
828 nullify_path();
829 }

regards,
dan carpenter

[David Gow]

On Fri, 26 May 2023 at 14:52, Dan Carpenter <dan.ca...@linaro.org> wrote:
>
> I sent this email and then I realized that actually Linaro does a lot of
> kunit testing so it's valuable for me to have static analysis on this...
>
> What would have helped is if the kunit_abort() were pulled out of
> match_kunit_do_failed_assertion() so the function either returns or it
> doesn't. Then it would have been parsed automatically. Smatch doesn't
> inline the match_kunit_do_failed_assertion() function.

I don't think that should be a problem (and could easily help with
other things, particularly if the tests are being built as separate
modules).

I've sent out an RFC to change this here, let me know if it helps:
https://lore.kernel.org/linux-kselftest/20230526075355.5...@google.com/

>
> Smatch tries to use various heuristics to separate returns into
> something concise but meaningful. For example, zero vs negative or NULL
> vs non-NULL. But that fails for match_kunit_do_failed_assertion().
>
> Anyway, I've created a special function to look at the caller and if we
> pass KUNIT_ASSERTION then the function doesn't return.
>
> 820 static void match_kunit_do_failed_assertion(const char *fn, struct expression *call, void *unused)
> 821 {
> 822 struct expression *arg;
> 823 sval_t sval;
> 824
> 825 arg = get_argument_from_call_expr(call->args, 2);
> 826 if (get_implied_value(arg, &sval) &&
> 827 sval.value == 0)
> 828 nullify_path();
> 829 }
>

That seems correct to me, thanks.

Cheers,
-- David