Linux Security Modules development
 help / color / mirror / Atom feed

* [PATCH v4 3/3] ima: add new critical data record to measure log trim
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
	serge, paul, jmorris, linux-security-module, anirudhve, chenste,
	gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>

Add a new critical data record to measure the trimming event when
ima event records are deleted since system boot up.

If all IMA event logs are saved in the userspace, use this log to get total
numbers of records deleted since system boot up at that point.

Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
 security/integrity/ima/ima_fs.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 7f805ab62f6c..1d6befa51044 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -43,6 +43,7 @@ static int valid_policy = 1;
 
 #define IMA_LOG_TRIM_REQ_NUM_LENGTH 15
 #define IMA_LOG_TRIM_REQ_TOTAL_LENGTH 32
+#define IMA_LOG_TRIM_EVENT_LEN 256
 
 static long trimcount;
 /* mutex protects atomicity of trimming measurement list
@@ -364,6 +365,22 @@ static const struct file_operations ima_ascii_measurements_ops = {
 	.release = ima_measurements_release,
 };
 
+static void ima_measure_trim_event(void)
+{
+	char ima_log_trim_event[IMA_LOG_TRIM_EVENT_LEN];
+	struct timespec64 ts;
+	u64 time_ns;
+	int n;
+
+	ktime_get_real_ts64(&ts);
+	time_ns = (u64)ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+	n = scnprintf(ima_log_trim_event, IMA_LOG_TRIM_EVENT_LEN,
+		      "time= %llu; number= %lu;", time_ns, trimcount);
+
+	ima_measure_critical_data("ima_log_trim", "trim ima event logs",
+				  ima_log_trim_event, n, false, NULL, 0);
+}
+
 static int ima_log_trim_open(struct inode *inode, struct file *file)
 {
 	bool write = !!(file->f_mode & FMODE_WRITE);
@@ -438,6 +455,8 @@ static ssize_t ima_log_trim_write(struct file *file,
 		goto out;
 
 	trimcount += ret;
+	if (ret > 0)
+		ima_measure_trim_event();
 
 	ret = datalen;
 out:
-- 
2.43.0


^ permalink raw reply related

* [PATCH v4 2/3] ima: trim N IMA event log records
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
	serge, paul, jmorris, linux-security-module, anirudhve, chenste,
	gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>

Trim N entries of the IMA event logs. Clean the hash table if
ima_flush_htable is set.

Provide a userspace interface ima_trim_log:
When read this interface, it returns total number T of entries trimmed
since system boot up.
When write to this interface need to provide two numbers T:N to let
kernel to trim N entries of IMA event logs.

when kernel get log trim request T:N
 - Get the T, compare with the total trimmed number
 - if equal, then do trim N and change T to T+N
 - else return error

Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
 .../admin-guide/kernel-parameters.txt         |   4 +
 security/integrity/ima/ima.h                  |   2 +
 security/integrity/ima/ima_fs.c               | 195 +++++++++++++++++-
 security/integrity/ima/ima_queue.c            | 100 +++++++++
 4 files changed, 297 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index e92c0056e4e0..cd1a1d0bf0e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2197,6 +2197,10 @@
 			Use the canonical format for the binary runtime
 			measurements, instead of host native format.
 
+	ima_flush_htable  [IMA]
+			Flush the measurement list hash table when trim all
+			or a part of it for deletion.
+
 	ima_hash=	[IMA]
 			Format: { md5 | sha1 | rmd160 | sha256 | sha384
 				   | sha512 | ... }
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e3d71d8d56e3..2102c523dca0 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -246,8 +246,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 
 #ifdef CONFIG_IMA_KEXEC
 void ima_measure_kexec_event(const char *event_name);
+long ima_delete_event_log(long req_val);
 #else
 static inline void ima_measure_kexec_event(const char *event_name) {}
+static inline long ima_delete_event_log(long req_val) { return 0; }
 #endif
 
 /*
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 87045b09f120..7f805ab62f6c 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -21,6 +21,9 @@
 #include <linux/rcupdate.h>
 #include <linux/parser.h>
 #include <linux/vmalloc.h>
+#include <linux/ktime.h>
+#include <linux/timekeeping.h>
+#include <linux/ima.h>
 
 #include "ima.h"
 
@@ -38,6 +41,17 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
 
 static int valid_policy = 1;
 
+#define IMA_LOG_TRIM_REQ_NUM_LENGTH 15
+#define IMA_LOG_TRIM_REQ_TOTAL_LENGTH 32
+
+static long trimcount;
+/* mutex protects atomicity of trimming measurement list
+ * and also protects atomicity the measurement list read
+ * write operation.
+ */
+static DEFINE_MUTEX(ima_measure_lock);
+static long ima_measure_users;
+
 static ssize_t ima_show_htable_value(char __user *buf, size_t count,
 				     loff_t *ppos, atomic_long_t *val)
 {
@@ -202,16 +216,77 @@ static const struct seq_operations ima_measurments_seqops = {
 	.show = ima_measurements_show
 };
 
+/*
+ * _ima_measurements_open - open the IMA measurements file
+ * @inode: inode of the file being opened
+ * @file: file being opened
+ * @seq_ops: sequence operations for the file
+ *
+ * Returns 0 on success, or negative error code.
+ * Implements mutual exclusion between readers and writer
+ * of the measurements file. Multiple readers are allowed,
+ * but writer get exclusive access only no other readers/writers.
+ * Readers is not allowed when there is a writer.
+ */
+static int _ima_measurements_open(struct inode *inode, struct file *file,
+				  const struct seq_operations *seq_ops)
+{
+	bool write = !!(file->f_mode & FMODE_WRITE);
+	int ret;
+
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&ima_measure_lock);
+	if ((write && ima_measure_users != 0) ||
+	    (!write && ima_measure_users < 0)) {
+		mutex_unlock(&ima_measure_lock);
+		return -EBUSY;
+	}
+
+	ret = seq_open(file, seq_ops);
+	if (ret < 0) {
+		mutex_unlock(&ima_measure_lock);
+		return ret;
+	}
+
+	if (write)
+		ima_measure_users--;
+	else
+		ima_measure_users++;
+
+	mutex_unlock(&ima_measure_lock);
+	return ret;
+}
+
 static int ima_measurements_open(struct inode *inode, struct file *file)
 {
-	return seq_open(file, &ima_measurments_seqops);
+	return _ima_measurements_open(inode, file, &ima_measurments_seqops);
+}
+
+static int ima_measurements_release(struct inode *inode, struct file *file)
+{
+	bool write = !!(file->f_mode & FMODE_WRITE);
+	int ret;
+
+	mutex_lock(&ima_measure_lock);
+	ret = seq_release(inode, file);
+	if (!ret) {
+		if (!write)
+			ima_measure_users--;
+		else
+			ima_measure_users++;
+	}
+
+	mutex_unlock(&ima_measure_lock);
+	return ret;
 }
 
 static const struct file_operations ima_measurements_ops = {
 	.open = ima_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
-	.release = seq_release,
+	.release = ima_measurements_release,
 };
 
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size)
@@ -279,14 +354,114 @@ static const struct seq_operations ima_ascii_measurements_seqops = {
 
 static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
 {
-	return seq_open(file, &ima_ascii_measurements_seqops);
+	return _ima_measurements_open(inode, file, &ima_ascii_measurements_seqops);
 }
 
 static const struct file_operations ima_ascii_measurements_ops = {
 	.open = ima_ascii_measurements_open,
 	.read = seq_read,
 	.llseek = seq_lseek,
-	.release = seq_release,
+	.release = ima_measurements_release,
+};
+
+static int ima_log_trim_open(struct inode *inode, struct file *file)
+{
+	bool write = !!(file->f_mode & FMODE_WRITE);
+
+	if (!write && capable(CAP_SYS_ADMIN))
+		return 0;
+	else if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	return _ima_measurements_open(inode, file, &ima_measurments_seqops);
+}
+
+static ssize_t ima_log_trim_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
+{
+	char tmpbuf[IMA_LOG_TRIM_REQ_NUM_LENGTH];
+	ssize_t len;
+
+	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", trimcount);
+	return simple_read_from_buffer(buf, size, ppos, tmpbuf, len);
+}
+
+static ssize_t ima_log_trim_write(struct file *file,
+				  const char __user *buf, size_t datalen, loff_t *ppos)
+{
+	char tmpbuf[IMA_LOG_TRIM_REQ_TOTAL_LENGTH];
+	char *p = tmpbuf;
+	long count, ret, val = 0, max = LONG_MAX;
+
+	if (*ppos > 0 || datalen > IMA_LOG_TRIM_REQ_TOTAL_LENGTH || datalen < 2) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (copy_from_user(tmpbuf, buf, datalen) != 0) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	p = tmpbuf;
+
+	while (*p && *p != ':') {
+		if (!isdigit((unsigned char)*p))
+			return -EINVAL;
+
+		/* digit value */
+		int d = *p - '0';
+
+		/* overflow check: val * 10 + d > max -> (val > (max - d) / 10) */
+		if (val > (max - d) / 10)
+			return -ERANGE;
+
+		val = val * 10 + d;
+		p++;
+	}
+
+	if (*p != ':')
+		return -EINVAL;
+
+	/* verify trim count matches */
+	if (val != trimcount)
+		return -EINVAL;
+
+	p++; /* skip ':' */
+	ret = kstrtoul(p, 0, &count);
+
+	if (ret < 0)
+		goto out;
+
+	ret = ima_delete_event_log(count);
+
+	if (ret < 0)
+		goto out;
+
+	trimcount += ret;
+
+	ret = datalen;
+out:
+	return ret;
+}
+
+static int ima_log_trim_release(struct inode *inode, struct file *file)
+{
+	bool write = !!(file->f_mode & FMODE_WRITE);
+
+	if (!write && capable(CAP_SYS_ADMIN))
+		return 0;
+	else if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	return ima_measurements_release(inode, file);
+}
+
+static const struct file_operations ima_log_trim_ops = {
+	.open = ima_log_trim_open,
+	.read = ima_log_trim_read,
+	.write = ima_log_trim_write,
+	.llseek = generic_file_llseek,
+	.release = ima_log_trim_release
 };
 
 static ssize_t ima_read_policy(char *path)
@@ -528,6 +703,18 @@ int __init ima_fs_init(void)
 		goto out;
 	}
 
+	if (IS_ENABLED(CONFIG_IMA_LOG_TRIMMING)) {
+		dentry = securityfs_create_file("ima_trim_log",
+						S_IRUSR | S_IRGRP | S_IWUSR | S_IWGRP,
+						ima_dir, NULL, &ima_log_trim_ops);
+		if (IS_ERR(dentry)) {
+			ret = PTR_ERR(dentry);
+			goto out;
+		}
+	}
+
+	trimcount = 0;
+
 	dentry = securityfs_create_file("runtime_measurements_count",
 				   S_IRUSR | S_IRGRP, ima_dir, NULL,
 				   &ima_measurements_count_ops);
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 590637e81ad1..5ef722d0fa24 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -22,6 +22,14 @@
 
 #define AUDIT_CAUSE_LEN_MAX 32
 
+bool ima_flush_htable;
+static int __init ima_flush_htable_setup(char *str)
+{
+	ima_flush_htable = true;
+	return 1;
+}
+__setup("ima_flush_htable", ima_flush_htable_setup);
+
 /* pre-allocated array of tpm_digest structures to extend a PCR */
 static struct tpm_digest *digests;
 
@@ -220,6 +228,98 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 	return result;
 }
 
+/**
+ * ima_delete_event_log - delete IMA event entry
+ * @num_records: number of records to delete
+ *
+ * delete num_records entries off the measurement list.
+ * Returns num_records, or negative error code.
+ */
+long ima_delete_event_log(long num_records)
+{
+	long len, cur = num_records, tmp_len = 0;
+	struct ima_queue_entry *qe, *qe_tmp;
+	LIST_HEAD(ima_measurements_to_delete);
+	struct list_head *list_ptr;
+
+	if (!IS_ENABLED(CONFIG_IMA_LOG_TRIMMING))
+		return -EOPNOTSUPP;
+
+	if (num_records <= 0)
+		return num_records;
+
+	mutex_lock(&ima_extend_list_mutex);
+	len = atomic_long_read(&ima_htable.len);
+
+	if (num_records > len) {
+		mutex_unlock(&ima_extend_list_mutex);
+		return -ENOENT;
+	}
+
+	list_ptr = &ima_measurements;
+
+	if (num_records == len) {
+		list_replace(&ima_measurements, &ima_measurements_to_delete);
+		INIT_LIST_HEAD(&ima_measurements);
+		atomic_long_set(&ima_htable.len, 0);
+		list_ptr = &ima_measurements_to_delete;
+		if (IS_ENABLED(CONFIG_IMA_KEXEC))
+			binary_runtime_size = 0;
+	}
+
+	list_for_each_entry(qe, list_ptr, later) {
+		if (cur > 0) {
+			if (!IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE) && ima_flush_htable)
+				hlist_del_rcu(&qe->hnext);
+
+			--cur;
+			if (cur == 0)
+				qe_tmp = qe;
+			continue;
+		}
+		if (len != num_records && IS_ENABLED(CONFIG_IMA_KEXEC))
+			tmp_len += get_binary_runtime_size(qe->entry);
+		else
+			break;
+	}
+
+	if (len != num_records) {
+		__list_cut_position(&ima_measurements_to_delete, &ima_measurements,
+				    &qe_tmp->later);
+		atomic_long_sub(num_records, &ima_htable.len);
+		if (IS_ENABLED(CONFIG_IMA_KEXEC))
+			binary_runtime_size = tmp_len;
+	}
+
+	mutex_unlock(&ima_extend_list_mutex);
+
+	if (ima_flush_htable)
+		synchronize_rcu();
+
+	list_for_each_entry_safe(qe, qe_tmp, &ima_measurements_to_delete, later) {
+		/*
+		 * Ok because after list delete qe is only accessed by
+		 * ima_lookup_digest_entry().
+		 */
+		for (int i = 0; i < qe->entry->template_desc->num_fields; i++) {
+			kfree(qe->entry->template_data[i].data);
+			qe->entry->template_data[i].data = NULL;
+			qe->entry->template_data[i].len = 0;
+		}
+
+		list_del(&qe->later);
+
+		/* No leak if !ima_flush_htable, referenced by ima_htable. */
+		if (ima_flush_htable) {
+			kfree(qe->entry->digests);
+			kfree(qe->entry);
+			kfree(qe);
+		}
+	}
+
+	return num_records;
+}
+
 int ima_restore_measurement_entry(struct ima_template_entry *entry)
 {
 	int result = 0;
-- 
2.43.0


^ permalink raw reply related

* [PATCH v4 1/3] ima: make ima event log trimming configurable
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
	serge, paul, jmorris, linux-security-module, anirudhve, chenste,
	gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>

Make ima event log trimming function configurable.

Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
 security/integrity/ima/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 976e75f9b9ba..322964ae4772 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -332,4 +332,16 @@ config IMA_KEXEC_EXTRA_MEMORY_KB
 	  If set to the default value of 0, an extra half page of memory for those
 	  additional measurements will be allocated.
 
+config IMA_LOG_TRIMMING
+	bool "IMA Event Log Trimming"
+	default n
+	help
+	  Say Y here if you want support for IMA Event Log Trimming.
+		This creates the file /sys/kernel/security/integrity/ima/ima_trim_log.
+		Userspace
+		  - writes to this file to trigger IMA event log trimming
+		  - reads this file to get number of entried trimming last time
+
+	  If unsure, say N.
+
 endif
-- 
2.43.0


^ permalink raw reply related

* [PATCH v4 0/1] Trim N entries of IMA event logs
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
  To: linux-integrity
  Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
	serge, paul, jmorris, linux-security-module, anirudhve, chenste,
	gregorylumen, nramas, sushring, linux-doc

The Integrity Measurement Architecture (IMA) maintains a measurement list
—a record of system events used for integrity verification. The IMA event
logs are the entries within this measurement list, each representing a
specific event or measurement that contributes to the system's integrity
assessment.

This update introduces the ability to trim, or remove, N entries from the
current measurement list. Trimming involves deleting N entries from the
list and clearing the corresponding entries from the hash table. This
action atomically truncates the measurement list, ensuring that no new
measurements can be added until the operation is complete. Importantly,
only one writer can initiate this trimming process at a time, maintaining
consistency and preventing race conditions.

A userspace interface, ima_trim_log, has been provided for this purpose.
When this interface is read, it returns the total number T of entries
trimmed since system boot up. This value T need to be preserved across
kexec soft reboots. By writing two number T:N to this interface, userspace
can request the kernel to trim N entries from the IMA event logs.

To maintain a complete record, userspace is responsible for concatenating
and storing the logs before initiating trimming. Userspace can then send
the collected data to remote verifiers for validation. After receiving
confirmation from the remote verifiers, userspace may instruct the kernel
to proceed with trimming the IMA event logs accordingly.

The primary benefit of this solution is the ability to free valuable
kernel memory by delegating the task of reconstructing the full
measurement list from log chunks to userspace. Trust is not required in
userspace for the integrity of the measurement list, as its integrity is
cryptographically protected by the Trusted Platform Module (TPM).

Multiple readers are allowed to access the ima_trim_log interface
concurrently, while only one writer can trigger log trimming at any time.
During trimming, readers do not see the list and cannot access it while
deletion is in progress, ensuring atomicity.

Introduce the new kernel option ima_flush_htable to decide whether or not
the digests of measurement entries are flushed from the hash table (from
reference [2]).

The ima_measure_users counter (protected by the ima_measure_lock mutex) has
been introduced to protect access to the measurement list part. The open
method of all the measurement interfaces has been extended to allow only
one writer at a time or, in alternative, multiple readers. The write
permission is used to stage/delete the measurements, the read permission
to read them. Write requires also the CAP_SYS_ADMIN capability (from
reference [2]). This ima_measure_users needs to be preserved across kexec
soft reboots

The total trimmed number T and the ima_measure_users both need to be
preserved across kexec soft reboot and new patch will be added for this
purpose in next version.

New IMA log trim event is added when trimming finish.

The time required for trimming is minimal, and IMA event logs are briefly
on hold during this process, preventing read or add operations. This short
interruption has no impact on the overall functionality of IMA.

A new critical data record "ima_log_trim" is added in this solution. This
record logs the trim event with number of entries deleted total T since
system start and time when this happened. User space can get the total
number T of entries trimmed by checking "ima_log_trim" event in the
measurement list.

The following are how user space to use the measurement list and
ima_log_trim interface
1. get the total numer trimmed T through "ima_log_trim" interface
2. get the PCR quote
3. read the measurement list file, close the file, send for verification
4. wait for response from verifier, until get the good response from
verifier with number N that matched the PCR quote got in step 2
5. get the number N from the above message
6. write the T:N to the ima_log_trim interface when no conflict
 
when kernel get log trim request T:N
 Get the T, compare with the total trimmed number
 if equal, then do trim N and change T to T+N
 else return error

Using above way to trim the log, the time for user space to hold the list
will be trimming T:N operation itself at the step 6. User space agent
race condition is solved too in this way.

References:
-----------
[1] [PATCH 0/1] Trim N entries of IMA event logs
https://lore.kernel.org/linux-integrity/20251202232857.8211-1-chenste@linux.microsoft.com/T/#t

[2] [RFC][PATCH] ima: Add support for staging measurements for deletion
https://lore.kernel.org/linux-integrity/207fd6d7-53c-57bb-36d8-13a0902052d1@linux.microsoft.com/T/#t

[3] [PATCH v2 0/1] Trim N entries of IMA event logs
https://lore.kernel.org/linux-integrity/20251210235314.3341-1-chenste@linux.microsoft.com/T/#t

[4] [PATCH v3 0/3] Trim N entries of IMA event logs
https://lore.kernel.org/linux-integrity/20260106020713.3994-1-chenste@linux.microsoft.com/T/#t

Change Log v4:
 - Incorporated feedback from Roberto on v3 series.
 - Update "ima_log_trim" interface definition
   When read this interface, return total number of records trimmed T
   need to write T:N to this interface to trim N records
 - Update user space use case on how to trim IMA event logs
 - Updated patch descriptions as necessary.

Change Log v3:
 - Incorporated feedback from Mimi on v2 series.
 - split patch into multiple patches
 - lock time performance improvement
 - Updated patch descriptions as necessary.

Change Log v2:
 - Incorporated feedback from the Roberto on v1 series.
 - Adapted code from Roberto's RFC [Reference 2]
 - Add IMA log trim event log to record trim event
 - Updated patch descriptions as necessary.

steven chen (3):
  ima: make ima event log trimming configurable
  ima: trim N IMA event log records
  ima: add new critical data record to measure log trim

 .../admin-guide/kernel-parameters.txt         |   4 +
 security/integrity/ima/Kconfig                |  12 +
 security/integrity/ima/ima.h                  |   2 +
 security/integrity/ima/ima_fs.c               | 214 +++++++++++++++++-
 security/integrity/ima/ima_queue.c            | 100 ++++++++
 5 files changed, 328 insertions(+), 4 deletions(-)

-- 
2.43.0


^ permalink raw reply

* Re: [PATCH v3 2/3] ima: trim N IMA event log records
From: steven chen @ 2026-02-05 23:53 UTC (permalink / raw)
  To: Roberto Sassu, linux-integrity
  Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
	serge, paul, jmorris, linux-security-module, anirudhve,
	gregorylumen, nramas, sushring, linux-doc, steven chen
In-Reply-To: <74ae5c5c4cb6644537b2643d02b649a2064b718d.camel@huaweicloud.com>

On 1/7/2026 2:06 AM, Roberto Sassu wrote:
> On Mon, 2026-01-05 at 18:07 -0800, steven chen wrote:
>> Trim N entries of the IMA event logs. Clean the hash table if
>> ima_flush_htable is set.
>>
>> Provide a userspace interface ima_trim_log that can be used to input
>> number N to let kernel to trim N entries of IMA event logs. When read
>> this interface, it returns number of entries trimmed last time.
>>
>> Signed-off-by: steven chen <chenste@linux.microsoft.com>
>> ---
>>   .../admin-guide/kernel-parameters.txt         |   4 +
>>   security/integrity/ima/ima.h                  |   2 +
>>   security/integrity/ima/ima_fs.c               | 164 +++++++++++++++++-
>>   security/integrity/ima/ima_queue.c            |  85 +++++++++
>>   4 files changed, 251 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index e92c0056e4e0..cd1a1d0bf0e2 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2197,6 +2197,10 @@
>>   			Use the canonical format for the binary runtime
>>   			measurements, instead of host native format.
>>   
>> +	ima_flush_htable  [IMA]
>> +			Flush the measurement list hash table when trim all
>> +			or a part of it for deletion.
>> +
>>   	ima_hash=	[IMA]
>>   			Format: { md5 | sha1 | rmd160 | sha256 | sha384
>>   				   | sha512 | ... }
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index e3d71d8d56e3..2102c523dca0 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -246,8 +246,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>>   
>>   #ifdef CONFIG_IMA_KEXEC
>>   void ima_measure_kexec_event(const char *event_name);
>> +long ima_delete_event_log(long req_val);
>>   #else
>>   static inline void ima_measure_kexec_event(const char *event_name) {}
>> +static inline long ima_delete_event_log(long req_val) { return 0; }
>>   #endif
>>   
>>   /*
>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>> index 87045b09f120..67ff0cfc3d3f 100644
>> --- a/security/integrity/ima/ima_fs.c
>> +++ b/security/integrity/ima/ima_fs.c
>> @@ -21,6 +21,9 @@
>>   #include <linux/rcupdate.h>
>>   #include <linux/parser.h>
>>   #include <linux/vmalloc.h>
>> +#include <linux/ktime.h>
>> +#include <linux/timekeeping.h>
>> +#include <linux/ima.h>
>>   
>>   #include "ima.h"
>>   
>> @@ -38,6 +41,17 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
>>   
>>   static int valid_policy = 1;
>>   
>> +#define IMA_LOG_TRIM_REQ_LENGTH 11
>> +#define IMA_LOG_TRIM_EVENT_LEN 256
> Shouldn't this belong to the next patch?
Will update this. Thanks
>> +
>> +static long trimcount;
>> +/* mutex protects atomicity of trimming measurement list
>> + * and also protects atomicity the measurement list read
>> + * write operation.
>> + */
>> +static DEFINE_MUTEX(ima_measure_lock);
>> +static long ima_measure_users;
>> +
>>   static ssize_t ima_show_htable_value(char __user *buf, size_t count,
>>   				     loff_t *ppos, atomic_long_t *val)
>>   {
>> @@ -202,16 +216,77 @@ static const struct seq_operations ima_measurments_seqops = {
>>   	.show = ima_measurements_show
>>   };
>>   
>> +/*
>> + * _ima_measurements_open - open the IMA measurements file
>> + * @inode: inode of the file being opened
>> + * @file: file being opened
>> + * @seq_ops: sequence operations for the file
>> + *
>> + * Returns 0 on success, or negative error code.
>> + * Implements mutual exclusion between readers and writer
>> + * of the measurements file. Multiple readers are allowed,
>> + * but writer get exclusive access only no other readers/writers.
>> + * Readers is not allowed when there is a writer.
>> + */
>> +static int _ima_measurements_open(struct inode *inode, struct file *file,
>> +				  const struct seq_operations *seq_ops)
>> +{
>> +	bool write = !!(file->f_mode & FMODE_WRITE);
>> +	int ret;
>> +
>> +	if (write && !capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	mutex_lock(&ima_measure_lock);
>> +	if ((write && ima_measure_users != 0) ||
>> +	    (!write && ima_measure_users < 0)) {
>> +		mutex_unlock(&ima_measure_lock);
>> +		return -EBUSY;
>> +	}
>> +
>> +	ret = seq_open(file, seq_ops);
>> +	if (ret < 0) {
>> +		mutex_unlock(&ima_measure_lock);
>> +		return ret;
>> +	}
>> +
>> +	if (write)
>> +		ima_measure_users--;
>> +	else
>> +		ima_measure_users++;
>> +
>> +	mutex_unlock(&ima_measure_lock);
>> +	return ret;
>> +}
>> +
>>   static int ima_measurements_open(struct inode *inode, struct file *file)
>>   {
>> -	return seq_open(file, &ima_measurments_seqops);
>> +	return _ima_measurements_open(inode, file, &ima_measurments_seqops);
>> +}
>> +
>> +static int ima_measurements_release(struct inode *inode, struct file *file)
>> +{
>> +	bool write = !!(file->f_mode & FMODE_WRITE);
>> +	int ret;
>> +
>> +	mutex_lock(&ima_measure_lock);
>> +	ret = seq_release(inode, file);
>> +	if (!ret) {
>> +		if (write)
>> +			ima_measure_users++;
>> +		else
>> +			ima_measure_users--;
>> +	}
>> +
>> +	mutex_unlock(&ima_measure_lock);
>> +	return ret;
>>   }
>>   
>>   static const struct file_operations ima_measurements_ops = {
>>   	.open = ima_measurements_open,
>>   	.read = seq_read,
>>   	.llseek = seq_lseek,
>> -	.release = seq_release,
>> +	.release = ima_measurements_release,
>>   };
>>   
>>   void ima_print_digest(struct seq_file *m, u8 *digest, u32 size)
>> @@ -279,14 +354,83 @@ static const struct seq_operations ima_ascii_measurements_seqops = {
>>   
>>   static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
>>   {
>> -	return seq_open(file, &ima_ascii_measurements_seqops);
>> +	return _ima_measurements_open(inode, file, &ima_ascii_measurements_seqops);
>>   }
>>   
>>   static const struct file_operations ima_ascii_measurements_ops = {
>>   	.open = ima_ascii_measurements_open,
>>   	.read = seq_read,
>>   	.llseek = seq_lseek,
>> -	.release = seq_release,
>> +	.release = ima_measurements_release,
>> +};
>> +
>> +static int ima_log_trim_open(struct inode *inode, struct file *file)
>> +{
>> +	bool write = !!(file->f_mode & FMODE_WRITE);
>> +
>> +	if (!write && capable(CAP_SYS_ADMIN))
>> +		return 0;
>> +	else if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	return _ima_measurements_open(inode, file, &ima_measurments_seqops);
>> +}
>> +
>> +static ssize_t ima_log_trim_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>> +{
>> +	char tmpbuf[IMA_LOG_TRIM_REQ_LENGTH];	/* greater than largest 'long' string value */
>> +	ssize_t len;
>> +
>> +	len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", trimcount);
>> +	return simple_read_from_buffer(buf, size, ppos, tmpbuf, len);
>> +}
>> +
>> +static ssize_t ima_log_trim_write(struct file *file,
>> +				  const char __user *buf, size_t datalen, loff_t *ppos)
>> +{
>> +	long count, n, ret;
>> +
>> +	if (*ppos > 0 || datalen > IMA_LOG_TRIM_REQ_LENGTH || datalen < 2) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	n = (int)datalen;
>> +
>> +	ret = kstrtol_from_user(buf, n, 10, &count);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	ret = ima_delete_event_log(count);
>> +
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	trimcount = ret;
>> +
>> +	ret = datalen;
>> +out:
>> +	return ret;
>> +}
>> +
>> +static int ima_log_trim_release(struct inode *inode, struct file *file)
>> +{
>> +	bool write = !!(file->f_mode & FMODE_WRITE);
>> +
>> +	if (!write && capable(CAP_SYS_ADMIN))
>> +		return 0;
>> +	else if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	return ima_measurements_release(inode, file);
>> +}
>> +
>> +static const struct file_operations ima_log_trim_ops = {
>> +	.open = ima_log_trim_open,
>> +	.read = ima_log_trim_read,
>> +	.write = ima_log_trim_write,
>> +	.llseek = generic_file_llseek,
>> +	.release = ima_log_trim_release
>>   };
>>   
>>   static ssize_t ima_read_policy(char *path)
>> @@ -528,6 +672,18 @@ int __init ima_fs_init(void)
>>   		goto out;
>>   	}
>>   
>> +	if (IS_ENABLED(CONFIG_IMA_LOG_TRIMMING)) {
>> +		dentry = securityfs_create_file("ima_trim_log",
>> +						S_IRUSR | S_IRGRP | S_IWUSR | S_IWGRP,
>> +						ima_dir, NULL, &ima_log_trim_ops);
>> +		if (IS_ERR(dentry)) {
>> +			ret = PTR_ERR(dentry);
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	trimcount = 0;
>> +
>>   	dentry = securityfs_create_file("runtime_measurements_count",
>>   				   S_IRUSR | S_IRGRP, ima_dir, NULL,
>>   				   &ima_measurements_count_ops);
>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
>> index 590637e81ad1..33bb5414b8cc 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -22,6 +22,14 @@
>>   
>>   #define AUDIT_CAUSE_LEN_MAX 32
>>   
>> +bool ima_flush_htable;
>> +static int __init ima_flush_htable_setup(char *str)
>> +{
>> +	ima_flush_htable = true;
>> +	return 1;
>> +}
>> +__setup("ima_flush_htable", ima_flush_htable_setup);
>> +
>>   /* pre-allocated array of tpm_digest structures to extend a PCR */
>>   static struct tpm_digest *digests;
>>   
>> @@ -220,6 +228,83 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>>   	return result;
>>   }
>>   
>> +/**
>> + * ima_delete_event_log - delete IMA event entry
>> + * @num_records: number of records to delete
>> + *
>> + * delete num_records entries off the measurement list.
>> + * Returns the number of entries deleted, or negative error code.
> This is not according to the format stated in the documentation.
Will update this. Thanks
>> + */
>> +long ima_delete_event_log(long num_records)
>> +{
>> +	long len, cur = num_records, tmp_len = 0;
>> +	struct ima_queue_entry *qe, *qe_tmp;
>> +	LIST_HEAD(ima_measurements_staged);
>> +	struct list_head *list_ptr;
>> +
>> +	if (num_records <= 0)
>> +		return num_records;
>> +
>> +	if (!IS_ENABLED(CONFIG_IMA_LOG_TRIMMING))
>> +		return -EOPNOTSUPP;
>> +
>> +	mutex_lock(&ima_extend_list_mutex);
>> +	len = atomic_long_read(&ima_htable.len);
>> +
>> +	if (num_records > len) {
>> +		mutex_unlock(&ima_extend_list_mutex);
>> +		return -ENOENT;
>> +	}
>> +
>> +	list_ptr = &ima_measurements;
>> +
>> +	if (cur == len) {
>> +		list_replace(&ima_measurements, &ima_measurements_staged);
>> +		INIT_LIST_HEAD(&ima_measurements);
>> +		atomic_long_set(&ima_htable.len, 0);
>> +		list_ptr = &ima_measurements_staged;
>> +		if (IS_ENABLED(CONFIG_IMA_KEXEC))
>> +			binary_runtime_size = 0;
> Like in my patch, we should have kept the original value of
> binary_runtime_size, to avoid breaking the kexec critical data records.
I think this value is updated when trimming finish.
>> +	}
>> +
>> +	list_for_each_entry(qe, list_ptr, later) {
>> +		if (num_records > 0) {
>> +			if (!IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE) && ima_flush_htable)
>> +				hlist_del_rcu(&qe->hnext);
>> +
>> +			--num_records;
>> +			if (num_records == 0)
>> +				qe_tmp = qe;
>> +			continue;
>> +		}
>> +		if (len != cur && IS_ENABLED(CONFIG_IMA_KEXEC))
>> +			tmp_len += get_binary_runtime_size(qe->entry);
>> +		else
>> +			break;
>> +	}
>> +
>> +	if (len != cur) {
>> +		__list_cut_position(&ima_measurements_staged, &ima_measurements,
>> +				    &qe_tmp->later);
>> +		atomic_long_sub(cur, &ima_htable.len);
>> +		if (IS_ENABLED(CONFIG_IMA_KEXEC))
>> +			binary_runtime_size = tmp_len;
>> +	}
>> +
>> +	mutex_unlock(&ima_extend_list_mutex);
>> +
>> +	if (ima_flush_htable)
>> +		synchronize_rcu();
>> +
>> +	list_for_each_entry_safe(qe, qe_tmp, &ima_measurements_staged, later) {
>> +		ima_free_template_entry(qe->entry);
>> +		list_del(&qe->later);
>> +		kfree(qe);
> If you don't flush the hash table, you cannot delete the entry.
>
> Roberto

Will update this. Thanks

Steven

>> +	}
>> +
>> +	return cur;
>> +}
>> +
>>   int ima_restore_measurement_entry(struct ima_template_entry *entry)
>>   {
>>   	int result = 0;



^ permalink raw reply

* Re: [PATCH v3 0/3] landlock: documentation improvements
From: Mickaël Salaün @ 2026-02-05 19:23 UTC (permalink / raw)
  To: Samasth Norway Ananda; +Cc: gnoack, linux-security-module, linux-kernel
In-Reply-To: <20260128031814.2945394-1-samasth.norway.ananda@oracle.com>

Thanks for these improvements, it helps!  I'll send these changes for
Linux 7.0

On Tue, Jan 27, 2026 at 07:18:09PM -0800, Samasth Norway Ananda wrote:
> This patch series improves Landlock documentation by addressing gaps in
> ABI compatibility examples, adding errata documentation, and documenting
> the audit blockers field format.
> 
> Changes since v2:
> =================
> 
> Patch 1/3:
> - Handle restrict_flags in a separate code block, not in the switch
> - Clear all three ABI v7 logging flags for a generic example
> - Reference sys_landlock_restrict_self() for available flags
> - Use restrict_flags in landlock_restrict_self()
> 
> Patch 2/3:
> - Use kernel-doc directives to include errata from header files
> - Move rephrased ABI version text before errata section
> 
> Patch 3/3:
> - No changes
> 
> Changes since v1:
> =================
> 
> Patch 1/3:
> - Add backwards compatibility section for restrict flags
> - Fix /usr rule description
> 
> Patch 2/3:
> - Enhance existing DOC sections with Impact descriptions
> - Add errata usage documentation
> 
> Patch 3/3:
> - Document audit blocker field format
> 
> Samasth Norway Ananda (3):
>   landlock: add backwards compatibility for restrict flags
>   landlock: add errata documentation section
>   landlock: document audit blockers field format
> 
>  Documentation/admin-guide/LSM/landlock.rst | 20 ++++-
>  Documentation/userspace-api/landlock.rst   | 97 +++++++++++++++++++---
>  security/landlock/errata/abi-1.h           |  8 ++
>  security/landlock/errata/abi-4.h           |  7 ++
>  security/landlock/errata/abi-6.h           | 10 +++
>  security/landlock/syscalls.c               |  4 +-
>  6 files changed, 131 insertions(+), 15 deletions(-)
> 
> -- 
> 2.50.1
> 
> 

^ permalink raw reply

* Re: [PATCH v2 0/6] Landlock: Implement scope control for pathname Unix sockets
From: Mickaël Salaün @ 2026-02-05 19:15 UTC (permalink / raw)
  To: Justin Suess
  Cc: Günther Noack, Tingmao Wang, Günther Noack,
	Demi Marie Obenour, Alyssa Ross, Jann Horn, Tahera Fahimi,
	linux-security-module, Matthieu Buffet
In-Reply-To: <44d216aa-9680-4cf5-bbf0-173869111212@gmail.com>

On Thu, Feb 05, 2026 at 10:18:54AM -0500, Justin Suess wrote:
> 
> On 2/4/26 13:28, Mickaël Salaün wrote:
> 
> >> [...]
> >> Tingmao:
> >>
> >> For connecting a pathname unix socket, the order of the hooks landlock sees is something like:
> >>
> >> 1.  security_unix_find. (to look up the paths)
> >>
> >> 2. security_unix_may_send, security_unix_stream_connect (after the path is looked up)
> >>
> >> Which for is called in DGRAM:
> >>
> >>  unix_dgram_connect OR  unix_dgram_sendmsg 
> >>
> >> and for STREAM:
> >>
> >>  unix_stream_connect
> >>
> >> IIRC, the path lookup only occurs in this order always, so in the logic as you have it the domain_is_scoped()
> >> would be called twice, once from the security_unix_find when you call it in step two, and once from the
> >> domain scope hooks. (If access was allowed from security_unix_find)
> >>
> >> There are a couple of things to consider.
> >>
> >> ---
> >>
> >> Audit blockers need special handling:
> >>
> >> Here's an example:
> >>
> >> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
> >> access bit (deny all for RESOLVE_UNIX).
> >>
> >> 2. Program A connects to /tmp/mysock.sock ran by program B, which is outside the domain.
> >>
> >> 2. security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
> >> /tmp/mysock.sock, so it calls domain_is_scoped
> >>
> >> 3. domain_is_scoped denies it as well, so now we must log an audit record.
> >>
> >> When logging the denial, we have to include both blockers "scope.unix_socket"  and "fs.resolve_unix" for the
> >> denial, because it is the absence of both that caused the denial. I think the refer right has similar cases for auditing, so there is precedent for this (multiple blockers for an audit message).
> > That's a good point, and it would give more informations to diagnose
> > issues.  However, being able to identify if both accesses are denied
> > would require to check both, whereas the first is enough to know that
> > Landlock denies the access.  So, if we can return both records without
> > continuing the security checks, that's good, otherwise we should stop
> > ASAP and return the error.
> Maybe I'm missing something, but if the flags interact in an "OR" manner
> wouldn't we need to check both?

Yes, but my point is that as soon as one or the other *denies* an
access, there is no need to check the other access type.

> In your proposal where RESOLVE_UNIX
> implies the scoped flag, if a program connects to a unix socket that is within
> the domain but does not have an explicit RESOLVE_UNIX exception, we must
> still check for the case that the access is scoped.
> 
> ---
> 
> (Given LANDLOCK_ACCESS_FS_RESOLVE_UNIX and LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
> are set)
> 
> case 1: access to socket within domain and no RESOLVE_UNIX rule covers the access
> 
> We check first in security_unix_find hook and find there is no rule allowing the access.
> After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
> we then check is_domain_scoped, the check allows the access.
> 
> case 2: access to socket outside domain but RESOLVE_UNIX rule covers the access
> 
> We check first in security_unix_find hook and find there is a rule allowing the access.
> We can allow the access early (short-circuit eval) without calling is_domain_scoped.
> 
> case 4: access to socket inside domain and RESOLVE_UNIX covers the access
> 
> We check first in security_unix_find hook and find there is a rule allowing the access.
> We can allow the access early (short-circuit eval) without calling is_domain_scoped. (same as case 2)
> 
> 
> case 4: access to socket outside domain and no RESOLVE_UNIX covers the access
> 
> We check first in security_unix_find hook and find there is no rule allowing the access.
> After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
> we then check is_domain_scoped, the check does not allow the access. (it is the combination
> of the two checks failing that denied the access).

BTW, we should first check is_domain_scoped() because it is quicker than
the FS checks.

> 
> ---
> 
> Case 4 is what I'm specifically considering would need to have both blockers listed in a denial audit.
> We can't short circuit in that case because we have to check the scoping before denying.
> Let me know if I'm misunderstanding this.

Indeed, if both deny the request, both should be listed.

Anyway, we're now going to only have one access flag that would merge
both semantic, so this should not be an issue.

However, this specific case will be relevant when we'll add e.g., a
signal attr that would then be complementary to the signal scope.  At
this point, I think it would be enough to only log the signal.* record
because it would be a superset of the scope.signal

> 
> (PS: IIRC the hooks used by the LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
> will never be hit if the check in security_unix_find fails. So some logic to check this
> access in security_unix_find will be needed).
> 
> >
> > Anyway, that might not be needed if we end up with my latest proposal
> > about always setting scope.unix_socket when fs.resolve_unix is set.
> >
> >> ---
> >>
> >> Dual lookup for domain_is_scoped. Consider this case:
> >>
> >> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
> >> access bit (deny all for RESOLVE_UNIX).
> >>
> >> 2. Program A connects to Program C's /tmp/foo.sock, which for the purposes of this example is in the domain of program A.
> >>
> >> 3.  security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
> >> /tmp/mysock.sock, so it calls domain_is_scoped. Access is granted, and continues. (LSM hook complete)
> >>
> >> 4.  The connection proceeds past the path lookup stage, and now security_unix_may_send, or security_unix_stream_connect gets called. This requires ANOTHER domain_is_scoped access check.
> >>
> >> While I don't THINK this introduces a TOCTOU, it is a little confusing.
> >>
> >> This does mean that we look up the domain twice, if this is implemented naively. I think we can then just
> >> skip the task credential checks then for security_unix_may_send and security_unix_stream_connect **for
> >> connecting to pathname sockets**, since the domain_is_scoped will already have been called in landlock's
> >> security_unix_find hook, eliminating the need for handling pathname socket domain checks layer on.
> >>
> >>>> I definitely agree that it is tricky, but making same-scope access be
> >>>> allowed (i.e. the suggested idea above) would only get rid of step 1,
> >>>> which I think is the "simpler" bit.  The extra logic in step 2 is still
> >>>> needed. 
> >>>>
> >>>> I definitely agree with pro1 tho.
> >>> Yes, you are describing the logic for the variant where
> >>> LANDLOCK_ACCESS_FS_RESOLVE_UNIX does not implicitly permit access from
> >>> within the same scope.  In that variant, there can be situations where
> >>> the first hook can deny the action immediately.
> >>>
> >>> In the variant where LANDLOCK_ACCESS_FS_RESOLVE_UNIX *does* implicitly
> >>> allow access from within the same scope, that shortcutting is not
> >>> possible.  On the upside however, there is no need to distinguish
> >>> whether the scope flag is set when we are in the security_unix_find()
> >>> hook, because access from within the same scope is always permitted.
> >>> (That is the simplification I meant.)
> >>>
> >>>
> >>>>> AGAINST:
> >>>>>
> >>>>> (con1) It would work differently than the other scoped access rights
> >>>>>        that we already have.
> >>>>>
> >>>>>        A speculative feature that could potentially be built with the
> >>>>>        scoped access rights is that we could add a rule to permit IPC
> >>>>>        to other Landlock scopes, e.g. introducing a new rule type
> >>>>>
> >>>>>          struct landlock_scope_attr {
> >>>>>            __u64 allowed_access;  /* for "scoped" bits */
> >>>>>            /* some way to identify domains */
> >>>>>          }
> >>>>>
> >>>>>        so that we could make IPC access to other Landlock domains
> >>>>>        configurable.
> >>>>>
> >>>>>        If the scoped bit and the FS RESOLVE_UNIX bit were both
> >>>>>        conflated in RESOLVE_UNIX, it would not be possible to make
> >>>>>        UNIX connections configurable in such a way.
> >>>> This exact API would no longer work, but if we give up the equivalence
> >>>> between scope bits and the landlock_scope_attr struct, then we can do
> >>>> something like:
> >>>>
> >>>> struct landlock_scope_attr {
> >>>>     __u64 ptrace:1;  /* Note that this is not a (user controllable) scope bit! */
> >>>>     __u64 abstract_unix_socket:1;
> >>>>     __u64 pathname_unix_socket:1;
> >>>>     /* ... */
> >>>>
> >>>>     __u64 allowed_signals;
> >>>>
> >>>>     /*
> >>>>      * some way to identify domains, maybe we could use the audit domain
> >>>>      * ID, with 0 denoting "allow access to non-Landlocked processes?
> >>>>      */
> >>>> }
> >>> Yes, it would be possible to use such a struct for that scenario where
> >>> IPC access gets allowed for other Landlock scopes.  It would mean that
> >>> we would not need to introduce a scoped flag for the pathname UNIX
> >>> socket connections.  But the relationship between that struct
> >>> landlock_scope_attr and the flags and access rights in struct
> >>> landlock_ruleset_attr would become less clear, which is a slight
> >>> downside, and maybe error prone for users to work with.
> >>>
> >>> If we introduced an additional scoped flag, it would also be
> >>> consistent though.
> >>>
> >>> (con1) was written under the assumption that we do not have an
> >>> additional scoped flag.  If that is lacking, it is not possible to
> >>> express UNIX connect() access to other Landlock domains with that
> >>> struct.  But as outlined in the proposal below, if we *do* (later)
> >>> introduce the additional scoped flag *in addition* to the FS access
> >>> right, this *both* stays consistent in semantics with the signal and
> >>> abstract UNIX support, *and* it starts working in a world where ICP
> >>> access can be allowed to talk to other Landlock domains.
> >>>
> >>>>> (con2) Consistent behaviour between scoped flags and their
> >>>>>        interactions with other access rights:
> >>>>>
> >>>>>        The existing scoped access rights (signal, abstract sockets)
> >>>>>        could hypothetically be extended with a related access right of
> >>>>>        another type. For instance, there could be an access right type
> >>>>>
> >>>>>          __u64 handled_signal_number;
> >>>>>
> >>>>>        and then you could add a rule to permit the use of certain
> >>>>>        signal numbers.  The interaction between the scoped flags and
> >>>>>        other access rights should work the same.
> >>>>>
> >>>>>
> >>>>> Constructive Proposal for consideration: Why not both?
> >>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>> I will think about the following a bit more but I'm afraid that I feel
> >>>> like it might get slightly confusing.  With this, the only reason for
> >>>> having LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is for API consistency when we
> >>>> later enable allowing access to other domains (if I understood correctly),
> >>>> in which case I personally feel like the suggestion on landlock_scope_attr
> >>>> above, where we essentially accept that it is decoupled with the scope
> >>>> bits in the ruleset, might be simpler...?
> >>> Mickaël expressed the opinion to me that he would like to APIs to stay
> >>> consistent between signals, abstract UNIX sockets, named UNIX sockets
> >>> and other future "scoped" operations, in scenarios where:
> >>>
> >>> * the "scoped" (IPC) operations can be configured to give access to
> >>>   other Landlock domains (and that should work for UNIX connections too)
> >>> * the existing "scoped" operations also start having matching access rights
> >>>
> >>> I think with the way I proposed, that would be consistent.
> >>>
> >>>
> >>>>> Why not do both what Tingmao proposed in [1] **and** reserve the
> >>>>> option to add the matching "scoped flag" later?
> >>>>>
> >>>>>   * Introduce LANDLOCK_ACCESS_FS_RESOLVE_UNIX.
> >>>>>
> >>>>>     If it is handled, UNIX connections are allowed either:
> >>>>>
> >>>>>     (1) if the connection is to a service in the same scope, or
> >>>>>     (2) if the path was allow-listed with a "path beneath" rule.
> >>>>>
> >>>>>   * Add LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET later, if needed.
> >>>>>
> >>>>>
> >>>>> Let's go through the arguments again:
> >>>>>
> >>>>> We have observed that it is harmless to allow connections to services
> >>>>> in the same scope (1), and that if users absolutely don't want that,
> >>>>> they can actually prohibit it through LANDLOCK_ACCESS_FS_MAKE_SOCK
> >>>>> (pro1).
> >>>>>
> >>>>> (con1): Can we still implement the feature idea where we poke a hole
> >>>>>         to get UNIX-connect() access to other Landlock domains?
> >>>>>
> >>>>>   I think the answer is yes.  The implementation strategy is:
> >>>>>
> >>>>>     * Add the scoped bit LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
> >>>>>     * The scoped bit can now be used to allow-list connections to
> >>>>>       other Landlock domains.
> >>>>>
> >>>>>   For users, just setting the scoped bit on its own does the same as
> >>>>>   handling LANDLOCK_ACCESS_FS_RESOLVE_UNIX.  That way, the kernel-side
> >>>>>   implementation can also stay simple.  The only reason why the scoped
> >>>>>   bit is needed is because it makes it possible to allow-list
> >>>>>   connections to other Landlock domains, but at the same time, it is
> >>>>>   safe if libraries set the scoped bit once it exists, as it does not
> >>>>>   have any bad runtime impact either.
> >>>>>
> >>>>> (con2): Consistency: Do all the scoped flags interact with their
> >>>>>         corresponding access rights in the same way?
> >>>>>
> >>>>>   The other scope flags do not have corresponding access rights, so
> >>>>>   far.
> >>>>>
> >>>>>   If we were to add corresponding access rights for the other scope
> >>>>>   flags, I would argue that we could apply a consistent logic there,
> >>>>>   because IPC access within the same scope is always safe:
> >>>>>
> >>>>>   - A hypothetical access right type for "signal numbers" would only
> >>>>>     restrict signals that go beyond the current scope.
> >>>>>
> >>>>>   - A hypothetical access right type for "abstract UNIX domain socket
> >>>>>     names" would only restrict connections to abstract UNIX domain
> >>>>>     servers that go beyond the current scope.
> >>>>>
> >>>>>   I can not come up with a scenario where this doesn't work.
> >> Gunther / Tingmao / Mickaël:
> >>
> >> I have a potential idea to make this concept cleaner.
> >>
> >> The docs for landlock currently say:
> >>
> >>
> >>        IPC scoping does not support exceptions via landlock_add_rule(2).
> >>        If an operation is scoped within a domain, no rules can be added
> >>        to allow access to resources or processes outside of the scope.
> > This part might indeed be confusing.  The idea was to explain the
> > difference between scoped rights and handled access rights (which may
> > have rules).
> >
> >> So if we go with the solution where we are now saying IPC scoping DOES support exceptions
> >> we will need to update the documentation, to say scoping for pathname unix sockets is an exception,
> >> and have to have the "exemptible scopes" (like this one) alongside "non-exemptible" scopes
> >> (ie the existing ones). This creates some friction for users.
> > The documentation will definitely require some updates.  I think it can
> > be explained in a simple way.
> >
> >> If we foresee other "exempt-able scopes" (which are scopes that also support creating exemptions w/ corresponding access rights) in the future, maybe we should consider separating the two in the ruleset
> >> attributes (I used scoped_fs as an example for the attribute name):
> >>
> >> structlandlock_ruleset_attrruleset_attr={
> >> .handled_access_fs=
> >> LANDLOCK_ACCESS_FS_EXECUTE|
> >> LANDLOCK_ACCESS_FS_WRITE_FILE|
> >> LANDLOCK_ACCESS_FS_READ_FILE|
> >> LANDLOCK_ACCESS_FS_READ_DIR|
> >> LANDLOCK_ACCESS_FS_REMOVE_DIR|
> >> LANDLOCK_ACCESS_FS_REMOVE_FILE|
> >> LANDLOCK_ACCESS_FS_MAKE_CHAR|
> >> LANDLOCK_ACCESS_FS_MAKE_DIR|
> >> LANDLOCK_ACCESS_FS_MAKE_REG|
> >> LANDLOCK_ACCESS_FS_MAKE_SOCK|
> >> LANDLOCK_ACCESS_FS_MAKE_FIFO|
> >> LANDLOCK_ACCESS_FS_MAKE_BLOCK|
> >> LANDLOCK_ACCESS_FS_MAKE_SYM|
> >> LANDLOCK_ACCESS_FS_REFER|
> >> LANDLOCK_ACCESS_FS_TRUNCATE|
> >> LANDLOCK_ACCESS_FS_IOCTL_DEV,
> >> .handled_access_net=
> >> LANDLOCK_ACCESS_NET_BIND_TCP|
> >> LANDLOCK_ACCESS_NET_CONNECT_TCP,
> >> .scoped=
> >> LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET|
> >> LANDLOCK_SCOPE_SIGNAL,
> >>     .scoped_fs = 
> >> 	LANDLOCK_SCOPE_FS_PATHNAME_UNIX_SOCKET
> >> };
> >>
> >> This more clearly distinguishes between scopes that have exceptions/corresponding fs rights,
> >> and ones that don't. Later we could add scoped_net, if needed. I feel like this would be more
> >> intuitive and better categorize future scoping rights. An obvious con is increasing the size of
> >> the ruleset attributes.
> > I see your point but I don't think it would be worth it to add
> > sub-scoped fields.  Each field has a clear semantic, and the scoped one
> > is related to the domain wrt other domains.
> As long as it's documented clearly, and future IPCs have similar behavior
> I agree that a separate field probably isn't needed.
> >
> >> Of course this separation is only worth it if there are other "exempt-able" rights in the future.
> >> I can think of a few potential future rights which COULD be scoped and have corresponding rights
> >> (binder, sysv-ipc, pipes, tcp/udp between two local programs). 
> > Yes, it would definitely be useful to add exception for other kind of
> > IPCs.  The idea would be to be able to describe the peer, either with a
> > file path, or PID, or cgroups, or a Landlock domain...  The inet case
> > is an interesting idea but that might be a challenging task to
> > implement, if even possible.
> 
> >
> >>>>>
> >>>>> In conclusion, I think the approach has significant upsides:
> >>>>>
> >>>>>   * Simpler UAPI: Users only have one access bit to deal with, in the
> >>>>>     near future.  Once we do add a scope flag for UNIX connections, it
> >>>>>     does not interact in a surprising way with the corresponding FS
> >>>>>     access right, because with either of these, scoped access is
> >>>>>     allowed.
> >>>>>
> >>>>>     If users absolutely need to restrict scoped access, they can
> >>>>>     restrict LANDLOCK_ACCESS_FS_MAKE_SOCK.  It is a slightly obscure
> >>>>>     API, but in line with the "make easy things easy, make hard things
> >>>>>     possible" API philosophy.  And needing this should be the
> >>>>>     exception rather than the norm, after all.
> >>>>>
> >>>>>   * Consistent behaviour between scoped flags and regular access
> >>>>>     rights, also for speculative access rights affecting the existing
> >>>>>     scoped flags for signals and abstract UNIX domain sockets.
> >>>>>
> >>>>> [1] https://lore.kernel.org/all/f07fe41a-96c5-4d3a-9966-35b30b3a71f1@maowtm.org/
> >>> —Günther

^ permalink raw reply

* Re: [PATCH v3 2/3] landlock: selftests for LANDLOCK_RESTRICT_SELF_TSYNC
From: Mickaël Salaün @ 2026-02-05 18:54 UTC (permalink / raw)
  To: Günther Noack
  Cc: linux-security-module, Jann Horn, Serge Hallyn,
	Konstantin Meskhidze, Tingmao Wang, Andrew G. Morgan,
	John Johansen, Paul Moore
In-Reply-To: <20251127115136.3064948-3-gnoack@google.com>

These tests are useful but there are two missing parts:
- testing the restriction of each thread;
- testing the domain ID of each thread.

I'll merge this whole series but could you please extend this test file
with a new patch?

On Thu, Nov 27, 2025 at 12:51:35PM +0100, Günther Noack wrote:
> Exercise various scenarios where Landlock domains are enforced across
> all of a processes' threads.
> 
> Cc: Andrew G. Morgan <morgan@kernel.org>
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Mickaël Salaün <mic@digikod.net>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  tools/testing/selftests/landlock/base_test.c  |   6 +-
>  tools/testing/selftests/landlock/tsync_test.c | 161 ++++++++++++++++++
>  2 files changed, 164 insertions(+), 3 deletions(-)
>  create mode 100644 tools/testing/selftests/landlock/tsync_test.c
> 
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index f4b1a275d8d9..0fea236ef4bd 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -288,7 +288,7 @@ TEST(restrict_self_fd)
>  	EXPECT_EQ(EBADFD, errno);
>  }
>  
> -TEST(restrict_self_fd_flags)
> +TEST(restrict_self_fd_logging_flags)
>  {
>  	int fd;
>  
> @@ -304,9 +304,9 @@ TEST(restrict_self_fd_flags)
>  	EXPECT_EQ(EBADFD, errno);
>  }
>  
> -TEST(restrict_self_flags)
> +TEST(restrict_self_logging_flags)
>  {
> -	const __u32 last_flag = LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF;
> +	const __u32 last_flag = LANDLOCK_RESTRICT_SELF_TSYNC;
>  
>  	/* Tests invalid flag combinations. */
>  
> diff --git a/tools/testing/selftests/landlock/tsync_test.c b/tools/testing/selftests/landlock/tsync_test.c
> new file mode 100644
> index 000000000000..3971e0f02c49
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/tsync_test.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Landlock tests - Enforcing the same restrictions across multiple threads
> + *
> + * Copyright © 2025 Günther Noack <gnoack3000@gmail.com>
> + */
> +
> +#define _GNU_SOURCE
> +#include <pthread.h>
> +#include <sys/prctl.h>
> +#include <linux/landlock.h>
> +
> +#include "common.h"
> +
> +/* create_ruleset - Create a simple ruleset FD common to all tests */
> +static int create_ruleset(struct __test_metadata *const _metadata)
> +{
> +	struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_fs = (LANDLOCK_ACCESS_FS_WRITE_FILE |
> +				      LANDLOCK_ACCESS_FS_TRUNCATE),
> +	};
> +	const int ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +
> +	ASSERT_LE(0, ruleset_fd)
> +	{
> +		TH_LOG("landlock_create_ruleset: %s", strerror(errno));
> +	}
> +	return ruleset_fd;
> +}
> +
> +TEST(single_threaded_success)
> +{
> +	const int ruleset_fd = create_ruleset(_metadata);
> +
> +	disable_caps(_metadata);
> +
> +	ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +	ASSERT_EQ(0, landlock_restrict_self(ruleset_fd,
> +					    LANDLOCK_RESTRICT_SELF_TSYNC));
> +
> +	ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +void store_no_new_privs(void *data)

I fixed the helpers to make them static.

> +{
> +	bool *nnp = data;
> +
> +	if (!nnp)
> +		return;
> +	*nnp = prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
> +}
> +
> +void *idle(void *data)
> +{
> +	pthread_cleanup_push(store_no_new_privs, data);
> +
> +	while (true)
> +		sleep(1);
> +
> +	pthread_cleanup_pop(1);
> +}
> +
> +TEST(multi_threaded_success)
> +{
> +	pthread_t t1, t2;
> +	bool no_new_privs1, no_new_privs2;
> +	const int ruleset_fd = create_ruleset(_metadata);
> +
> +	disable_caps(_metadata);
> +
> +	ASSERT_EQ(0, pthread_create(&t1, NULL, idle, &no_new_privs1));
> +	ASSERT_EQ(0, pthread_create(&t2, NULL, idle, &no_new_privs2));
> +
> +	ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +
> +	EXPECT_EQ(0, landlock_restrict_self(ruleset_fd,
> +					    LANDLOCK_RESTRICT_SELF_TSYNC));
> +
> +	ASSERT_EQ(0, pthread_cancel(t1));
> +	ASSERT_EQ(0, pthread_cancel(t2));
> +	ASSERT_EQ(0, pthread_join(t1, NULL));
> +	ASSERT_EQ(0, pthread_join(t2, NULL));
> +
> +	/* The no_new_privs flag was implicitly enabled on all threads. */
> +	EXPECT_TRUE(no_new_privs1);
> +	EXPECT_TRUE(no_new_privs2);
> +
> +	ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +TEST(multi_threaded_success_despite_diverging_domains)
> +{
> +	pthread_t t1, t2;
> +	const int ruleset_fd = create_ruleset(_metadata);
> +
> +	disable_caps(_metadata);
> +
> +	ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +
> +	ASSERT_EQ(0, pthread_create(&t1, NULL, idle, NULL));
> +	ASSERT_EQ(0, pthread_create(&t2, NULL, idle, NULL));
> +
> +	/*
> +	 * The main thread enforces a ruleset,
> +	 * thereby bringing the threads' Landlock domains out of sync.
> +	 */
> +	EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
> +
> +	/* Still, TSYNC succeeds, bringing the threads in sync again. */
> +	EXPECT_EQ(0, landlock_restrict_self(ruleset_fd,
> +					    LANDLOCK_RESTRICT_SELF_TSYNC));
> +
> +	ASSERT_EQ(0, pthread_cancel(t1));
> +	ASSERT_EQ(0, pthread_cancel(t2));
> +	ASSERT_EQ(0, pthread_join(t1, NULL));
> +	ASSERT_EQ(0, pthread_join(t2, NULL));
> +	ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +struct thread_restrict_data {
> +	pthread_t t;
> +	int ruleset_fd;
> +	int result;
> +};
> +
> +void *thread_restrict(void *data)
> +{
> +	struct thread_restrict_data *d = data;
> +
> +	d->result = landlock_restrict_self(d->ruleset_fd,
> +					   LANDLOCK_RESTRICT_SELF_TSYNC);
> +	return NULL;
> +}
> +
> +TEST(competing_enablement)
> +{
> +	const int ruleset_fd = create_ruleset(_metadata);
> +	struct thread_restrict_data d[] = {
> +		{ .ruleset_fd = ruleset_fd },
> +		{ .ruleset_fd = ruleset_fd },
> +	};
> +
> +	disable_caps(_metadata);
> +
> +	ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +	ASSERT_EQ(0, pthread_create(&d[0].t, NULL, thread_restrict, &d[0]));
> +	ASSERT_EQ(0, pthread_create(&d[1].t, NULL, thread_restrict, &d[1]));
> +
> +	/* Wait for threads to finish. */
> +	ASSERT_EQ(0, pthread_join(d[0].t, NULL));
> +	ASSERT_EQ(0, pthread_join(d[1].t, NULL));
> +
> +	/* Expect that both succeeded. */
> +	EXPECT_EQ(0, d[0].result);
> +	EXPECT_EQ(0, d[1].result);
> +
> +	ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +TEST_HARNESS_MAIN
> -- 
> 2.52.0.177.g9f829587af-goog
> 
> 

^ permalink raw reply

* Re: [PATCH v3 0/3] Landlock multithreaded enforcement
From: Mickaël Salaün @ 2026-02-05 18:53 UTC (permalink / raw)
  To: Günther Noack
  Cc: linux-security-module, Jann Horn, Serge Hallyn,
	Konstantin Meskhidze, Tingmao Wang, Matthieu Buffet
In-Reply-To: <20251127115136.3064948-1-gnoack@google.com>

Good job for writing this complex mechanic (and the related doc), this
patch series is great!  It's been in linux-next for a few weeks and I'll
take it for Linux 7.0

I did some cosmetic changes though, you'll find them in my commits.
Some more tests are needed but I'll take this series for now.

Thanks!

On Thu, Nov 27, 2025 at 12:51:33PM +0100, Günther Noack wrote:
> This patch set adds the LANDLOCK_RESTRICT_SELF_TSYNC flag to
> landlock_restrict_self().  With this flag, the passed Landlock ruleset
> will not only be applied to the calling thread, but to all threads
> which belong to the same process.
> 
> Motivation
> ==========
> 
> TL;DR: The libpsx/nptl(7) signal hack which we use in user space for
> multi-threaded Landlock enforcement is incompatible with Landlock's
> signal scoping support.  Landlock can restrict the use of signals
> across Landlock domains, but we need signals ourselves in user space
> in ways that are not permitted any more under these restrictions.
> 
> Enabling Landlock proves to be difficult in processes that are already
> multi-threaded at the time of enforcement:
> 
> * Enforcement in only one thread is usually a mistake because threads
>   do not normally have proper security boundaries between them.
> 
> * Also, multithreading is unavoidable in some circumstances, such as
>   when using Landlock from a Go program.  Go programs are already
>   multithreaded by the time that they enter the "func main()".
> 
> So far, the approach in Go[1] was to use libpsx[2].  This library
> implements the mechanism described in nptl(7) [3]: It keeps track of
> all threads with a linker hack and then makes all threads do the same
> syscall by registering a signal handler for them and invoking it.
> 
> With commit 54a6e6bbf3be ("landlock: Add signal scoping"), Landlock
> gained the ability to restrict the use of signals across different
> Landlock domains.
> 
> Landlock's signal scoping support is incompatible with the libpsx
> approach of enabling Landlock:
> 
> (1) With libpsx, although all threads enforce the same ruleset object,
>     they technically do the operation separately and end up in
>     distinct Landlock domains.  This breaks signaling across threads
>     when using LANDLOCK_SCOPE_SIGNAL.
> 
> (2) Cross-thread Signals are themselves needed to enforce further
>     nested Landlock domains across multiple threads.  So nested
>     Landlock policies become impossible there.
> 
> In addition to Landlock itself, cross-thread signals are also needed
> for other seemingly-harmless API calls like the setuid(2) [4] and for
> the use of libcap (co-developed with libpsx), which have the same
> problem where the underlying syscall only applies to the calling
> thread.
> 
> Implementation details
> ======================
> 
> Enforcement prerequisites
> -------------------------
> 
> Normally, the prerequisite for enforcing a Landlock policy is to
> either have CAP_SYS_ADMIN or the no_new_privs flag.  With
> LANDLOCK_RESTRICT_SELF_TSYNC, the no_new_privs flag will automatically
> be applied for sibling threads if the caller had it.
> 
> These prerequisites and the "TSYNC" behavior work the same as for
> Seccomp and its SECCOMP_FILTER_FLAG_TSYNC flag.
> 
> Pseudo-signals
> --------------
> 
> Landlock domains are stored in struct cred, and a task's struct cred
> can only be modified by the task itself [6].
> 
> To make that work, we use task_work_add() to register a pseudo-signal
> for each of the affected threads.  At signal execution time, these
> tasks will coordinate to switch out their Landlock policy in lockstep
> with each other, guaranteeing all-or-nothing semantics.
> 
> This implementation can be thought of as a kernel-side implementation
> of the userspace hack that glibc/NPTL use for setuid(2) [3] [4], and
> which libpsx implements for libcap [2].
> 
> Finding all sibling threads
> ---------------------------
> 
> In order to avoid grabbing the global task_list_lock, we employ the
> scheme proposed by Jann Horn in [7]:
> 
> 1. Loop through the list of sibling threads
> 2. Schedule a pseudo-signal for each and make each thread wait in the
>    pseudo-signal
> 3. Go back to 1. and look for more sibling thread that we have not
>    seen yet
> 
> Do this until no more new threads are found.  As all threads were
> waiting in their pseudo-signals, they can not spawn additional threads
> and we found them all.
> 
> Coordination between tasks
> --------------------------
> 
> As tasks run their pseudo-signal task work, they coordinate through
> the following completions:
> 
>  - all_prepared (with counter num_preparing)
>  
>    When done, all new sibling threads in the inner loop(!) of finding
>    new threads are now in their pseudo-signal handlers and have
>    prepared the struct cred object to commit (or written an error into
>    the shared "preparation_error").
> 
>    The lifetime of all_prepared is only the inner loop of finding new
>    threads.
> 
>  - ready_to_commit
> 
>    When done, the outer loop of finding new threads is done and all
>    sibling threads have prepared their struct cred object.  Marked
>    completed by the calling thread.
> 
>  - all_finished
> 
>    When done, all sibling threads are done executing their
>    pseudo-signal handlers.
> 
> Use of credentials API
> ----------------------
> 
> Under normal circumstances, sibling threads share the same struct cred
> object.  To avoid unnecessary duplication, if we find that a thread
> uses the same struct cred as the calling thread, we side-step the
> normal use of the credentials API [6] and place a pointer to that
> existing struct cred instead of creating a new one using
> prepare_creds() in the sibling thread.
> 
> Noteworthy discussion points
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> * We are side-stepping the normal credentials API [6], by re-wiring an
>   existing struct cred object instead of calling prepare_creds().
> 
>   We can technically avoid it, but it would create unnecessary
>   duplicate struct cred objects in multithreaded scenarios.
> 
> Change Log
> ==========
> 
> v3:
>  - bigger organizational changes
>    - move tsync logic into own file
>    - tsync: extract count_additional_threads() and
>      schedule_task_work()
>  - code style
>    - restrict_one_thread, syscalls.c: use err instead of res (mic)
>    - restrict_one_thread: inline current_cred variable
>    - restrict_one_thread: add comment to shortcut logic (mic)
>    - rsync_works helpers: use size_t i for loop vars
>    - landlock_cred_copy: skip redundant NULL checks
>    - function name: s,tsync_works_free,tsync_works_release, (mic)
>    - tsync_works_grow_by: kzalloc into a temporary variable for
>      clarity (mic)
>    - tsync_works_contains_task: make struct task_works const
>  - bugs
>    - handle kmalloc family failures correctly (jannh)
>    - tsync_works_release: check task NULL ptr before put
>    - s/put_task_struct_rcu_user/put_task_struct/ (jannh)
>  - concurrency bugs
>    - schedule_task_work: do not return error when encountering exiting
>      tasks This can happen during normal operation, we should not
>      error due to it (jannh)
>    - landlock_restrict_sibling_threads: make current hold the
>      num_unfinished/all_finished barrier (more robust, jannh)
>    - un-wedge the deadlock using wait_for_completion_interruptible
>      (jannh) See "testing" below and discussion in
>      https://lore.kernel.org/all/CAG48ez1oS9kANZBq1bt+D76MX03DPHAFp76GJt7z5yx-Na1VLQ@mail.gmail.com/
>  - logic
>    - tsync_works_grow_by(): grow to size+n, not capacity+n
>    - tsync_works_grow_by(): add overflow check for capacity increase
>    - landlock_restrict_self(): make TSYNC and LOG flags work together
>    - set no_new_privs in the same way as seccomp,
>      whenever the calling thread had it
>  - testing
>    - add test where multiple threads call landlock_restrict_self()
>      concurrently
>    - test that no_new_privs is implicitly enabled for sibling threads
>  - bump ABI version to v8
>  - documentation improvements
>    - document ABI v8
>    - move flag documentation into the landlock.h header
>    - comment: Explain why we do not need sighand->siglock or
>      cred_guard_mutex
>    - various comment improvements
>    - reminder above struct landlock_cred_security about updating
>      landlock_cred_copy on changes
> 
> v2:
>  - https://lore.kernel.org/all/20250221184417.27954-2-gnoack3000@gmail.com/
>  - Semantics:
>    - Threads implicitly set NO_NEW_PRIVS unless they have
>      CAP_SYS_ADMIN, to fulfill Landlock policy enforcement
>      prerequisites
>    - Landlock policy gets unconditionally overridden even if the
>      previously established Landlock domains in sibling threads were
>      diverging.
>  - Restructure discovery of all sibling threads, with the algorithm
>    proposed by Jann Horn [7]: Loop through threads multiple times, and
>    get them all stuck in the pseudo signal (task work), until no new
>    sibling threads show up.
>  - Use RCU lock when iterating over sibling threads.
>  - Override existing Landlock domains of other threads,
>    instead of applying a new Landlock policy on top
>  - Directly re-wire the struct cred for sibling threads,
>    instread of creating a new one with prepare_creds().
>  - Tests:
>    - Remove multi_threaded_failure test
>      (The only remaining failure case is ENOMEM,
>      there is no good way to provoke that in a selftest)
>    - Add test for success despite diverging Landlock domains.
> 
> [1] https://github.com/landlock-lsm/go-landlock
> [2] https://sites.google.com/site/fullycapable/who-ordered-libpsx
> [3] https://man.gnoack.org/7/nptl
> [4] https://man.gnoack.org/2/setuid#VERSIONS
> [5] https://lore.kernel.org/all/20240805-remove-cred-transfer-v2-0-a2aa1d45e6b8@google.com/
> [6] https://www.kernel.org/doc/html/latest/security/credentials.html
> [7] https://lore.kernel.org/all/CAG48ez0pWg3OTABfCKRk5sWrURM-HdJhQMcWedEppc_z1rrVJw@mail.gmail.com/
> 
> Günther Noack (3):
>   landlock: Multithreading support for landlock_restrict_self()
>   landlock: selftests for LANDLOCK_RESTRICT_SELF_TSYNC
>   landlock: Document LANDLOCK_RESTRICT_SELF_TSYNC
> 
>  Documentation/userspace-api/landlock.rst      |   8 +
>  include/uapi/linux/landlock.h                 |  13 +
>  security/landlock/Makefile                    |   2 +-
>  security/landlock/cred.h                      |  12 +
>  security/landlock/limits.h                    |   2 +-
>  security/landlock/syscalls.c                  |  66 ++-
>  security/landlock/tsync.c                     | 555 ++++++++++++++++++
>  security/landlock/tsync.h                     |  16 +
>  tools/testing/selftests/landlock/base_test.c  |   8 +-
>  tools/testing/selftests/landlock/tsync_test.c | 161 +++++
>  10 files changed, 810 insertions(+), 33 deletions(-)
>  create mode 100644 security/landlock/tsync.c
>  create mode 100644 security/landlock/tsync.h
>  create mode 100644 tools/testing/selftests/landlock/tsync_test.c
> 
> -- 
> 2.52.0.177.g9f829587af-goog
> 
> 

^ permalink raw reply

* Re: [PATCH v2 0/6] Landlock: Implement scope control for pathname Unix sockets
From: Justin Suess @ 2026-02-05 15:22 UTC (permalink / raw)
  To: mic
  Cc: demiobenour, fahimitahera, gnoack3000, gnoack, hi, jannh,
	linux-security-module, m, matthieu, utilityemal77
In-Reply-To: <20260204.vug7Osheiwou@digikod.net>

On 2/4/26 13:28, Mickaël Salaün wrote:

>> [...]
>> Tingmao:
>>
>> For connecting a pathname unix socket, the order of the hooks landlock sees is something like:
>>
>> 1.  security_unix_find. (to look up the paths)
>>
>> 2. security_unix_may_send, security_unix_stream_connect (after the path is looked up)
>>
>> Which for is called in DGRAM:
>>
>>  unix_dgram_connect OR  unix_dgram_sendmsg 
>>
>> and for STREAM:
>>
>>  unix_stream_connect
>>
>> IIRC, the path lookup only occurs in this order always, so in the logic as you have it the domain_is_scoped()
>> would be called twice, once from the security_unix_find when you call it in step two, and once from the
>> domain scope hooks. (If access was allowed from security_unix_find)
>>
>> There are a couple of things to consider.
>>
>> ---
>>
>> Audit blockers need special handling:
>>
>> Here's an example:
>>
>> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
>> access bit (deny all for RESOLVE_UNIX).
>>
>> 2. Program A connects to /tmp/mysock.sock ran by program B, which is outside the domain.
>>
>> 2. security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
>> /tmp/mysock.sock, so it calls domain_is_scoped
>>
>> 3. domain_is_scoped denies it as well, so now we must log an audit record.
>>
>> When logging the denial, we have to include both blockers "scope.unix_socket"  and "fs.resolve_unix" for the
>> denial, because it is the absence of both that caused the denial. I think the refer right has similar cases for auditing, so there is precedent for this (multiple blockers for an audit message).
> That's a good point, and it would give more informations to diagnose
> issues.  However, being able to identify if both accesses are denied
> would require to check both, whereas the first is enough to know that
> Landlock denies the access.  So, if we can return both records without
> continuing the security checks, that's good, otherwise we should stop
> ASAP and return the error.

Maybe I'm missing something, but if the flags interact in an "OR" manner
wouldn't we need to check both? In your proposal where RESOLVE_UNIX
implies the scoped flag, if a program connects to a unix socket that is within
the domain but does not have an explicit RESOLVE_UNIX exception, we must
still check for the case that the access is scoped.

---

(Given LANDLOCK_ACCESS_FS_RESOLVE_UNIX and LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
are set)

case 1: access to socket within domain and no RESOLVE_UNIX rule covers the access

We check first in security_unix_find hook and find there is no rule allowing the access.
After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
we then check is_domain_scoped, the check allows the access.

case 2: access to socket outside domain but RESOLVE_UNIX rule covers the access

We check first in security_unix_find hook and find there is a rule allowing the access.
We can allow the access early (short-circuit eval) without calling is_domain_scoped.

case 4: access to socket inside domain and RESOLVE_UNIX covers the access

We check first in security_unix_find hook and find there is a rule allowing the access.
We can allow the access early (short-circuit eval) without calling is_domain_scoped. (same as case 2)


case 4: access to socket outside domain and no RESOLVE_UNIX covers the access

We check first in security_unix_find hook and find there is no rule allowing the access.
After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
we then check is_domain_scoped, the check does not allow the access. (it is the combination
of the two checks failing that denied the access).

---

Case 4 is what I'm specifically considering would need to have both blockers listed in a denial audit.
We can't short circuit in that case because we have to check the scoping before denying.
Let me know if I'm misunderstanding this.

(PS: IIRC the hooks used by the LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
will never be hit if the check in security_unix_find fails. So some logic to check this
access in security_unix_find will be needed).

> Anyway, that might not be needed if we end up with my latest proposal
> about always setting scope.unix_socket when fs.resolve_unix is set.
>
>> ---
>>
>> Dual lookup for domain_is_scoped. Consider this case:
>>
>> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
>> access bit (deny all for RESOLVE_UNIX).
>>
>> 2. Program A connects to Program C's /tmp/foo.sock, which for the purposes of this example is in the domain of program A.
>>
>> 3.  security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
>> /tmp/mysock.sock, so it calls domain_is_scoped. Access is granted, and continues. (LSM hook complete)
>>
>> 4.  The connection proceeds past the path lookup stage, and now security_unix_may_send, or security_unix_stream_connect gets called. This requires ANOTHER domain_is_scoped access check.
>>
>> While I don't THINK this introduces a TOCTOU, it is a little confusing.
>>
>> This does mean that we look up the domain twice, if this is implemented naively. I think we can then just
>> skip the task credential checks then for security_unix_may_send and security_unix_stream_connect **for
>> connecting to pathname sockets**, since the domain_is_scoped will already have been called in landlock's
>> security_unix_find hook, eliminating the need for handling pathname socket domain checks layer on.
>>
>>>> I definitely agree that it is tricky, but making same-scope access be
>>>> allowed (i.e. the suggested idea above) would only get rid of step 1,
>>>> which I think is the "simpler" bit.  The extra logic in step 2 is still
>>>> needed. 
>>>>
>>>> I definitely agree with pro1 tho.
>>> Yes, you are describing the logic for the variant where
>>> LANDLOCK_ACCESS_FS_RESOLVE_UNIX does not implicitly permit access from
>>> within the same scope.  In that variant, there can be situations where
>>> the first hook can deny the action immediately.
>>>
>>> In the variant where LANDLOCK_ACCESS_FS_RESOLVE_UNIX *does* implicitly
>>> allow access from within the same scope, that shortcutting is not
>>> possible.  On the upside however, there is no need to distinguish
>>> whether the scope flag is set when we are in the security_unix_find()
>>> hook, because access from within the same scope is always permitted.
>>> (That is the simplification I meant.)
>>>
>>>
>>>>> AGAINST:
>>>>>
>>>>> (con1) It would work differently than the other scoped access rights
>>>>>        that we already have.
>>>>>
>>>>>        A speculative feature that could potentially be built with the
>>>>>        scoped access rights is that we could add a rule to permit IPC
>>>>>        to other Landlock scopes, e.g. introducing a new rule type
>>>>>
>>>>>          struct landlock_scope_attr {
>>>>>            __u64 allowed_access;  /* for "scoped" bits */
>>>>>            /* some way to identify domains */
>>>>>          }
>>>>>
>>>>>        so that we could make IPC access to other Landlock domains
>>>>>        configurable.
>>>>>
>>>>>        If the scoped bit and the FS RESOLVE_UNIX bit were both
>>>>>        conflated in RESOLVE_UNIX, it would not be possible to make
>>>>>        UNIX connections configurable in such a way.
>>>> This exact API would no longer work, but if we give up the equivalence
>>>> between scope bits and the landlock_scope_attr struct, then we can do
>>>> something like:
>>>>
>>>> struct landlock_scope_attr {
>>>>     __u64 ptrace:1;  /* Note that this is not a (user controllable) scope bit! */
>>>>     __u64 abstract_unix_socket:1;
>>>>     __u64 pathname_unix_socket:1;
>>>>     /* ... */
>>>>
>>>>     __u64 allowed_signals;
>>>>
>>>>     /*
>>>>      * some way to identify domains, maybe we could use the audit domain
>>>>      * ID, with 0 denoting "allow access to non-Landlocked processes?
>>>>      */
>>>> }
>>> Yes, it would be possible to use such a struct for that scenario where
>>> IPC access gets allowed for other Landlock scopes.  It would mean that
>>> we would not need to introduce a scoped flag for the pathname UNIX
>>> socket connections.  But the relationship between that struct
>>> landlock_scope_attr and the flags and access rights in struct
>>> landlock_ruleset_attr would become less clear, which is a slight
>>> downside, and maybe error prone for users to work with.
>>>
>>> If we introduced an additional scoped flag, it would also be
>>> consistent though.
>>>
>>> (con1) was written under the assumption that we do not have an
>>> additional scoped flag.  If that is lacking, it is not possible to
>>> express UNIX connect() access to other Landlock domains with that
>>> struct.  But as outlined in the proposal below, if we *do* (later)
>>> introduce the additional scoped flag *in addition* to the FS access
>>> right, this *both* stays consistent in semantics with the signal and
>>> abstract UNIX support, *and* it starts working in a world where ICP
>>> access can be allowed to talk to other Landlock domains.
>>>
>>>>> (con2) Consistent behaviour between scoped flags and their
>>>>>        interactions with other access rights:
>>>>>
>>>>>        The existing scoped access rights (signal, abstract sockets)
>>>>>        could hypothetically be extended with a related access right of
>>>>>        another type. For instance, there could be an access right type
>>>>>
>>>>>          __u64 handled_signal_number;
>>>>>
>>>>>        and then you could add a rule to permit the use of certain
>>>>>        signal numbers.  The interaction between the scoped flags and
>>>>>        other access rights should work the same.
>>>>>
>>>>>
>>>>> Constructive Proposal for consideration: Why not both?
>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> I will think about the following a bit more but I'm afraid that I feel
>>>> like it might get slightly confusing.  With this, the only reason for
>>>> having LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is for API consistency when we
>>>> later enable allowing access to other domains (if I understood correctly),
>>>> in which case I personally feel like the suggestion on landlock_scope_attr
>>>> above, where we essentially accept that it is decoupled with the scope
>>>> bits in the ruleset, might be simpler...?
>>> Mickaël expressed the opinion to me that he would like to APIs to stay
>>> consistent between signals, abstract UNIX sockets, named UNIX sockets
>>> and other future "scoped" operations, in scenarios where:
>>>
>>> * the "scoped" (IPC) operations can be configured to give access to
>>>   other Landlock domains (and that should work for UNIX connections too)
>>> * the existing "scoped" operations also start having matching access rights
>>>
>>> I think with the way I proposed, that would be consistent.
>>>
>>>
>>>>> Why not do both what Tingmao proposed in [1] **and** reserve the
>>>>> option to add the matching "scoped flag" later?
>>>>>
>>>>>   * Introduce LANDLOCK_ACCESS_FS_RESOLVE_UNIX.
>>>>>
>>>>>     If it is handled, UNIX connections are allowed either:
>>>>>
>>>>>     (1) if the connection is to a service in the same scope, or
>>>>>     (2) if the path was allow-listed with a "path beneath" rule.
>>>>>
>>>>>   * Add LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET later, if needed.
>>>>>
>>>>>
>>>>> Let's go through the arguments again:
>>>>>
>>>>> We have observed that it is harmless to allow connections to services
>>>>> in the same scope (1), and that if users absolutely don't want that,
>>>>> they can actually prohibit it through LANDLOCK_ACCESS_FS_MAKE_SOCK
>>>>> (pro1).
>>>>>
>>>>> (con1): Can we still implement the feature idea where we poke a hole
>>>>>         to get UNIX-connect() access to other Landlock domains?
>>>>>
>>>>>   I think the answer is yes.  The implementation strategy is:
>>>>>
>>>>>     * Add the scoped bit LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
>>>>>     * The scoped bit can now be used to allow-list connections to
>>>>>       other Landlock domains.
>>>>>
>>>>>   For users, just setting the scoped bit on its own does the same as
>>>>>   handling LANDLOCK_ACCESS_FS_RESOLVE_UNIX.  That way, the kernel-side
>>>>>   implementation can also stay simple.  The only reason why the scoped
>>>>>   bit is needed is because it makes it possible to allow-list
>>>>>   connections to other Landlock domains, but at the same time, it is
>>>>>   safe if libraries set the scoped bit once it exists, as it does not
>>>>>   have any bad runtime impact either.
>>>>>
>>>>> (con2): Consistency: Do all the scoped flags interact with their
>>>>>         corresponding access rights in the same way?
>>>>>
>>>>>   The other scope flags do not have corresponding access rights, so
>>>>>   far.
>>>>>
>>>>>   If we were to add corresponding access rights for the other scope
>>>>>   flags, I would argue that we could apply a consistent logic there,
>>>>>   because IPC access within the same scope is always safe:
>>>>>
>>>>>   - A hypothetical access right type for "signal numbers" would only
>>>>>     restrict signals that go beyond the current scope.
>>>>>
>>>>>   - A hypothetical access right type for "abstract UNIX domain socket
>>>>>     names" would only restrict connections to abstract UNIX domain
>>>>>     servers that go beyond the current scope.
>>>>>
>>>>>   I can not come up with a scenario where this doesn't work.
>> Gunther / Tingmao / Mickaël:
>>
>> I have a potential idea to make this concept cleaner.
>>
>> The docs for landlock currently say:
>>
>>
>>        IPC scoping does not support exceptions via landlock_add_rule(2).
>>        If an operation is scoped within a domain, no rules can be added
>>        to allow access to resources or processes outside of the scope.
> This part might indeed be confusing.  The idea was to explain the
> difference between scoped rights and handled access rights (which may
> have rules).
>
>> So if we go with the solution where we are now saying IPC scoping DOES support exceptions
>> we will need to update the documentation, to say scoping for pathname unix sockets is an exception,
>> and have to have the "exemptible scopes" (like this one) alongside "non-exemptible" scopes
>> (ie the existing ones). This creates some friction for users.
> The documentation will definitely require some updates.  I think it can
> be explained in a simple way.
>
>> If we foresee other "exempt-able scopes" (which are scopes that also support creating exemptions w/ corresponding access rights) in the future, maybe we should consider separating the two in the ruleset
>> attributes (I used scoped_fs as an example for the attribute name):
>>
>> structlandlock_ruleset_attrruleset_attr={
>> .handled_access_fs=
>> LANDLOCK_ACCESS_FS_EXECUTE|
>> LANDLOCK_ACCESS_FS_WRITE_FILE|
>> LANDLOCK_ACCESS_FS_READ_FILE|
>> LANDLOCK_ACCESS_FS_READ_DIR|
>> LANDLOCK_ACCESS_FS_REMOVE_DIR|
>> LANDLOCK_ACCESS_FS_REMOVE_FILE|
>> LANDLOCK_ACCESS_FS_MAKE_CHAR|
>> LANDLOCK_ACCESS_FS_MAKE_DIR|
>> LANDLOCK_ACCESS_FS_MAKE_REG|
>> LANDLOCK_ACCESS_FS_MAKE_SOCK|
>> LANDLOCK_ACCESS_FS_MAKE_FIFO|
>> LANDLOCK_ACCESS_FS_MAKE_BLOCK|
>> LANDLOCK_ACCESS_FS_MAKE_SYM|
>> LANDLOCK_ACCESS_FS_REFER|
>> LANDLOCK_ACCESS_FS_TRUNCATE|
>> LANDLOCK_ACCESS_FS_IOCTL_DEV,
>> .handled_access_net=
>> LANDLOCK_ACCESS_NET_BIND_TCP|
>> LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> .scoped=
>> LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET|
>> LANDLOCK_SCOPE_SIGNAL,
>>     .scoped_fs = 
>> 	LANDLOCK_SCOPE_FS_PATHNAME_UNIX_SOCKET
>> };
>>
>> This more clearly distinguishes between scopes that have exceptions/corresponding fs rights,
>> and ones that don't. Later we could add scoped_net, if needed. I feel like this would be more
>> intuitive and better categorize future scoping rights. An obvious con is increasing the size of
>> the ruleset attributes.
> I see your point but I don't think it would be worth it to add
> sub-scoped fields.  Each field has a clear semantic, and the scoped one
> is related to the domain wrt other domains.
As long as it's documented clearly, and future IPCs have similar behavior
I agree that a separate field probably isn't needed.
>> Of course this separation is only worth it if there are other "exempt-able" rights in the future.
>> I can think of a few potential future rights which COULD be scoped and have corresponding rights
>> (binder, sysv-ipc, pipes, tcp/udp between two local programs). 
> Yes, it would definitely be useful to add exception for other kind of
> IPCs.  The idea would be to be able to describe the peer, either with a
> file path, or PID, or cgroups, or a Landlock domain...  The inet case
> is an interesting idea but that might be a challenging task to
> implement, if even possible.

>>>>> In conclusion, I think the approach has significant upsides:
>>>>>
>>>>>   * Simpler UAPI: Users only have one access bit to deal with, in the
>>>>>     near future.  Once we do add a scope flag for UNIX connections, it
>>>>>     does not interact in a surprising way with the corresponding FS
>>>>>     access right, because with either of these, scoped access is
>>>>>     allowed.
>>>>>
>>>>>     If users absolutely need to restrict scoped access, they can
>>>>>     restrict LANDLOCK_ACCESS_FS_MAKE_SOCK.  It is a slightly obscure
>>>>>     API, but in line with the "make easy things easy, make hard things
>>>>>     possible" API philosophy.  And needing this should be the
>>>>>     exception rather than the norm, after all.
>>>>>
>>>>>   * Consistent behaviour between scoped flags and regular access
>>>>>     rights, also for speculative access rights affecting the existing
>>>>>     scoped flags for signals and abstract UNIX domain sockets.
>>>>>
>>>>> [1] https://lore.kernel.org/all/f07fe41a-96c5-4d3a-9966-35b30b3a71f1@maowtm.org/
>>> —Günther


^ permalink raw reply

* Re: [PATCH 1/5] export file_close_fd and task_work_add
From: Alice Ryhl @ 2026-02-05 13:45 UTC (permalink / raw)
  To: Lorenzo Stoakes
  Cc: Greg Kroah-Hartman, Carlos Llamas, Alexander Viro,
	Christian Brauner, Jan Kara, Paul Moore, James Morris,
	Serge E. Hallyn, Andrew Morton, Dave Chinner, Qi Zheng,
	Roman Gushchin, Muchun Song, David Hildenbrand, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	kernel-team, linux-fsdevel, linux-kernel, linux-security-module,
	linux-mm, rust-for-linux
In-Reply-To: <9a037fdf-1a98-437f-8b80-7fdc53d5b0fa@lucifer.local>

On Thu, Feb 05, 2026 at 11:53:19AM +0000, Lorenzo Stoakes wrote:
> On Thu, Feb 05, 2026 at 11:42:46AM +0000, Alice Ryhl wrote:
> > On Thu, Feb 05, 2026 at 11:20:33AM +0000, Lorenzo Stoakes wrote:
> > > On Thu, Feb 05, 2026 at 10:51:26AM +0000, Alice Ryhl wrote:
> > > > This exports the functionality needed by Binder to close file
> > > > descriptors.
> > > >
> > > > When you send a fd over Binder, what happens is this:
> > > >
> > > > 1. The sending process turns the fd into a struct file and stores it in
> > > >    the transaction object.
> > > > 2. When the receiving process gets the message, the fd is installed as a
> > > >    fd into the current process.
> > > > 3. When the receiving process is done handling the message, it tells
> > > >    Binder to clean up the transaction. As part of this, fds embedded in
> > > >    the transaction are closed.
> > > >
> > > > Note that it was not always implemented like this. Previously the
> > > > sending process would install the fd directly into the receiving proc in
> > > > step 1, but as discussed previously [1] this is not ideal and has since
> > > > been changed so that fd install happens during receive.
> > > >
> > > > The functions being exported here are for closing the fd in step 3. They
> > > > are required because closing a fd from an ioctl is in general not safe.
> > > > This is to meet the requirements for using fdget(), which is used by the
> > > > ioctl framework code before calling into the driver's implementation of
> > > > the ioctl. Binder works around this with this sequence of operations:
> > > >
> > > > 1. file_close_fd()
> > > > 2. get_file()
> > > > 3. filp_close()
> > > > 4. task_work_add(current, TWA_RESUME)
> > > > 5. <binder returns from ioctl>
> > > > 6. fput()
> > > >
> > > > This ensures that when fput() is called in the task work, the fdget()
> > > > that the ioctl framework code uses has already been fdput(), so if the
> > > > fd being closed happens to be the same fd, then the fd is not closed
> > > > in violation of the fdget() rules.
> > >
> > > I'm not really familiar with this mechanism but you're already talking about
> > > this being a workaround so strikes me the correct thing to do is to find a way
> > > to do this in the kernel sensibly rather than exporting internal implementation
> > > details and doing it in binder.
> >
> > I did previously submit a patch that implemented this logic outside of
> > Binder, but I was advised to move it into Binder.
> 
> Right yeah that's just odd to me, we really do not want to be adding internal
> implementation details to drivers.
> 
> This is based on bitter experience of bugs being caused by drivers abusing every
> interface they get, which is basically exactly what always happens, sadly.
> 
> And out-of-tree is heavily discouraged.
> 
> Also can we use EXPORT_SYMBOL_FOR_MODULES() for anything we do need to export to
> make it explicitly only for binder, perhaps?
> 
> >
> > But I'm happy to submit a patch to extract this logic into some sort of
> > close_fd_safe() method that can be called even if said fd is currently
> > held using fdget().
> 
> Yup, especially given Christian's view on the kernel task export here I think
> that's a more sensible approach.
> 
> But obviously I defer the sensible-ness of this to him as I am but an mm dev :)

Quick sketch of how this would look:

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index adde1e40cccd..6fb7175ff69b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -64,7 +64,6 @@
 #include <linux/spinlock.h>
 #include <linux/ratelimit.h>
 #include <linux/syscalls.h>
-#include <linux/task_work.h>
 #include <linux/sizes.h>
 #include <linux/ktime.h>
 
@@ -1962,68 +1961,6 @@ static bool binder_validate_fixup(struct binder_proc *proc,
 	return (fixup_offset >= last_min_offset);
 }
 
-/**
- * struct binder_task_work_cb - for deferred close
- *
- * @twork:                callback_head for task work
- * @file:                 file to close
- *
- * Structure to pass task work to be handled after
- * returning from binder_ioctl() via task_work_add().
- */
-struct binder_task_work_cb {
-	struct callback_head twork;
-	struct file *file;
-};
-
-/**
- * binder_do_fd_close() - close list of file descriptors
- * @twork:	callback head for task work
- *
- * It is not safe to call ksys_close() during the binder_ioctl()
- * function if there is a chance that binder's own file descriptor
- * might be closed. This is to meet the requirements for using
- * fdget() (see comments for __fget_light()). Therefore use
- * task_work_add() to schedule the close operation once we have
- * returned from binder_ioctl(). This function is a callback
- * for that mechanism and does the actual ksys_close() on the
- * given file descriptor.
- */
-static void binder_do_fd_close(struct callback_head *twork)
-{
-	struct binder_task_work_cb *twcb = container_of(twork,
-			struct binder_task_work_cb, twork);
-
-	fput(twcb->file);
-	kfree(twcb);
-}
-
-/**
- * binder_deferred_fd_close() - schedule a close for the given file-descriptor
- * @fd:		file-descriptor to close
- *
- * See comments in binder_do_fd_close(). This function is used to schedule
- * a file-descriptor to be closed after returning from binder_ioctl().
- */
-static void binder_deferred_fd_close(int fd)
-{
-	struct binder_task_work_cb *twcb;
-
-	twcb = kzalloc(sizeof(*twcb), GFP_KERNEL);
-	if (!twcb)
-		return;
-	init_task_work(&twcb->twork, binder_do_fd_close);
-	twcb->file = file_close_fd(fd);
-	if (twcb->file) {
-		// pin it until binder_do_fd_close(); see comments there
-		get_file(twcb->file);
-		filp_close(twcb->file, current->files);
-		task_work_add(current, &twcb->twork, TWA_RESUME);
-	} else {
-		kfree(twcb);
-	}
-}
-
 static void binder_transaction_buffer_release(struct binder_proc *proc,
 					      struct binder_thread *thread,
 					      struct binder_buffer *buffer,
@@ -2183,7 +2120,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
 						offset, sizeof(fd));
 				WARN_ON(err);
 				if (!err) {
-					binder_deferred_fd_close(fd);
+					/*
+					 * Intentionally ignore EBADF errors here.
+					 */
+					close_fd_safe(fd, GFP_KERNEL | __GFP_NOFAIL);
 					/*
 					 * Need to make sure the thread goes
 					 * back to userspace to complete the
diff --git a/fs/file.c b/fs/file.c
index 0a4f3bdb2dec..58e3825e846c 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -21,6 +21,7 @@
 #include <linux/rcupdate.h>
 #include <linux/close_range.h>
 #include <linux/file_ref.h>
+#include <linux/task_work.h>
 #include <net/sock.h>
 #include <linux/init_task.h>
 
@@ -1525,3 +1526,47 @@ int iterate_fd(struct files_struct *files, unsigned n,
 	return res;
 }
 EXPORT_SYMBOL(iterate_fd);
+
+struct close_fd_safe_task_work {
+	struct callback_head twork;
+	struct file *file;
+};
+
+static void close_fd_safe_callback(struct callback_head *twork)
+{
+	struct close_fd_safe_task_work *twcb = container_of(twork,
+			struct close_fd_safe_task_work, twork);
+
+	fput(twcb->file);
+	kfree(twcb);
+}
+
+/**
+ * close_fd_safe - close the given fd
+ * @fd: file descriptor to close
+ * @flags: gfp flags for allocation of task work
+ *
+ * This closes an fd. Unlike close_fd(), this may be used even if the fd is
+ * currently held with fdget().
+ *
+ * Returns: 0 or an error code
+ */
+int close_fd_safe(unsigned int fd, gfp_t flags)
+{
+	struct close_fd_safe_task_work *twcb;
+
+	twcb = kzalloc(sizeof(*twcb), flags);
+	if (!twcb)
+		return -ENOMEM;
+	init_task_work(&twcb->twork, close_fd_safe_callback);
+	twcb->file = file_close_fd(fd);
+	if (!twcb->file) {
+		kfree(twcb);
+		return -EBADF;
+	}
+
+	get_file(twcb->file);
+	filp_close(twcb->file, current->files);
+	task_work_add(current, &twcb->twork, TWA_RESUME);
+	return 0;
+}
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index c45306a9f007..1c99a56c0cdf 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -111,6 +111,7 @@ int iterate_fd(struct files_struct *, unsigned,
 		const void *);
 
 extern int close_fd(unsigned int fd);
+extern int close_fd_safe(unsigned int fd, gfp_t flags);
 extern struct file *file_close_fd(unsigned int fd);
 
 extern struct kmem_cache *files_cachep;

^ permalink raw reply related

* Re: [PATCH v14 2/9] rust: rename `AlwaysRefCounted` to `RefCounted`.
From: Gary Guo @ 2026-02-05 13:33 UTC (permalink / raw)
  To: Gary Guo, Andreas Hindborg, Miguel Ojeda, Boqun Feng,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
	Leon Romanovsky, Paul Moore, Serge Hallyn, Rafael J. Wysocki,
	David Airlie, Simona Vetter, Alexander Viro, Christian Brauner,
	Jan Kara, Igor Korotin, Daniel Almeida, Lorenzo Stoakes,
	Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Bjorn Helgaas, Krzysztof Wilczyński
  Cc: linux-kernel, rust-for-linux, linux-block, linux-security-module,
	dri-devel, linux-fsdevel, linux-mm, linux-pm, linux-pci,
	Oliver Mangold
In-Reply-To: <DG72CY2P36F9.2O7OIN36KW8F8@garyguo.net>

On Thu Feb 5, 2026 at 1:31 PM GMT, Gary Guo wrote:
> On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
>> From: Oliver Mangold <oliver.mangold@pm.me>
>> 
>> There are types where it may both be reference counted in some cases and
>> owned in others. In such cases, obtaining `ARef<T>` from `&T` would be
>> unsound as it allows creation of `ARef<T>` copy from `&Owned<T>`.
>> 
>> Therefore, we split `AlwaysRefCounted` into `RefCounted` (which `ARef<T>`
>> would require) and a marker trait to indicate that the type is always
>> reference counted (and not `Ownable`) so the `&T` -> `ARef<T>` conversion
>> is possible.
>> 
>> - Rename `AlwaysRefCounted` to `RefCounted`.
>> - Add a new unsafe trait `AlwaysRefCounted`.
>> - Implement the new trait `AlwaysRefCounted` for the newly renamed
>>   `RefCounted` implementations. This leaves functionality of existing
>>   implementers of `AlwaysRefCounted` intact.
>> 
>> Original patch by Oliver Mangold <oliver.mangold@pm.me> [1].
>> 
>> Link: https://lore.kernel.org/r/20251117-unique-ref-v13-2-b5b243df1250@pm.me [1]
>> Suggested-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> I think you also need to update the `AlwaysRefCounted` reference mentioned in
> the `Owned` patch too? (Or perhaps this patch should be moved before `Owned`
> instead?)

Actually I re-read the comment in first patch, the text indeed should refer to
`AlwaysRefCounted`. Please disregard this comment.

Best,
Gary

>
> With that fixed:
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
>
>> ---
>>  rust/kernel/auxiliary.rs        |  7 +++++-
>>  rust/kernel/block/mq/request.rs | 15 +++++++------
>>  rust/kernel/cred.rs             | 13 ++++++++++--
>>  rust/kernel/device.rs           | 10 ++++++---
>>  rust/kernel/device/property.rs  |  7 +++++-
>>  rust/kernel/drm/device.rs       | 10 ++++++---
>>  rust/kernel/drm/gem/mod.rs      |  8 ++++---
>>  rust/kernel/fs/file.rs          | 16 ++++++++++----
>>  rust/kernel/i2c.rs              | 16 +++++++++-----
>>  rust/kernel/mm.rs               | 15 +++++++++----
>>  rust/kernel/mm/mmput_async.rs   |  9 ++++++--
>>  rust/kernel/opp.rs              | 10 ++++++---
>>  rust/kernel/owned.rs            |  2 +-
>>  rust/kernel/pci.rs              | 10 ++++++++-
>>  rust/kernel/pid_namespace.rs    | 12 +++++++++--
>>  rust/kernel/platform.rs         |  7 +++++-
>>  rust/kernel/sync/aref.rs        | 47 ++++++++++++++++++++++++++---------------
>>  rust/kernel/task.rs             | 10 ++++++---
>>  rust/kernel/types.rs            |  3 ++-
>>  19 files changed, 164 insertions(+), 63 deletions(-)


^ permalink raw reply

* Re: [PATCH v14 2/9] rust: rename `AlwaysRefCounted` to `RefCounted`.
From: Gary Guo @ 2026-02-05 13:31 UTC (permalink / raw)
  To: Andreas Hindborg, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
	Leon Romanovsky, Paul Moore, Serge Hallyn, Rafael J. Wysocki,
	David Airlie, Simona Vetter, Alexander Viro, Christian Brauner,
	Jan Kara, Igor Korotin, Daniel Almeida, Lorenzo Stoakes,
	Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Bjorn Helgaas, Krzysztof Wilczyński
  Cc: linux-kernel, rust-for-linux, linux-block, linux-security-module,
	dri-devel, linux-fsdevel, linux-mm, linux-pm, linux-pci,
	Oliver Mangold
In-Reply-To: <20260204-unique-ref-v14-2-17cb29ebacbb@kernel.org>

On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
> From: Oliver Mangold <oliver.mangold@pm.me>
> 
> There are types where it may both be reference counted in some cases and
> owned in others. In such cases, obtaining `ARef<T>` from `&T` would be
> unsound as it allows creation of `ARef<T>` copy from `&Owned<T>`.
> 
> Therefore, we split `AlwaysRefCounted` into `RefCounted` (which `ARef<T>`
> would require) and a marker trait to indicate that the type is always
> reference counted (and not `Ownable`) so the `&T` -> `ARef<T>` conversion
> is possible.
> 
> - Rename `AlwaysRefCounted` to `RefCounted`.
> - Add a new unsafe trait `AlwaysRefCounted`.
> - Implement the new trait `AlwaysRefCounted` for the newly renamed
>   `RefCounted` implementations. This leaves functionality of existing
>   implementers of `AlwaysRefCounted` intact.
> 
> Original patch by Oliver Mangold <oliver.mangold@pm.me> [1].
> 
> Link: https://lore.kernel.org/r/20251117-unique-ref-v13-2-b5b243df1250@pm.me [1]
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

I think you also need to update the `AlwaysRefCounted` reference mentioned in
the `Owned` patch too? (Or perhaps this patch should be moved before `Owned`
instead?)

With that fixed:

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/auxiliary.rs        |  7 +++++-
>  rust/kernel/block/mq/request.rs | 15 +++++++------
>  rust/kernel/cred.rs             | 13 ++++++++++--
>  rust/kernel/device.rs           | 10 ++++++---
>  rust/kernel/device/property.rs  |  7 +++++-
>  rust/kernel/drm/device.rs       | 10 ++++++---
>  rust/kernel/drm/gem/mod.rs      |  8 ++++---
>  rust/kernel/fs/file.rs          | 16 ++++++++++----
>  rust/kernel/i2c.rs              | 16 +++++++++-----
>  rust/kernel/mm.rs               | 15 +++++++++----
>  rust/kernel/mm/mmput_async.rs   |  9 ++++++--
>  rust/kernel/opp.rs              | 10 ++++++---
>  rust/kernel/owned.rs            |  2 +-
>  rust/kernel/pci.rs              | 10 ++++++++-
>  rust/kernel/pid_namespace.rs    | 12 +++++++++--
>  rust/kernel/platform.rs         |  7 +++++-
>  rust/kernel/sync/aref.rs        | 47 ++++++++++++++++++++++++++---------------
>  rust/kernel/task.rs             | 10 ++++++---
>  rust/kernel/types.rs            |  3 ++-
>  19 files changed, 164 insertions(+), 63 deletions(-)


^ permalink raw reply

* Re: [PATCH v14 1/9] rust: types: Add Ownable/Owned types
From: Gary Guo @ 2026-02-05 13:29 UTC (permalink / raw)
  To: Andreas Hindborg, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Alice Ryhl, Trevor Gross,
	Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman, Ira Weiny,
	Leon Romanovsky, Paul Moore, Serge Hallyn, Rafael J. Wysocki,
	David Airlie, Simona Vetter, Alexander Viro, Christian Brauner,
	Jan Kara, Igor Korotin, Daniel Almeida, Lorenzo Stoakes,
	Liam R. Howlett, Viresh Kumar, Nishanth Menon, Stephen Boyd,
	Bjorn Helgaas, Krzysztof Wilczyński
  Cc: linux-kernel, rust-for-linux, linux-block, linux-security-module,
	dri-devel, linux-fsdevel, linux-mm, linux-pm, linux-pci,
	Asahi Lina
In-Reply-To: <20260204-unique-ref-v14-1-17cb29ebacbb@kernel.org>

On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
> From: Asahi Lina <lina+kernel@asahilina.net>
>
> By analogy to `AlwaysRefCounted` and `ARef`, an `Ownable` type is a
> (typically C FFI) type that *may* be owned by Rust, but need not be. Unlike
> `AlwaysRefCounted`, this mechanism expects the reference to be unique
> within Rust, and does not allow cloning.
>
> Conceptually, this is similar to a `KBox<T>`, except that it delegates
> resource management to the `T` instead of using a generic allocator.
>
> This change is a derived work based on work by Asahi Lina
> <lina+kernel@asahilina.net> [1] and Oliver Mangold <oliver.mangold@pm.me>.
>
> Link: https://lore.kernel.org/rust-for-linux/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/ [1]
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/lib.rs       |   1 +
>  rust/kernel/owned.rs     | 196 +++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/sync/aref.rs |   5 ++
>  rust/kernel/types.rs     |  11 ++-
>  4 files changed, 212 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index f812cf1200428..96a3fadc3377a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -119,6 +119,7 @@
>  pub mod of;
>  #[cfg(CONFIG_PM_OPP)]
>  pub mod opp;
> +pub mod owned;
>  pub mod page;
>  #[cfg(CONFIG_PCI)]
>  pub mod pci;
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> new file mode 100644
> index 0000000000000..fe30580331df9
> --- /dev/null
> +++ b/rust/kernel/owned.rs
> <snip>
> +
> +    /// Get a pinned mutable reference to the data owned by this `Owned<T>`.
> +    pub fn get_pin_mut(&mut self) -> Pin<&mut T> {
> +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> +        // return a mutable reference to it.
> +        let unpinned = unsafe { self.ptr.as_mut() };
> +
> +        // SAFETY: We never hand out unpinned mutable references to the data in
> +        // `Self`, unless the contained type is `Unpin`.
> +        unsafe { Pin::new_unchecked(unpinned) }
> +    }

Probably should be name `as_pin_mut` instead.

With name changed and SOB fixed:

Reviewed-by: Gary Guo <gary@garyguo.net>

Best,
Gary

> +}
> +
> +// SAFETY: It is safe to send an [`Owned<T>`] to another thread when the underlying `T` is [`Send`],
> +// because of the ownership invariant. Sending an [`Owned<T>`] is equivalent to sending the `T`.
> +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> +
> +// SAFETY: It is safe to send [`&Owned<T>`] to another thread when the underlying `T` is [`Sync`],
> +// because of the ownership invariant. Sending an [`&Owned<T>`] is equivalent to sending the `&T`.
> +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> +

^ permalink raw reply

* Re: [PATCH 5/5] rust_binder: mark ANDROID_BINDER_IPC_RUST tristate
From: Gary Guo @ 2026-02-05 13:21 UTC (permalink / raw)
  To: Alice Ryhl, Greg Kroah-Hartman, Carlos Llamas
  Cc: Alexander Viro, Christian Brauner, Jan Kara, Paul Moore,
	James Morris, Serge E. Hallyn, Andrew Morton, Dave Chinner,
	Qi Zheng, Roman Gushchin, Muchun Song, David Hildenbrand,
	Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Miguel Ojeda, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Trevor Gross, Danilo Krummrich, kernel-team, linux-fsdevel,
	linux-kernel, linux-security-module, linux-mm, rust-for-linux
In-Reply-To: <20260205-binder-tristate-v1-5-dfc947c35d35@google.com>

On Thu Feb 5, 2026 at 10:51 AM GMT, Alice Ryhl wrote:
> Currently Binder only builds as built-in module, but in downstream
> Android branches we update the build system to make Rust Binder
> buildable as a module. The same situation applies to distros, as there
> are many distros that enable Binder for support of apps such as
> waydroid, which would benefit from the ability to build Binder as a
> module.
>
> Note that although the situation in Android may be temporary - once we
> no longer have a C implementation, it makes sense for Rust Binder to be
> built-in. But that will both take a while, and in any case, distros
> enabling Binder will benefit from it being a module even if Android goes
> back to built-in.
>
> This doesn't mark C Binder buildable as a module. That would require
> more intrusive Makefile changes as it's built from multiple objects, and
> I'm not sure there's any way to produce a file called 'binder.ko'
> containing all of those objects linked together without renaming
> 'binder.c', as right now there will be naming conflicts between the
> object built from binder.c, and the object that results from linking
> binder.o,binderfs.o,binder_alloc.o and so on together. (As an aside,
> this issue is why the Rust Binder entry-point is called
> rust_binder_main.rs instead of just rust_binder.rs)
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  drivers/android/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> index e2e402c9d1759c81591473ad02ab7ad011bc61d0..3c1755e53195b0160d0ed244f078eed96e16272c 100644
> --- a/drivers/android/Kconfig
> +++ b/drivers/android/Kconfig
> @@ -15,7 +15,7 @@ config ANDROID_BINDER_IPC
>  	  between said processes.
>  
>  config ANDROID_BINDER_IPC_RUST
> -	bool "Rust version of Android Binder IPC Driver"
> +	tristate "Rust version of Android Binder IPC Driver"
>  	depends on RUST && MMU && !ANDROID_BINDER_IPC
>  	help
>  	  This enables the Rust implementation of the Binder driver.

Hi Alice,

AFAIK Rust binder doesn't specifically handle module unloading, so global
statics (e.g. CONTEXTS doesn't get dropped).

If we're going to build Binder as module, we need to ensure that we have the
mechanism in the module macro to prevent unloading of Binder.

Best,
Gary

^ permalink raw reply

* Re: [PATCH] man/man7/kernel_lockdown.7: remove Secure Boot untruth
From: Alejandro Colomar @ 2026-02-05 13:10 UTC (permalink / raw)
  To: Xiu Jianfeng
  Cc: Alyssa Ross, Heinrich Schuchardt, David Howells,
	Nicolas Bouchinet, linux-security-module, linux-man
In-Reply-To: <aa62e24c-537e-4141-9507-37cd0af19dfc@huawei.com>

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

Hi Xiu,

On 2026-02-05T19:48:02+0800, Xiu Jianfeng wrote:
> On 2/4/2026 3:50 AM, Alyssa Ross wrote:
> > This is true for Fedora, where this page was sourced from, but I don't
> > believe it has ever been true for the mainline kernel, because Linus
> > rejected it.
> 
> Yeah, I also found this issue not long ago, but I haven't had time to submit
> a fix patch yet.
> 
> > 
> > Link: https://bbs.archlinux.org/viewtopic.php?pid=2088704#p2088704
> > Link: https://lore.kernel.org/lkml/CA+55aFzYbpRAdma0PvqE+9ygySuKzNKByqOzzMufBoovXVnfPw@mail.gmail.com/
> > Fixes: bb509e6fc ("kernel_lockdown.7: New page documenting the Kernel Lockdown feature")
> > Signed-off-by: Alyssa Ross <hi@alyssa.is>
> 
> I am not sure if appropriate to add my ACK here, if needed, feel free to
> add:
> 
> Acked-by: Xiu Jianfeng <xiujianfeng@huawei.com>

It's appropriate.  Thanks!


Have a lovely day!
Alex

> 
> > ---
> >   man/man7/kernel_lockdown.7 | 3 ---
> >   1 file changed, 3 deletions(-)
> > 
> > diff --git a/man/man7/kernel_lockdown.7 b/man/man7/kernel_lockdown.7
> > index 5090484ea..5986c8f01 100644
> > --- a/man/man7/kernel_lockdown.7
> > +++ b/man/man7/kernel_lockdown.7
> > @@ -23,9 +23,6 @@ Lockdown: X: Y is restricted, see man kernel_lockdown.7
> >   .in
> >   .P
> >   where X indicates the process name and Y indicates what is restricted.
> > -.P
> > -On an EFI-enabled x86 or arm64 machine, lockdown will be automatically enabled
> > -if the system boots in EFI Secure Boot mode.
> >   .\"
> >   .SS Coverage
> >   When lockdown is in effect, a number of features are disabled or have their
> 
> 

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
From: Amir Goldstein @ 2026-02-05 13:04 UTC (permalink / raw)
  To: Jeff Layton
  Cc: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, John Johansen, Paul Moore,
	James Morris, Serge E. Hallyn, Stephen Smalley, linux-kernel,
	netfs, linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
	linux-security-module, selinux
In-Reply-To: <5d273a008fc51a2fded785efbe30e5bd2a89b985.camel@kernel.org>

On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > The primary purpose of this patch is to remove the locking from
> > ovl_lookup_real_one() as part of centralising all locking of directories
> > for name operations.
> >
> > The locking here isn't needed.  By performing consistency tests after
> > the lookup we can be sure that the result of the lookup was valid at
> > least for a moment, which is all the original code promised.
> >
> > lookup_noperm_unlocked() is used for the lookup and it will take the
> > lock if needed only where it is needed.
> >
> > Also:
> >  - don't take a reference to real->d_parent.  The parent is
> >    only use for a pointer comparison, and no reference is needed for
> >    that.
> >  - Several "if" statements have a "goto" followed by "else" - the
> >    else isn't needed: the following statement can directly follow
> >    the "if" as a new statement
> >  - Use a consistent pattern of setting "err" before performing a test
> >    and possibly going to "fail".
> >  - remove the "out" label (now that we don't need to dput(parent) or
> >    unlock) and simply return from fail:.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> >  1 file changed, 26 insertions(+), 35 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index 83f80fdb1567..dcd28ffc4705 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> >                                         struct dentry *real,
> >                                         const struct ovl_layer *layer)
> >  {
> > -     struct inode *dir = d_inode(connected);
> > -     struct dentry *this, *parent = NULL;
> > +     struct dentry *this;
> >       struct name_snapshot name;
> >       int err;
> >
> >       /*
> > -      * Lookup child overlay dentry by real name. The dir mutex protects us
> > -      * from racing with overlay rename. If the overlay dentry that is above
> > -      * real has already been moved to a parent that is not under the
> > -      * connected overlay dir, we return -ECHILD and restart the lookup of
> > -      * connected real path from the top.
> > +      * @connected is a directory in the overlay and @real is an object
> > +      * on @layer which is expected to be a child of @connected.
> > +      * The goal is to return a dentry from the overlay which corresponds

As the header comment already says:
"...return a connected overlay dentry whose real dentry is @real"

The wording "corresponds to @real" reduces clarity IMO.

> > +      * to @real.  This is done by looking up the name from @real in
> > +      * @connected and checking that the result meets expectations.
> > +      *
> > +      * Return %-ECHILD if the parent of @real no-longer corresponds to
> > +      * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > +      * correspond to @real.
> >        */

I dislike kernel-doc inside code comments.
I think this is actively discouraged and I haven't found a single example
of this style in fs code.

If you want to keep this format, please lift the comment to function
header comment - it is anyway a very generic comment that explains the
function in general.

> > -     inode_lock_nested(dir, I_MUTEX_PARENT);
> > -     err = -ECHILD;
> > -     parent = dget_parent(real);
> > -     if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > -             goto fail;
> >
> > -     /*
> > -      * We also need to take a snapshot of real dentry name to protect us
> > -      * from racing with underlying layer rename. In this case, we don't
> > -      * care about returning ESTALE, only from dereferencing a free name
> > -      * pointer because we hold no lock on the real dentry.
> > -      */
> >       take_dentry_name_snapshot(&name, real);
> > -     /*
> > -      * No idmap handling here: it's an internal lookup.
> > -      */
> > -     this = lookup_noperm(&name.name, connected);
> > +     this = lookup_noperm_unlocked(&name.name, connected);
> >       release_dentry_name_snapshot(&name);
> > +
> > +     err = -ECHILD;
> > +     if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > +             goto fail;
> > +
> >       err = PTR_ERR(this);
> > -     if (IS_ERR(this)) {
> > +     if (IS_ERR(this))
> >               goto fail;
> > -     } else if (!this || !this->d_inode) {
> > -             dput(this);
> > -             err = -ENOENT;
> > +
> > +     err = -ENOENT;
> > +     if (!this || !this->d_inode)
> >               goto fail;
> > -     } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> > -             dput(this);
> > -             err = -ESTALE;
> > +
> > +     err = -ESTALE;
> > +     if (ovl_dentry_real_at(this, layer->idx) != real)
> >               goto fail;
> > -     }
> >
> > -out:
> > -     dput(parent);
> > -     inode_unlock(dir);
> >       return this;
> >
> >  fail:
> >       pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> >                           real, layer->idx, connected, err);
> > -     this = ERR_PTR(err);
> > -     goto out;
> > +     if (!IS_ERR(this))
> > +             dput(this);
> > +     return ERR_PTR(err);
> >  }
> >
> >  static struct dentry *ovl_lookup_real(struct super_block *sb,
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Otherwise, it looks fine.

Thanks,
Amir.

^ permalink raw reply

* Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
From: Jeff Layton @ 2026-02-05 12:58 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-13-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> This function is unused.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/overlayfs.h |  2 --
>  fs/overlayfs/util.c      | 25 -------------------------
>  2 files changed, 27 deletions(-)
> 
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4fb4750a83e4..3eedc2684c23 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
>  bool ovl_need_index(struct dentry *dentry);
>  int ovl_nlink_start(struct dentry *dentry);
>  void ovl_nlink_end(struct dentry *dentry);
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
>  			     struct ovl_metacopy *data);
>  int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 94986d11a166..810c8752b4f7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
>  	ovl_inode_unlock(inode);
>  }
>  
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper)
> -{
> -	struct dentry *trap;
> -
> -	/* Workdir should not be subdir of upperdir and vice versa */
> -	trap = lock_rename(workdir, upperdir);
> -	if (IS_ERR(trap))
> -		goto err;
> -	if (trap)
> -		goto err_unlock;
> -	if (work && (work->d_parent != workdir || d_unhashed(work)))
> -		goto err_unlock;
> -	if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
> -		goto err_unlock;
> -
> -	return 0;
> -
> -err_unlock:
> -	unlock_rename(workdir, upperdir);
> -err:
> -	pr_err("failed to lock workdir+upperdir\n");
> -	return -EIO;
> -}
> -
>  /*
>   * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
>   * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()
From: Jeff Layton @ 2026-02-05 12:58 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-5-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Instead of explicitly locking the parent and performing a look up in
> apparmor, use simple_start_creating(), and then simple_done_creating()
> to unlock and drop the dentry.
> 
> This removes the need to check for an existing entry (as
> simple_start_creating() acts like an exclusive create and can return
> -EEXIST), simplifies error paths, and keeps dir locking code
> centralised.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
>  1 file changed, 9 insertions(+), 29 deletions(-)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 907bd2667e28..7f78c36e6e50 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
>  
>  	dir = d_inode(parent);
>  
> -	inode_lock(dir);
> -	dentry = lookup_noperm(&QSTR(name), parent);
> +	dentry = simple_start_creating(parent, name);
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
> -		goto fail_lock;
> -	}
> -
> -	if (d_really_is_positive(dentry)) {
> -		error = -EEXIST;
> -		goto fail_dentry;
> +		goto fail;
>  	}
>  
>  	error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
> +	simple_done_creating(dentry);
>  	if (error)
> -		goto fail_dentry;
> -	inode_unlock(dir);
> -
> -	return dentry;
> -
> -fail_dentry:
> -	dput(dentry);
> -
> -fail_lock:
> -	inode_unlock(dir);
> +		goto fail;
> +	return 0;

As KTR points out, this should be "return NULL;"

> +fail:
>  	simple_release_fs(&aafs_mnt, &aafs_count);
> -
>  	return ERR_PTR(error);
>  }
>  
> @@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
>  	if (error)
>  		return error;
>  
> -	inode_lock(d_inode(parent));
> -	dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
> +	dentry = simple_start_creating(parent, NULL_FILE_NAME);
>  	if (IS_ERR(dentry)) {
>  		error = PTR_ERR(dentry);
>  		goto out;
> @@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
>  	inode = new_inode(parent->d_inode->i_sb);
>  	if (!inode) {
>  		error = -ENOMEM;
> -		goto out1;
> +		goto out;
>  	}
>  
>  	inode->i_ino = get_next_ino();
> @@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
>  	aa_null.dentry = dget(dentry);
>  	aa_null.mnt = mntget(mount);
>  
> -	error = 0;
> -
> -out1:
> -	dput(dentry);
>  out:
> -	inode_unlock(d_inode(parent));
> +	simple_done_creating(dentry);
>  	simple_release_fs(&mount, &count);
>  	return error;
>  }
>  
> -
> -
>  static const char *policy_get_link(struct dentry *dentry,
>  				   struct inode *inode,
>  				   struct delayed_call *done)

Assuming you fix the minor problem above.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 13/13] VFS: unexport lock_rename(), lock_rename_child(), unlock_rename()
From: Jeff Layton @ 2026-02-05 12:41 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-14-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> These three function are now only used in namei.c, so they don't need to
> be exported.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/porting.rst | 7 +++++++
>  fs/namei.c                            | 9 +++------
>  include/linux/namei.h                 | 3 ---
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index ed86c95d9d01..5f7008172f14 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1347,3 +1347,10 @@ implementation should set it to generic_setlease().
>  
>  lookup_one_qstr_excl() is no longer exported - use start_creating() or
>  similar.
> +---
> +
> +** mandatory**
> +
> +lock_rename(), lock_rename_child(), unlock_rename() are no
> +longer available.  Use start_renaming() or similar.
> +
> diff --git a/fs/namei.c b/fs/namei.c
> index 307b4d0866b8..0bc82bf90adc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3713,7 +3713,7 @@ static struct dentry *lock_two_directories(struct dentry *p1, struct dentry *p2)
>  /*
>   * p1 and p2 should be directories on the same fs.
>   */
> -struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
> +static struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
>  {
>  	if (p1 == p2) {
>  		inode_lock_nested(p1->d_inode, I_MUTEX_PARENT);
> @@ -3723,12 +3723,11 @@ struct dentry *lock_rename(struct dentry *p1, struct dentry *p2)
>  	mutex_lock(&p1->d_sb->s_vfs_rename_mutex);
>  	return lock_two_directories(p1, p2);
>  }
> -EXPORT_SYMBOL(lock_rename);
>  
>  /*
>   * c1 and p2 should be on the same fs.
>   */
> -struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
> +static struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
>  {
>  	if (READ_ONCE(c1->d_parent) == p2) {
>  		/*
> @@ -3765,9 +3764,8 @@ struct dentry *lock_rename_child(struct dentry *c1, struct dentry *p2)
>  	mutex_unlock(&c1->d_sb->s_vfs_rename_mutex);
>  	return NULL;
>  }
> -EXPORT_SYMBOL(lock_rename_child);
>  
> -void unlock_rename(struct dentry *p1, struct dentry *p2)
> +static void unlock_rename(struct dentry *p1, struct dentry *p2)
>  {
>  	inode_unlock(p1->d_inode);
>  	if (p1 != p2) {
> @@ -3775,7 +3773,6 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
>  		mutex_unlock(&p1->d_sb->s_vfs_rename_mutex);
>  	}
>  }
> -EXPORT_SYMBOL(unlock_rename);
>  
>  /**
>   * __start_renaming - lookup and lock names for rename
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index c7a7288cdd25..2ad6dd9987b9 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -165,9 +165,6 @@ extern int follow_down_one(struct path *);
>  extern int follow_down(struct path *path, unsigned int flags);
>  extern int follow_up(struct path *);
>  
> -extern struct dentry *lock_rename(struct dentry *, struct dentry *);
> -extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
> -extern void unlock_rename(struct dentry *, struct dentry *);
>  int start_renaming(struct renamedata *rd, int lookup_flags,
>  		   struct qstr *old_last, struct qstr *new_last);
>  int start_renaming_dentry(struct renamedata *rd, int lookup_flags,

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
From: Jeff Layton @ 2026-02-05 12:40 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-13-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> This function is unused.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/overlayfs.h |  2 --
>  fs/overlayfs/util.c      | 25 -------------------------
>  2 files changed, 27 deletions(-)
> 
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 4fb4750a83e4..3eedc2684c23 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -569,8 +569,6 @@ bool ovl_is_inuse(struct dentry *dentry);
>  bool ovl_need_index(struct dentry *dentry);
>  int ovl_nlink_start(struct dentry *dentry);
>  void ovl_nlink_end(struct dentry *dentry);
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper);
>  int ovl_check_metacopy_xattr(struct ovl_fs *ofs, const struct path *path,
>  			     struct ovl_metacopy *data);
>  int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 94986d11a166..810c8752b4f7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -1213,31 +1213,6 @@ void ovl_nlink_end(struct dentry *dentry)
>  	ovl_inode_unlock(inode);
>  }
>  
> -int ovl_lock_rename_workdir(struct dentry *workdir, struct dentry *work,
> -			    struct dentry *upperdir, struct dentry *upper)
> -{
> -	struct dentry *trap;
> -
> -	/* Workdir should not be subdir of upperdir and vice versa */
> -	trap = lock_rename(workdir, upperdir);
> -	if (IS_ERR(trap))
> -		goto err;
> -	if (trap)
> -		goto err_unlock;
> -	if (work && (work->d_parent != workdir || d_unhashed(work)))
> -		goto err_unlock;
> -	if (upper && (upper->d_parent != upperdir || d_unhashed(upper)))
> -		goto err_unlock;
> -
> -	return 0;
> -
> -err_unlock:
> -	unlock_rename(workdir, upperdir);
> -err:
> -	pr_err("failed to lock workdir+upperdir\n");
> -	return -EIO;
> -}
> -
>  /*
>   * err < 0, 0 if no metacopy xattr, metacopy data size if xattr found.
>   * an empty xattr returns OVL_METACOPY_MIN_SIZE to distinguish from no xattr value.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 11/13] ovl: use is_subdir() for testing if one thing is a subdir of another
From: Jeff Layton @ 2026-02-05 12:38 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-12-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Rather than using lock_rename(), use the more obvious is_subdir() for
> ensuring that neither upper nor workdir contain the other.
> Also be explicit in the comment that the two directories cannot be the
> same.
> 
> As this is a point-it-time sanity check and does not provide any
> on-going guarantees, the removal of locking does not introduce any
> interesting races.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/super.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index ba9146f22a2c..2fd3e0aee50e 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -451,18 +451,13 @@ static int ovl_lower_dir(const char *name, const struct path *path,
>  	return 0;
>  }
>  
> -/* Workdir should not be subdir of upperdir and vice versa */
> +/*
> + * Workdir should not be subdir of upperdir and vice versa, and
> + * they should not be the same.
> + */
>  static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
>  {
> -	bool ok = false;
> -
> -	if (workdir != upperdir) {
> -		struct dentry *trap = lock_rename(workdir, upperdir);
> -		if (!IS_ERR(trap))
> -			unlock_rename(workdir, upperdir);
> -		ok = (trap == NULL);
> -	}
> -	return ok;
> +	return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
>  }
>  
>  static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 09/13] cachefiles: change cachefiles_bury_object to use start_renaming_dentry()
From: Jeff Layton @ 2026-02-05 12:38 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-10-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Rather then using lock_rename() and lookup_one() etc we can use
> the new start_renaming_dentry().  This is part of centralising dir
> locking and lookup so that locking rules can be changed.
> 
> Some error check are removed as not necessary.  Checks for rep being a
> non-dir or IS_DEADDIR and the check that ->graveyard is still a
> directory only provide slightly more informative errors and have been
> dropped.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/cachefiles/namei.c | 76 ++++++++-----------------------------------
>  1 file changed, 14 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index e5ec90dccc27..3af42ec78411 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -270,7 +270,8 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  			   struct dentry *rep,
>  			   enum fscache_why_object_killed why)
>  {
> -	struct dentry *grave, *trap;
> +	struct dentry *grave;
> +	struct renamedata rd = {};
>  	struct path path, path_to_graveyard;
>  	char nbuffer[8 + 8 + 1];
>  	int ret;
> @@ -302,77 +303,36 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  		(uint32_t) ktime_get_real_seconds(),
>  		(uint32_t) atomic_inc_return(&cache->gravecounter));
>  
> -	/* do the multiway lock magic */
> -	trap = lock_rename(cache->graveyard, dir);
> -	if (IS_ERR(trap))
> -		return PTR_ERR(trap);
> -
> -	/* do some checks before getting the grave dentry */
> -	if (rep->d_parent != dir || IS_DEADDIR(d_inode(rep))) {
> -		/* the entry was probably culled when we dropped the parent dir
> -		 * lock */
> -		unlock_rename(cache->graveyard, dir);
> -		_leave(" = 0 [culled?]");
> -		return 0;
> -	}
> -
> -	if (!d_can_lookup(cache->graveyard)) {
> -		unlock_rename(cache->graveyard, dir);
> -		cachefiles_io_error(cache, "Graveyard no longer a directory");
> -		return -EIO;
> -	}
> -
> -	if (trap == rep) {
> -		unlock_rename(cache->graveyard, dir);
> -		cachefiles_io_error(cache, "May not make directory loop");
> +	rd.mnt_idmap = &nop_mnt_idmap;
> +	rd.old_parent = dir;
> +	rd.new_parent = cache->graveyard;
> +	rd.flags = 0;
> +	ret = start_renaming_dentry(&rd, 0, rep, &QSTR(nbuffer));
> +	if (ret) {
> +		cachefiles_io_error(cache, "Cannot lock/lookup in graveyard");
>  		return -EIO;
>  	}
>  
>  	if (d_mountpoint(rep)) {
> -		unlock_rename(cache->graveyard, dir);
> +		end_renaming(&rd);
>  		cachefiles_io_error(cache, "Mountpoint in cache");
>  		return -EIO;
>  	}
>  
> -	grave = lookup_one(&nop_mnt_idmap, &QSTR(nbuffer), cache->graveyard);
> -	if (IS_ERR(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		trace_cachefiles_vfs_error(object, d_inode(cache->graveyard),
> -					   PTR_ERR(grave),
> -					   cachefiles_trace_lookup_error);
> -
> -		if (PTR_ERR(grave) == -ENOMEM) {
> -			_leave(" = -ENOMEM");
> -			return -ENOMEM;
> -		}
> -
> -		cachefiles_io_error(cache, "Lookup error %ld", PTR_ERR(grave));
> -		return -EIO;
> -	}
> -
> +	grave = rd.new_dentry;
>  	if (d_is_positive(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> +		end_renaming(&rd);
>  		grave = NULL;
>  		cond_resched();
>  		goto try_again;
>  	}
>  
>  	if (d_mountpoint(grave)) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> +		end_renaming(&rd);
>  		cachefiles_io_error(cache, "Mountpoint in graveyard");
>  		return -EIO;
>  	}
>  
> -	/* target should not be an ancestor of source */
> -	if (trap == grave) {
> -		unlock_rename(cache->graveyard, dir);
> -		dput(grave);
> -		cachefiles_io_error(cache, "May not make directory loop");
> -		return -EIO;
> -	}
> -
>  	/* attempt the rename */
>  	path.mnt = cache->mnt;
>  	path.dentry = dir;
> @@ -382,13 +342,6 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  	if (ret < 0) {
>  		cachefiles_io_error(cache, "Rename security error %d", ret);
>  	} else {
> -		struct renamedata rd = {
> -			.mnt_idmap	= &nop_mnt_idmap,
> -			.old_parent	= dir,
> -			.old_dentry	= rep,
> -			.new_parent	= cache->graveyard,
> -			.new_dentry	= grave,
> -		};
>  		trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
>  		ret = cachefiles_inject_read_error();
>  		if (ret == 0)
> @@ -402,8 +355,7 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
>  	}
>  
>  	__cachefiles_unmark_inode_in_use(object, d_inode(rep));
> -	unlock_rename(cache->graveyard, dir);
> -	dput(grave);
> +	end_renaming(&rd);
>  	_leave(" = 0");
>  	return 0;
>  }

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
From: Jeff Layton @ 2026-02-05 12:38 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-9-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
> 
> The locking here isn't needed.  By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
> 
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
> 
> Also:
>  - don't take a reference to real->d_parent.  The parent is
>    only use for a pointer comparison, and no reference is needed for
>    that.
>  - Several "if" statements have a "goto" followed by "else" - the
>    else isn't needed: the following statement can directly follow
>    the "if" as a new statement
>  - Use a consistent pattern of setting "err" before performing a test
>    and possibly going to "fail".
>  - remove the "out" label (now that we don't need to dput(parent) or
>    unlock) and simply return from fail:.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
>  1 file changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb1567..dcd28ffc4705 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
>  					  struct dentry *real,
>  					  const struct ovl_layer *layer)
>  {
> -	struct inode *dir = d_inode(connected);
> -	struct dentry *this, *parent = NULL;
> +	struct dentry *this;
>  	struct name_snapshot name;
>  	int err;
>  
>  	/*
> -	 * Lookup child overlay dentry by real name. The dir mutex protects us
> -	 * from racing with overlay rename. If the overlay dentry that is above
> -	 * real has already been moved to a parent that is not under the
> -	 * connected overlay dir, we return -ECHILD and restart the lookup of
> -	 * connected real path from the top.
> +	 * @connected is a directory in the overlay and @real is an object
> +	 * on @layer which is expected to be a child of @connected.
> +	 * The goal is to return a dentry from the overlay which corresponds
> +	 * to @real.  This is done by looking up the name from @real in
> +	 * @connected and checking that the result meets expectations.
> +	 *
> +	 * Return %-ECHILD if the parent of @real no-longer corresponds to
> +	 * @connected, and %-ESTALE if the dentry found by lookup doesn't
> +	 * correspond to @real.
>  	 */
> -	inode_lock_nested(dir, I_MUTEX_PARENT);
> -	err = -ECHILD;
> -	parent = dget_parent(real);
> -	if (ovl_dentry_real_at(connected, layer->idx) != parent)
> -		goto fail;
>  
> -	/*
> -	 * We also need to take a snapshot of real dentry name to protect us
> -	 * from racing with underlying layer rename. In this case, we don't
> -	 * care about returning ESTALE, only from dereferencing a free name
> -	 * pointer because we hold no lock on the real dentry.
> -	 */
>  	take_dentry_name_snapshot(&name, real);
> -	/*
> -	 * No idmap handling here: it's an internal lookup.
> -	 */
> -	this = lookup_noperm(&name.name, connected);
> +	this = lookup_noperm_unlocked(&name.name, connected);
>  	release_dentry_name_snapshot(&name);
> +
> +	err = -ECHILD;
> +	if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> +		goto fail;
> +
>  	err = PTR_ERR(this);
> -	if (IS_ERR(this)) {
> +	if (IS_ERR(this))
>  		goto fail;
> -	} else if (!this || !this->d_inode) {
> -		dput(this);
> -		err = -ENOENT;
> +
> +	err = -ENOENT;
> +	if (!this || !this->d_inode)
>  		goto fail;
> -	} else if (ovl_dentry_real_at(this, layer->idx) != real) {
> -		dput(this);
> -		err = -ESTALE;
> +
> +	err = -ESTALE;
> +	if (ovl_dentry_real_at(this, layer->idx) != real)
>  		goto fail;
> -	}
>  
> -out:
> -	dput(parent);
> -	inode_unlock(dir);
>  	return this;
>  
>  fail:
>  	pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
>  			    real, layer->idx, connected, err);
> -	this = ERR_PTR(err);
> -	goto out;
> +	if (!IS_ERR(this))
> +		dput(this);
> +	return ERR_PTR(err);
>  }
>  
>  static struct dentry *ovl_lookup_real(struct super_block *sb,

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

* Re: [PATCH 07/13] VFS: make lookup_one_qstr_excl() static.
From: Jeff Layton @ 2026-02-05 12:37 UTC (permalink / raw)
  To: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
	Jan Kara, Chuck Lever, Miklos Szeredi, Amir Goldstein,
	John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
	Stephen Smalley
  Cc: linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
	apparmor, linux-security-module, selinux
In-Reply-To: <20260204050726.177283-8-neilb@ownmail.net>

On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> lookup_one_qstr_excl() is no longer used outside of namei.c, so
> make it static.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/porting.rst | 7 +++++++
>  fs/namei.c                            | 5 ++---
>  include/linux/namei.h                 | 3 ---
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index ed3ac56e3c76..ed86c95d9d01 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1340,3 +1340,10 @@ The ->setlease() file_operation must now be explicitly set in order to provide
>  support for leases. When set to NULL, the kernel will now return -EINVAL to
>  attempts to set a lease. Filesystems that wish to use the kernel-internal lease
>  implementation should set it to generic_setlease().
> +
> +---
> +
> +**mandatory**
> +
> +lookup_one_qstr_excl() is no longer exported - use start_creating() or
> +similar.
> diff --git a/fs/namei.c b/fs/namei.c
> index 40af78ddfb1b..307b4d0866b8 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1730,8 +1730,8 @@ static struct dentry *lookup_dcache(const struct qstr *name,
>   * Will return -ENOENT if name isn't found and LOOKUP_CREATE wasn't passed.
>   * Will return -EEXIST if name is found and LOOKUP_EXCL was passed.
>   */
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> -				    struct dentry *base, unsigned int flags)
> +static struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> +					   struct dentry *base, unsigned int flags)
>  {
>  	struct dentry *dentry;
>  	struct dentry *old;
> @@ -1768,7 +1768,6 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
>  	}
>  	return dentry;
>  }
> -EXPORT_SYMBOL(lookup_one_qstr_excl);
>  
>  /**
>   * lookup_fast - do fast lockless (but racy) lookup of a dentry
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 58600cf234bc..c7a7288cdd25 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -54,9 +54,6 @@ extern int path_pts(struct path *path);
>  
>  extern int user_path_at(int, const char __user *, unsigned, struct path *);
>  
> -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> -				    struct dentry *base,
> -				    unsigned int flags);
>  extern int kern_path(const char *, unsigned, struct path *);
>  struct dentry *kern_path_parent(const char *name, struct path *parent);
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox