Re: [PATCH v5 02/24] landlock: Add unique ID generator

["Günther Noack" <gnoack3000@gmail.com>]

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