Re: [PATCH v2 1/2] landlock: Fully release unused TSYNC work entries

["Günther Noack" <gnoack@google.com>]

On Tue, Feb 17, 2026 at 01:23:39PM +0100, Mickaël Salaün wrote:
> If task_work_add() failed, ctx->task is put but the tsync_works struct
> is not reset to its previous state.  The first consequence is that the
> kernel allocates memory for dying threads, which could lead to
> user-accounted memory exhaustion (not very useful nor specific to this
> case).  The second consequence is that task_work_cancel(), called by
> cancel_tsync_works(), can dereference a NULL task pointer.
> 
> Fix this issues by keeping a consistent works->size wrt the added task
> work.  This is done in a new tsync_works_trim() helper which also cleans
> up the shared_ctx and work fields.
> 
> As a safeguard, add a pointer check to cancel_tsync_works() and update
> tsync_works_release() accordingly.
> 
> Cc: Günther Noack <gnoack@google.com>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
> 
> Changes since v1:
> https://lore.kernel.org/all/20260216142641.2100407-1-mic@digikod.net/
> - Move the return/release logic into a new tsync_works_trim() helper
>   (suggested by Günther).
> - Reset the whole ctx with memset().
> - Add an unlinkely(err).
> ---
>  security/landlock/tsync.c | 47 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
> index 0d2b9c646030..42cc0ef0c704 100644
> --- a/security/landlock/tsync.c
> +++ b/security/landlock/tsync.c
> @@ -203,6 +203,40 @@ static struct tsync_work *tsync_works_provide(struct tsync_works *s,
>  	return ctx;
>  }
>  
> +/**
> + * tsync_works_trim - Put the last tsync_work element
> + *
> + * @s: TSYNC works to trim.
> + *
> + * Put the last task and decrement the size of @s.
> + *
> + * This helper does not cancel a running task, but just reset the last element
> + * to zero.
> + */
> +static void tsync_works_trim(struct tsync_works *s)
> +{
> +	struct tsync_work *ctx;
> +
> +	if (WARN_ON_ONCE(s->size <= 0))
> +		return;
> +
> +	ctx = s->works[s->size - 1];
> +
> +	/*
> +	 * For consistency, remove the task from ctx so that it does not look like
> +	 * we handed it a task_work.
> +	 */
> +	put_task_struct(ctx->task);
> +	memset(ctx, 0, sizeof(*ctx));

Minor (and highly optional) remark, this is the same as

  *ctx = (struct tsync_work){};

which I find slightly easier to read when resetting a struct value.
Both is fine though.

> +
> +	/*
> +	 * Cancel the tsync_works_provide() change to recycle the reserved memory
> +	 * for the next thread, if any.  This also ensures that cancel_tsync_works()
> +	 * and tsync_works_release() do not see any NULL task pointers.
> +	 */
> +	s->size--;
> +}
> +
>  /*
>   * tsync_works_grow_by - preallocates space for n more contexts in s
>   *
> @@ -276,7 +310,7 @@ static void tsync_works_release(struct tsync_works *s)
>  	size_t i;
>  
>  	for (i = 0; i < s->size; i++) {
> -		if (!s->works[i]->task)
> +		if (WARN_ON_ONCE(!s->works[i]->task))
>  			continue;
>  
>  		put_task_struct(s->works[i]->task);
> @@ -379,16 +413,14 @@ static bool schedule_task_work(struct tsync_works *works,
>  
>  		init_task_work(&ctx->work, restrict_one_thread_callback);
>  		err = task_work_add(thread, &ctx->work, TWA_SIGNAL);
> -		if (err) {
> +		if (unlikely(err)) {
>  			/*
>  			 * task_work_add() only fails if the task is about to exit.  We
>  			 * checked that earlier, but it can happen as a race.  Resume
>  			 * without setting an error, as the task is probably gone in the
> -			 * next loop iteration.  For consistency, remove the task from ctx
> -			 * so that it does not look like we handed it a task_work.
> +			 * next loop iteration.
>  			 */
> -			put_task_struct(ctx->task);
> -			ctx->task = NULL;
> +			tsync_works_trim(works);
>  
>  			atomic_dec(&shared_ctx->num_preparing);
>  			atomic_dec(&shared_ctx->num_unfinished);
> @@ -412,6 +444,9 @@ static void cancel_tsync_works(struct tsync_works *works,
>  	int i;
>  
>  	for (i = 0; i < works->size; i++) {
> +		if (WARN_ON_ONCE(!works->works[i]->task))
> +			continue;
> +
>  		if (!task_work_cancel(works->works[i]->task,
>  				      &works->works[i]->work))
>  			continue;
> -- 
> 2.53.0
> 

Reviewed-by: Günther Noack <gnoack@google.com>

Thanks for spotting and fixing this!
—Günther