On 13/02/18 01:50, Kees Cook wrote:

On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa [off-list ref] wrote:

[...]

quoted

 lib/genalloc-selftest.c           | 400 ++++++++++++++++++++++++++++++++++++++

Nit: make this test_genalloc.c instead.

ok

[...]

quoted

+       genalloc_selftest();

I wonder if it's possible to make this module-loadable instead? That
way it could be built and tested separately.

In my case modules are not an option.
Of course it could be still built in, but what is the real gain?

[...]

quoted

+config GENERIC_ALLOCATOR_SELFTEST

Like the other lib/test_*.c targets, I'd call this TEST_GENERIC_ALLOCATOR.

ok

[...]

quoted

+       BUG_ON(compare_bitmaps(pool, action->pattern));

There's been a lot recently on BUG vs WARN. It does seem crazy to not
BUG for an allocator selftest, but if we can avoid it, we should.

If this fails, I would expect that memory corruption is almost guaranteed.
Do we really want to allow the boot to continue, possibly mounting a
filesystem, only to corrupt it? It seems very dangerous.

Also, I wonder if it might make sense to split this series up a little
more, as in:

1/n: add genalloc selftest
2/n: update bitmaps
3/n: add/change bitmap tests to selftest

Maybe I'm over-thinking it, but the great thing about this self test
is that it's checking much more than just the bitmap changes you're
making, and that can be used to "prove" that genalloc continues to
work after the changes (i.e. the selftest passes before the changes,
and after, rather than just after).

If I really have to ... but to me the evidence that the changes to the
bitmaps do really work is already provided by the bitmap patch itself.

Since the patch doesn't remove the parameter indicating the space to be
freed, it can actually compare what the kernel passes to it vs. what it
thinks the space should be.

If the values were different, it would complain, but it doesn't ...
Isn't that even stronger evidence that the bitmap changes work as expected?


(eventually the parameter can be removed, but I intentionally left it,
for facilitating the merge)

--
igor
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html