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

["Günther Noack" <gnoack3000@gmail.com>]

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