On Fri, Jan 31, 2025 at 05:30:37PM +0100, Mickaël Salaün wrote:
> --- /dev/null
> +++ b/security/landlock/id.c
> +static atomic64_t next_id = ATOMIC64_INIT(COUNTER_PRE_INIT);
> +
> +static void __init init_id(atomic64_t *const counter, const u32
> random_32bits)
> +{
> + u64 init;
> +
> + /*
> + * Ensures sure 64-bit values are always used by user space (or may
> + * fail with -EOVERFLOW), and makes this testable.
> + */
> + init = 1ULL << 32;
> +
> + /*
> + * Makes a large (2^32) boot-time value to limit ID collision in logs
> + * from different boots, and to limit info leak about the number of
> + * initially (relative to the reader) created elements (e.g. domains).
> + */
> + init += random_32bits;
> +
> + /* Sets first or ignores. This will be the first ID. */
> + atomic64_cmpxchg(counter, COUNTER_PRE_INIT, init);
It feels like this should always need to succeed. Or to say it the
other way around: If this cmpxchg were to fail, the guarantees from
your commit message would be broken. Maybe it would be worth handling
that error case in a more direct way?
> +static void __init test_init_once(struct kunit *const test)
> +{
> + const u64 first_init = 1ULL + U32_MAX;
> + atomic64_t counter = ATOMIC64_INIT(COUNTER_PRE_INIT);
> +
> + init_id(&counter, 0);
> + KUNIT_EXPECT_EQ(test, atomic64_read(&counter), first_init);
> +
> + init_id(&counter, ~0);
> + KUNIT_EXPECT_EQ(test, atomic64_read(&counter), first_init);
Maybe we can annotate this with an explanatory message,
to make it slightly clearer that this is the point of the test:
KUNIT_EXPECT_EQ_MSG(test, atomic64_read(&counter), first_init,
"should still have the same value after the subsequent init_id()");
–Günther
