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

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

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
>