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

[PATCH v1 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.  For completeness, clean up ctx->shared_ctx dangling pointer as
well.

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>
---
 security/landlock/tsync.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
index 0d2b9c646030..8e9b8ed7d53c 100644
--- a/security/landlock/tsync.c
+++ b/security/landlock/tsync.c
@@ -276,7 +276,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);
@@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
 			 */
 			put_task_struct(ctx->task);
 			ctx->task = NULL;
+			ctx->shared_ctx = NULL;
+
+			/*
+			 * 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.
+			 */
+			works->size--;
 
 			atomic_dec(&shared_ctx->num_preparing);
 			atomic_dec(&shared_ctx->num_unfinished);
@@ -412,6 +421,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 v1 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: Günther Noack <gnoack@google.com>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 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 8e9b8ed7d53c..9a65e3e96186 100644
--- a/security/landlock/tsync.c
+++ b/security/landlock/tsync.c
@@ -256,13 +256,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;
 }
 
@@ -284,6 +285,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;
@@ -295,7 +297,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;
@@ -334,7 +336,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;
 
@@ -415,10 +418,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 v1 2/2] landlock: Improve TSYNC types

[Günther Noack]

On Mon, Feb 16, 2026 at 03:26:39PM +0100, Mickaël Salaün wrote:
> 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: Günther Noack <gnoack@google.com>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> ---
>  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 8e9b8ed7d53c..9a65e3e96186 100644
> --- a/security/landlock/tsync.c
> +++ b/security/landlock/tsync.c
> @@ -256,13 +256,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;
>  }
>  
> @@ -284,6 +285,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;
> @@ -295,7 +297,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;
> @@ -334,7 +336,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;
>  
> @@ -415,10 +418,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
> 

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

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

[Günther Noack]

Hello!

On Mon, Feb 16, 2026 at 03:26:38PM +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.

I think it is very difficult to get into this situation, but this is
obviously not an excuse - if we already do the error handling, we
should do it right. 👍

> 
> Fix this issues by keeping a consistent works->size wrt the added task
> work.  For completeness, clean up ctx->shared_ctx dangling pointer as
> well.
> 
> 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>
> ---
>  security/landlock/tsync.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
> index 0d2b9c646030..8e9b8ed7d53c 100644
> --- a/security/landlock/tsync.c
> +++ b/security/landlock/tsync.c
> @@ -276,7 +276,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))

Is this a condition we should warn on?  It is very unlikely, but it
can technically happen that a thread exits at the same time as TSYNC
and happens to hit that narrow race condition window.  As long as it
happens only sporadically, I don't think there is anything wrong (and
in particular, it's not actionable for the user - I don't think there
is a way to fix it if that warning appears?)


>  			continue;
>  
>  		put_task_struct(s->works[i]->task);
> @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
>  			 */
>  			put_task_struct(ctx->task);
>  			ctx->task = NULL;
> +			ctx->shared_ctx = NULL;
> +
> +			/*
> +			 * 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.
> +			 */
> +			works->size--;

Looks good.

[Optional code arrangement remarks:

I would recommend to put that logic in a helper function
"tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
be in line with the existing implementation where the manipulation of
struct tsync_works is encapsulated in the "tsync_*" helper functions.

The scope of that function would be to do the inverse of
"tsync_works_provide()" -- putting the task_struct, decreasing
works->size, and then, to be safe, also clearing the contents of the
tsync_work struct (although that is strictly speaking not required if
we decrease the size, I think).

The only unusual thing about the tsync_works_return() function would
be that it is only OK to return the very last tsync_work struct which
was returned from tsync_works_provide().

]

It's an improvement either way though; If you want to prioritize
fixing this and don't want to extract the extra function now, we can
also look into it in a follow-up.  From a functional standpoint, I
think your code works as well.

>  
>  			atomic_dec(&shared_ctx->num_preparing);
>  			atomic_dec(&shared_ctx->num_unfinished);
> @@ -412,6 +421,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;
> +

Well spotted!

>  		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 having another closer look at this!

—Günther

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

[Mickaël Salaün]

On Mon, Feb 16, 2026 at 04:25:53PM +0100, Günther Noack wrote:
> Hello!
> 
> On Mon, Feb 16, 2026 at 03:26:38PM +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.
> 
> I think it is very difficult to get into this situation, but this is
> obviously not an excuse - if we already do the error handling, we
> should do it right. 👍
> 
> > 
> > Fix this issues by keeping a consistent works->size wrt the added task
> > work.  For completeness, clean up ctx->shared_ctx dangling pointer as
> > well.
> > 
> > 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>
> > ---
> >  security/landlock/tsync.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
> > index 0d2b9c646030..8e9b8ed7d53c 100644
> > --- a/security/landlock/tsync.c
> > +++ b/security/landlock/tsync.c
> > @@ -276,7 +276,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))
> 
> Is this a condition we should warn on?  It is very unlikely, but it
> can technically happen that a thread exits at the same time as TSYNC
> and happens to hit that narrow race condition window.  As long as it
> happens only sporadically, I don't think there is anything wrong (and
> in particular, it's not actionable for the user - I don't think there
> is a way to fix it if that warning appears?)

WARN() should definitely not be called if the condition can legitimately
be true.

"task" is only set by tsync_works_provide(), so only by the caller
thread.  How could "task" be NULL (within the works->size range)?

> 
> 
> >  			continue;
> >  
> >  		put_task_struct(s->works[i]->task);
> > @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
> >  			 */
> >  			put_task_struct(ctx->task);
> >  			ctx->task = NULL;
> > +			ctx->shared_ctx = NULL;
> > +
> > +			/*
> > +			 * 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.
> > +			 */
> > +			works->size--;
> 
> Looks good.
> 
> [Optional code arrangement remarks:
> 
> I would recommend to put that logic in a helper function
> "tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
> be in line with the existing implementation where the manipulation of
> struct tsync_works is encapsulated in the "tsync_*" helper functions.
> 
> The scope of that function would be to do the inverse of
> "tsync_works_provide()" -- putting the task_struct, decreasing
> works->size, and then, to be safe, also clearing the contents of the
> tsync_work struct (although that is strictly speaking not required if
> we decrease the size, I think).

Should we move the atomic_inc() to tsync_works_provide() and the
atomic_dec() to this new helper?

> 
> The only unusual thing about the tsync_works_return() function would
> be that it is only OK to return the very last tsync_work struct which
> was returned from tsync_works_provide().

What about renaming tsync_works_provide() to tsync_works_push() and this
new one to tsync_works_pop()?

> 
> ]
> 
> It's an improvement either way though; If you want to prioritize
> fixing this and don't want to extract the extra function now, we can
> also look into it in a follow-up.  From a functional standpoint, I
> think your code works as well.

It's a small refactoring, so better to do it now.

> 
> >  
> >  			atomic_dec(&shared_ctx->num_preparing);
> >  			atomic_dec(&shared_ctx->num_unfinished);
> > @@ -412,6 +421,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;
> > +
> 
> Well spotted!
> 
> >  		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 having another closer look at this!
> 
> —Günther
> 

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

[Günther Noack]

On Mon, Feb 16, 2026 at 06:43:25PM +0100, Mickaël Salaün wrote:
> On Mon, Feb 16, 2026 at 04:25:53PM +0100, Günther Noack wrote:
> > On Mon, Feb 16, 2026 at 03:26:38PM +0100, Mickaël Salaün wrote:
> > >  	for (i = 0; i < s->size; i++) {
> > > -		if (!s->works[i]->task)
> > > +		if (WARN_ON_ONCE(!s->works[i]->task))
> > 
> > Is this a condition we should warn on?  It is very unlikely, but it
> > can technically happen that a thread exits at the same time as TSYNC
> > and happens to hit that narrow race condition window.  As long as it
> > happens only sporadically, I don't think there is anything wrong (and
> > in particular, it's not actionable for the user - I don't think there
> > is a way to fix it if that warning appears?)
> 
> WARN() should definitely not be called if the condition can legitimately
> be true.
> 
> "task" is only set by tsync_works_provide(), so only by the caller
> thread.  How could "task" be NULL (within the works->size range)?

Ah, you are right.  This could have become NULL before, but now it
can't become NULL any more.  Please ignore my remark.


> > >  			continue;
> > >  
> > >  		put_task_struct(s->works[i]->task);
> > > @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
> > >  			 */
> > >  			put_task_struct(ctx->task);
> > >  			ctx->task = NULL;
> > > +			ctx->shared_ctx = NULL;
> > > +
> > > +			/*
> > > +			 * 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.
> > > +			 */
> > > +			works->size--;
> > 
> > Looks good.
> > 
> > [Optional code arrangement remarks:
> > 
> > I would recommend to put that logic in a helper function
> > "tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
> > be in line with the existing implementation where the manipulation of
> > struct tsync_works is encapsulated in the "tsync_*" helper functions.
> > 
> > The scope of that function would be to do the inverse of
> > "tsync_works_provide()" -- putting the task_struct, decreasing
> > works->size, and then, to be safe, also clearing the contents of the
> > tsync_work struct (although that is strictly speaking not required if
> > we decrease the size, I think).
> 
> Should we move the atomic_inc() to tsync_works_provide() and the
> atomic_dec() to this new helper?

No, I would keep the atomic_inc() and atomic_dec() calls in the
functions where they are now.  

The atomic counters belong logically to the synchronization scheme
between the different threads, and I think it's clearer if we keep
that synchronization code outside of the struct task_works
abstraction.

I see the struct tsync_works and its operations (functions starting
with "tsync_works_") as logically belonging together in an
OO/encapsulation sense, and I think it's useful to have a clear
boundary of responsibilities.  These functions are only in the
business of managing the direct values stored in the "struct
tsync_works", and in the business of allocating the memory for that
data structure and incrementing refcounts to the struct task_struct.
(The latter is mostly useful to have in tsync_works_provide() because
the inverse put_task_struct() is useful to have in
tsync_works_release(), and then it is symmetric.)


> > The only unusual thing about the tsync_works_return() function would
> > be that it is only OK to return the very last tsync_work struct which
> > was returned from tsync_works_provide().
> 
> What about renaming tsync_works_provide() to tsync_works_push() and this
> new one to tsync_works_pop()?

I think I would find that naming slightly confusing: When a function
is called "push", I would normally expect to pass a value to it, but
we're getting one from it.  And when a method is called "pop" I would
expect to get a value from it.  But the inverse is true here.  With
the names "provide" and "return" it feel that the directionality of
argument passing would be clearer.


> > It's an improvement either way though; If you want to prioritize
> > fixing this and don't want to extract the extra function now, we can
> > also look into it in a follow-up.  From a functional standpoint, I
> > think your code works as well.
> 
> It's a small refactoring, so better to do it now.

Sounds good. 👍

–Günther

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

[Mickaël Salaün]

On Mon, Feb 16, 2026 at 08:33:05PM +0100, Günther Noack wrote:
> On Mon, Feb 16, 2026 at 06:43:25PM +0100, Mickaël Salaün wrote:
> > On Mon, Feb 16, 2026 at 04:25:53PM +0100, Günther Noack wrote:
> > > On Mon, Feb 16, 2026 at 03:26:38PM +0100, Mickaël Salaün wrote:
> > > >  	for (i = 0; i < s->size; i++) {
> > > > -		if (!s->works[i]->task)
> > > > +		if (WARN_ON_ONCE(!s->works[i]->task))
> > > 
> > > Is this a condition we should warn on?  It is very unlikely, but it
> > > can technically happen that a thread exits at the same time as TSYNC
> > > and happens to hit that narrow race condition window.  As long as it
> > > happens only sporadically, I don't think there is anything wrong (and
> > > in particular, it's not actionable for the user - I don't think there
> > > is a way to fix it if that warning appears?)
> > 
> > WARN() should definitely not be called if the condition can legitimately
> > be true.
> > 
> > "task" is only set by tsync_works_provide(), so only by the caller
> > thread.  How could "task" be NULL (within the works->size range)?
> 
> Ah, you are right.  This could have become NULL before, but now it
> can't become NULL any more.  Please ignore my remark.
> 
> 
> > > >  			continue;
> > > >  
> > > >  		put_task_struct(s->works[i]->task);
> > > > @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
> > > >  			 */
> > > >  			put_task_struct(ctx->task);
> > > >  			ctx->task = NULL;
> > > > +			ctx->shared_ctx = NULL;
> > > > +
> > > > +			/*
> > > > +			 * 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.
> > > > +			 */
> > > > +			works->size--;
> > > 
> > > Looks good.
> > > 
> > > [Optional code arrangement remarks:
> > > 
> > > I would recommend to put that logic in a helper function
> > > "tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
> > > be in line with the existing implementation where the manipulation of
> > > struct tsync_works is encapsulated in the "tsync_*" helper functions.
> > > 
> > > The scope of that function would be to do the inverse of
> > > "tsync_works_provide()" -- putting the task_struct, decreasing
> > > works->size, and then, to be safe, also clearing the contents of the
> > > tsync_work struct (although that is strictly speaking not required if
> > > we decrease the size, I think).
> > 
> > Should we move the atomic_inc() to tsync_works_provide() and the
> > atomic_dec() to this new helper?
> 
> No, I would keep the atomic_inc() and atomic_dec() calls in the
> functions where they are now.  
> 
> The atomic counters belong logically to the synchronization scheme
> between the different threads, and I think it's clearer if we keep
> that synchronization code outside of the struct task_works
> abstraction.
> 
> I see the struct tsync_works and its operations (functions starting
> with "tsync_works_") as logically belonging together in an
> OO/encapsulation sense, and I think it's useful to have a clear
> boundary of responsibilities.  These functions are only in the
> business of managing the direct values stored in the "struct
> tsync_works", and in the business of allocating the memory for that
> data structure and incrementing refcounts to the struct task_struct.
> (The latter is mostly useful to have in tsync_works_provide() because
> the inverse put_task_struct() is useful to have in
> tsync_works_release(), and then it is symmetric.)

This makes sense.

> 
> 
> > > The only unusual thing about the tsync_works_return() function would
> > > be that it is only OK to return the very last tsync_work struct which
> > > was returned from tsync_works_provide().
> > 
> > What about renaming tsync_works_provide() to tsync_works_push() and this
> > new one to tsync_works_pop()?
> 
> I think I would find that naming slightly confusing: When a function
> is called "push", I would normally expect to pass a value to it, but
> we're getting one from it.

Well, it pushes the thread and returns the wrapped object.

> And when a method is called "pop" I would
> expect to get a value from it.  But the inverse is true here.

Fair

> With
> the names "provide" and "return" it feel that the directionality of
> argument passing would be clearer.

I don't understand the logic with "return": this tsync_works_return()
would not return anything.

What about something like tsync_works_shrink()?

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

[Mickaël Salaün]

On Mon, Feb 16, 2026 at 08:57:34PM +0100, Mickaël Salaün wrote:
> On Mon, Feb 16, 2026 at 08:33:05PM +0100, Günther Noack wrote:
> > On Mon, Feb 16, 2026 at 06:43:25PM +0100, Mickaël Salaün wrote:
> > > On Mon, Feb 16, 2026 at 04:25:53PM +0100, Günther Noack wrote:
> > > > On Mon, Feb 16, 2026 at 03:26:38PM +0100, Mickaël Salaün wrote:

> > > > > @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
> > > > >  			 */
> > > > >  			put_task_struct(ctx->task);
> > > > >  			ctx->task = NULL;
> > > > > +			ctx->shared_ctx = NULL;
> > > > > +
> > > > > +			/*
> > > > > +			 * 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.
> > > > > +			 */
> > > > > +			works->size--;
> > > > 
> > > > Looks good.
> > > > 
> > > > [Optional code arrangement remarks:
> > > > 
> > > > I would recommend to put that logic in a helper function
> > > > "tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
> > > > be in line with the existing implementation where the manipulation of
> > > > struct tsync_works is encapsulated in the "tsync_*" helper functions.
> > > > 
> > > > The scope of that function would be to do the inverse of
> > > > "tsync_works_provide()" -- putting the task_struct, decreasing
> > > > works->size, and then, to be safe, also clearing the contents of the
> > > > tsync_work struct (although that is strictly speaking not required if
> > > > we decrease the size, I think).
> > > 
> > > Should we move the atomic_inc() to tsync_works_provide() and the
> > > atomic_dec() to this new helper?
> > 
> > No, I would keep the atomic_inc() and atomic_dec() calls in the
> > functions where they are now.  
> > 
> > The atomic counters belong logically to the synchronization scheme
> > between the different threads, and I think it's clearer if we keep
> > that synchronization code outside of the struct task_works
> > abstraction.
> > 
> > I see the struct tsync_works and its operations (functions starting
> > with "tsync_works_") as logically belonging together in an
> > OO/encapsulation sense, and I think it's useful to have a clear
> > boundary of responsibilities.  These functions are only in the
> > business of managing the direct values stored in the "struct
> > tsync_works", and in the business of allocating the memory for that
> > data structure and incrementing refcounts to the struct task_struct.
> > (The latter is mostly useful to have in tsync_works_provide() because
> > the inverse put_task_struct() is useful to have in
> > tsync_works_release(), and then it is symmetric.)
> 
> This makes sense.
> 
> > 
> > 
> > > > The only unusual thing about the tsync_works_return() function would
> > > > be that it is only OK to return the very last tsync_work struct which
> > > > was returned from tsync_works_provide().
> > > 
> > > What about renaming tsync_works_provide() to tsync_works_push() and this
> > > new one to tsync_works_pop()?
> > 
> > I think I would find that naming slightly confusing: When a function
> > is called "push", I would normally expect to pass a value to it, but
> > we're getting one from it.
> 
> Well, it pushes the thread and returns the wrapped object.
> 
> > And when a method is called "pop" I would
> > expect to get a value from it.  But the inverse is true here.
> 
> Fair
> 
> > With
> > the names "provide" and "return" it feel that the directionality of
> > argument passing would be clearer.
> 
> I don't understand the logic with "return": this tsync_works_return()
> would not return anything.
> 
> What about something like tsync_works_shrink()?

tsync_works_trim() may be better.

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

[Günther Noack]

On Mon, Feb 16, 2026 at 09:10:59PM +0100, Mickaël Salaün wrote:
> On Mon, Feb 16, 2026 at 08:57:34PM +0100, Mickaël Salaün wrote:
> > On Mon, Feb 16, 2026 at 08:33:05PM +0100, Günther Noack wrote:
> > > On Mon, Feb 16, 2026 at 06:43:25PM +0100, Mickaël Salaün wrote:
> > > > On Mon, Feb 16, 2026 at 04:25:53PM +0100, Günther Noack wrote:
> > > > > On Mon, Feb 16, 2026 at 03:26:38PM +0100, Mickaël Salaün wrote:
> 
> > > > > > @@ -389,6 +389,15 @@ static bool schedule_task_work(struct tsync_works *works,
> > > > > >  			 */
> > > > > >  			put_task_struct(ctx->task);
> > > > > >  			ctx->task = NULL;
> > > > > > +			ctx->shared_ctx = NULL;
> > > > > > +
> > > > > > +			/*
> > > > > > +			 * 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.
> > > > > > +			 */
> > > > > > +			works->size--;
> > > > > 
> > > > > Looks good.
> > > > > 
> > > > > [Optional code arrangement remarks:
> > > > > 
> > > > > I would recommend to put that logic in a helper function
> > > > > "tsync_works_return(struct tsync_works *s, struct tsync_work *)", to
> > > > > be in line with the existing implementation where the manipulation of
> > > > > struct tsync_works is encapsulated in the "tsync_*" helper functions.
> > > > > 
> > > > > The scope of that function would be to do the inverse of
> > > > > "tsync_works_provide()" -- putting the task_struct, decreasing
> > > > > works->size, and then, to be safe, also clearing the contents of the
> > > > > tsync_work struct (although that is strictly speaking not required if
> > > > > we decrease the size, I think).
> > > > 
> > > > Should we move the atomic_inc() to tsync_works_provide() and the
> > > > atomic_dec() to this new helper?
> > > 
> > > No, I would keep the atomic_inc() and atomic_dec() calls in the
> > > functions where they are now.  
> > > 
> > > The atomic counters belong logically to the synchronization scheme
> > > between the different threads, and I think it's clearer if we keep
> > > that synchronization code outside of the struct task_works
> > > abstraction.
> > > 
> > > I see the struct tsync_works and its operations (functions starting
> > > with "tsync_works_") as logically belonging together in an
> > > OO/encapsulation sense, and I think it's useful to have a clear
> > > boundary of responsibilities.  These functions are only in the
> > > business of managing the direct values stored in the "struct
> > > tsync_works", and in the business of allocating the memory for that
> > > data structure and incrementing refcounts to the struct task_struct.
> > > (The latter is mostly useful to have in tsync_works_provide() because
> > > the inverse put_task_struct() is useful to have in
> > > tsync_works_release(), and then it is symmetric.)
> > 
> > This makes sense.
> > 
> > > 
> > > 
> > > > > The only unusual thing about the tsync_works_return() function would
> > > > > be that it is only OK to return the very last tsync_work struct which
> > > > > was returned from tsync_works_provide().
> > > > 
> > > > What about renaming tsync_works_provide() to tsync_works_push() and this
> > > > new one to tsync_works_pop()?
> > > 
> > > I think I would find that naming slightly confusing: When a function
> > > is called "push", I would normally expect to pass a value to it, but
> > > we're getting one from it.
> > 
> > Well, it pushes the thread and returns the wrapped object.
> > 
> > > And when a method is called "pop" I would
> > > expect to get a value from it.  But the inverse is true here.
> > 
> > Fair
> > 
> > > With
> > > the names "provide" and "return" it feel that the directionality of
> > > argument passing would be clearer.
> > 
> > I don't understand the logic with "return": this tsync_works_return()
> > would not return anything.
> > 
> > What about something like tsync_works_shrink()?
> 
> tsync_works_trim() may be better.

The idea with "return" is that we are returning the previously
provided tsync_work item back into the struct tsync_works.  But I can
see that it can be confused with C's "return" statement.
tsync_works_shrink() or tsync_works_trim() is also OK.

Other options, btw, include "reclaim()" or "recycle()", if you like
that better (these LLMs are useful as thesaurus... 8-)).

I'm fine with either name, as long as the function still puts the
task_struct of the returned task_work item.  (That would be good to
keep doing, for symmetry with the _provide() and _release()
functions.)

–Günther