From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com [209.85.208.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 240F73CCA0A for ; Wed, 4 Mar 2026 19:05:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772651143; cv=none; b=tylMtswge6cOFiylcbgWcP+nT484RI0c3vE1AlA8mCZoubvX3BhmLIs2Y7ghIjCUwfxnmUQVkf0W5CgBq1YsS/8duHKefX6H+484DTyDRLQFCKM1iGGbSL8l+F4BoHJJkSWUxazXJP09x8Hc7YCCxLiSFxZ5hLPlg6qs2Pd15ds= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772651143; c=relaxed/simple; bh=EnYhy+TuuyxL6bTrDS8XAlmj5oIe9epdawpeCPxdN9E=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=FD14wv79/JLgI+shwoIo/+4oN5F6KEEjxcqQ5uZyVuhmKEcywWb9cLadHhSS7/z0OLDAQMXxA/AoipjypfIt8+at7Gclah1euy97kkBAUSycbHqq3Bin5chCi0nItCWddO3BC0YiZA2n6ZjBLbSDx6njJeWk3/CJ6JUFpNgCLTM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=f5/Lvngr; arc=none smtp.client-ip=209.85.208.169 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="f5/Lvngr" Received: by mail-lj1-f169.google.com with SMTP id 38308e7fff4ca-38a33070b32so6547871fa.0 for ; Wed, 04 Mar 2026 11:05:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1772651139; x=1773255939; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=3jFUpHh9piiC4hG9dP2zuQYvuYEb2t/t97Zqgys+0B0=; b=f5/LvngrRyRbKHVFBoBAt+WBfSKNe0z5zYIVUcEubTFc8jLW4K1P0SXyNBVEBzjtBR iUv/m9KEHxe9Xc92FzYBvL0Ts4Rq3+dUp+j4wbYu3aWHH7b/Lis4VkKVBrrI9Cqnh8nB sIilM48NEcjh0Mq9XdFQMwubauRCfSs8pwLbrVlN6Az+RrB2AC3Q7ToPNdkJhpjrk+z8 GXdRejqwvugIj0bFYd4NY+bmlGia0QGzIruouGEDnTj16W3kYQK7hnx7ObNsLAKMvr4t UxADrLv1k0ud2L4xoyd7Wyd4nEw/62TEeJEJLO03rOyvUjLQadbD9rDMcQ1qQQfz4sDQ 9D2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772651139; x=1773255939; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3jFUpHh9piiC4hG9dP2zuQYvuYEb2t/t97Zqgys+0B0=; b=QdZa7winoAavp6uBaOZNrZ9loLq88ke27/c/sohjpzIBgFqPkhEOC3vlsO5jg0BJWw gwiaKWv/cs/VweBjQrTv/FLoeGgJiVjx7jAwS5CQ4m4Mib6KCx4MpxVJymnpiRVj5F+C utoIKYG+B0lAUQNzgq/NH9NDsPL83AT5NbDY9cvLd2XquC0a7NnZAajtGtdpIiCqImnG ktM0ZXT+/YWAPCvFUc87kCPJpV2JBMb3Zn9vIhaWQhJQIaFj85T4IUeee2pNP/AqvPYi VaUVF89mruho0AA2SKVOXVu6i9CYECnrQ9LsYJf4BU9Eq1a7rdWKQlOPgUoh2N4AZcgq ZlOA== X-Forwarded-Encrypted: i=1; AJvYcCXMFxCpNYJsecBBrh+2/G2xI+N+WQfeb8yPttUpV1up5diC39cxjPhfNvWNliZa8hVm5DXXf55A3caRg6Q=@vger.kernel.org X-Gm-Message-State: AOJu0YwPero7Bil9XYnesxunxj88rRGQBQYNlOhQ4Ci4Rank+4fYC6wW qEz2YEkRqQvvzqxDlANmjSoqtIEH5rKvkXzzndXHje1vs5A0OuRYw9i1 X-Gm-Gg: ATEYQzwjwaDSaKcknumYiy9f5Jz8Ie7+kuNy1GZWrLRoyVnCwccKgpHHmKxGsQ8vBJu QpJ9zrscOwNcxNv1RAKWoXYqjrrt8zhdfYPIqQpTqPVTmZj13n1aomRS6wd6p65hwJZ7xd/epIZ xB4zuvbqKHNe8b2Htv3oGKXq32neSWXUMlkSFm1JJxLyRDqWZUf+fUVeER9saD0adZYjdZuoSYM 4LQrm9HLhF5kmOQWxcj+GKAPsjumPJQIqLK0fQpkADXzjU19XSvHfELmaeq3POM6DANoMqG9NA+ 6eQIxKXmVYH0CUjHNgLtT9JRWh8DBD4dZZP1bMuSsOffHokABUNYcnI+h+ypQS+WujuzHy1vZik /nW3ixLS8PNZ07xEdXdwwAV+XRjJvVhQJdx4ItF6E5TKH2SCe4C/i0UZCNCv24AmOpaDxATxwlr cXjgcdggGCm/eDe0NbOGysrAEVSFIcQUXRucO1mMIbcFHMuQyw X-Received: by 2002:a2e:91c2:0:b0:38a:30c0:1cd with SMTP id 38308e7fff4ca-38a30c004e4mr11607791fa.7.1772651139047; Wed, 04 Mar 2026 11:05:39 -0800 (PST) Received: from localhost (ip87-106-108-193.pbiaas.com. [87.106.108.193]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-5a123585025sm1429702e87.11.2026.03.04.11.05.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Mar 2026 11:05:38 -0800 (PST) Date: Wed, 4 Mar 2026 20:05:33 +0100 From: =?iso-8859-1?Q?G=FCnther?= Noack To: Yihan Ding Cc: jannh@google.com, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, m@maowtm.org, mic@digikod.net, paul@paul-moore.com, syzbot+7ea2f5e9dfd468201817@syzkaller.appspotmail.com, utilityemal77@gmail.com Subject: Re: [PATCH v4 1/2] landlock: Serialize TSYNC thread restriction Message-ID: <20260304.8cc838dcc68c@gnoack.org> References: <20260304095418.465594-1-dingyihan@uniontech.com> <20260304095418.465594-2-dingyihan@uniontech.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260304095418.465594-2-dingyihan@uniontech.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 > Signed-off-by: Yihan Ding 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(¤t->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(¤t->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