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

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

Hello!

Thank you for the revised patch.  I tested this and it passes the
selftests now.

I noticed that the comment is unfortunately wrongly indented, which is
also caught by clang-format (see below); we should fix that before
submit; I remarked on another two less important nits in the commit
message as well.

On Wed, Mar 04, 2026 at 05:54:17PM +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_trylock() and return -ERESTARTNOINTR if the lock
                                  ^^^^^^^^^^^^^^^^^^^^^^

Nit: Maybe it would be better to write "restart the syscall" now,
as we are not just returning -ERESTARTNOINTR here any more.

> cannot be acquired immediately. This ensures that if a thread fails
> to get the lock, it will return to userspace, allowing it to process
> any pending TSYNC task_works from the lock holder, and then
> transparently restart the syscall.
> 
> Fixes: 42fc7e6543f6 ("landlock: Multithreading support for landlock_restrict_self()")
> Reported-by: syzbot+7ea2f5e9dfd468201817@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7ea2f5e9dfd468201817
> Suggested-by: Günther Noack <gnoack3000@gmail.com>
> Signed-off-by: Yihan Ding <dingyihan@uniontech.com>

FYI This would be a good place to add a Reported-by: Justin and
Suggested-by: Tingmao as well.


> ---
> Changes in v4:
> - Use restart_syscall() instead of returning -ERESTARTNOINTR.
>   This ensures the syscall is properly restarted without leaking the
>   internal error code to userspace, fixing a test failure in
>   tsync_test.competing_enablement. (Caught by Justin Suess, suggested
>   by Tingmao Wang).
> 
> Changes in v3:
> - Replaced down_write_killable() with down_write_trylock() and
>   returned -ERESTARTNOINTR to avoid a secondary deadlock caused by
>   blocking the execution of task_works. (Caught by Günther Noack).
> 
> ---
>  security/landlock/tsync.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/security/landlock/tsync.c b/security/landlock/tsync.c
> index de01aa899751..1f460b9ec833 100644
> --- a/security/landlock/tsync.c
> +++ b/security/landlock/tsync.c
> @@ -446,6 +446,15 @@ int landlock_restrict_sibling_threads(const struct cred *old_cred,
>  	shared_ctx.old_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 the lock is already held, we gracefully yield by restarting the
> +		 * syscall. This allows the current thread to process pending
> +		 * task_works before retrying.
> +		 */
^^^^^^^^^^^^^^^^
This comment should be indented with one tab, not with two, the same
as the code that follows it.

The comment text is good.

Remark on the side, the Landlock code is formatted with clang-format;
I have a small tool that runs it for me on these files:

  clang-format -i \
      security/landlock/*.[ch] \
      include/uapi/linux/landlock.h \
      tools/testing/selftests/landlock/*.[ch]

clang-format does not fix the indentation perfectly here either, but
it discovers the issue and reindents at least the first line.

> +	if (!down_write_trylock(&current->signal->exec_update_lock))
> +		return restart_syscall();
>  
>  	/*
>  	 * We schedule a pseudo-signal task_work for each of the calling task's
> @@ -556,6 +565,6 @@ 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.20.1
> 

These remarks are all in comments and documentation though; the code
looks good to me and the selftests work!  Thank you for looking into it!

–Günther