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

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

[Mickaël Salaün]

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));
+
+	/*
+	 * 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

[PATCH v2 2/2] landlock: Improve TSYNC types

[Mickaël Salaün]

Constify pointers when it makes sense.

Consistently use size_t for loops, especially to match works->size type.

Add new lines to improve readability.

Cc: Jann Horn <jannh@google.com>
Reviewed-by: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

Changes since v1:
- Added Reviewed-by Günther.
---
 security/landlock/tsync.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
index 42cc0ef0c704..c588cdd111d3 100644
--- a/security/landlock/tsync.c
+++ b/security/landlock/tsync.c
@@ -290,13 +290,14 @@ static int tsync_works_grow_by(struct tsync_works *s, size_t n, gfp_t flags)
  * tsync_works_contains - checks for presence of task in s
  */
 static bool tsync_works_contains_task(const struct tsync_works *s,
-				      struct task_struct *task)
+				      const struct task_struct *task)
 {
 	size_t i;
 
 	for (i = 0; i < s->size; i++)
 		if (s->works[i]->task == task)
 			return true;
+
 	return false;
 }
 
@@ -318,6 +319,7 @@ static void tsync_works_release(struct tsync_works *s)
 
 	for (i = 0; i < s->capacity; i++)
 		kfree(s->works[i]);
+
 	kfree(s->works);
 	s->works = NULL;
 	s->size = 0;
@@ -329,7 +331,7 @@ static void tsync_works_release(struct tsync_works *s)
  */
 static size_t count_additional_threads(const struct tsync_works *works)
 {
-	struct task_struct *thread, *caller;
+	const struct task_struct *caller, *thread;
 	size_t n = 0;
 
 	caller = current;
@@ -368,7 +370,8 @@ static bool schedule_task_work(struct tsync_works *works,
 			       struct tsync_shared_context *shared_ctx)
 {
 	int err;
-	struct task_struct *thread, *caller;
+	const struct task_struct *caller;
+	struct task_struct *thread;
 	struct tsync_work *ctx;
 	bool found_more_threads = false;
 
@@ -438,10 +441,10 @@ static bool schedule_task_work(struct tsync_works *works,
  * shared_ctx->num_preparing and shared_ctx->num_unfished and mark the two
  * completions if needed, as if the task was never scheduled.
  */
-static void cancel_tsync_works(struct tsync_works *works,
+static void cancel_tsync_works(const struct tsync_works *works,
 			       struct tsync_shared_context *shared_ctx)
 {
-	int i;
+	size_t i;
 
 	for (i = 0; i < works->size; i++) {
 		if (WARN_ON_ONCE(!works->works[i]->task))
-- 
2.53.0

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

[Günther Noack]

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

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

[Mickaël Salaün]

On Tue, Feb 17, 2026 at 01:41:11PM +0100, Günther Noack wrote:
> 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){};

What about:

*ctx = (typeof(*ctx)){};

> 
> 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
> 

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

[Günther Noack]

On Tue, Feb 17, 2026 at 02:52:46PM +0100, Mickaël Salaün wrote:
> On Tue, Feb 17, 2026 at 01:41:11PM +0100, Günther Noack wrote:
> > On Tue, Feb 17, 2026 at 01:23:39PM +0100, Mickaël Salaün wrote:
> > > +	memset(ctx, 0, sizeof(*ctx));
> > 
> > Minor (and highly optional) remark, this is the same as
> > 
> >   *ctx = (struct tsync_work){};
> 
> What about:
> 
> *ctx = (typeof(*ctx)){};

I find that harder to read, because it is less commonly seen and the typeof() is
an indirection that makes me think as a reader.  But at this point, this is only
a vague opinion and I don't feel strongly about it.  Please submit either one of
these three options :)

—Günther