[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()?