Re: [PATCH v2 1/2] landlock: Multithreading support for landlock_restrict_self()

[Jann Horn <jannh@google.com>]

On Fri, Oct 17, 2025 at 5:04 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Wed, Oct 01, 2025 at 01:18:06PM +0200, Günther Noack wrote:
> > +     /* If needed, establish enforcement prerequisites. */
> > +     if (!ns_capable_noaudit(current_user_ns(), CAP_SYS_ADMIN))
> > +             task_set_no_new_privs(current);
>
> We should always set PR_SET_NO_NEW_PRIVS if it is set on the calling
> thread as done by seccomp.  We should just store the result of
> task_no_new_privs() in tsync_shared_context and use it as condition here.
> This should be explained in the documentation.
>
> This also mean that if the calling thread has CAP_SYS_ADMIN but not
> PR_SET_NO_NEW_PRIVS, then a sibling thread could not have CAP_SYS_ADMIN
> nor PR_SET_NO_NEW_PRIVS.  This would be a risky state but mainly because
> of the CAP_SYS_ADMIN inconsistency, not really the missing
> PR_SET_NO_NEW_PRIVS.

Agreed, it would be nice to have behavior that is consistent with seccomp.

[...]
> > +/*
> > + * tsync_works_provide - provides a preallocated tsync_work for the given task
> > + *
> > + * This also stores a task pointer in the context and increments the reference
> > + * count of the task.
> > + *
> > + * Returns:
> > + *   A pointer to the preallocated context struct, with task filled in.
> > + *
> > + *   NULL, if we ran out of preallocated context structs.
> > + */
> > +static struct tsync_work *tsync_works_provide(struct tsync_works *s,
> > +                                           struct task_struct *task)
> > +{
> > +     struct tsync_work *ctx;
> > +
> > +     if (s->size >= s->capacity)
>
> In which case can this happen?  Should we wrap this in a WARN_ON_ONCE()?

No, this can legitimately happen if new sibling threads are created
between the time we pre-allocate memory and the time we loop over them
to call tsync_works_provide().

[...]
> > +             return 0;
> > +
> > +     works = krealloc_array(s->works, new_capacity, sizeof(s->works[0]),
> > +                            flags);
> > +     if (IS_ERR(works))
> > +             return PTR_ERR(works);
> > +
> > +     s->works = works;
> > +
> > +     for (i = s->capacity; i < new_capacity; i++) {
> > +             s->works[i] = kzalloc(sizeof(*s->works[i]), flags);
>
> We should use a local variable to avoid storing an error code in
> s->works[i] and potentially dereferencing it later (e.g. in
> tsync_work_free).
>
> Why can't we avoid this loop entirely and allocate a flat array with
> only one call to krealloc_array()?  Why struct tsync_works->works needs
> to be a pointer to a pointer?

Because pointers to some "struct tsync_work" items might already be in
use as task work or such, so we can't move them to a different address
anymore at this point.