Linux userland API discussions
 help / color / mirror / Atom feed

* [PATCH 3/4] tools/selftests: expand all guard region tests to file-backed
From: Lorenzo Stoakes @ 2025-02-13 18:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
	Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
	linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
	John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <cover.1739469950.git.lorenzo.stoakes@oracle.com>

Extend the guard region tests to allow for test fixture variants for anon,
shmem, and local file files.

This allows us to assert that each of the expected behaviours of anonymous
memory also applies correctly to file-backed (both shmem and an a file
created locally in the current working directory) and thus asserts the same
correctness guarantees as all the remaining tests do.

The fixture teardown is now performed in the parent process rather than
child forked ones, meaning cleanup is always performed, including unlinking
any generated temporary files.

Additionally the variant fixture data type now contains an enum value
indicating the type of backing store and the mmap() invocation is
abstracted to allow for the mapping of whichever backing store the variant
is testing.

We adjust tests as necessary to account for the fact they may now reference
files rather than anonymous memory.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 tools/testing/selftests/mm/guard-regions.c | 290 +++++++++++++++------
 1 file changed, 205 insertions(+), 85 deletions(-)

diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
index 7a41cf9ffbdf..0469c783f4fa 100644
--- a/tools/testing/selftests/mm/guard-regions.c
+++ b/tools/testing/selftests/mm/guard-regions.c
@@ -6,6 +6,7 @@
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <linux/limits.h>
 #include <linux/userfaultfd.h>
 #include <setjmp.h>
 #include <signal.h>
@@ -37,6 +38,79 @@ static sigjmp_buf signal_jmp_buf;
  */
 #define FORCE_READ(x) (*(volatile typeof(x) *)x)
 
+/*
+ * How is the test backing the mapping being tested?
+ */
+enum backing_type {
+	ANON_BACKED,
+	SHMEM_BACKED,
+	LOCAL_FILE_BACKED,
+};
+
+FIXTURE(guard_regions)
+{
+	unsigned long page_size;
+	char path[PATH_MAX];
+	int fd;
+};
+
+FIXTURE_VARIANT(guard_regions)
+{
+	enum backing_type backing;
+};
+
+FIXTURE_VARIANT_ADD(guard_regions, anon)
+{
+	.backing = ANON_BACKED,
+};
+
+FIXTURE_VARIANT_ADD(guard_regions, shmem)
+{
+	.backing = SHMEM_BACKED,
+};
+
+FIXTURE_VARIANT_ADD(guard_regions, file)
+{
+	.backing = LOCAL_FILE_BACKED,
+};
+
+static bool is_anon_backed(const FIXTURE_VARIANT(guard_regions) * variant)
+{
+	switch (variant->backing) {
+	case  ANON_BACKED:
+	case  SHMEM_BACKED:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static void *mmap_(FIXTURE_DATA(guard_regions) * self,
+		   const FIXTURE_VARIANT(guard_regions) * variant,
+		   void *addr, size_t length, int prot, int extra_flags,
+		   off_t offset)
+{
+	int fd;
+	int flags = extra_flags;
+
+	switch (variant->backing) {
+	case ANON_BACKED:
+		flags |= MAP_PRIVATE | MAP_ANON;
+		fd = -1;
+		break;
+	case SHMEM_BACKED:
+	case LOCAL_FILE_BACKED:
+		flags |= MAP_SHARED;
+		fd = self->fd;
+		break;
+	default:
+		ksft_exit_fail();
+		break;
+	}
+
+	return mmap(addr, length, prot, flags, fd, offset);
+}
+
 static int userfaultfd(int flags)
 {
 	return syscall(SYS_userfaultfd, flags);
@@ -107,12 +181,7 @@ static bool try_read_write_buf(char *ptr)
 	return try_read_buf(ptr) && try_write_buf(ptr);
 }
 
-FIXTURE(guard_regions)
-{
-	unsigned long page_size;
-};
-
-FIXTURE_SETUP(guard_regions)
+static void setup_sighandler(void)
 {
 	struct sigaction act = {
 		.sa_handler = &handle_fatal,
@@ -122,11 +191,9 @@ FIXTURE_SETUP(guard_regions)
 	sigemptyset(&act.sa_mask);
 	if (sigaction(SIGSEGV, &act, NULL))
 		ksft_exit_fail_perror("sigaction");
+}
 
-	self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
-};
-
-FIXTURE_TEARDOWN(guard_regions)
+static void teardown_sighandler(void)
 {
 	struct sigaction act = {
 		.sa_handler = SIG_DFL,
@@ -137,6 +204,48 @@ FIXTURE_TEARDOWN(guard_regions)
 	sigaction(SIGSEGV, &act, NULL);
 }
 
+static int open_file(const char *prefix, char *path)
+{
+	int fd;
+
+	snprintf(path, PATH_MAX, "%sguard_regions_test_file_XXXXXX", prefix);
+	fd = mkstemp(path);
+	if (fd < 0)
+		ksft_exit_fail_perror("mkstemp");
+
+	return fd;
+}
+
+FIXTURE_SETUP(guard_regions)
+{
+	self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
+	setup_sighandler();
+
+	if (variant->backing == ANON_BACKED)
+		return;
+
+	self->fd = open_file(
+		variant->backing == SHMEM_BACKED ? "/tmp/" : "",
+		self->path);
+
+	/* We truncate file to at least 100 pages, tests can modify as needed. */
+	ASSERT_EQ(ftruncate(self->fd, 100 * self->page_size), 0);
+};
+
+FIXTURE_TEARDOWN_PARENT(guard_regions)
+{
+	teardown_sighandler();
+
+	if (variant->backing == ANON_BACKED)
+		return;
+
+	if (self->fd >= 0)
+		close(self->fd);
+
+	if (self->path[0] != '\0')
+		unlink(self->path);
+}
+
 TEST_F(guard_regions, basic)
 {
 	const unsigned long NUM_PAGES = 10;
@@ -144,8 +253,8 @@ TEST_F(guard_regions, basic)
 	char *ptr;
 	int i;
 
-	ptr = mmap(NULL, NUM_PAGES * page_size, PROT_READ | PROT_WRITE,
-		   MAP_PRIVATE | MAP_ANON, -1, 0);
+	ptr = mmap_(self, variant, NULL, NUM_PAGES * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Trivially assert we can touch the first page. */
@@ -238,25 +347,23 @@ TEST_F(guard_regions, multi_vma)
 	int i;
 
 	/* Reserve a 100 page region over which we can install VMAs. */
-	ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
-			  MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_region = mmap_(self, variant, NULL, 100 * page_size,
+			   PROT_NONE, 0, 0);
 	ASSERT_NE(ptr_region, MAP_FAILED);
 
 	/* Place a VMA of 10 pages size at the start of the region. */
-	ptr1 = mmap(ptr_region, 10 * page_size, PROT_READ | PROT_WRITE,
-		    MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr1 = mmap_(self, variant, ptr_region, 10 * page_size,
+		     PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr1, MAP_FAILED);
 
 	/* Place a VMA of 5 pages size 50 pages into the region. */
-	ptr2 = mmap(&ptr_region[50 * page_size], 5 * page_size,
-		    PROT_READ | PROT_WRITE,
-		    MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr2 = mmap_(self, variant, &ptr_region[50 * page_size], 5 * page_size,
+		     PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr2, MAP_FAILED);
 
 	/* Place a VMA of 20 pages size at the end of the region. */
-	ptr3 = mmap(&ptr_region[80 * page_size], 20 * page_size,
-		    PROT_READ | PROT_WRITE,
-		    MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr3 = mmap_(self, variant, &ptr_region[80 * page_size], 20 * page_size,
+		     PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr3, MAP_FAILED);
 
 	/* Unmap gaps. */
@@ -326,13 +433,11 @@ TEST_F(guard_regions, multi_vma)
 	}
 
 	/* Now map incompatible VMAs in the gaps. */
-	ptr = mmap(&ptr_region[10 * page_size], 40 * page_size,
-		   PROT_READ | PROT_WRITE | PROT_EXEC,
-		   MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, &ptr_region[10 * page_size], 40 * page_size,
+		    PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
-	ptr = mmap(&ptr_region[55 * page_size], 25 * page_size,
-		   PROT_READ | PROT_WRITE | PROT_EXEC,
-		   MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, &ptr_region[55 * page_size], 25 * page_size,
+		    PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/*
@@ -379,8 +484,8 @@ TEST_F(guard_regions, process_madvise)
 	ASSERT_NE(pidfd, -1);
 
 	/* Reserve region to map over. */
-	ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
-			  MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_region = mmap_(self, variant, NULL, 100 * page_size,
+			   PROT_NONE, 0, 0);
 	ASSERT_NE(ptr_region, MAP_FAILED);
 
 	/*
@@ -388,9 +493,8 @@ TEST_F(guard_regions, process_madvise)
 	 * overwrite existing entries and test this code path against
 	 * overwriting existing entries.
 	 */
-	ptr1 = mmap(&ptr_region[page_size], 10 * page_size,
-		    PROT_READ | PROT_WRITE,
-		    MAP_FIXED | MAP_ANON | MAP_PRIVATE | MAP_POPULATE, -1, 0);
+	ptr1 = mmap_(self, variant, &ptr_region[page_size], 10 * page_size,
+		     PROT_READ | PROT_WRITE, MAP_FIXED | MAP_POPULATE, 0);
 	ASSERT_NE(ptr1, MAP_FAILED);
 	/* We want guard markers at start/end of each VMA. */
 	vec[0].iov_base = ptr1;
@@ -399,9 +503,8 @@ TEST_F(guard_regions, process_madvise)
 	vec[1].iov_len = page_size;
 
 	/* 5 pages offset 50 pages into reserve region. */
-	ptr2 = mmap(&ptr_region[50 * page_size], 5 * page_size,
-		    PROT_READ | PROT_WRITE,
-		    MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr2 = mmap_(self, variant, &ptr_region[50 * page_size], 5 * page_size,
+		     PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr2, MAP_FAILED);
 	vec[2].iov_base = ptr2;
 	vec[2].iov_len = page_size;
@@ -409,9 +512,8 @@ TEST_F(guard_regions, process_madvise)
 	vec[3].iov_len = page_size;
 
 	/* 20 pages offset 79 pages into reserve region. */
-	ptr3 = mmap(&ptr_region[79 * page_size], 20 * page_size,
-		    PROT_READ | PROT_WRITE,
-		    MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr3 = mmap_(self, variant, &ptr_region[79 * page_size], 20 * page_size,
+		    PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr3, MAP_FAILED);
 	vec[4].iov_base = ptr3;
 	vec[4].iov_len = page_size;
@@ -472,8 +574,8 @@ TEST_F(guard_regions, munmap)
 	const unsigned long page_size = self->page_size;
 	char *ptr, *ptr_new1, *ptr_new2;
 
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Guard first and last pages. */
@@ -489,11 +591,11 @@ TEST_F(guard_regions, munmap)
 	ASSERT_EQ(munmap(&ptr[9 * page_size], page_size), 0);
 
 	/* Map over them.*/
-	ptr_new1 = mmap(ptr, page_size, PROT_READ | PROT_WRITE,
-			MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_new1 = mmap_(self, variant, ptr, page_size, PROT_READ | PROT_WRITE,
+			 MAP_FIXED, 0);
 	ASSERT_NE(ptr_new1, MAP_FAILED);
-	ptr_new2 = mmap(&ptr[9 * page_size], page_size, PROT_READ | PROT_WRITE,
-			MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_new2 = mmap_(self, variant, &ptr[9 * page_size], page_size,
+			 PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr_new2, MAP_FAILED);
 
 	/* Assert that they are now not guarded. */
@@ -511,8 +613,8 @@ TEST_F(guard_regions, mprotect)
 	char *ptr;
 	int i;
 
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Guard the middle of the range. */
@@ -559,8 +661,8 @@ TEST_F(guard_regions, split_merge)
 	char *ptr, *ptr_new;
 	int i;
 
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Guard the whole range. */
@@ -601,14 +703,14 @@ TEST_F(guard_regions, split_merge)
 	}
 
 	/* Now map them again - the unmap will have cleared the guards. */
-	ptr_new = mmap(&ptr[2 * page_size], page_size, PROT_READ | PROT_WRITE,
-		       MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_new = mmap_(self, variant, &ptr[2 * page_size], page_size,
+			PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr_new, MAP_FAILED);
-	ptr_new = mmap(&ptr[5 * page_size], page_size, PROT_READ | PROT_WRITE,
-		       MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_new = mmap_(self, variant, &ptr[5 * page_size], page_size,
+			PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr_new, MAP_FAILED);
-	ptr_new = mmap(&ptr[8 * page_size], page_size, PROT_READ | PROT_WRITE,
-		       MAP_FIXED | MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_new = mmap_(self, variant, &ptr[8 * page_size], page_size,
+			PROT_READ | PROT_WRITE, MAP_FIXED, 0);
 	ASSERT_NE(ptr_new, MAP_FAILED);
 
 	/* Now make sure guard pages are established. */
@@ -690,8 +792,8 @@ TEST_F(guard_regions, dontneed)
 	char *ptr;
 	int i;
 
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Back the whole range. */
@@ -721,8 +823,16 @@ TEST_F(guard_regions, dontneed)
 			ASSERT_FALSE(result);
 		} else {
 			ASSERT_TRUE(result);
-			/* Make sure we really did get reset to zero page. */
-			ASSERT_EQ(*curr, '\0');
+			switch (variant->backing) {
+			case ANON_BACKED:
+				/* If anon, then we get a zero page. */
+				ASSERT_EQ(*curr, '\0');
+				break;
+			default:
+				/* Otherwise, we get the file data. */
+				ASSERT_EQ(*curr, 'y');
+				break;
+			}
 		}
 
 		/* Now write... */
@@ -743,8 +853,8 @@ TEST_F(guard_regions, mlock)
 	char *ptr;
 	int i;
 
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Populate. */
@@ -816,8 +926,8 @@ TEST_F(guard_regions, mremap_move)
 	char *ptr, *ptr_new;
 
 	/* Map 5 pages. */
-	ptr = mmap(NULL, 5 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 5 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Place guard markers at both ends of the 5 page span. */
@@ -831,8 +941,7 @@ TEST_F(guard_regions, mremap_move)
 	/* Map a new region we will move this range into. Doing this ensures
 	 * that we have reserved a range to map into.
 	 */
-	ptr_new = mmap(NULL, 5 * page_size, PROT_NONE, MAP_ANON | MAP_PRIVATE,
-		       -1, 0);
+	ptr_new = mmap_(self, variant, NULL, 5 * page_size, PROT_NONE, 0, 0);
 	ASSERT_NE(ptr_new, MAP_FAILED);
 
 	ASSERT_EQ(mremap(ptr, 5 * page_size, 5 * page_size,
@@ -863,8 +972,8 @@ TEST_F(guard_regions, mremap_expand)
 	char *ptr, *ptr_new;
 
 	/* Map 10 pages... */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 	/* ...But unmap the last 5 so we can ensure we can expand into them. */
 	ASSERT_EQ(munmap(&ptr[5 * page_size], 5 * page_size), 0);
@@ -888,8 +997,7 @@ TEST_F(guard_regions, mremap_expand)
 	ASSERT_FALSE(try_read_write_buf(&ptr[4 * page_size]));
 
 	/* Reserve a region which we can move to and expand into. */
-	ptr_new = mmap(NULL, 20 * page_size, PROT_NONE,
-		       MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr_new = mmap_(self, variant, NULL, 20 * page_size, PROT_NONE, 0, 0);
 	ASSERT_NE(ptr_new, MAP_FAILED);
 
 	/* Now move and expand into it. */
@@ -927,8 +1035,8 @@ TEST_F(guard_regions, mremap_shrink)
 	int i;
 
 	/* Map 5 pages. */
-	ptr = mmap(NULL, 5 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 5 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Place guard markers at both ends of the 5 page span. */
@@ -992,8 +1100,8 @@ TEST_F(guard_regions, fork)
 	int i;
 
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Establish guard pages in the first 5 pages. */
@@ -1046,9 +1154,12 @@ TEST_F(guard_regions, fork_cow)
 	pid_t pid;
 	int i;
 
+	if (variant->backing != ANON_BACKED)
+		SKIP(return, "CoW only supported on anon mappings");
+
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Populate range. */
@@ -1117,9 +1228,12 @@ TEST_F(guard_regions, fork_wipeonfork)
 	pid_t pid;
 	int i;
 
+	if (variant->backing != ANON_BACKED)
+		SKIP(return, "Wipe on fork only supported on anon mappings");
+
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Mark wipe on fork. */
@@ -1166,9 +1280,12 @@ TEST_F(guard_regions, lazyfree)
 	char *ptr;
 	int i;
 
+	if (variant->backing != ANON_BACKED)
+		SKIP(return, "MADV_FREE only supported on anon mappings");
+
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Guard range. */
@@ -1202,8 +1319,8 @@ TEST_F(guard_regions, populate)
 	char *ptr;
 
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Guard range. */
@@ -1229,8 +1346,8 @@ TEST_F(guard_regions, cold_pageout)
 	int i;
 
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Guard range. */
@@ -1281,6 +1398,9 @@ TEST_F(guard_regions, uffd)
 	struct uffdio_register reg;
 	struct uffdio_range range;
 
+	if (!is_anon_backed(variant))
+		SKIP(return, "uffd only works on anon backing");
+
 	/* Set up uffd. */
 	uffd = userfaultfd(0);
 	if (uffd == -1 && errno == EPERM)
@@ -1290,8 +1410,8 @@ TEST_F(guard_regions, uffd)
 	ASSERT_EQ(ioctl(uffd, UFFDIO_API, &api), 0);
 
 	/* Map 10 pages. */
-	ptr = mmap(NULL, 10 * page_size, PROT_READ | PROT_WRITE,
-		   MAP_ANON | MAP_PRIVATE, -1, 0);
+	ptr = mmap_(self, variant, NULL, 10 * page_size,
+		    PROT_READ | PROT_WRITE, 0, 0);
 	ASSERT_NE(ptr, MAP_FAILED);
 
 	/* Register the range with uffd. */
-- 
2.48.1


^ permalink raw reply related

* [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-13 18:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
	Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
	linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
	John Hubbard, Juan Yescas, Kalesh Singh

The guard regions feature was initially implemented to support anonymous
mappings only, excluding shmem.

This was done such as to introduce the feature carefully and incrementally
and to be conservative when considering the various caveats and corner
cases that are applicable to file-backed mappings but not to anonymous
ones.

Now this feature has landed in 6.13, it is time to revisit this and to
extend this functionality to file-backed and shmem mappings.

In order to make this maximally useful, and since one may map file-backed
mappings read-only (for instance ELF images), we also remove the
restriction on read-only mappings and permit the establishment of guard
regions in any non-hugetlb, non-mlock()'d mapping.

It is permissible to permit the establishment of guard regions in read-only
mappings because the guard regions only reduce access to the mapping, and
when removed simply reinstate the existing attributes of the underlying
VMA, meaning no access violations can occur.

While the change in kernel code introduced in this series is small, the
majority of the effort here is spent in extending the testing to assert
that the feature works correctly across numerous file-backed mapping
scenarios.

Every single guard region self-test performed against anonymous memory
(which is relevant and not anon-only) has now been updated to also be
performed against shmem and a mapping of a file in the working directory.

This confirms that all cases also function correctly for file-backed guard
regions.

In addition a number of other tests are added for specific file-backed
mapping scenarios.

There are a number of other concerns that one might have with regard to
guard regions, addressed below:

Readahead
~~~~~~~~~

Readahead is a process through which the page cache is populated on the
assumption that sequential reads will occur, thus amortising I/O and,
through a clever use of the PG_readahead folio flag establishing during
major fault and checked upon minor fault, provides for asynchronous I/O to
occur as dat is processed, reducing I/O stalls as data is faulted in.

Guard regions do not alter this mechanism which operations at the folio and
fault level, but do of course prevent the faulting of folios that would
otherwise be mapped.

In the instance of a major fault prior to a guard region, synchronous
readahead will occur including populating folios in the page cache which
the guard regions will, in the case of the mapping in question, prevent
access to.

In addition, if PG_readahead is placed in a folio that is now inaccessible,
this will prevent asynchronous readahead from occurring as it would
otherwise do.

However, there are mechanisms for heuristically resetting this within
readahead regardless, which will 'recover' correct readahead behaviour.

Readahead presumes sequential data access, the presence of a guard region
clearly indicates that, at least in the guard region, no such sequential
access will occur, as it cannot occur there.

So this should have very little impact on any real workload. The far more
important point is as to whether readahead causes incorrect or
inappropriate mapping of ranges disallowed by the presence of guard
regions - this is not the case, as readahead does not 'pre-fault' memory in
this fashion.

At any rate, any mechanism which would attempt to do so would hit the usual
page fault paths, which correctly handle PTE markers as with anonymous
mappings.

Fault-Around
~~~~~~~~~~~~

The fault-around logic, in a similar vein to readahead, attempts to improve
efficiency with regard to file-backed memory mappings, however it differs
in that it does not try to fetch folios into the page cache that are about
to be accessed, but rather pre-maps a range of folios around the faulting
address.

Guard regions making use of PTE markers makes this relatively trivial, as
this case is already handled - see filemap_map_folio_range() and
filemap_map_order0_folio() - in both instances, the solution is to simply
keep the established page table mappings and let the fault handler take
care of PTE markers, as per the comment:

	/*
	 * NOTE: If there're PTE markers, we'll leave them to be
	 * handled in the specific fault path, and it'll prohibit
	 * the fault-around logic.
	 */

This works, as establishing guard regions results in page table mappings
with PTE markers, and clearing them removes them.

Truncation
~~~~~~~~~~

File truncation will not eliminate existing guard regions, as the
truncation operation will ultimately zap the range via
unmap_mapping_range(), which specifically excludes PTE markers.

Zapping
~~~~~~~

Zapping is, as with anonymous mappings, handled by zap_nonpresent_ptes(),
which specifically deals with guard entries, leaving them intact except in
instances such as process teardown or munmap() where they need to be
removed.

Reclaim
~~~~~~~

When reclaim is performed on file-backed folios, it ultimately invokes
try_to_unmap_one() via the rmap. If the folio is non-large, then map_pte()
will ultimately abort the operation for the guard region mapping. If large,
then check_pte() will determine that this is a non-device private
entry/device-exclusive entry 'swap' PTE and thus abort the operation in
that instance.

Therefore, no odd things happen in the instance of reclaim being attempted
upon a file-backed guard region.

Hole Punching
~~~~~~~~~~~~~

This updates the page cache and ultimately invokes unmap_mapping_range(),
which explicitly leaves PTE markers in place.

Because the establishment of guard regions zapped any existing mappings to
file-backed folios, once the guard regions are removed then the
hole-punched region will be faulted in as usual and everything will behave
as expected.

Lorenzo Stoakes (4):
  mm: allow guard regions in file-backed and read-only mappings
  selftests/mm: rename guard-pages to guard-regions
  tools/selftests: expand all guard region tests to file-backed
  tools/selftests: add file/shmem-backed mapping guard region tests

 mm/madvise.c                                  |   8 +-
 tools/testing/selftests/mm/.gitignore         |   2 +-
 tools/testing/selftests/mm/Makefile           |   2 +-
 .../mm/{guard-pages.c => guard-regions.c}     | 921 ++++++++++++++++--
 4 files changed, 821 insertions(+), 112 deletions(-)
 rename tools/testing/selftests/mm/{guard-pages.c => guard-regions.c} (58%)

--
2.48.1

^ permalink raw reply

* [PATCH 2/4] selftests/mm: rename guard-pages to guard-regions
From: Lorenzo Stoakes @ 2025-02-13 18:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
	Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
	linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
	John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <cover.1739469950.git.lorenzo.stoakes@oracle.com>

The feature formerly referred to as guard pages is more correctly referred
to as 'guard regions', as in fact no pages are ever allocated in the
process of installing the regions.

To avoid confusion, rename the tests accordingly.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 tools/testing/selftests/mm/.gitignore         |  2 +-
 tools/testing/selftests/mm/Makefile           |  2 +-
 .../mm/{guard-pages.c => guard-regions.c}     | 42 +++++++++----------
 3 files changed, 23 insertions(+), 23 deletions(-)
 rename tools/testing/selftests/mm/{guard-pages.c => guard-regions.c} (98%)

diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index 121000c28c10..c5241b193db8 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -57,4 +57,4 @@ droppable
 hugetlb_dio
 pkey_sighandler_tests_32
 pkey_sighandler_tests_64
-guard-pages
+guard-regions
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 63ce39d024bb..8270895039d1 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -97,7 +97,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv
 TEST_GEN_FILES += hugetlb_madv_vs_map
 TEST_GEN_FILES += hugetlb_dio
 TEST_GEN_FILES += droppable
-TEST_GEN_FILES += guard-pages
+TEST_GEN_FILES += guard-regions
 
 ifneq ($(ARCH),arm64)
 TEST_GEN_FILES += soft-dirty
diff --git a/tools/testing/selftests/mm/guard-pages.c b/tools/testing/selftests/mm/guard-regions.c
similarity index 98%
rename from tools/testing/selftests/mm/guard-pages.c
rename to tools/testing/selftests/mm/guard-regions.c
index ece37212a8a2..7a41cf9ffbdf 100644
--- a/tools/testing/selftests/mm/guard-pages.c
+++ b/tools/testing/selftests/mm/guard-regions.c
@@ -107,12 +107,12 @@ static bool try_read_write_buf(char *ptr)
 	return try_read_buf(ptr) && try_write_buf(ptr);
 }
 
-FIXTURE(guard_pages)
+FIXTURE(guard_regions)
 {
 	unsigned long page_size;
 };
 
-FIXTURE_SETUP(guard_pages)
+FIXTURE_SETUP(guard_regions)
 {
 	struct sigaction act = {
 		.sa_handler = &handle_fatal,
@@ -126,7 +126,7 @@ FIXTURE_SETUP(guard_pages)
 	self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
 };
 
-FIXTURE_TEARDOWN(guard_pages)
+FIXTURE_TEARDOWN(guard_regions)
 {
 	struct sigaction act = {
 		.sa_handler = SIG_DFL,
@@ -137,7 +137,7 @@ FIXTURE_TEARDOWN(guard_pages)
 	sigaction(SIGSEGV, &act, NULL);
 }
 
-TEST_F(guard_pages, basic)
+TEST_F(guard_regions, basic)
 {
 	const unsigned long NUM_PAGES = 10;
 	const unsigned long page_size = self->page_size;
@@ -231,7 +231,7 @@ TEST_F(guard_pages, basic)
 }
 
 /* Assert that operations applied across multiple VMAs work as expected. */
-TEST_F(guard_pages, multi_vma)
+TEST_F(guard_regions, multi_vma)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr_region, *ptr, *ptr1, *ptr2, *ptr3;
@@ -367,7 +367,7 @@ TEST_F(guard_pages, multi_vma)
  * Assert that batched operations performed using process_madvise() work as
  * expected.
  */
-TEST_F(guard_pages, process_madvise)
+TEST_F(guard_regions, process_madvise)
 {
 	const unsigned long page_size = self->page_size;
 	pid_t pid = getpid();
@@ -467,7 +467,7 @@ TEST_F(guard_pages, process_madvise)
 }
 
 /* Assert that unmapping ranges does not leave guard markers behind. */
-TEST_F(guard_pages, munmap)
+TEST_F(guard_regions, munmap)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr, *ptr_new1, *ptr_new2;
@@ -505,7 +505,7 @@ TEST_F(guard_pages, munmap)
 }
 
 /* Assert that mprotect() operations have no bearing on guard markers. */
-TEST_F(guard_pages, mprotect)
+TEST_F(guard_regions, mprotect)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -553,7 +553,7 @@ TEST_F(guard_pages, mprotect)
 }
 
 /* Split and merge VMAs and make sure guard pages still behave. */
-TEST_F(guard_pages, split_merge)
+TEST_F(guard_regions, split_merge)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr, *ptr_new;
@@ -684,7 +684,7 @@ TEST_F(guard_pages, split_merge)
 }
 
 /* Assert that MADV_DONTNEED does not remove guard markers. */
-TEST_F(guard_pages, dontneed)
+TEST_F(guard_regions, dontneed)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -737,7 +737,7 @@ TEST_F(guard_pages, dontneed)
 }
 
 /* Assert that mlock()'ed pages work correctly with guard markers. */
-TEST_F(guard_pages, mlock)
+TEST_F(guard_regions, mlock)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -810,7 +810,7 @@ TEST_F(guard_pages, mlock)
  *
  * - Moving a mapping alone should retain markers as they are.
  */
-TEST_F(guard_pages, mremap_move)
+TEST_F(guard_regions, mremap_move)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr, *ptr_new;
@@ -857,7 +857,7 @@ TEST_F(guard_pages, mremap_move)
  * will have to remove guard pages manually to fix up (they'd have to do the
  * same if it were a PROT_NONE mapping).
  */
-TEST_F(guard_pages, mremap_expand)
+TEST_F(guard_regions, mremap_expand)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr, *ptr_new;
@@ -920,7 +920,7 @@ TEST_F(guard_pages, mremap_expand)
  * if the user were using a PROT_NONE mapping they'd have to manually fix this
  * up also so this is OK.
  */
-TEST_F(guard_pages, mremap_shrink)
+TEST_F(guard_regions, mremap_shrink)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -984,7 +984,7 @@ TEST_F(guard_pages, mremap_shrink)
  * Assert that forking a process with VMAs that do not have VM_WIPEONFORK set
  * retain guard pages.
  */
-TEST_F(guard_pages, fork)
+TEST_F(guard_regions, fork)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -1039,7 +1039,7 @@ TEST_F(guard_pages, fork)
  * Assert expected behaviour after we fork populated ranges of anonymous memory
  * and then guard and unguard the range.
  */
-TEST_F(guard_pages, fork_cow)
+TEST_F(guard_regions, fork_cow)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -1110,7 +1110,7 @@ TEST_F(guard_pages, fork_cow)
  * Assert that forking a process with VMAs that do have VM_WIPEONFORK set
  * behave as expected.
  */
-TEST_F(guard_pages, fork_wipeonfork)
+TEST_F(guard_regions, fork_wipeonfork)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -1160,7 +1160,7 @@ TEST_F(guard_pages, fork_wipeonfork)
 }
 
 /* Ensure that MADV_FREE retains guard entries as expected. */
-TEST_F(guard_pages, lazyfree)
+TEST_F(guard_regions, lazyfree)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -1196,7 +1196,7 @@ TEST_F(guard_pages, lazyfree)
 }
 
 /* Ensure that MADV_POPULATE_READ, MADV_POPULATE_WRITE behave as expected. */
-TEST_F(guard_pages, populate)
+TEST_F(guard_regions, populate)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -1222,7 +1222,7 @@ TEST_F(guard_pages, populate)
 }
 
 /* Ensure that MADV_COLD, MADV_PAGEOUT do not remove guard markers. */
-TEST_F(guard_pages, cold_pageout)
+TEST_F(guard_regions, cold_pageout)
 {
 	const unsigned long page_size = self->page_size;
 	char *ptr;
@@ -1268,7 +1268,7 @@ TEST_F(guard_pages, cold_pageout)
 }
 
 /* Ensure that guard pages do not break userfaultd. */
-TEST_F(guard_pages, uffd)
+TEST_F(guard_regions, uffd)
 {
 	const unsigned long page_size = self->page_size;
 	int uffd;
-- 
2.48.1


^ permalink raw reply related

* [PATCH 1/4] mm: allow guard regions in file-backed and read-only mappings
From: Lorenzo Stoakes @ 2025-02-13 18:17 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
	Vlastimil Babka, Paul E . McKenney, Jann Horn, David Hildenbrand,
	linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
	John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <cover.1739469950.git.lorenzo.stoakes@oracle.com>

There is no reason to disallow guard regions in file-backed mappings -
readahead and fault-around both function correctly in the presence of PTE
markers, equally other operations relating to memory-mapped files function
correctly.

Additionally, read-only mappings if introducing guard-regions, only
restrict the mapping further, which means there is no violation of any
access rights by permitting this to be so.

Removing this restriction allows for read-only mapped files (such as
executable files) correctly which would otherwise not be permitted.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/madvise.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 6ecead476a80..e01e93e179a8 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1051,13 +1051,7 @@ static bool is_valid_guard_vma(struct vm_area_struct *vma, bool allow_locked)
 	if (!allow_locked)
 		disallowed |= VM_LOCKED;
 
-	if (!vma_is_anonymous(vma))
-		return false;
-
-	if ((vma->vm_flags & (VM_MAYWRITE | disallowed)) != VM_MAYWRITE)
-		return false;
-
-	return true;
+	return !(vma->vm_flags & disallowed);
 }
 
 static bool is_guard_pte_marker(pte_t ptent)
-- 
2.48.1


^ permalink raw reply related

* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Andy Lutomirski @ 2025-02-13 17:58 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Andy Lutomirski, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
	Peter Zijlstra, Andrii Nakryiko, Kees Cook, Eyal Birger, stable,
	Jann Horn, linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Deepak Gupta, Stephen Rothwell
In-Reply-To: <Z623ZcZj6Wsbnrhs@krava>

On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Jann reported [1] possible issue when trampoline_check_ip returns
> > > address near the bottom of the address space that is allowed to
> > > call into the syscall if uretprobes are not set up.
> > >
> > > Though the mmap minimum address restrictions will typically prevent
> > > creating mappings there, let's make sure uretprobe syscall checks
> > > for that.
> >
> > It would be a layering violation, but we could perhaps do better here:
> >
> > > -       if (regs->ip != trampoline_check_ip())
> > > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> > >                 goto sigill;
> >
> > Instead of SIGILL, perhaps this should do the seccomp action?  So the
> > logic in seccomp would be (sketchily, with some real mode1 mess):
> >
> > if (is_a_real_uretprobe())
> >     skip seccomp;
>
> IIUC you want to move the address check earlier to the seccomp path..
> with the benefit that we would kill not allowed caller sooner?

The benefit would be that seccomp users that want to do something
other than killing a process (returning an error code, getting
notified, etc) could retain that functionality without the new
automatic hole being poked for uretprobe() in cases where uprobes
aren't in use or where the calling address doesn't match the uprobe
trampoline.  IOW it would reduce the scope to which we're making
seccomp behave unexpectedly.

>
> jirka
>
> >
> > where is_a_real_uretprobe() is only true if the nr and arch match
> > uretprobe *and* the address is right.
> >
> > --Andy
>

^ permalink raw reply

* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Jiri Olsa @ 2025-02-13  9:12 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko, Kees Cook, Eyal Birger, stable, Jann Horn,
	linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Deepak Gupta, Stephen Rothwell
In-Reply-To: <CALCETrVFdAFVinbpPK+q7pSQHo3=JgGxZSPZVz-y7oaG=xP3fA@mail.gmail.com>

On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:
> On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Jann reported [1] possible issue when trampoline_check_ip returns
> > address near the bottom of the address space that is allowed to
> > call into the syscall if uretprobes are not set up.
> >
> > Though the mmap minimum address restrictions will typically prevent
> > creating mappings there, let's make sure uretprobe syscall checks
> > for that.
> 
> It would be a layering violation, but we could perhaps do better here:
> 
> > -       if (regs->ip != trampoline_check_ip())
> > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> >                 goto sigill;
> 
> Instead of SIGILL, perhaps this should do the seccomp action?  So the
> logic in seccomp would be (sketchily, with some real mode1 mess):
> 
> if (is_a_real_uretprobe())
>     skip seccomp;

IIUC you want to move the address check earlier to the seccomp path..
with the benefit that we would kill not allowed caller sooner?

jirka

> 
> where is_a_real_uretprobe() is only true if the nr and arch match
> uretprobe *and* the address is right.
> 
> --Andy

^ permalink raw reply

* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Eyal Birger @ 2025-02-13  2:58 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jiri Olsa, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
	Peter Zijlstra, Andrii Nakryiko, Kees Cook, stable, Jann Horn,
	linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Deepak Gupta, Stephen Rothwell
In-Reply-To: <CALCETrVFdAFVinbpPK+q7pSQHo3=JgGxZSPZVz-y7oaG=xP3fA@mail.gmail.com>

(sorry for the HTML spam)

On Wed, Feb 12, 2025 at 5:37 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Jann reported [1] possible issue when trampoline_check_ip returns
> > address near the bottom of the address space that is allowed to
> > call into the syscall if uretprobes are not set up.
> >
> > Though the mmap minimum address restrictions will typically prevent
> > creating mappings there, let's make sure uretprobe syscall checks
> > for that.
>
> It would be a layering violation, but we could perhaps do better here:
>
> > -       if (regs->ip != trampoline_check_ip())
> > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
> >                 goto sigill;
>
> Instead of SIGILL, perhaps this should do the seccomp action?  So the
> logic in seccomp would be (sketchily, with some real mode1 mess):
>
> if (is_a_real_uretprobe())
>     skip seccomp;
>
> where is_a_real_uretprobe() is only true if the nr and arch match
> uretprobe *and* the address is right.



Why would it make sense to rely on CONFIG_SECCOMP for this check? seems
this check should be done regardless of seccomp.

Or maybe I missed something in the suggestion.

Eyal.

>
>
> --Andy

^ permalink raw reply

* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Andy Lutomirski @ 2025-02-13  1:37 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko, Kees Cook, Eyal Birger, stable, Jann Horn,
	linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
	Stephen Rothwell
In-Reply-To: <20250212220433.3624297-1-jolsa@kernel.org>

On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
>
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.

It would be a layering violation, but we could perhaps do better here:

> -       if (regs->ip != trampoline_check_ip())
> +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> +       if (unlikely(regs->ip != trampoline_check_ip(tramp)))
>                 goto sigill;

Instead of SIGILL, perhaps this should do the seccomp action?  So the
logic in seccomp would be (sketchily, with some real mode1 mess):

if (is_a_real_uretprobe())
    skip seccomp;

where is_a_real_uretprobe() is only true if the nr and arch match
uretprobe *and* the address is right.

--Andy

^ permalink raw reply

* Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Masami Hiramatsu @ 2025-02-13  0:10 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Steven Rostedt, Oleg Nesterov, Peter Zijlstra, Andrii Nakryiko,
	Kees Cook, Eyal Birger, stable, Jann Horn, linux-kernel,
	linux-trace-kernel, linux-api, x86, bpf, Thomas Gleixner,
	Ingo Molnar, Andy Lutomirski, Deepak Gupta, Stephen Rothwell
In-Reply-To: <20250212220433.3624297-1-jolsa@kernel.org>

On Wed, 12 Feb 2025 23:04:33 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
> 
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.
> 
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <kees@kernel.org>
> Cc: Eyal Birger <eyal.birger@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Reported-by: Jann Horn <jannh@google.com>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,

> ---
> v3 changes:
>  - used ~0UL instead of -1 [Alexei]
>  - used UPROBE_NO_TRAMPOLINE_VADDR in uprobe_get_trampoline_vaddr [Masami]
>  - added unlikely [Andrii]
>  - I kept the review/ack tags, because I think the change is basically
>    the same, please scream otherwise
> 
>  arch/x86/kernel/uprobes.c | 14 +++++++++-----
>  include/linux/uprobes.h   |  2 ++
>  kernel/events/uprobes.c   |  2 +-
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..9194695662b2 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
>  	return &insn;
>  }
>  
> -static unsigned long trampoline_check_ip(void)
> +static unsigned long trampoline_check_ip(unsigned long tramp)
>  {
> -	unsigned long tramp = uprobe_get_trampoline_vaddr();
> -
>  	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
>  }
>  
>  SYSCALL_DEFINE0(uretprobe)
>  {
>  	struct pt_regs *regs = task_pt_regs(current);
> -	unsigned long err, ip, sp, r11_cx_ax[3];
> +	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
> +
> +	/* If there's no trampoline, we are called from wrong place. */
> +	tramp = uprobe_get_trampoline_vaddr();
> +	if (unlikely(tramp == UPROBE_NO_TRAMPOLINE_VADDR))
> +		goto sigill;
>  
> -	if (regs->ip != trampoline_check_ip())
> +	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
> +	if (unlikely(regs->ip != trampoline_check_ip(tramp)))
>  		goto sigill;
>  
>  	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index a40efdda9052..2e46b69ff0a6 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -39,6 +39,8 @@ struct page;
>  
>  #define MAX_URETPROBE_DEPTH		64
>  
> +#define UPROBE_NO_TRAMPOLINE_VADDR	(~0UL)
> +
>  struct uprobe_consumer {
>  	/*
>  	 * handler() can return UPROBE_HANDLER_REMOVE to signal the need to
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 597b9e036e5f..c5d6307bc5bc 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -2156,8 +2156,8 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
>   */
>  unsigned long uprobe_get_trampoline_vaddr(void)
>  {
> +	unsigned long trampoline_vaddr = UPROBE_NO_TRAMPOLINE_VADDR;
>  	struct xol_area *area;
> -	unsigned long trampoline_vaddr = -1;
>  
>  	/* Pairs with xol_add_vma() smp_store_release() */
>  	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */
> -- 
> 2.48.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply

* [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Jiri Olsa @ 2025-02-12 22:04 UTC (permalink / raw)
  To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko
  Cc: Kees Cook, Eyal Birger, stable, Jann Horn, linux-kernel,
	linux-trace-kernel, linux-api, x86, bpf, Thomas Gleixner,
	Ingo Molnar, Andy Lutomirski, Deepak Gupta, Stephen Rothwell

Jann reported [1] possible issue when trampoline_check_ip returns
address near the bottom of the address space that is allowed to
call into the syscall if uretprobes are not set up.

Though the mmap minimum address restrictions will typically prevent
creating mappings there, let's make sure uretprobe syscall checks
for that.

[1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
Cc: Kees Cook <kees@kernel.org>
Cc: Eyal Birger <eyal.birger@gmail.com>
Cc: stable@vger.kernel.org
Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reported-by: Jann Horn <jannh@google.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
v3 changes:
 - used ~0UL instead of -1 [Alexei]
 - used UPROBE_NO_TRAMPOLINE_VADDR in uprobe_get_trampoline_vaddr [Masami]
 - added unlikely [Andrii]
 - I kept the review/ack tags, because I think the change is basically
   the same, please scream otherwise

 arch/x86/kernel/uprobes.c | 14 +++++++++-----
 include/linux/uprobes.h   |  2 ++
 kernel/events/uprobes.c   |  2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..9194695662b2 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
-static unsigned long trampoline_check_ip(void)
+static unsigned long trampoline_check_ip(unsigned long tramp)
 {
-	unsigned long tramp = uprobe_get_trampoline_vaddr();
-
 	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
 }
 
 SYSCALL_DEFINE0(uretprobe)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	unsigned long err, ip, sp, r11_cx_ax[3];
+	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+
+	/* If there's no trampoline, we are called from wrong place. */
+	tramp = uprobe_get_trampoline_vaddr();
+	if (unlikely(tramp == UPROBE_NO_TRAMPOLINE_VADDR))
+		goto sigill;
 
-	if (regs->ip != trampoline_check_ip())
+	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
+	if (unlikely(regs->ip != trampoline_check_ip(tramp)))
 		goto sigill;
 
 	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index a40efdda9052..2e46b69ff0a6 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -39,6 +39,8 @@ struct page;
 
 #define MAX_URETPROBE_DEPTH		64
 
+#define UPROBE_NO_TRAMPOLINE_VADDR	(~0UL)
+
 struct uprobe_consumer {
 	/*
 	 * handler() can return UPROBE_HANDLER_REMOVE to signal the need to
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 597b9e036e5f..c5d6307bc5bc 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2156,8 +2156,8 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
  */
 unsigned long uprobe_get_trampoline_vaddr(void)
 {
+	unsigned long trampoline_vaddr = UPROBE_NO_TRAMPOLINE_VADDR;
 	struct xol_area *area;
-	unsigned long trampoline_vaddr = -1;
 
 	/* Pairs with xol_add_vma() smp_store_release() */
 	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */
-- 
2.48.1


^ permalink raw reply related

* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Jiri Olsa @ 2025-02-12 13:51 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Oleg Nesterov, Alexei Starovoitov, Andrii Nakryiko,
	Steven Rostedt, Peter Zijlstra, Andrii Nakryiko, Kees Cook,
	Eyal Birger, stable, Jann Horn, LKML, linux-trace-kernel,
	Linux API, X86 ML, bpf, Thomas Gleixner, Ingo Molnar,
	Andy Lutomirski, Deepak Gupta, Stephen Rothwell
In-Reply-To: <20250212130509.ce1987095c6b17b26d3ee40a@kernel.org>

On Wed, Feb 12, 2025 at 01:05:09PM +0900, Masami Hiramatsu wrote:
> On Tue, 11 Feb 2025 17:59:41 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > On 02/11, Alexei Starovoitov wrote:
> > >
> > > > > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> > >
> > > If you respin anyway maybe use ~0UL instead?
> > > In the above and in
> > > uprobe_get_trampoline_vaddr(),
> > > since
> > >
> > > unsigned long trampoline_vaddr = -1;
> > 
> > ... or -1ul in both cases.
> > 
> > I agree, UPROBE_NO_TRAMPOLINE_VADDR has a single user, looks
> > a bit strange...
> 
> I think both this function and uprobe_get_trampoline_vaddr()
> should use the same macro as a token.
> (and ~0UL is a bit more comfortable for me too :) )
> 
> ----
> unsigned long uprobe_get_trampoline_vaddr(void)
> {
> 	struct xol_area *area;
> 	unsigned long trampoline_vaddr = -1;
> ----


sounds good, I'll send new version with change below if there
are no objections

thanks,
jirka


---
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..015b2a6bac11 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
-static unsigned long trampoline_check_ip(void)
+static unsigned long trampoline_check_ip(unsigned long tramp)
 {
-	unsigned long tramp = uprobe_get_trampoline_vaddr();
-
 	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
 }
 
 SYSCALL_DEFINE0(uretprobe)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	unsigned long err, ip, sp, r11_cx_ax[3];
+	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+
+	/* If there's no trampoline, we are called from wrong place. */
+	tramp = uprobe_get_trampoline_vaddr();
+	if (tramp == UPROBE_NO_TRAMPOLINE_VADDR)
+		goto sigill;
 
-	if (regs->ip != trampoline_check_ip())
+	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
+	if (regs->ip != trampoline_check_ip(tramp))
 		goto sigill;
 
 	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index b1df7d792fa1..a6bec560bdbc 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -39,6 +39,8 @@ struct page;
 
 #define MAX_URETPROBE_DEPTH		64
 
+#define UPROBE_NO_TRAMPOLINE_VADDR	(~0UL)
+
 struct uprobe_consumer {
 	/*
 	 * handler() can return UPROBE_HANDLER_REMOVE to signal the need to
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2ca797cbe465..e8af2f75b094 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -2160,8 +2160,8 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
  */
 unsigned long uprobe_get_trampoline_vaddr(void)
 {
+	unsigned long trampoline_vaddr = UPROBE_NO_TRAMPOLINE_VADDR;
 	struct xol_area *area;
-	unsigned long trampoline_vaddr = -1;
 
 	/* Pairs with xol_add_vma() smp_store_release() */
 	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */

^ permalink raw reply related

* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Masami Hiramatsu @ 2025-02-12  4:05 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Alexei Starovoitov, Andrii Nakryiko, Jiri Olsa, Steven Rostedt,
	Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko, Kees Cook,
	Eyal Birger, stable, Jann Horn, LKML, linux-trace-kernel,
	Linux API, X86 ML, bpf, Thomas Gleixner, Ingo Molnar,
	Andy Lutomirski, Deepak Gupta, Stephen Rothwell
In-Reply-To: <20250211165940.GB9174@redhat.com>

On Tue, 11 Feb 2025 17:59:41 +0100
Oleg Nesterov <oleg@redhat.com> wrote:

> On 02/11, Alexei Starovoitov wrote:
> >
> > > > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> >
> > If you respin anyway maybe use ~0UL instead?
> > In the above and in
> > uprobe_get_trampoline_vaddr(),
> > since
> >
> > unsigned long trampoline_vaddr = -1;
> 
> ... or -1ul in both cases.
> 
> I agree, UPROBE_NO_TRAMPOLINE_VADDR has a single user, looks
> a bit strange...

I think both this function and uprobe_get_trampoline_vaddr()
should use the same macro as a token.
(and ~0UL is a bit more comfortable for me too :) )

----
unsigned long uprobe_get_trampoline_vaddr(void)
{
	struct xol_area *area;
	unsigned long trampoline_vaddr = -1;
----

Thank you,

> 
> Oleg.
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

^ permalink raw reply

* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Arnd Bergmann @ 2025-02-11 19:24 UTC (permalink / raw)
  To: Andrey Albershteyn, Richard Henderson, Matt Turner, Russell King,
	Catalin Marinas, Will Deacon, Geert Uytterhoeven, Michal Simek,
	Thomas Bogendoerfer, James E . J . Bottomley, Helge Deller,
	Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
	Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
	David S . Miller, Andreas Larsson, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
	Christian Brauner, Jan Kara, Mickaël Salaün,
	Günther Noack
  Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
	linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, linux-fsdevel, linux-security-module, linux-api,
	Linux-Arch, linux-xfs
In-Reply-To: <20250211-xattrat-syscall-v3-1-a07d15f898b2@kernel.org>

On Tue, Feb 11, 2025, at 18:22, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@redhat.com>
>
> Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
> extended attributes/flags. The syscalls take parent directory fd and
> path to the child together with struct fsxattr.
>
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open as we can reference it with a path
> instead of fd. By having this we can manipulated inode extended
> attributes not only on regular files but also on special ones. This
> is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> we can not call ioctl() directly on the filesystem inode using fd.
>
> This patch adds two new syscalls which allows userspace to get/set
> extended inode attributes on special files by using parent directory
> and a path - *at() like syscall.
>
> Also, as vfs_fileattr_set() is now will be called on special files
> too, let's forbid any other attributes except projid and nextents
> (symlink can have an extent).
>
> CC: linux-api@vger.kernel.org
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>

I checked the syscall.tbl additions and the ABI to ensure that
it follows the usual guidelines and is portable across
all architectures, this looks good. Thanks for addressing
my v1 comments:

Acked-by: Arnd Bergmann <arnd@arndb.de>

Disclaimer: I have no idea if the new syscalls are a good
idea or if they are fit for the purpose, I trust the
VFS maintainers will take care of reviewing that.

^ permalink raw reply

* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: H. Peter Anvin @ 2025-02-11 19:09 UTC (permalink / raw)
  To: Andrey Albershteyn, Richard Henderson, Matt Turner, Russell King,
	Catalin Marinas, Will Deacon, Geert Uytterhoeven, Michal Simek,
	Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
	Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
	Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
	David S. Miller, Andreas Larsson, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	Chris Zankel, Max Filippov, Alexander Viro, Christian Brauner,
	Jan Kara, Mickaël Salaün, Günther Noack,
	Arnd Bergmann
  Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
	linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, linux-fsdevel, linux-security-module, linux-api,
	linux-arch, linux-xfs
In-Reply-To: <20250211-xattrat-syscall-v3-1-a07d15f898b2@kernel.org>

On February 11, 2025 9:22:47 AM PST, Andrey Albershteyn <aalbersh@redhat.com> wrote:
>From: Andrey Albershteyn <aalbersh@redhat.com>
>
>Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
>extended attributes/flags. The syscalls take parent directory fd and
>path to the child together with struct fsxattr.
>
>This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
>that file don't need to be open as we can reference it with a path
>instead of fd. By having this we can manipulated inode extended
>attributes not only on regular files but also on special ones. This
>is not possible with FS_IOC_FSSETXATTR ioctl as with special files
>we can not call ioctl() directly on the filesystem inode using fd.
>
>This patch adds two new syscalls which allows userspace to get/set
>extended inode attributes on special files by using parent directory
>and a path - *at() like syscall.
>
>Also, as vfs_fileattr_set() is now will be called on special files
>too, let's forbid any other attributes except projid and nextents
>(symlink can have an extent).
>
>CC: linux-api@vger.kernel.org
>CC: linux-fsdevel@vger.kernel.org
>CC: linux-xfs@vger.kernel.org
>Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
>---
>v1:
>https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
>
>Previous discussion:
>https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/
>
>XFS has project quotas which could be attached to a directory. All
>new inodes in these directories inherit project ID set on parent
>directory.
>
>The project is created from userspace by opening and calling
>FS_IOC_FSSETXATTR on each inode. This is not possible for special
>files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
>with empty project ID. Those inodes then are not shown in the quota
>accounting but still exist in the directory. Moreover, in the case
>when special files are created in the directory with already
>existing project quota, these inode inherit extended attributes.
>This than leaves them with these attributes without the possibility
>to clear them out. This, in turn, prevents userspace from
>re-creating quota project on these existing files.
>---
>Changes in v3:
>- Remove unnecessary "dfd is dir" check as it checked in user_path_at()
>- Remove unnecessary "same filesystem" check
>- Use CLASS() instead of directly calling fdget/fdput
>- Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org
>---
> arch/alpha/kernel/syscalls/syscall.tbl      |  2 +
> arch/arm/tools/syscall.tbl                  |  2 +
> arch/arm64/tools/syscall_32.tbl             |  2 +
> arch/m68k/kernel/syscalls/syscall.tbl       |  2 +
> arch/microblaze/kernel/syscalls/syscall.tbl |  2 +
> arch/mips/kernel/syscalls/syscall_n32.tbl   |  2 +
> arch/mips/kernel/syscalls/syscall_n64.tbl   |  2 +
> arch/mips/kernel/syscalls/syscall_o32.tbl   |  2 +
> arch/parisc/kernel/syscalls/syscall.tbl     |  2 +
> arch/powerpc/kernel/syscalls/syscall.tbl    |  2 +
> arch/s390/kernel/syscalls/syscall.tbl       |  2 +
> arch/sh/kernel/syscalls/syscall.tbl         |  2 +
> arch/sparc/kernel/syscalls/syscall.tbl      |  2 +
> arch/x86/entry/syscalls/syscall_32.tbl      |  2 +
> arch/x86/entry/syscalls/syscall_64.tbl      |  2 +
> arch/xtensa/kernel/syscalls/syscall.tbl     |  2 +
> fs/inode.c                                  | 75 +++++++++++++++++++++++++++++
> fs/ioctl.c                                  | 16 +++++-
> include/linux/fileattr.h                    |  1 +
> include/linux/syscalls.h                    |  4 ++
> include/uapi/asm-generic/unistd.h           |  8 ++-
> 21 files changed, 133 insertions(+), 3 deletions(-)
>
>diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
>index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 100644
>--- a/arch/alpha/kernel/syscalls/syscall.tbl
>+++ b/arch/alpha/kernel/syscalls/syscall.tbl
>@@ -506,3 +506,5 @@
> 574	common	getxattrat			sys_getxattrat
> 575	common	listxattrat			sys_listxattrat
> 576	common	removexattrat			sys_removexattrat
>+577	common	getfsxattrat			sys_getfsxattrat
>+578	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
>index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
>--- a/arch/arm/tools/syscall.tbl
>+++ b/arch/arm/tools/syscall.tbl
>@@ -481,3 +481,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
>index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
>--- a/arch/arm64/tools/syscall_32.tbl
>+++ b/arch/arm64/tools/syscall_32.tbl
>@@ -478,3 +478,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
>index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 100644
>--- a/arch/m68k/kernel/syscalls/syscall.tbl
>+++ b/arch/m68k/kernel/syscalls/syscall.tbl
>@@ -466,3 +466,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
>index 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 100644
>--- a/arch/microblaze/kernel/syscalls/syscall.tbl
>+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
>@@ -472,3 +472,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
>index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
>--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
>+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
>@@ -405,3 +405,5 @@
> 464	n32	getxattrat			sys_getxattrat
> 465	n32	listxattrat			sys_listxattrat
> 466	n32	removexattrat			sys_removexattrat
>+467	n32	getfsxattrat			sys_getfsxattrat
>+468	n32	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
>index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
>--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
>+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
>@@ -381,3 +381,5 @@
> 464	n64	getxattrat			sys_getxattrat
> 465	n64	listxattrat			sys_listxattrat
> 466	n64	removexattrat			sys_removexattrat
>+467	n64	getfsxattrat			sys_getfsxattrat
>+468	n64	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
>index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
>--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
>+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
>@@ -454,3 +454,5 @@
> 464	o32	getxattrat			sys_getxattrat
> 465	o32	listxattrat			sys_listxattrat
> 466	o32	removexattrat			sys_removexattrat
>+467	o32	getfsxattrat			sys_getfsxattrat
>+468	o32	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
>index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 100644
>--- a/arch/parisc/kernel/syscalls/syscall.tbl
>+++ b/arch/parisc/kernel/syscalls/syscall.tbl
>@@ -465,3 +465,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
>index d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 100644
>--- a/arch/powerpc/kernel/syscalls/syscall.tbl
>+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
>@@ -557,3 +557,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
>index e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 100644
>--- a/arch/s390/kernel/syscalls/syscall.tbl
>+++ b/arch/s390/kernel/syscalls/syscall.tbl
>@@ -469,3 +469,5 @@
> 464  common	getxattrat		sys_getxattrat			sys_getxattrat
> 465  common	listxattrat		sys_listxattrat			sys_listxattrat
> 466  common	removexattrat		sys_removexattrat		sys_removexattrat
>+467  common	getfsxattrat		sys_getfsxattrat		sys_getfsxattrat
>+468  common	setfsxattrat		sys_setfsxattrat		sys_setfsxattrat
>diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
>index c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 100644
>--- a/arch/sh/kernel/syscalls/syscall.tbl
>+++ b/arch/sh/kernel/syscalls/syscall.tbl
>@@ -470,3 +470,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
>index 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 100644
>--- a/arch/sparc/kernel/syscalls/syscall.tbl
>+++ b/arch/sparc/kernel/syscalls/syscall.tbl
>@@ -512,3 +512,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
>index 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 100644
>--- a/arch/x86/entry/syscalls/syscall_32.tbl
>+++ b/arch/x86/entry/syscalls/syscall_32.tbl
>@@ -472,3 +472,5 @@
> 464	i386	getxattrat		sys_getxattrat
> 465	i386	listxattrat		sys_listxattrat
> 466	i386	removexattrat		sys_removexattrat
>+467	i386	getfsxattrat		sys_getfsxattrat
>+468	i386	setfsxattrat		sys_setfsxattrat
>diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
>index 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 100644
>--- a/arch/x86/entry/syscalls/syscall_64.tbl
>+++ b/arch/x86/entry/syscalls/syscall_64.tbl
>@@ -390,6 +390,8 @@
> 464	common	getxattrat		sys_getxattrat
> 465	common	listxattrat		sys_listxattrat
> 466	common	removexattrat		sys_removexattrat
>+467	common	getfsxattrat		sys_getfsxattrat
>+468	common	setfsxattrat		sys_setfsxattrat
> 
> #
> # Due to a historical design error, certain syscalls are numbered differently
>diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
>index 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 100644
>--- a/arch/xtensa/kernel/syscalls/syscall.tbl
>+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
>@@ -437,3 +437,5 @@
> 464	common	getxattrat			sys_getxattrat
> 465	common	listxattrat			sys_listxattrat
> 466	common	removexattrat			sys_removexattrat
>+467	common	getfsxattrat			sys_getfsxattrat
>+468	common	setfsxattrat			sys_setfsxattrat
>diff --git a/fs/inode.c b/fs/inode.c
>index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..b2dddd9db4fabaf67a6cbf541a86978b290411ec 100644
>--- a/fs/inode.c
>+++ b/fs/inode.c
>@@ -23,6 +23,9 @@
> #include <linux/rw_hint.h>
> #include <linux/seq_file.h>
> #include <linux/debugfs.h>
>+#include <linux/syscalls.h>
>+#include <linux/fileattr.h>
>+#include <linux/namei.h>
> #include <trace/events/writeback.h>
> #define CREATE_TRACE_POINTS
> #include <trace/events/timestamp.h>
>@@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
> 	return mode & ~S_ISGID;
> }
> EXPORT_SYMBOL(mode_strip_sgid);
>+
>+SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
>+		struct fsxattr __user *, fsx, unsigned int, at_flags)
>+{
>+	CLASS(fd, dir)(dfd);
>+	struct fileattr fa;
>+	struct path filepath;
>+	int error;
>+	unsigned int lookup_flags = 0;
>+
>+	if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
>+		return -EINVAL;
>+
>+	if (at_flags & AT_SYMLINK_FOLLOW)
>+		lookup_flags |= LOOKUP_FOLLOW;
>+
>+	if (at_flags & AT_EMPTY_PATH)
>+		lookup_flags |= LOOKUP_EMPTY;
>+
>+	if (fd_empty(dir))
>+		return -EBADF;
>+
>+	error = user_path_at(dfd, filename, lookup_flags, &filepath);
>+	if (error)
>+		return error;
>+
>+	error = vfs_fileattr_get(filepath.dentry, &fa);
>+	if (!error)
>+		error = copy_fsxattr_to_user(&fa, fsx);
>+
>+	path_put(&filepath);
>+	return error;
>+}
>+
>+SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
>+		struct fsxattr __user *, fsx, unsigned int, at_flags)
>+{
>+	CLASS(fd, dir)(dfd);
>+	struct fileattr fa;
>+	struct path filepath;
>+	int error;
>+	unsigned int lookup_flags = 0;
>+
>+	if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
>+		return -EINVAL;
>+
>+	if (at_flags & AT_SYMLINK_FOLLOW)
>+		lookup_flags |= LOOKUP_FOLLOW;
>+
>+	if (at_flags & AT_EMPTY_PATH)
>+		lookup_flags |= LOOKUP_EMPTY;
>+
>+	if (fd_empty(dir))
>+		return -EBADF;
>+
>+	if (copy_fsxattr_from_user(&fa, fsx))
>+		return -EFAULT;
>+
>+	error = user_path_at(dfd, filename, lookup_flags, &filepath);
>+	if (error)
>+		return error;
>+
>+	error = mnt_want_write(filepath.mnt);
>+	if (!error) {
>+		error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)),
>+					 filepath.dentry, &fa);
>+		mnt_drop_write(filepath.mnt);
>+	}
>+
>+	path_put(&filepath);
>+	return error;
>+}
>diff --git a/fs/ioctl.c b/fs/ioctl.c
>index 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 100644
>--- a/fs/ioctl.c
>+++ b/fs/ioctl.c
>@@ -558,8 +558,7 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> }
> EXPORT_SYMBOL(copy_fsxattr_to_user);
> 
>-static int copy_fsxattr_from_user(struct fileattr *fa,
>-				  struct fsxattr __user *ufa)
>+int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
> {
> 	struct fsxattr xfa;
> 
>@@ -646,6 +645,19 @@ static int fileattr_set_prepare(struct inode *inode,
> 	if (fa->fsx_cowextsize == 0)
> 		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> 
>+	/*
>+	 * The only use case for special files is to set project ID, forbid any
>+	 * other attributes
>+	 */
>+	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
>+		if (fa->fsx_xflags & ~FS_XFLAG_PROJINHERIT)
>+			return -EINVAL;
>+		if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)
>+			return -EINVAL;
>+		if (fa->fsx_extsize || fa->fsx_cowextsize)
>+			return -EINVAL;
>+	}
>+
> 	return 0;
> }
> 
>diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
>index 47c05a9851d0600964b644c9c7218faacfd865f8..8598e94b530b8b280a2697eaf918dd60f573d6ee 100644
>--- a/include/linux/fileattr.h
>+++ b/include/linux/fileattr.h
>@@ -34,6 +34,7 @@ struct fileattr {
> };
> 
> int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
>+int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa);
> 
> void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
> void fileattr_fill_flags(struct fileattr *fa, u32 flags);
>diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>index c6333204d45130eb022f6db460eea34a1f6e91db..3134d463d9af64c6e78adb37bff4b91f77b5305f 100644
>--- a/include/linux/syscalls.h
>+++ b/include/linux/syscalls.h
>@@ -371,6 +371,10 @@ asmlinkage long sys_removexattrat(int dfd, const char __user *path,
> asmlinkage long sys_lremovexattr(const char __user *path,
> 				 const char __user *name);
> asmlinkage long sys_fremovexattr(int fd, const char __user *name);
>+asmlinkage long sys_getfsxattrat(int dfd, const char __user *filename,
>+				 struct fsxattr *fsx, unsigned int at_flags);
>+asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
>+				 struct fsxattr *fsx, unsigned int at_flags);
> asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
> asmlinkage long sys_eventfd2(unsigned int count, int flags);
> asmlinkage long sys_epoll_create1(int flags);
>diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
>index 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 100644
>--- a/include/uapi/asm-generic/unistd.h
>+++ b/include/uapi/asm-generic/unistd.h
>@@ -850,8 +850,14 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
> #define __NR_removexattrat 466
> __SYSCALL(__NR_removexattrat, sys_removexattrat)
> 
>+/* fs/inode.c */
>+#define __NR_getfsxattrat 467
>+__SYSCALL(__NR_getfsxattrat, sys_getfsxattrat)
>+#define __NR_setfsxattrat 468
>+__SYSCALL(__NR_setfsxattrat, sys_setfsxattrat)
>+
> #undef __NR_syscalls
>-#define __NR_syscalls 467
>+#define __NR_syscalls 469
> 
> /*
>  * 32 bit systems traditionally used different
>
>---
>base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
>change-id: 20250114-xattrat-syscall-6a1136d2db59
>
>Best regards,

Could you please give a quick description of the API – even just the prototype – and, for the future, include in the cover letter?

^ permalink raw reply

* [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-02-11 17:22 UTC (permalink / raw)
  To: Richard Henderson, Matt Turner, Russell King, Catalin Marinas,
	Will Deacon, Geert Uytterhoeven, Michal Simek,
	Thomas Bogendoerfer, James E.J. Bottomley, Helge Deller,
	Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
	Christophe Leroy, Naveen N Rao, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
	Yoshinori Sato, Rich Felker, John Paul Adrian Glaubitz,
	David S. Miller, Andreas Larsson, Andy Lutomirski,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Chris Zankel, Max Filippov, Alexander Viro,
	Christian Brauner, Jan Kara, Mickaël Salaün,
	Günther Noack, Arnd Bergmann
  Cc: linux-alpha, linux-kernel, linux-arm-kernel, linux-m68k,
	linux-mips, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
	sparclinux, linux-fsdevel, linux-security-module, linux-api,
	linux-arch, Andrey Albershteyn, linux-xfs

From: Andrey Albershteyn <aalbersh@redhat.com>

Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
extended attributes/flags. The syscalls take parent directory fd and
path to the child together with struct fsxattr.

This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
that file don't need to be open as we can reference it with a path
instead of fd. By having this we can manipulated inode extended
attributes not only on regular files but also on special ones. This
is not possible with FS_IOC_FSSETXATTR ioctl as with special files
we can not call ioctl() directly on the filesystem inode using fd.

This patch adds two new syscalls which allows userspace to get/set
extended inode attributes on special files by using parent directory
and a path - *at() like syscall.

Also, as vfs_fileattr_set() is now will be called on special files
too, let's forbid any other attributes except projid and nextents
(symlink can have an extent).

CC: linux-api@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
CC: linux-xfs@vger.kernel.org
Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
v1:
https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/

Previous discussion:
https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/

XFS has project quotas which could be attached to a directory. All
new inodes in these directories inherit project ID set on parent
directory.

The project is created from userspace by opening and calling
FS_IOC_FSSETXATTR on each inode. This is not possible for special
files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
with empty project ID. Those inodes then are not shown in the quota
accounting but still exist in the directory. Moreover, in the case
when special files are created in the directory with already
existing project quota, these inode inherit extended attributes.
This than leaves them with these attributes without the possibility
to clear them out. This, in turn, prevents userspace from
re-creating quota project on these existing files.
---
Changes in v3:
- Remove unnecessary "dfd is dir" check as it checked in user_path_at()
- Remove unnecessary "same filesystem" check
- Use CLASS() instead of directly calling fdget/fdput
- Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org
---
 arch/alpha/kernel/syscalls/syscall.tbl      |  2 +
 arch/arm/tools/syscall.tbl                  |  2 +
 arch/arm64/tools/syscall_32.tbl             |  2 +
 arch/m68k/kernel/syscalls/syscall.tbl       |  2 +
 arch/microblaze/kernel/syscalls/syscall.tbl |  2 +
 arch/mips/kernel/syscalls/syscall_n32.tbl   |  2 +
 arch/mips/kernel/syscalls/syscall_n64.tbl   |  2 +
 arch/mips/kernel/syscalls/syscall_o32.tbl   |  2 +
 arch/parisc/kernel/syscalls/syscall.tbl     |  2 +
 arch/powerpc/kernel/syscalls/syscall.tbl    |  2 +
 arch/s390/kernel/syscalls/syscall.tbl       |  2 +
 arch/sh/kernel/syscalls/syscall.tbl         |  2 +
 arch/sparc/kernel/syscalls/syscall.tbl      |  2 +
 arch/x86/entry/syscalls/syscall_32.tbl      |  2 +
 arch/x86/entry/syscalls/syscall_64.tbl      |  2 +
 arch/xtensa/kernel/syscalls/syscall.tbl     |  2 +
 fs/inode.c                                  | 75 +++++++++++++++++++++++++++++
 fs/ioctl.c                                  | 16 +++++-
 include/linux/fileattr.h                    |  1 +
 include/linux/syscalls.h                    |  4 ++
 include/uapi/asm-generic/unistd.h           |  8 ++-
 21 files changed, 133 insertions(+), 3 deletions(-)

diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -506,3 +506,5 @@
 574	common	getxattrat			sys_getxattrat
 575	common	listxattrat			sys_listxattrat
 576	common	removexattrat			sys_removexattrat
+577	common	getfsxattrat			sys_getfsxattrat
+578	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -481,3 +481,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
--- a/arch/arm64/tools/syscall_32.tbl
+++ b/arch/arm64/tools/syscall_32.tbl
@@ -478,3 +478,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -466,3 +466,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -472,3 +472,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -405,3 +405,5 @@
 464	n32	getxattrat			sys_getxattrat
 465	n32	listxattrat			sys_listxattrat
 466	n32	removexattrat			sys_removexattrat
+467	n32	getfsxattrat			sys_getfsxattrat
+468	n32	setfsxattrat			sys_setfsxattrat
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -381,3 +381,5 @@
 464	n64	getxattrat			sys_getxattrat
 465	n64	listxattrat			sys_listxattrat
 466	n64	removexattrat			sys_removexattrat
+467	n64	getfsxattrat			sys_getfsxattrat
+468	n64	setfsxattrat			sys_setfsxattrat
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -454,3 +454,5 @@
 464	o32	getxattrat			sys_getxattrat
 465	o32	listxattrat			sys_listxattrat
 466	o32	removexattrat			sys_removexattrat
+467	o32	getfsxattrat			sys_getfsxattrat
+468	o32	setfsxattrat			sys_setfsxattrat
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -465,3 +465,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -557,3 +557,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -469,3 +469,5 @@
 464  common	getxattrat		sys_getxattrat			sys_getxattrat
 465  common	listxattrat		sys_listxattrat			sys_listxattrat
 466  common	removexattrat		sys_removexattrat		sys_removexattrat
+467  common	getfsxattrat		sys_getfsxattrat		sys_getfsxattrat
+468  common	setfsxattrat		sys_setfsxattrat		sys_setfsxattrat
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -470,3 +470,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -512,3 +512,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -472,3 +472,5 @@
 464	i386	getxattrat		sys_getxattrat
 465	i386	listxattrat		sys_listxattrat
 466	i386	removexattrat		sys_removexattrat
+467	i386	getfsxattrat		sys_getfsxattrat
+468	i386	setfsxattrat		sys_setfsxattrat
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -390,6 +390,8 @@
 464	common	getxattrat		sys_getxattrat
 465	common	listxattrat		sys_listxattrat
 466	common	removexattrat		sys_removexattrat
+467	common	getfsxattrat		sys_getfsxattrat
+468	common	setfsxattrat		sys_setfsxattrat
 
 #
 # Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -437,3 +437,5 @@
 464	common	getxattrat			sys_getxattrat
 465	common	listxattrat			sys_listxattrat
 466	common	removexattrat			sys_removexattrat
+467	common	getfsxattrat			sys_getfsxattrat
+468	common	setfsxattrat			sys_setfsxattrat
diff --git a/fs/inode.c b/fs/inode.c
index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..b2dddd9db4fabaf67a6cbf541a86978b290411ec 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -23,6 +23,9 @@
 #include <linux/rw_hint.h>
 #include <linux/seq_file.h>
 #include <linux/debugfs.h>
+#include <linux/syscalls.h>
+#include <linux/fileattr.h>
+#include <linux/namei.h>
 #include <trace/events/writeback.h>
 #define CREATE_TRACE_POINTS
 #include <trace/events/timestamp.h>
@@ -2953,3 +2956,75 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
 	return mode & ~S_ISGID;
 }
 EXPORT_SYMBOL(mode_strip_sgid);
+
+SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
+		struct fsxattr __user *, fsx, unsigned int, at_flags)
+{
+	CLASS(fd, dir)(dfd);
+	struct fileattr fa;
+	struct path filepath;
+	int error;
+	unsigned int lookup_flags = 0;
+
+	if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+		return -EINVAL;
+
+	if (at_flags & AT_SYMLINK_FOLLOW)
+		lookup_flags |= LOOKUP_FOLLOW;
+
+	if (at_flags & AT_EMPTY_PATH)
+		lookup_flags |= LOOKUP_EMPTY;
+
+	if (fd_empty(dir))
+		return -EBADF;
+
+	error = user_path_at(dfd, filename, lookup_flags, &filepath);
+	if (error)
+		return error;
+
+	error = vfs_fileattr_get(filepath.dentry, &fa);
+	if (!error)
+		error = copy_fsxattr_to_user(&fa, fsx);
+
+	path_put(&filepath);
+	return error;
+}
+
+SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
+		struct fsxattr __user *, fsx, unsigned int, at_flags)
+{
+	CLASS(fd, dir)(dfd);
+	struct fileattr fa;
+	struct path filepath;
+	int error;
+	unsigned int lookup_flags = 0;
+
+	if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
+		return -EINVAL;
+
+	if (at_flags & AT_SYMLINK_FOLLOW)
+		lookup_flags |= LOOKUP_FOLLOW;
+
+	if (at_flags & AT_EMPTY_PATH)
+		lookup_flags |= LOOKUP_EMPTY;
+
+	if (fd_empty(dir))
+		return -EBADF;
+
+	if (copy_fsxattr_from_user(&fa, fsx))
+		return -EFAULT;
+
+	error = user_path_at(dfd, filename, lookup_flags, &filepath);
+	if (error)
+		return error;
+
+	error = mnt_want_write(filepath.mnt);
+	if (!error) {
+		error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)),
+					 filepath.dentry, &fa);
+		mnt_drop_write(filepath.mnt);
+	}
+
+	path_put(&filepath);
+	return error;
+}
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -558,8 +558,7 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
 }
 EXPORT_SYMBOL(copy_fsxattr_to_user);
 
-static int copy_fsxattr_from_user(struct fileattr *fa,
-				  struct fsxattr __user *ufa)
+int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
 {
 	struct fsxattr xfa;
 
@@ -646,6 +645,19 @@ static int fileattr_set_prepare(struct inode *inode,
 	if (fa->fsx_cowextsize == 0)
 		fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
 
+	/*
+	 * The only use case for special files is to set project ID, forbid any
+	 * other attributes
+	 */
+	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
+		if (fa->fsx_xflags & ~FS_XFLAG_PROJINHERIT)
+			return -EINVAL;
+		if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)
+			return -EINVAL;
+		if (fa->fsx_extsize || fa->fsx_cowextsize)
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 47c05a9851d0600964b644c9c7218faacfd865f8..8598e94b530b8b280a2697eaf918dd60f573d6ee 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -34,6 +34,7 @@ struct fileattr {
 };
 
 int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
+int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa);
 
 void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
 void fileattr_fill_flags(struct fileattr *fa, u32 flags);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index c6333204d45130eb022f6db460eea34a1f6e91db..3134d463d9af64c6e78adb37bff4b91f77b5305f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -371,6 +371,10 @@ asmlinkage long sys_removexattrat(int dfd, const char __user *path,
 asmlinkage long sys_lremovexattr(const char __user *path,
 				 const char __user *name);
 asmlinkage long sys_fremovexattr(int fd, const char __user *name);
+asmlinkage long sys_getfsxattrat(int dfd, const char __user *filename,
+				 struct fsxattr *fsx, unsigned int at_flags);
+asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
+				 struct fsxattr *fsx, unsigned int at_flags);
 asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
 asmlinkage long sys_eventfd2(unsigned int count, int flags);
 asmlinkage long sys_epoll_create1(int flags);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -850,8 +850,14 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
 #define __NR_removexattrat 466
 __SYSCALL(__NR_removexattrat, sys_removexattrat)
 
+/* fs/inode.c */
+#define __NR_getfsxattrat 467
+__SYSCALL(__NR_getfsxattrat, sys_getfsxattrat)
+#define __NR_setfsxattrat 468
+__SYSCALL(__NR_setfsxattrat, sys_setfsxattrat)
+
 #undef __NR_syscalls
-#define __NR_syscalls 467
+#define __NR_syscalls 469
 
 /*
  * 32 bit systems traditionally used different

---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250114-xattrat-syscall-6a1136d2db59

Best regards,
-- 
Andrey Albershteyn <aalbersh@kernel.org>


^ permalink raw reply related

* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Oleg Nesterov @ 2025-02-11 16:59 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, Jiri Olsa, Steven Rostedt, Masami Hiramatsu,
	Peter Zijlstra, Andrii Nakryiko, Kees Cook, Eyal Birger, stable,
	Jann Horn, LKML, linux-trace-kernel, Linux API, X86 ML, bpf,
	Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
	Stephen Rothwell
In-Reply-To: <CAADnVQJ05xkXw+c_T1qB+ECUqO5sJxDVJ3bypjS3KSQCTJb-1g@mail.gmail.com>

On 02/11, Alexei Starovoitov wrote:
>
> > > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
>
> If you respin anyway maybe use ~0UL instead?
> In the above and in
> uprobe_get_trampoline_vaddr(),
> since
>
> unsigned long trampoline_vaddr = -1;

... or -1ul in both cases.

I agree, UPROBE_NO_TRAMPOLINE_VADDR has a single user, looks
a bit strange...

Oleg.


^ permalink raw reply

* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Alexei Starovoitov @ 2025-02-11 16:52 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Jiri Olsa, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
	Peter Zijlstra, Andrii Nakryiko, Kees Cook, Eyal Birger, stable,
	Jann Horn, LKML, linux-trace-kernel, Linux API, X86 ML, bpf,
	Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
	Stephen Rothwell
In-Reply-To: <CAEf4BzYPmtUirnO3Bp+3F3d4++4ttL_MZAG+yGcTTKTRK2X2vw@mail.gmail.com>

On Tue, Feb 11, 2025 at 8:48 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 3:16 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Jann reported [1] possible issue when trampoline_check_ip returns
> > address near the bottom of the address space that is allowed to
> > call into the syscall if uretprobes are not set up.
> >
> > Though the mmap minimum address restrictions will typically prevent
> > creating mappings there, let's make sure uretprobe syscall checks
> > for that.
> >
> > [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> > Cc: Kees Cook <kees@kernel.org>
> > Cc: Eyal Birger <eyal.birger@gmail.com>
> > Cc: stable@vger.kernel.org
> > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> > Reported-by: Jann Horn <jannh@google.com>
> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > Reviewed-by: Kees Cook <kees@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > v2 changes:
> > - adding UPROBE_NO_TRAMPOLINE_VADDR macro (Andrii)
> > - rebased on top of perf/core
> >
> >  arch/x86/kernel/uprobes.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 5a952c5ea66b..e8d3c59aa9f7 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -357,19 +357,25 @@ void *arch_uprobe_trampoline(unsigned long *psize)
> >         return &insn;
> >  }
> >
> > -static unsigned long trampoline_check_ip(void)
> > +static unsigned long trampoline_check_ip(unsigned long tramp)
> >  {
> > -       unsigned long tramp = uprobe_get_trampoline_vaddr();
> > -
> >         return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
> >  }
> >
> > +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)

If you respin anyway maybe use ~0UL instead?
In the above and in
uprobe_get_trampoline_vaddr(),
since

unsigned long trampoline_vaddr = -1;

looks odd too.

^ permalink raw reply

* Re: [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Andrii Nakryiko @ 2025-02-11 16:47 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko, Kees Cook, Eyal Birger, stable, Jann Horn,
	linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
	Stephen Rothwell
In-Reply-To: <20250211111559.2984778-1-jolsa@kernel.org>

On Tue, Feb 11, 2025 at 3:16 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
>
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.
>
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <kees@kernel.org>
> Cc: Eyal Birger <eyal.birger@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Reported-by: Jann Horn <jannh@google.com>
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> v2 changes:
> - adding UPROBE_NO_TRAMPOLINE_VADDR macro (Andrii)
> - rebased on top of perf/core
>
>  arch/x86/kernel/uprobes.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..e8d3c59aa9f7 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,19 +357,25 @@ void *arch_uprobe_trampoline(unsigned long *psize)
>         return &insn;
>  }
>
> -static unsigned long trampoline_check_ip(void)
> +static unsigned long trampoline_check_ip(unsigned long tramp)
>  {
> -       unsigned long tramp = uprobe_get_trampoline_vaddr();
> -
>         return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
>  }
>
> +#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> +
>  SYSCALL_DEFINE0(uretprobe)
>  {
>         struct pt_regs *regs = task_pt_regs(current);
> -       unsigned long err, ip, sp, r11_cx_ax[3];
> +       unsigned long err, ip, sp, r11_cx_ax[3], tramp;
> +
> +       /* If there's no trampoline, we are called from wrong place. */
> +       tramp = uprobe_get_trampoline_vaddr();
> +       if (tramp == UPROBE_NO_TRAMPOLINE_VADDR)
> +               goto sigill;
>
> -       if (regs->ip != trampoline_check_ip())
> +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> +       if (regs->ip != trampoline_check_ip(tramp))
>                 goto sigill;
>

LGTM. I don't know if that would make any difference, but I'd sprinkle
unlikely() around these two conditions to make sure they don't
interfere with instruction flow much

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>         err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
> --
> 2.48.1
>

^ permalink raw reply

* Re: [PATCH v7 1/6] pidfd: add PIDFD_SELF* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2025-02-11 15:45 UTC (permalink / raw)
  To: Michal Koutný
  Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
	Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
	linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
	Oliver Sang, John Hubbard, Tejun Heo, Johannes Weiner,
	Andrew Morton, Shakeel Butt
In-Reply-To: <gij5nh63s73dj5u33uvzl5lbmsvoh6zr5xnqpnfltwi6aamy7j@47iop2wgtdac>

On Tue, Feb 11, 2025 at 04:24:07PM +0100, Michal Koutný wrote:
> On Thu, Jan 30, 2025 at 08:40:26PM +0000, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  include/uapi/linux/pidfd.h |  24 +++++++++
> >  kernel/pid.c               |  24 +++++++--
> >  kernel/signal.c            | 106 ++++++++++++++++++++++---------------
> >  3 files changed, 107 insertions(+), 47 deletions(-)
>
> Practical idea, thanks.

Thanks!

>
> > diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> > + * To cut the Gideon knot, for internal kernel usage, we refer to
>
> A nit
> https://en.wikipedia.org/wiki/Gordian_Knot
>
> (if still applicable)

MY GOD. Hahaha. How embarrassing. God knows how 'Gideon' ended up
there. Apologies to all, I appear to have had a senior moment there...

Feel free to correct Christian, unless we want to leave this as an Easter
Egg?  :P

>
> Michal

^ permalink raw reply

* Re: [PATCH v7 1/6] pidfd: add PIDFD_SELF* sentinels to refer to own thread/process
From: Michal Koutný @ 2025-02-11 15:24 UTC (permalink / raw)
  To: Lorenzo Stoakes
  Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
	Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
	linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
	Oliver Sang, John Hubbard, Tejun Heo, Johannes Weiner,
	Andrew Morton, Shakeel Butt
In-Reply-To: <24315a16a3d01a548dd45c7515f7d51c767e954e.1738268370.git.lorenzo.stoakes@oracle.com>

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

On Thu, Jan 30, 2025 at 08:40:26PM +0000, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  include/uapi/linux/pidfd.h |  24 +++++++++
>  kernel/pid.c               |  24 +++++++--
>  kernel/signal.c            | 106 ++++++++++++++++++++++---------------
>  3 files changed, 107 insertions(+), 47 deletions(-)

Practical idea, thanks.

> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> + * To cut the Gideon knot, for internal kernel usage, we refer to

A nit
https://en.wikipedia.org/wiki/Gordian_Knot

(if still applicable)

Michal

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

^ permalink raw reply

* [PATCHv2 perf/core] uprobes: Harden uretprobe syscall trampoline check
From: Jiri Olsa @ 2025-02-11 11:15 UTC (permalink / raw)
  To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko
  Cc: Kees Cook, Eyal Birger, stable, Jann Horn, linux-kernel,
	linux-trace-kernel, linux-api, x86, bpf, Thomas Gleixner,
	Ingo Molnar, Andy Lutomirski, Deepak Gupta, Stephen Rothwell

Jann reported [1] possible issue when trampoline_check_ip returns
address near the bottom of the address space that is allowed to
call into the syscall if uretprobes are not set up.

Though the mmap minimum address restrictions will typically prevent
creating mappings there, let's make sure uretprobe syscall checks
for that.

[1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
Cc: Kees Cook <kees@kernel.org>
Cc: Eyal Birger <eyal.birger@gmail.com>
Cc: stable@vger.kernel.org
Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Reported-by: Jann Horn <jannh@google.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
v2 changes:
- adding UPROBE_NO_TRAMPOLINE_VADDR macro (Andrii)
- rebased on top of perf/core

 arch/x86/kernel/uprobes.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..e8d3c59aa9f7 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -357,19 +357,25 @@ void *arch_uprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
-static unsigned long trampoline_check_ip(void)
+static unsigned long trampoline_check_ip(unsigned long tramp)
 {
-	unsigned long tramp = uprobe_get_trampoline_vaddr();
-
 	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
 }
 
+#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
+
 SYSCALL_DEFINE0(uretprobe)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	unsigned long err, ip, sp, r11_cx_ax[3];
+	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+
+	/* If there's no trampoline, we are called from wrong place. */
+	tramp = uprobe_get_trampoline_vaddr();
+	if (tramp == UPROBE_NO_TRAMPOLINE_VADDR)
+		goto sigill;
 
-	if (regs->ip != trampoline_check_ip())
+	/* Make sure the ip matches the only allowed sys_uretprobe caller. */
+	if (regs->ip != trampoline_check_ip(tramp))
 		goto sigill;
 
 	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
-- 
2.48.1


^ permalink raw reply related

* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
From: Jiri Olsa @ 2025-02-11  8:35 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko, Kees Cook, Eyal Birger, stable, Jann Horn,
	linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
	Stephen Rothwell
In-Reply-To: <CAEf4BzbpKReuNhdH6RnwYOyYxFwgJjjgUB_2xwU=dGkC--K=Kg@mail.gmail.com>

On Mon, Feb 10, 2025 at 09:26:53AM -0800, Andrii Nakryiko wrote:
> On Sun, Feb 9, 2025 at 2:05 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Jann reported [1] possible issue when trampoline_check_ip returns
> > address near the bottom of the address space that is allowed to
> > call into the syscall if uretprobes are not set up.
> >
> > Though the mmap minimum address restrictions will typically prevent
> > creating mappings there, let's make sure uretprobe syscall checks
> > for that.
> >
> > [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> > Cc: Kees Cook <kees@kernel.org>
> > Cc: Eyal Birger <eyal.birger@gmail.com>
> > Cc: stable@vger.kernel.org
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  arch/x86/kernel/uprobes.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 5a952c5ea66b..109d6641a1b3 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
> >         return &insn;
> >  }
> >
> > -static unsigned long trampoline_check_ip(void)
> > +static unsigned long trampoline_check_ip(unsigned long tramp)
> >  {
> > -       unsigned long tramp = uprobe_get_trampoline_vaddr();
> > -
> >         return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
> >  }
> >
> >  SYSCALL_DEFINE0(uretprobe)
> >  {
> >         struct pt_regs *regs = task_pt_regs(current);
> > -       unsigned long err, ip, sp, r11_cx_ax[3];
> > +       unsigned long err, ip, sp, r11_cx_ax[3], tramp;
> > +
> > +       /* If there's no trampoline, we are called from wrong place. */
> > +       tramp = uprobe_get_trampoline_vaddr();
> > +       if (tramp == -1)
> 
> slight nit: mixing -1 and unsigned long looks sloppy. Maybe let's add
> something like
> 
> #define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)
> 
> and return that from uprobe_get_trampoline_vaddr()?

ok, will add that

thanks,
jirka

> 
> > +               goto sigill;
> >
> > -       if (regs->ip != trampoline_check_ip())
> > +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> > +       if (regs->ip != trampoline_check_ip(tramp))
> >                 goto sigill;
> >
> >         err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
> > --
> > 2.48.1
> >

^ permalink raw reply

* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
From: Kees Cook @ 2025-02-10 23:00 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko, Eyal Birger, stable, Jann Horn, linux-kernel,
	linux-trace-kernel, linux-api, x86, bpf, Thomas Gleixner,
	Ingo Molnar, Andy Lutomirski, Deepak Gupta, Stephen Rothwell
In-Reply-To: <20250209220515.2554058-1-jolsa@kernel.org>

On Sun, Feb 09, 2025 at 11:05:15PM +0100, Jiri Olsa wrote:
> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
> 
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.
> 
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <kees@kernel.org>
> Cc: Eyal Birger <eyal.birger@gmail.com>
> Cc: stable@vger.kernel.org
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Thanks for this!

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

^ permalink raw reply

* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
From: Andrii Nakryiko @ 2025-02-10 17:26 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov, Peter Zijlstra,
	Andrii Nakryiko, Kees Cook, Eyal Birger, stable, Jann Horn,
	linux-kernel, linux-trace-kernel, linux-api, x86, bpf,
	Thomas Gleixner, Ingo Molnar, Andy Lutomirski, Deepak Gupta,
	Stephen Rothwell
In-Reply-To: <20250209220515.2554058-1-jolsa@kernel.org>

On Sun, Feb 9, 2025 at 2:05 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Jann reported [1] possible issue when trampoline_check_ip returns
> address near the bottom of the address space that is allowed to
> call into the syscall if uretprobes are not set up.
>
> Though the mmap minimum address restrictions will typically prevent
> creating mappings there, let's make sure uretprobe syscall checks
> for that.
>
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <kees@kernel.org>
> Cc: Eyal Birger <eyal.birger@gmail.com>
> Cc: stable@vger.kernel.org
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/x86/kernel/uprobes.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..109d6641a1b3 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -357,19 +357,23 @@ void *arch_uprobe_trampoline(unsigned long *psize)
>         return &insn;
>  }
>
> -static unsigned long trampoline_check_ip(void)
> +static unsigned long trampoline_check_ip(unsigned long tramp)
>  {
> -       unsigned long tramp = uprobe_get_trampoline_vaddr();
> -
>         return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
>  }
>
>  SYSCALL_DEFINE0(uretprobe)
>  {
>         struct pt_regs *regs = task_pt_regs(current);
> -       unsigned long err, ip, sp, r11_cx_ax[3];
> +       unsigned long err, ip, sp, r11_cx_ax[3], tramp;
> +
> +       /* If there's no trampoline, we are called from wrong place. */
> +       tramp = uprobe_get_trampoline_vaddr();
> +       if (tramp == -1)

slight nit: mixing -1 and unsigned long looks sloppy. Maybe let's add
something like

#define UPROBE_NO_TRAMPOLINE_VADDR ((unsigned long)-1)

and return that from uprobe_get_trampoline_vaddr()?

> +               goto sigill;
>
> -       if (regs->ip != trampoline_check_ip())
> +       /* Make sure the ip matches the only allowed sys_uretprobe caller. */
> +       if (regs->ip != trampoline_check_ip(tramp))
>                 goto sigill;
>
>         err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
> --
> 2.48.1
>

^ permalink raw reply

* Re: [PATCH bpf-next] uprobes: Harden uretprobe syscall trampoline check
From: Oleg Nesterov @ 2025-02-10 12:07 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Steven Rostedt, Masami Hiramatsu, Peter Zijlstra, Andrii Nakryiko,
	Kees Cook, Eyal Birger, stable, Jann Horn, linux-kernel,
	linux-trace-kernel, linux-api, x86, bpf, Thomas Gleixner,
	Ingo Molnar, Andy Lutomirski, Deepak Gupta, Stephen Rothwell
In-Reply-To: <20250209220515.2554058-1-jolsa@kernel.org>

On 02/09, Jiri Olsa wrote:
>
> [1] https://lore.kernel.org/bpf/202502081235.5A6F352985@keescook/T/#m9d416df341b8fbc11737dacbcd29f0054413cbbf
> Cc: Kees Cook <kees@kernel.org>
> Cc: Eyal Birger <eyal.birger@gmail.com>
> Cc: stable@vger.kernel.org
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/x86/kernel/uprobes.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Oleg Nesterov <oleg@redhat.com>


^ 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