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

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Mar 07, 2025 at 03:15:44PM +0100, Günther Noack wrote:
> 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?

This should always succeed and with the current code it always succeed
because there is only one call to this function.  This
atomic64_cmpxchg() is a safeguard to be sure that, even if there are
several calls to this function, the counter will only be initialized
once (i.e. cmpxchg only sets the counter if its value was 0)

We could add a WARN_ON(atomic64_cmpxchg()) but I don't see the point.

> 
> 
> > +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()");

Yep, good idea.

> 
> –Günther
>