Re: [PATCH v2 1/2] landlock: Serialize TSYNC thread restriction

[Ding Yihan]

Hi Günther, great catch with the QEMU test! Returning -ERESTARTNOINTR via down_write_trylock() 
is indeed the perfect way to allow the blocked thread to process the TWA_SIGNAL and retry. 
I have sent v3 with this update.

在 2026/2/26 06:33, Günther Noack 写道:
> On Wed, Feb 25, 2026 at 01:07:26PM +0100, Günther Noack wrote:
>> On Wed, Feb 25, 2026 at 10:47:33AM +0800, Yihan Ding wrote:
>>> syzbot found a deadlock in landlock_restrict_sibling_threads().
>>> When multiple threads concurrently call landlock_restrict_self() with
>>> sibling thread restriction enabled, they can deadlock by mutually
>>> queueing task_works on each other and then blocking in kernel space
>>> (waiting for the other to finish).
>>>
>>> Fix this by serializing the TSYNC operations within the same process
>>> using the exec_update_lock. This prevents concurrent invocations
>>> from deadlocking. We use down_write_killable() to ensure the thread
>>> remains responsive to fatal signals while waiting for the lock.
>>>
>>> Fixes: 42fc7e6543f6 ("landlock: Multithreading support for landlock_restrict_self()")
>>> Reported-by: syzbot+7ea2f5e9dfd468201817@xxxxxxxxxxxxxxxxxxxxxxxxx
>>> Closes: https://syzkaller.appspot.com/bug?extid=7ea2f5e9dfd468201817
>>> Suggested-by: Günther Noack <gnoack3000@xxxxxxxxx>
>>> Signed-off-by: Yihan Ding <dingyihan@xxxxxxxxxxxxx>
>>> ---
>>>  security/landlock/tsync.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
>>> index de01aa899751..420fcfc2fe9a 100644
>>> --- a/security/landlock/tsync.c
>>> +++ b/security/landlock/tsync.c
>>> @@ -447,6 +447,13 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
>>>  	shared_ctx.new_cred = new_cred;
>>>  	shared_ctx.set_no_new_privs = task_no_new_privs(current);
>>>  
>>> +	/*
>>> +	 * Serialize concurrent TSYNC operations to prevent deadlocks
>>> +	 * when multiple threads call landlock_restrict_self() simultaneously.
>>> +	 */
>>> +	if (down_write_killable(&current->signal->exec_update_lock))
>>> +		return -EINTR;
>>> +
>>>  	/*
>>>  	 * We schedule a pseudo-signal task_work for each of the calling task's
>>>  	 * sibling threads.  In the task work, each thread:
>>> @@ -556,6 +563,7 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
>>>  		wait_for_completion(&shared_ctx.all_finished);
>>>  
>>>  	tsync_works_release(&works);
>>> +	up_write(&current->signal->exec_update_lock);
>>>  
>>>  	return atomic_read(&shared_ctx.preparation_error);
>>>  }
>>> -- 
>>> 2.51.0
>>>
>>
>> Thank you!
>>
>> Reviewed-by: Günther Noack <gnoack@xxxxxxxxxx>
> 
> Hello Yihan Ding!
> 
> Apologies, I have to take this back -- applying the patch in this form
> would be a mistake.  When I tried this out with the Syzkaller test
> case, I noticed that the tests started taking multiple seconds per
> run.  The way I reproduced it was by running the Syzkaller reproducer
> under Qemu and looking for the frequency of the "executing program"
> lines that it prints for each test run.
> 
> When I looked deeper, what was happening was actually that we got
> ourselves into a deadlock again, which, in hindsight should have been
> obvious: When two threads call landlock_restrict_self() roughly at the
> same time, then the one that grabs the lock first will (a) keep the
> other (killably) blocked on the lock acquisition, and (b) later ask
> the other thread to run a task work.  But in order to run a task work,
> the blocked thread must first return from the syscall.  However,
> down_write_killable() only returns when either the lock is available
> or when the thread was killed.
> 
> To resolve this, we need to actually use a lock acquisition that
> respects other ways of interruption as well; we can either use a
> down_write_trylock() and return -ERESTARTNOINTR, or we can use
> down_write_interruptible().
> 
> Sorry for the poor advice to use the _killable variant earlier.  Could
> I ask you to please send another revision using _trylock() or
> _interruptible()?
> 
> Thanks,
> –Günther
>