* [PATCH 0/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Lorenzo Stoakes @ 2025-02-21 12:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Kalesh Singh, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
Juan Yescas, linux-mm, linux-doc, linux-kernel, linux-fsdevel,
linux-kselftest, linux-api
Currently there is no means of determining whether a give page in a mapping
range is designated a guard region (as installed via madvise() using the
MADV_GUARD_INSTALL flag).
This is generally not an issue, but in some instances users may wish to
determine whether this is the case.
This series adds this ability via /proc/$pid/pagemap, updates the
documentation and adds a self test to assert that this functions correctly.
Lorenzo Stoakes (2):
fs/proc/task_mmu: add guard region bit to pagemap
tools/selftests: add guard region test for /proc/$pid/pagemap
Documentation/admin-guide/mm/pagemap.rst | 3 +-
fs/proc/task_mmu.c | 6 ++-
tools/testing/selftests/mm/guard-regions.c | 47 ++++++++++++++++++++++
tools/testing/selftests/mm/vm_util.h | 1 +
4 files changed, 55 insertions(+), 2 deletions(-)
--
2.48.1
^ permalink raw reply
* [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Lorenzo Stoakes @ 2025-02-21 12:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Kalesh Singh, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
Juan Yescas, linux-mm, linux-doc, linux-kernel, linux-fsdevel,
linux-kselftest, linux-api
In-Reply-To: <cover.1740139449.git.lorenzo.stoakes@oracle.com>
Currently there is no means by which users can determine whether a given
page in memory is in fact a guard region, that is having had the
MADV_GUARD_INSTALL madvise() flag applied to it.
This is intentional, as to provide this information in VMA metadata would
contradict the intent of the feature (providing a means to change fault
behaviour at a page table level rather than a VMA level), and would require
VMA metadata operations to scan page tables, which is unacceptable.
In many cases, users have no need to reflect and determine what regions
have been designated guard regions, as it is the user who has established
them in the first place.
But in some instances, such as monitoring software, or software that relies
upon being able to ascertain the nature of mappings within a remote process
for instance, it becomes useful to be able to determine which pages have
the guard region marker applied.
This patch makes use of an unused pagemap bit (58) to provide this
information.
This patch updates the documentation at the same time as making the change
such that the implementation of the feature and the documentation of it are
tied together.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
Documentation/admin-guide/mm/pagemap.rst | 3 ++-
fs/proc/task_mmu.c | 6 +++++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
index caba0f52dd36..a297e824f990 100644
--- a/Documentation/admin-guide/mm/pagemap.rst
+++ b/Documentation/admin-guide/mm/pagemap.rst
@@ -21,7 +21,8 @@ There are four components to pagemap:
* Bit 56 page exclusively mapped (since 4.2)
* Bit 57 pte is uffd-wp write-protected (since 5.13) (see
Documentation/admin-guide/mm/userfaultfd.rst)
- * Bits 58-60 zero
+ * Bit 58 pte is a guard region (since 6.15) (see madvise (2) man page)
+ * Bits 59-60 zero
* Bit 61 page is file-page or shared-anon (since 3.5)
* Bit 62 page swapped
* Bit 63 page present
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f02cd362309a..c17615e21a5d 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1632,6 +1632,7 @@ struct pagemapread {
#define PM_SOFT_DIRTY BIT_ULL(55)
#define PM_MMAP_EXCLUSIVE BIT_ULL(56)
#define PM_UFFD_WP BIT_ULL(57)
+#define PM_GUARD_REGION BIT_ULL(58)
#define PM_FILE BIT_ULL(61)
#define PM_SWAP BIT_ULL(62)
#define PM_PRESENT BIT_ULL(63)
@@ -1732,6 +1733,8 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
page = pfn_swap_entry_to_page(entry);
if (pte_marker_entry_uffd_wp(entry))
flags |= PM_UFFD_WP;
+ if (is_guard_swp_entry(entry))
+ flags |= PM_GUARD_REGION;
}
if (page) {
@@ -1931,7 +1934,8 @@ static const struct mm_walk_ops pagemap_ops = {
* Bit 55 pte is soft-dirty (see Documentation/admin-guide/mm/soft-dirty.rst)
* Bit 56 page exclusively mapped
* Bit 57 pte is uffd-wp write-protected
- * Bits 58-60 zero
+ * Bit 58 pte is a guard region
+ * Bits 59-60 zero
* Bit 61 page is file-page or shared-anon
* Bit 62 page swapped
* Bit 63 page present
--
2.48.1
^ permalink raw reply related
* [PATCH 2/2] tools/selftests: add guard region test for /proc/$pid/pagemap
From: Lorenzo Stoakes @ 2025-02-21 12:05 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Kalesh Singh, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
Juan Yescas, linux-mm, linux-doc, linux-kernel, linux-fsdevel,
linux-kselftest, linux-api
In-Reply-To: <cover.1740139449.git.lorenzo.stoakes@oracle.com>
Add a test to the guard region self tests to assert that the
/proc/$pid/pagemap information now made availabile to the user correctly
identifies and reports guard regions.
As a part of this change, update vm_util.h to add the new bit (note there
is no header file in the kernel where this is exposed, the user is expected
to provide their own mask) and utilise the helper functions there for
pagemap functionality.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/mm/guard-regions.c | 47 ++++++++++++++++++++++
tools/testing/selftests/mm/vm_util.h | 1 +
2 files changed, 48 insertions(+)
diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
index ea9b5815e828..0c7183e8b661 100644
--- a/tools/testing/selftests/mm/guard-regions.c
+++ b/tools/testing/selftests/mm/guard-regions.c
@@ -19,6 +19,7 @@
#include <sys/syscall.h>
#include <sys/uio.h>
#include <unistd.h>
+#include "vm_util.h"
/*
* Ignore the checkpatch warning, as per the C99 standard, section 7.14.1.1:
@@ -2032,4 +2033,50 @@ TEST_F(guard_regions, anon_zeropage)
ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
}
+/*
+ * Assert that /proc/$pid/pagemap correctly identifies guard region ranges.
+ */
+TEST_F(guard_regions, pagemap)
+{
+ const unsigned long page_size = self->page_size;
+ int proc_fd;
+ char *ptr;
+ int i;
+
+ proc_fd = open("/proc/self/pagemap", O_RDONLY);
+ ASSERT_NE(proc_fd, -1);
+
+ ptr = mmap_(self, variant, NULL, 10 * page_size,
+ PROT_READ | PROT_WRITE, 0, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+
+ /* Read from pagemap, and assert no guard regions are detected. */
+ for (i = 0; i < 10; i++) {
+ char *ptr_p = &ptr[i * page_size];
+ unsigned long entry = pagemap_get_entry(proc_fd, ptr_p);
+ unsigned long masked = entry & PM_GUARD_REGION_MASK;
+
+ ASSERT_EQ(masked, 0);
+ }
+
+ /* Install a guard region in every other page. */
+ for (i = 0; i < 10; i += 2) {
+ char *ptr_p = &ptr[i * page_size];
+
+ ASSERT_EQ(madvise(ptr_p, page_size, MADV_GUARD_INSTALL), 0);
+ }
+
+ /* Re-read from pagemap, and assert guard regions are detected. */
+ for (i = 0; i < 10; i++) {
+ char *ptr_p = &ptr[i * page_size];
+ unsigned long entry = pagemap_get_entry(proc_fd, ptr_p);
+ unsigned long masked = entry & PM_GUARD_REGION_MASK;
+
+ ASSERT_EQ(masked, i % 2 == 0 ? PM_GUARD_REGION_MASK : 0);
+ }
+
+ ASSERT_EQ(close(proc_fd), 0);
+ ASSERT_EQ(munmap(ptr, 10 * page_size), 0);
+}
+
TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index b60ac68a9dc8..73a11443b7f6 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -10,6 +10,7 @@
#define PM_SOFT_DIRTY BIT_ULL(55)
#define PM_MMAP_EXCLUSIVE BIT_ULL(56)
#define PM_UFFD_WP BIT_ULL(57)
+#define PM_GUARD_REGION_MASK BIT_ULL(58)
#define PM_FILE BIT_ULL(61)
#define PM_SWAP BIT_ULL(62)
#define PM_PRESENT BIT_ULL(63)
--
2.48.1
^ permalink raw reply related
* Re: [PATCH 2/2] tools/selftests: add guard region test for /proc/$pid/pagemap
From: Lorenzo Stoakes @ 2025-02-21 13:51 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Kalesh Singh, Liam R . Howlett,
Matthew Wilcox, Vlastimil Babka, Paul E . McKenney, Jann Horn,
Juan Yescas, linux-mm, linux-doc, linux-kernel, linux-fsdevel,
linux-kselftest, linux-api
In-Reply-To: <164feb0a43ae72650e6b20c3910213f469566311.1740139449.git.lorenzo.stoakes@oracle.com>
On Fri, Feb 21, 2025 at 12:05:23PM +0000, Lorenzo Stoakes wrote:
> Add a test to the guard region self tests to assert that the
> /proc/$pid/pagemap information now made availabile to the user correctly
> identifies and reports guard regions.
>
> As a part of this change, update vm_util.h to add the new bit (note there
> is no header file in the kernel where this is exposed, the user is expected
> to provide their own mask) and utilise the helper functions there for
> pagemap functionality.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Andrew - Apologies,
I managed to not commit a change I quickly made before sending this out
(I'm ill, seems it is having an impact...)
If the series is ok would you mind tacking on this fix-patch? It's simply
to rename a clumsily named define here.
No functional changes...
Thanks!
----8<----
From 60be19e88b3bfe9a6ec459115f0027721c494b30 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Fri, 21 Feb 2025 13:45:48 +0000
Subject: [PATCH] fixup define name
Fix badly named define so it's consistent with the others.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/mm/guard-regions.c | 6 +++---
tools/testing/selftests/mm/vm_util.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
index 0c7183e8b661..280d1831bf73 100644
--- a/tools/testing/selftests/mm/guard-regions.c
+++ b/tools/testing/selftests/mm/guard-regions.c
@@ -2054,7 +2054,7 @@ TEST_F(guard_regions, pagemap)
for (i = 0; i < 10; i++) {
char *ptr_p = &ptr[i * page_size];
unsigned long entry = pagemap_get_entry(proc_fd, ptr_p);
- unsigned long masked = entry & PM_GUARD_REGION_MASK;
+ unsigned long masked = entry & PM_GUARD_REGION;
ASSERT_EQ(masked, 0);
}
@@ -2070,9 +2070,9 @@ TEST_F(guard_regions, pagemap)
for (i = 0; i < 10; i++) {
char *ptr_p = &ptr[i * page_size];
unsigned long entry = pagemap_get_entry(proc_fd, ptr_p);
- unsigned long masked = entry & PM_GUARD_REGION_MASK;
+ unsigned long masked = entry & PM_GUARD_REGION;
- ASSERT_EQ(masked, i % 2 == 0 ? PM_GUARD_REGION_MASK : 0);
+ ASSERT_EQ(masked, i % 2 == 0 ? PM_GUARD_REGION : 0);
}
ASSERT_EQ(close(proc_fd), 0);
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index 73a11443b7f6..0e629586556b 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -10,7 +10,7 @@
#define PM_SOFT_DIRTY BIT_ULL(55)
#define PM_MMAP_EXCLUSIVE BIT_ULL(56)
#define PM_UFFD_WP BIT_ULL(57)
-#define PM_GUARD_REGION_MASK BIT_ULL(58)
+#define PM_GUARD_REGION BIT_ULL(58)
#define PM_FILE BIT_ULL(61)
#define PM_SWAP BIT_ULL(62)
#define PM_PRESENT BIT_ULL(63)
--
2.48.1
^ permalink raw reply related
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Mickaël Salaün @ 2025-02-21 15:08 UTC (permalink / raw)
To: Andrey Albershteyn, Paul Moore
Cc: 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, Günther Noack, Arnd Bergmann,
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>
It looks security checks are missing. With IOCTL commands, file
permissions are checked at open time, but with these syscalls the path
is only resolved but no specific access seems to be checked (except
inode_owner_or_capable via vfs_fileattr_set).
On Tue, Feb 11, 2025 at 06:22:47PM +0100, 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>
> ---
> 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/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;
security_inode_getattr() should probably be called here.
> +
> + 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) {
security_inode_setattr() should probably be called too.
> + 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
[...]
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Kalesh Singh @ 2025-02-21 17:10 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, Juan Yescas,
linux-mm, linux-doc, linux-kernel, linux-fsdevel, linux-kselftest,
linux-api
In-Reply-To: <521d99c08b975fb06a1e7201e971cc24d68196d1.1740139449.git.lorenzo.stoakes@oracle.com>
On Fri, Feb 21, 2025 at 4:05 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> Currently there is no means by which users can determine whether a given
> page in memory is in fact a guard region, that is having had the
> MADV_GUARD_INSTALL madvise() flag applied to it.
>
> This is intentional, as to provide this information in VMA metadata would
> contradict the intent of the feature (providing a means to change fault
> behaviour at a page table level rather than a VMA level), and would require
> VMA metadata operations to scan page tables, which is unacceptable.
>
> In many cases, users have no need to reflect and determine what regions
> have been designated guard regions, as it is the user who has established
> them in the first place.
>
> But in some instances, such as monitoring software, or software that relies
> upon being able to ascertain the nature of mappings within a remote process
> for instance, it becomes useful to be able to determine which pages have
> the guard region marker applied.
>
> This patch makes use of an unused pagemap bit (58) to provide this
> information.
>
> This patch updates the documentation at the same time as making the change
> such that the implementation of the feature and the documentation of it are
> tied together.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> Documentation/admin-guide/mm/pagemap.rst | 3 ++-
> fs/proc/task_mmu.c | 6 +++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
> index caba0f52dd36..a297e824f990 100644
> --- a/Documentation/admin-guide/mm/pagemap.rst
> +++ b/Documentation/admin-guide/mm/pagemap.rst
> @@ -21,7 +21,8 @@ There are four components to pagemap:
> * Bit 56 page exclusively mapped (since 4.2)
> * Bit 57 pte is uffd-wp write-protected (since 5.13) (see
> Documentation/admin-guide/mm/userfaultfd.rst)
> - * Bits 58-60 zero
> + * Bit 58 pte is a guard region (since 6.15) (see madvise (2) man page)
Should this be 6.14 ?
Other than that: Reviewed-by: Kalesh Singh <kaleshsingh@google.com>
Thanks,
Kalesh
> + * Bits 59-60 zero
> * Bit 61 page is file-page or shared-anon (since 3.5)
> * Bit 62 page swapped
> * Bit 63 page present
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index f02cd362309a..c17615e21a5d 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1632,6 +1632,7 @@ struct pagemapread {
> #define PM_SOFT_DIRTY BIT_ULL(55)
> #define PM_MMAP_EXCLUSIVE BIT_ULL(56)
> #define PM_UFFD_WP BIT_ULL(57)
> +#define PM_GUARD_REGION BIT_ULL(58)
> #define PM_FILE BIT_ULL(61)
> #define PM_SWAP BIT_ULL(62)
> #define PM_PRESENT BIT_ULL(63)
> @@ -1732,6 +1733,8 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
> page = pfn_swap_entry_to_page(entry);
> if (pte_marker_entry_uffd_wp(entry))
> flags |= PM_UFFD_WP;
> + if (is_guard_swp_entry(entry))
> + flags |= PM_GUARD_REGION;
> }
>
> if (page) {
> @@ -1931,7 +1934,8 @@ static const struct mm_walk_ops pagemap_ops = {
> * Bit 55 pte is soft-dirty (see Documentation/admin-guide/mm/soft-dirty.rst)
> * Bit 56 page exclusively mapped
> * Bit 57 pte is uffd-wp write-protected
> - * Bits 58-60 zero
> + * Bit 58 pte is a guard region
> + * Bits 59-60 zero
> * Bit 61 page is file-page or shared-anon
> * Bit 62 page swapped
> * Bit 63 page present
> --
> 2.48.1
>
^ permalink raw reply
* Re: [PATCH 2/2] tools/selftests: add guard region test for /proc/$pid/pagemap
From: Kalesh Singh @ 2025-02-21 17:14 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, Juan Yescas,
linux-mm, linux-doc, linux-kernel, linux-fsdevel, linux-kselftest,
linux-api
In-Reply-To: <32e83941-e6f5-42ee-9292-a44c16463cf1@lucifer.local>
On Fri, Feb 21, 2025 at 5:51 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Fri, Feb 21, 2025 at 12:05:23PM +0000, Lorenzo Stoakes wrote:
> > Add a test to the guard region self tests to assert that the
> > /proc/$pid/pagemap information now made availabile to the user correctly
> > identifies and reports guard regions.
> >
> > As a part of this change, update vm_util.h to add the new bit (note there
> > is no header file in the kernel where this is exposed, the user is expected
> > to provide their own mask) and utilise the helper functions there for
> > pagemap functionality.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Kalesh Singh <kaleshsingh@google.com>
>
> Andrew - Apologies,
>
> I managed to not commit a change I quickly made before sending this out
> (I'm ill, seems it is having an impact...)
>
> If the series is ok would you mind tacking on this fix-patch? It's simply
> to rename a clumsily named define here.
>
> No functional changes...
>
> Thanks!
>
> ----8<----
> From 60be19e88b3bfe9a6ec459115f0027721c494b30 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Fri, 21 Feb 2025 13:45:48 +0000
> Subject: [PATCH] fixup define name
>
> Fix badly named define so it's consistent with the others.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> tools/testing/selftests/mm/guard-regions.c | 6 +++---
> tools/testing/selftests/mm/vm_util.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/guard-regions.c b/tools/testing/selftests/mm/guard-regions.c
> index 0c7183e8b661..280d1831bf73 100644
> --- a/tools/testing/selftests/mm/guard-regions.c
> +++ b/tools/testing/selftests/mm/guard-regions.c
> @@ -2054,7 +2054,7 @@ TEST_F(guard_regions, pagemap)
> for (i = 0; i < 10; i++) {
> char *ptr_p = &ptr[i * page_size];
> unsigned long entry = pagemap_get_entry(proc_fd, ptr_p);
> - unsigned long masked = entry & PM_GUARD_REGION_MASK;
> + unsigned long masked = entry & PM_GUARD_REGION;
>
> ASSERT_EQ(masked, 0);
> }
> @@ -2070,9 +2070,9 @@ TEST_F(guard_regions, pagemap)
> for (i = 0; i < 10; i++) {
> char *ptr_p = &ptr[i * page_size];
> unsigned long entry = pagemap_get_entry(proc_fd, ptr_p);
> - unsigned long masked = entry & PM_GUARD_REGION_MASK;
> + unsigned long masked = entry & PM_GUARD_REGION;
>
> - ASSERT_EQ(masked, i % 2 == 0 ? PM_GUARD_REGION_MASK : 0);
> + ASSERT_EQ(masked, i % 2 == 0 ? PM_GUARD_REGION : 0);
> }
>
> ASSERT_EQ(close(proc_fd), 0);
> diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
> index 73a11443b7f6..0e629586556b 100644
> --- a/tools/testing/selftests/mm/vm_util.h
> +++ b/tools/testing/selftests/mm/vm_util.h
> @@ -10,7 +10,7 @@
> #define PM_SOFT_DIRTY BIT_ULL(55)
> #define PM_MMAP_EXCLUSIVE BIT_ULL(56)
> #define PM_UFFD_WP BIT_ULL(57)
> -#define PM_GUARD_REGION_MASK BIT_ULL(58)
> +#define PM_GUARD_REGION BIT_ULL(58)
> #define PM_FILE BIT_ULL(61)
> #define PM_SWAP BIT_ULL(62)
> #define PM_PRESENT BIT_ULL(63)
> --
> 2.48.1
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Kalesh Singh @ 2025-02-21 17:24 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Suren Baghdasaryan, David Hildenbrand, Andrew Morton,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, linux-mm, linux-kernel, Shuah Khan,
linux-kselftest, linux-api, John Hubbard, Juan Yescas
In-Reply-To: <87bae232-b01d-4962-bbe1-3677b71ff752@lucifer.local>
On Fri, Feb 21, 2025 at 3:04 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Feb 20, 2025 at 10:08:36AM -0800, Kalesh Singh wrote:
> > On Thu, Feb 20, 2025 at 8:22 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 5:18 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > On Thu, Feb 20, 2025 at 01:44:20PM +0100, David Hildenbrand wrote:
> > > > > On 20.02.25 11:15, Lorenzo Stoakes wrote:
> > > > > > On Thu, Feb 20, 2025 at 11:03:02AM +0100, David Hildenbrand wrote:
> > > > > > > > > Your conclusion is 'did not participate with upstream'; I don't agree with
> > > > > > > > > that. But maybe you and Kalesh have a history on that that let's you react
> > > > > > > > > on his questions IMHO more emotionally than it should have been.
> > > > > > > >
> > > > > > > > This is wholly unfair, I have been very reasonable in response to this
> > > > > > > > thread. I have offered to find solutions, I have tried to understand the
> > > > > > > > problem in spite of having gone to great lengths to try to discuss the
> > > > > > > > limitations of the proposed approach in every venue I possibly could.
> > > > > > > >
> > > > > > > > I go out of my way to deal professionally and objectively with what is
> > > > > > > > presented. Nothing here is emotional. So I'd ask that you please abstain
> > > > > > > > from making commentary like this which has no basis.
> > > > > > >
> > > > > > > I appreciate everything you write below. But this request is just
> > > > > > > impossible. I will keep raising my opinion and misunderstandings will
> > > > > > > happen.
> > > > > >
> > > > > > Well I wouldn't ask you not to express your opinion David, you know I respect
> > > > > > and like you, and by all means push back hard or call out what you think is bad
> > > > > > behaviour :)
> > > > > >
> > > > > > I just meant to say, in my view, that there was no basis, but I appreciate
> > > > > > miscommunications happen.
> > > > > > > So apologies if I came off as being difficult or rude, it actually
> > > > > wasn't
> > > > > > intended. And to re-emphasise - I have zero personal issue with anybody in this
> > > > > > thread whatsoever!
> > > > >
> > > > > It sounded to me like you were trying to defend your work (again, IMHO too
> > > > > emotionally, and I might have completely misinterpreted that) and slowly
> > > > > switching to "friendly fire" (towards me). Apologies from my side if I
> > > > > completely misunderstood/misinterpreted that.
> > > >
> > > > Right this was not at all my intent, sorry if it seemed that way. I may well
> > > > have communicated terribly, so apologies on my side too.
> >
> > Hi everyone,
> >
> > Thank you for all the discussion.
> >
> > I don't find any personal issues with the communication in this
> > thread, but I appreciate David being the object voice of reason.
> >
> > I understand it can be frustrating since you have made many efforts to
> > communicate these tradeoffs. Unfortunately these issues were not known
> > for the file-backed ELF guard regions for my particular use case.
> >
> > >
> > > Sorry for being late to the party. Was sick for a couple of days.
> > > Lorenzo is right, there was a breakdown in communication at Google and
> > > he has all the rights to be upset. The issue with obfuscators should
> > > have been communicated once it was discovered. I was in regular
> > > discussions with Lorenzo but wasn't directly involved with this
> > > particular project and wasn't aware or did not realize that the
> > > obfuscator issue renders guards unusable for this usecase. My
> > > apologies, I should have asked more questions about it. I suspect
> > > Lorenzo would have implemented this anyway...
> > >
> >
> > Suren's use case is different from mine and this design fits perfectly
> > for anon guard regions from the allocator. :)
> >
> > So I think in conclusion, these aren't VMAs and shouldn't be treated
> > as such; we will advertise them from pagemap for those who need to
> > know.
> >
>
> Thanks Kalesh, glad there were no issues here and we have found
> constructive common ground! :)
>
> It turns out implementing the pagemap side of things is _really_
> straightforward, so I'll be sending a series for that shortly. Hopefully
> this provides some basis for whichever use cases need this information, as
> it is the best and least invasive place for this information at this stage.
Hi Lorenzo,
Reviewed your patches, agreed that is the cleanest way to advertise
this information.
Thanks,
Kalesh
>
> Cheers, Lorenzo
>
> > -- Kalesh
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Lorenzo Stoakes @ 2025-02-21 17:45 UTC (permalink / raw)
To: Kalesh Singh
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, David Hildenbrand,
Suren Baghdasaryan, Liam R . Howlett, Matthew Wilcox,
Vlastimil Babka, Paul E . McKenney, Jann Horn, Juan Yescas,
linux-mm, linux-doc, linux-kernel, linux-fsdevel, linux-kselftest,
linux-api
In-Reply-To: <CAC_TJvf-R6MuSS9e0b4orhxLrFwXTnvZV-vf3sB+BnSbEqsprw@mail.gmail.com>
On Fri, Feb 21, 2025 at 09:10:42AM -0800, Kalesh Singh wrote:
> On Fri, Feb 21, 2025 at 4:05 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > Currently there is no means by which users can determine whether a given
> > page in memory is in fact a guard region, that is having had the
> > MADV_GUARD_INSTALL madvise() flag applied to it.
> >
> > This is intentional, as to provide this information in VMA metadata would
> > contradict the intent of the feature (providing a means to change fault
> > behaviour at a page table level rather than a VMA level), and would require
> > VMA metadata operations to scan page tables, which is unacceptable.
> >
> > In many cases, users have no need to reflect and determine what regions
> > have been designated guard regions, as it is the user who has established
> > them in the first place.
> >
> > But in some instances, such as monitoring software, or software that relies
> > upon being able to ascertain the nature of mappings within a remote process
> > for instance, it becomes useful to be able to determine which pages have
> > the guard region marker applied.
> >
> > This patch makes use of an unused pagemap bit (58) to provide this
> > information.
> >
> > This patch updates the documentation at the same time as making the change
> > such that the implementation of the feature and the documentation of it are
> > tied together.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> > Documentation/admin-guide/mm/pagemap.rst | 3 ++-
> > fs/proc/task_mmu.c | 6 +++++-
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
> > index caba0f52dd36..a297e824f990 100644
> > --- a/Documentation/admin-guide/mm/pagemap.rst
> > +++ b/Documentation/admin-guide/mm/pagemap.rst
> > @@ -21,7 +21,8 @@ There are four components to pagemap:
> > * Bit 56 page exclusively mapped (since 4.2)
> > * Bit 57 pte is uffd-wp write-protected (since 5.13) (see
> > Documentation/admin-guide/mm/userfaultfd.rst)
> > - * Bits 58-60 zero
> > + * Bit 58 pte is a guard region (since 6.15) (see madvise (2) man page)
>
> Should this be 6.14 ?
We're aiming for the 6.15 merge window so this is correct :>) I don't think
this could be considered a hotfix haha!
>
> Other than that: Reviewed-by: Kalesh Singh <kaleshsingh@google.com>
Thanks! And thanks for review on the other patch also! Appreciated.
>
> Thanks,
> Kalesh
>
> > + * Bits 59-60 zero
> > * Bit 61 page is file-page or shared-anon (since 3.5)
> > * Bit 62 page swapped
> > * Bit 63 page present
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index f02cd362309a..c17615e21a5d 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1632,6 +1632,7 @@ struct pagemapread {
> > #define PM_SOFT_DIRTY BIT_ULL(55)
> > #define PM_MMAP_EXCLUSIVE BIT_ULL(56)
> > #define PM_UFFD_WP BIT_ULL(57)
> > +#define PM_GUARD_REGION BIT_ULL(58)
> > #define PM_FILE BIT_ULL(61)
> > #define PM_SWAP BIT_ULL(62)
> > #define PM_PRESENT BIT_ULL(63)
> > @@ -1732,6 +1733,8 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
> > page = pfn_swap_entry_to_page(entry);
> > if (pte_marker_entry_uffd_wp(entry))
> > flags |= PM_UFFD_WP;
> > + if (is_guard_swp_entry(entry))
> > + flags |= PM_GUARD_REGION;
> > }
> >
> > if (page) {
> > @@ -1931,7 +1934,8 @@ static const struct mm_walk_ops pagemap_ops = {
> > * Bit 55 pte is soft-dirty (see Documentation/admin-guide/mm/soft-dirty.rst)
> > * Bit 56 page exclusively mapped
> > * Bit 57 pte is uffd-wp write-protected
> > - * Bits 58-60 zero
> > + * Bit 58 pte is a guard region
> > + * Bits 59-60 zero
> > * Bit 61 page is file-page or shared-anon
> > * Bit 62 page swapped
> > * Bit 63 page present
> > --
> > 2.48.1
> >
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Darrick J. Wong @ 2025-02-21 18:11 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: 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, 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 06:22:47PM +0100, 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>
> ---
> 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(-)
>
<cut to the syscall definitions>
> 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)
Should the kernel require userspace to pass the size of the fsx buffer?
That way we avoid needing to rev the interface when we decide to grow
the structure.
--D
> +{
> + 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
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andreas Dilger @ 2025-02-21 19:10 UTC (permalink / raw)
To: Darrick J. Wong
Cc: 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, Arnd Bergmann, 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: <20250221181135.GW21808@frogsfrogsfrogs>
[-- Attachment #1: Type: text/plain, Size: 6234 bytes --]
On Feb 21, 2025, at 11:11 AM, Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 11, 2025 at 06:22:47PM +0100, 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>
>> ---
>> 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(-)
>>
>
> <cut to the syscall definitions>
>
>> 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)
>
> Should the kernel require userspace to pass the size of the fsx buffer?
> That way we avoid needing to rev the interface when we decide to grow
> the structure.
Definitely having some extensibility would be good, and there isn't much
room left today.
The struct size change would be handled automatically by the ioctl()
interface, but not the new syscall interface.
Another option would be to use an xflags to indicate "larger struct"
and then store the size after the end of the current struct. It would
also be possible to use one of the few remaining fields for this, but
one is earmarked for the DOS flags and/or a bitmask of supported flags,
and there isn't really any value to it until more fields are needed.
#define FS_XFLAG_LARGE_STRUCT 0x40000000
struct fsxattr {
__u32 fsx_xflags; /* xflags field value (get/set) */
__u32 fsx_extsize; /* extsize field value (get/set)*/
__u32 fsx_nextents; /* nextents field value (get) */
__u32 fsx_projid; /* project identifier (get/set) */
__u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
unsigned char fsx_pad[8];
__u32 fsx_fsxattr_size; /* struct size in bytes (get/set) */
:
/* future fields */
:
};
Not su
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Amir Goldstein @ 2025-02-21 19:15 UTC (permalink / raw)
To: Darrick J. Wong
Cc: 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, Arnd Bergmann, 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,
Pali Rohár, Theodore Tso
In-Reply-To: <20250221181135.GW21808@frogsfrogsfrogs>
On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 11, 2025 at 06:22:47PM +0100, 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>
> > ---
> > 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(-)
> >
>
> <cut to the syscall definitions>
>
> > 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)
>
> Should the kernel require userspace to pass the size of the fsx buffer?
> That way we avoid needing to rev the interface when we decide to grow
> the structure.
>
This makes sense to me, but I see that Andreas proposed other ways,
as long as we have a plan on how to extend the struct if we need more space.
Andrey, I am sorry to bring this up in v3, but I would like to request
two small changes before merging this API.
This patch by Pali [1] adds fsx_xflags_mask for the filesystem to
report the supported set of xflags.
It was argued that we can make this change with the existing ioctl,
because it is not going to break xfs_io -c lsattr/chattr, which is fine,
but I think that we should merge the fsx_xflags_mask change along
with getfsxattrat() which is a new UAPI.
The second request is related to setfsxattrat().
With current FS_IOC_FSSETXATTR, IIUC, xfs ignores unsupported
fsx_xflags. I think this needs to be fixed before merging setfsxattrat().
It's ok that a program calling FS_IOC_FSSETXATTR will not know
if unsupported flags will be ignored, because that's the way it is,
but I think that setfsxattrat() must return -EINVAL for trying to
set unsupported xflags.
As I explained in [2] I think it is fine if FS_IOC_FSSETXATTR
will also start returning -EINVAL for unsupported flags, but I would
like setfsxattrat() to make that a guarantee.
There was an open question, what does fsx_xflags_mask mean
for setfsxattrat() - it is a mask like in inode_set_flags() as Andreas
suggested? I think that would be a good idea.
Thanks,
Amir.
[1] https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/
[2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjwQJiKAqyjEmKUnq-VihyeSsxyEy2F+J38NXwrAXurFQ@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Paul Moore @ 2025-02-22 0:33 UTC (permalink / raw)
To: Andrey Albershteyn, Mickaël Salaün
Cc: 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, Günther Noack, Arnd Bergmann,
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: <20250221.ahB8jei2Chie@digikod.net>
On Fri, Feb 21, 2025 at 10:08 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> It looks security checks are missing. With IOCTL commands, file
> permissions are checked at open time, but with these syscalls the path
> is only resolved but no specific access seems to be checked (except
> inode_owner_or_capable via vfs_fileattr_set).
Thanks for reviewing the patch and catching this Mickaël. I agree
with the hooks identified and their placement; it should be fairly
straightforward with only a few lines added in each case.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: David Hildenbrand @ 2025-02-24 9:27 UTC (permalink / raw)
To: Lorenzo Stoakes, Andrew Morton
Cc: Jonathan Corbet, Shuah Khan, Suren Baghdasaryan, Kalesh Singh,
Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, Juan Yescas, linux-mm, linux-doc,
linux-kernel, linux-fsdevel, linux-kselftest, linux-api
In-Reply-To: <521d99c08b975fb06a1e7201e971cc24d68196d1.1740139449.git.lorenzo.stoakes@oracle.com>
On 21.02.25 13:05, Lorenzo Stoakes wrote:
> Currently there is no means by which users can determine whether a given
> page in memory is in fact a guard region, that is having had the
> MADV_GUARD_INSTALL madvise() flag applied to it.
>
> This is intentional, as to provide this information in VMA metadata would
> contradict the intent of the feature (providing a means to change fault
> behaviour at a page table level rather than a VMA level), and would require
> VMA metadata operations to scan page tables, which is unacceptable.
>
> In many cases, users have no need to reflect and determine what regions
> have been designated guard regions, as it is the user who has established
> them in the first place.
>
> But in some instances, such as monitoring software, or software that relies
> upon being able to ascertain the nature of mappings within a remote process
> for instance, it becomes useful to be able to determine which pages have
> the guard region marker applied.
>
> This patch makes use of an unused pagemap bit (58) to provide this
> information.
>
> This patch updates the documentation at the same time as making the change
> such that the implementation of the feature and the documentation of it are
> tied together.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
Acked-by: David Hildenbrand <david@redhat.com>
Something that might be interesting is also extending the PAGEMAP_SCAN
ioctl.
See do_pagemap_scan().
The benefit here might be that one could effectively search/filter for
guard regions without copying 64bit per base-page to user space.
But the idea would be to indicate something like PAGE_IS_GUARD_REGION as
a category when we hit a guard region entry in pagemap_page_category().
(the code is a bit complicated, and I am not sure why we indicate
PAGE_IS_SWAPPED for non-swap entries, likely wrong ...)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Lorenzo Stoakes @ 2025-02-24 10:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, Suren Baghdasaryan,
Kalesh Singh, Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, Juan Yescas, linux-mm, linux-doc,
linux-kernel, linux-fsdevel, linux-kselftest, linux-api
In-Reply-To: <857b2c3f-7be7-44e8-a825-82a7353665fb@redhat.com>
On Mon, Feb 24, 2025 at 10:27:28AM +0100, David Hildenbrand wrote:
> On 21.02.25 13:05, Lorenzo Stoakes wrote:
> > Currently there is no means by which users can determine whether a given
> > page in memory is in fact a guard region, that is having had the
> > MADV_GUARD_INSTALL madvise() flag applied to it.
> >
> > This is intentional, as to provide this information in VMA metadata would
> > contradict the intent of the feature (providing a means to change fault
> > behaviour at a page table level rather than a VMA level), and would require
> > VMA metadata operations to scan page tables, which is unacceptable.
> >
> > In many cases, users have no need to reflect and determine what regions
> > have been designated guard regions, as it is the user who has established
> > them in the first place.
> >
> > But in some instances, such as monitoring software, or software that relies
> > upon being able to ascertain the nature of mappings within a remote process
> > for instance, it becomes useful to be able to determine which pages have
> > the guard region marker applied.
> >
> > This patch makes use of an unused pagemap bit (58) to provide this
> > information.
> >
> > This patch updates the documentation at the same time as making the change
> > such that the implementation of the feature and the documentation of it are
> > tied together.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
>
>
> Acked-by: David Hildenbrand <david@redhat.com>
Thanks! :)
>
> Something that might be interesting is also extending the PAGEMAP_SCAN
> ioctl.
Yeah, funny you should mention that, I did see that, but on reading the man
page it struck me that it requires the region to be uffd afaict? All the
tests seem to establish uffd, and the man page implies it:
To start tracking the written state (flag) of a page or range of
memory, the UFFD_FEATURE_WP_ASYNC must be enabled by UFFDIO_API
ioctl(2) on userfaultfd and memory range must be registered with
UFFDIO_REGISTER ioctl(2) in UFFDIO_REGISTER_MODE_WP mode.
It would be a bit of a weird edge case to add support there. I was excited
when I first saw this ioctl, then disappointed afterwards... but maybe I
got it wrong?
>
>
> See do_pagemap_scan().
>
> The benefit here might be that one could effectively search/filter for guard
> regions without copying 64bit per base-page to user space.
>
> But the idea would be to indicate something like PAGE_IS_GUARD_REGION as a
> category when we hit a guard region entry in pagemap_page_category().
>
> (the code is a bit complicated, and I am not sure why we indicate
> PAGE_IS_SWAPPED for non-swap entries, likely wrong ...)
Yeah, I could go on here about how much I hate how uffd does a 'parallel
implementation' of a ton of stuff and then chucks in if (uffd) { go do
something weird + wonderful } but I'll resist the urge :P :))
Do you think, if it were uffd-specific, this would be useful?
At any rate, I'm not sure it's _hugely_ beneficial in this form as pagemap
is binary in any case so you're not having to deal with overhead of parsing
a text file at least!
>
> --
> Cheers,
>
> David / dhildenb
>
Thanks!
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: David Hildenbrand @ 2025-02-24 10:37 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, Suren Baghdasaryan,
Kalesh Singh, Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, Juan Yescas, linux-mm, linux-doc,
linux-kernel, linux-fsdevel, linux-kselftest, linux-api
In-Reply-To: <cd57ed04-c6b1-4df3-a5cb-a33078a08e74@lucifer.local>
On 24.02.25 11:18, Lorenzo Stoakes wrote:
> On Mon, Feb 24, 2025 at 10:27:28AM +0100, David Hildenbrand wrote:
>> On 21.02.25 13:05, Lorenzo Stoakes wrote:
>>> Currently there is no means by which users can determine whether a given
>>> page in memory is in fact a guard region, that is having had the
>>> MADV_GUARD_INSTALL madvise() flag applied to it.
>>>
>>> This is intentional, as to provide this information in VMA metadata would
>>> contradict the intent of the feature (providing a means to change fault
>>> behaviour at a page table level rather than a VMA level), and would require
>>> VMA metadata operations to scan page tables, which is unacceptable.
>>>
>>> In many cases, users have no need to reflect and determine what regions
>>> have been designated guard regions, as it is the user who has established
>>> them in the first place.
>>>
>>> But in some instances, such as monitoring software, or software that relies
>>> upon being able to ascertain the nature of mappings within a remote process
>>> for instance, it becomes useful to be able to determine which pages have
>>> the guard region marker applied.
>>>
>>> This patch makes use of an unused pagemap bit (58) to provide this
>>> information.
>>>
>>> This patch updates the documentation at the same time as making the change
>>> such that the implementation of the feature and the documentation of it are
>>> tied together.
>>>
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>>> ---
>>
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>
> Thanks! :)
>>
>> Something that might be interesting is also extending the PAGEMAP_SCAN
>> ioctl.
>
> Yeah, funny you should mention that, I did see that, but on reading the man
> page it struck me that it requires the region to be uffd afaict? All the
> tests seem to establish uffd, and the man page implies it:
>
> To start tracking the written state (flag) of a page or range of
> memory, the UFFD_FEATURE_WP_ASYNC must be enabled by UFFDIO_API
> ioctl(2) on userfaultfd and memory range must be registered with
> UFFDIO_REGISTER ioctl(2) in UFFDIO_REGISTER_MODE_WP mode.
>
> It would be a bit of a weird edge case to add support there. I was excited
> when I first saw this ioctl, then disappointed afterwards... but maybe I
> got it wrong?
>
I never managed to review that fully, but I thing that
UFFD_FEATURE_WP_ASYNC thingy is only required for PM_SCAN_CHECK_WPASYNC
and PM_SCAN_WP_MATCHING.
See pagemap_scan_test_walk().
I do recall that it works on any VMA.
Ah yes, tools/testing/selftests/mm/vm_util.c ends up using it for
pagemap_is_swapped() and friends via page_entry_is() to sanity check
that what pagemap gives us is consistent with what pagemap_scan gives us.
So it should work independent of the uffd magic.
I might be wrong, though ...
>>
>>
>> See do_pagemap_scan().
>>
>> The benefit here might be that one could effectively search/filter for guard
>> regions without copying 64bit per base-page to user space.
>>
>> But the idea would be to indicate something like PAGE_IS_GUARD_REGION as a
>> category when we hit a guard region entry in pagemap_page_category().
>>
>> (the code is a bit complicated, and I am not sure why we indicate
>> PAGE_IS_SWAPPED for non-swap entries, likely wrong ...)
>
> Yeah, I could go on here about how much I hate how uffd does a 'parallel
> implementation' of a ton of stuff and then chucks in if (uffd) { go do
> something weird + wonderful } but I'll resist the urge :P :))
>
> Do you think, if it were uffd-specific, this would be useful?
If it really is completely uffd-specific for now, I agree that we should
rather leave it alone.
>
> At any rate, I'm not sure it's _hugely_ beneficial in this form as pagemap
> is binary in any case so you're not having to deal with overhead of parsing
> a text file at least!
My thinking was, that if you have a large VMA, with ordinary pagemap you
have to copy 8byte per entry (and have room for that somewhere in user
space). In theory, with the scanning feature, you can leave that ...
scanning to the kernel and don't have to do any copying/allocate space
for it in user space etc.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: Lorenzo Stoakes @ 2025-02-24 10:49 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, Suren Baghdasaryan,
Kalesh Singh, Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, Juan Yescas, linux-mm, linux-doc,
linux-kernel, linux-fsdevel, linux-kselftest, linux-api
In-Reply-To: <09d7ca19-e6cc-4aa9-8474-8975373bdebd@redhat.com>
On Mon, Feb 24, 2025 at 11:37:24AM +0100, David Hildenbrand wrote:
> On 24.02.25 11:18, Lorenzo Stoakes wrote:
> > On Mon, Feb 24, 2025 at 10:27:28AM +0100, David Hildenbrand wrote:
> > > On 21.02.25 13:05, Lorenzo Stoakes wrote:
> > > > Currently there is no means by which users can determine whether a given
> > > > page in memory is in fact a guard region, that is having had the
> > > > MADV_GUARD_INSTALL madvise() flag applied to it.
> > > >
> > > > This is intentional, as to provide this information in VMA metadata would
> > > > contradict the intent of the feature (providing a means to change fault
> > > > behaviour at a page table level rather than a VMA level), and would require
> > > > VMA metadata operations to scan page tables, which is unacceptable.
> > > >
> > > > In many cases, users have no need to reflect and determine what regions
> > > > have been designated guard regions, as it is the user who has established
> > > > them in the first place.
> > > >
> > > > But in some instances, such as monitoring software, or software that relies
> > > > upon being able to ascertain the nature of mappings within a remote process
> > > > for instance, it becomes useful to be able to determine which pages have
> > > > the guard region marker applied.
> > > >
> > > > This patch makes use of an unused pagemap bit (58) to provide this
> > > > information.
> > > >
> > > > This patch updates the documentation at the same time as making the change
> > > > such that the implementation of the feature and the documentation of it are
> > > > tied together.
> > > >
> > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > > ---
> > >
> > >
> > > Acked-by: David Hildenbrand <david@redhat.com>
> >
> > Thanks! :)
> > >
> > > Something that might be interesting is also extending the PAGEMAP_SCAN
> > > ioctl.
> >
> > Yeah, funny you should mention that, I did see that, but on reading the man
> > page it struck me that it requires the region to be uffd afaict? All the
> > tests seem to establish uffd, and the man page implies it:
> >
> > To start tracking the written state (flag) of a page or range of
> > memory, the UFFD_FEATURE_WP_ASYNC must be enabled by UFFDIO_API
> > ioctl(2) on userfaultfd and memory range must be registered with
> > UFFDIO_REGISTER ioctl(2) in UFFDIO_REGISTER_MODE_WP mode.
> >
> > It would be a bit of a weird edge case to add support there. I was excited
> > when I first saw this ioctl, then disappointed afterwards... but maybe I
> > got it wrong?
> >
>
> I never managed to review that fully, but I thing that UFFD_FEATURE_WP_ASYNC
> thingy is only required for PM_SCAN_CHECK_WPASYNC and PM_SCAN_WP_MATCHING.
>
> See pagemap_scan_test_walk().
>
> I do recall that it works on any VMA.
Oh ok well that's handy then!
>
> Ah yes, tools/testing/selftests/mm/vm_util.c ends up using it for
> pagemap_is_swapped() and friends via page_entry_is() to sanity check that
> what pagemap gives us is consistent with what pagemap_scan gives us.
>
> So it should work independent of the uffd magic.
> I might be wrong, though ...
No a quick glance makes me think you're right actually.
>
> > >
> > >
> > > See do_pagemap_scan().
> > >
> > > The benefit here might be that one could effectively search/filter for guard
> > > regions without copying 64bit per base-page to user space.
> > >
> > > But the idea would be to indicate something like PAGE_IS_GUARD_REGION as a
> > > category when we hit a guard region entry in pagemap_page_category().
> > >
> > > (the code is a bit complicated, and I am not sure why we indicate
> > > PAGE_IS_SWAPPED for non-swap entries, likely wrong ...)
> >
> > Yeah, I could go on here about how much I hate how uffd does a 'parallel
> > implementation' of a ton of stuff and then chucks in if (uffd) { go do
> > something weird + wonderful } but I'll resist the urge :P :))
> >
> > Do you think, if it were uffd-specific, this would be useful?
>
> If it really is completely uffd-specific for now, I agree that we should
> rather leave it alone.
Yeah agreed.
>
> >
> > At any rate, I'm not sure it's _hugely_ beneficial in this form as pagemap
> > is binary in any case so you're not having to deal with overhead of parsing
> > a text file at least!
>
> My thinking was, that if you have a large VMA, with ordinary pagemap you
> have to copy 8byte per entry (and have room for that somewhere in user
> space). In theory, with the scanning feature, you can leave that ...
> scanning to the kernel and don't have to do any copying/allocate space for
> it in user space etc.
That makes perfect sense!
I think this one will go a little lower on priorities + I'll come back to it but
I"ll put it on the one reliable todo list I have, the whiteboard in my home
office :) everything on that list at least eventually gets looked at, majority
get done.
>
> --
> Cheers,
>
> David / dhildenb
>
Great minds think alike though ;) as soon as I saw this I did think about
extending it, but seems I mistakenly dismissed for uffd reasons.
Cheers, Lorenzo
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Jan Kara @ 2025-02-24 10:54 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: 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, 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 11-02-25 18:22:47, 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>
Some comments below:
> +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)
^^ This should be !(at_flags & AT_SYMLINK_NOFOLLOW)?
In the check above you verify for AT_SYMLINK_NOFOLLOW and that also matches
what setxattrat() does...
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (at_flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + if (fd_empty(dir))
> + return -EBADF;
This check is wrong and in fact the whole dfd handling looks buggy.
openat(2) manpage describes the expected behavior:
The dirfd argument is used in conjunction with the pathname argument as
follows:
• If the pathname given in pathname is absolute, then dirfd is ig-
nored.
^^^^ This is what you break. If the pathname is absolute, you're
not expected to touch dirfd.
• If the pathname given in pathname is relative and dirfd is the spe-
cial value AT_FDCWD, then pathname is interpreted relative to the
current working directory of the calling process (like open()).
^^^ Also AT_FDCWD handling would be broken by the above check.
• If the pathname given in pathname is relative, then it is inter-
preted relative to the directory referred to by the file descriptor
dirfd (rather than relative to the current working directory of the
calling process, as is done by open() for a relative pathname). In
this case, dirfd must be a directory that was opened for reading
(O_RDONLY) or using the O_PATH flag.
If the pathname given in pathname is relative, and dirfd is not a valid
file descriptor, an error (EBADF) results. (Specifying an invalid file
descriptor number in dirfd can be used as a means to ensure that path-
name is absolute.)
> +
> + error = user_path_at(dfd, filename, lookup_flags, &filepath);
^^^ And user_path_at() isn't quite what you need either
because with AT_EMPTY_PATH we also want to allow for filename to be NULL
(not just empty string) and user_path_at() does not support that. That's
why I in my previous replies suggested you should follow what setxattrat()
does and that sadly it is more painful than it should be. You need
something like:
name = getname_maybe_null(filename, at_flags);
if (!name) {
CLASS(fd, f)(dfd);
if (fd_empty(f))
return -EBADF;
error = vfs_fileattr_get(file_dentry(fd_file(f)), &fa);
} else {
error = filename_lookup(dfd, filename, lookup_flags, &filepath,
NULL);
if (error)
goto out;
error = vfs_fileattr_get(filepath.dentry, &fa);
path_put(&filepath);
}
if (!error)
error = copy_fsxattr_to_user(&fa, fsx);
out:
putname(name);
return error;
Longer term, we need to provide user_path_maybe_null_at() for this but I
don't want to drag you into this cleanup :)
> + 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;
I think using AT_SYMLINK_NOFOLLOW is actually more traditional and thus
less surprising to users so I'd prefer that. Definitely this needs to be
consistent with getfsxattrat().
> +
> + if (at_flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + if (fd_empty(dir))
> + return -EBADF;
Same comment regarding dfd handling as above.
> +
> + 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;
> +}
Otherwise the patch looks good to me.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-02-24 11:32 UTC (permalink / raw)
Cc: Amir Goldstein, Andrey Albershteyn, Darrick J. Wong,
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,
Jan Kara, Mickaël Salaün, Günther Noack,
Arnd Bergmann, 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, Pali Rohár, Theodore Tso
In-Reply-To: <CAOQ4uxgyYBFqkq6cQsso4LxJsPJ4uECOdskXmz-nmGhhV5BQWg@mail.gmail.com>
On Fri, Feb 21, 2025 at 08:15:24PM +0100, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Feb 11, 2025 at 06:22:47PM +0100, 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>
> > > ---
> > > 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(-)
> > >
> >
> > <cut to the syscall definitions>
> >
> > > 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)
> >
> > Should the kernel require userspace to pass the size of the fsx buffer?
> > That way we avoid needing to rev the interface when we decide to grow
> > the structure.
Please version the struct by size as we do for clone3(),
mount_setattr(), listmount()'s struct mnt_id_req, sched_setattr(), all
the new xattrat*() system calls and a host of others. So laying out the
struct 64bit and passing a size alongside it.
This is all handled by copy_struct_from_user() and copy_struct_to_user()
so nothing to reinvent. And it's easy to copy from existing system
calls.
^ permalink raw reply
* Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap
From: David Hildenbrand @ 2025-02-24 13:28 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Jonathan Corbet, Shuah Khan, Suren Baghdasaryan,
Kalesh Singh, Liam R . Howlett, Matthew Wilcox, Vlastimil Babka,
Paul E . McKenney, Jann Horn, Juan Yescas, linux-mm, linux-doc,
linux-kernel, linux-fsdevel, linux-kselftest, linux-api
In-Reply-To: <e687dd75-b76f-4eab-805d-7b1bb18b1365@lucifer.local>
>> My thinking was, that if you have a large VMA, with ordinary pagemap you
>> have to copy 8byte per entry (and have room for that somewhere in user
>> space). In theory, with the scanning feature, you can leave that ...
>> scanning to the kernel and don't have to do any copying/allocate space for
>> it in user space etc.
>
> That makes perfect sense!
>
> I think this one will go a little lower on priorities + I'll come back to it but
> I"ll put it on the one reliable todo list I have, the whiteboard in my home
> office :) everything on that list at least eventually gets looked at, majority
> get done.
Sounds good. I'm sure Android folks will speak up in case they require
more efficient scanning.
>
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
>
> Great minds think alike though ;) as soon as I saw this I did think about
> extending it, but seems I mistakenly dismissed for uffd reasons.
We should probably look into cleaning up + improving the documentation
around the pagemap scan feature at some point. Well, something for
another day :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 1/4] mm: allow guard regions in file-backed and read-only mappings
From: Lorenzo Stoakes @ 2025-02-24 14:02 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: <d885cb259174736c2830a5dfe07f81b214ef3faa.1739469950.git.lorenzo.stoakes@oracle.com>
One thing to note with this series is that it now implies file-backed VMAs
which install guard regions will now have an anon_vma installed if not
already present (i.e. if not post-CoW MAP_PRIVATE).
I have audited kernel source for instances of vma->anon_vma checks and
found nowhere where this would be problematic for pure file-backed mappings.
I also discussed (off-list) with Matthew who confirmed he can't see any
issue with this.
In effect, we treat these VMAs as if they are MAP_PRIVATE, only with 0
CoW'd pages. As a result, the rmap never has a reason to reference the
anon_vma from folios at any point and thus no unexpected or weird behaviour
results.
The anon_vma logic tries to avoid unnecessary anon_vma propagation on fork
so we ought to at least minimise overhead.
However, this is still overhead, and unwelcome overhead.
The whole reason we do this (in madvise_guard_install()) is to ensure that
fork _copies page tables_. Otherwise, in vma_needs_copy(), nothing will
indicate that we should do so.
This was already an unpleasant thing to have to do, but without a new VMA
flag we really have no reasonable means of ensuring this happens.
Going forward, I intend to add a new VMA flag, VM_MAYBE_GUARDED or
something like this.
This would have specific behaviour - for the purposes of merging, it would
be ignored. However on both split and merge, it will be propagated. It is
therefore 'sticky'.
This is to avoid having to traverse page tables to determine which parts of
a VMA contain guard regions and of course to maintain the desirable
qualities of guard regions - the lack of VMA propagation (+ thus slab
allocations of VMAs).
Adding this flag and adjusting vma_needs_copy() to reference it would
resolve the issue.
However :) we have a VMA flag space issue, so it'd render this a 64-bit
feature only.
Having discussed with Matthew a plan by which to perhaps extend available
flags for 32-bit we may going forward be able to avoid this. But this may
be a longer term project.
In the meantime, we'd have to resort to the anon_vma hack for 32-bit, using
the flag for 64-bit. The issue with this however is if we do then intend to
allow the flag to enable /proc/$pid/maps visibility (something this could
allow), it would also end up being 64-bit only which would be a pity.
Regardless - I wanted to highlight this behaviour as it is perhaps somewhat
surprising.
On Thu, Feb 13, 2025 at 06:17:00PM +0000, Lorenzo Stoakes wrote:
> 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
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-02-24 16:00 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Paul Moore, 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, Günther Noack, Arnd Bergmann,
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: <20250221.ahB8jei2Chie@digikod.net>
On 2025-02-21 16:08:33, Mickaël Salaün wrote:
> It looks security checks are missing. With IOCTL commands, file
> permissions are checked at open time, but with these syscalls the path
> is only resolved but no specific access seems to be checked (except
> inode_owner_or_capable via vfs_fileattr_set).
>
> On Tue, Feb 11, 2025 at 06:22:47PM +0100, 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>
> > ---
> > 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/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;
>
> security_inode_getattr() should probably be called here.
>
> > +
> > + 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) {
>
> security_inode_setattr() should probably be called too.
Aren't those checks for something different - inode attributes
ATTR_*?
(sorry, the naming can't be more confusing)
Looking into security_inode_setattr() it seems to expect struct
iattr, which works with inode attributes (mode, time, uid/gid...).
These new syscalls work with filesystem inode extended flags/attributes
FS_XFLAG_* in fsxattr->fsx_xflags. Let me know if I missing
something here
--
- Andrey
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-02-24 16:21 UTC (permalink / raw)
To: Christian Brauner
Cc: Amir Goldstein, Darrick J. Wong, 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, Jan Kara, Mickaël Salaün,
Günther Noack, Arnd Bergmann, 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,
Pali Rohár, Theodore Tso
In-Reply-To: <20250224-klinke-hochdekoriert-3f6be89005a8@brauner>
On 2025-02-24 12:32:17, Christian Brauner wrote:
> On Fri, Feb 21, 2025 at 08:15:24PM +0100, Amir Goldstein wrote:
> > On Fri, Feb 21, 2025 at 7:13 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Tue, Feb 11, 2025 at 06:22:47PM +0100, 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>
> > > > ---
> > > > 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(-)
> > > >
> > >
> > > <cut to the syscall definitions>
> > >
> > > > 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)
> > >
> > > Should the kernel require userspace to pass the size of the fsx buffer?
> > > That way we avoid needing to rev the interface when we decide to grow
> > > the structure.
>
> Please version the struct by size as we do for clone3(),
> mount_setattr(), listmount()'s struct mnt_id_req, sched_setattr(), all
> the new xattrat*() system calls and a host of others. So laying out the
> struct 64bit and passing a size alongside it.
>
> This is all handled by copy_struct_from_user() and copy_struct_to_user()
> so nothing to reinvent. And it's easy to copy from existing system
> calls.
>
Oh, thanks for pointing to these, will use them.
--
- Andrey
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-02-24 16:38 UTC (permalink / raw)
To: Jan Kara
Cc: 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, Mickaël Salaün, Günther Noack,
Arnd Bergmann, 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: <fyp7gcbeo3xlrh7zi7k6m5aa6h5otbufxq3kh5zvgr3sjdbxl3@4nkuwx46yajk>
On 2025-02-24 11:54:34, Jan Kara wrote:
> On Tue 11-02-25 18:22:47, 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>
>
> Some comments below:
>
> > +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)
> ^^ This should be !(at_flags & AT_SYMLINK_NOFOLLOW)?
>
> In the check above you verify for AT_SYMLINK_NOFOLLOW and that also matches
> what setxattrat() does...
Right, didn't notice that this is actually opposite to setxattrat(),
will change that.
>
>
> > + lookup_flags |= LOOKUP_FOLLOW;
> > +
> > + if (at_flags & AT_EMPTY_PATH)
> > + lookup_flags |= LOOKUP_EMPTY;
> > +
> > + if (fd_empty(dir))
> > + return -EBADF;
>
> This check is wrong and in fact the whole dfd handling looks buggy.
> openat(2) manpage describes the expected behavior:
>
> The dirfd argument is used in conjunction with the pathname argument as
> follows:
>
> • If the pathname given in pathname is absolute, then dirfd is ig-
> nored.
> ^^^^ This is what you break. If the pathname is absolute, you're
> not expected to touch dirfd.
>
> • If the pathname given in pathname is relative and dirfd is the spe-
> cial value AT_FDCWD, then pathname is interpreted relative to the
> current working directory of the calling process (like open()).
> ^^^ Also AT_FDCWD handling would be broken by the above check.
>
> • If the pathname given in pathname is relative, then it is inter-
> preted relative to the directory referred to by the file descriptor
> dirfd (rather than relative to the current working directory of the
> calling process, as is done by open() for a relative pathname). In
> this case, dirfd must be a directory that was opened for reading
> (O_RDONLY) or using the O_PATH flag.
>
> If the pathname given in pathname is relative, and dirfd is not a valid
> file descriptor, an error (EBADF) results. (Specifying an invalid file
> descriptor number in dirfd can be used as a means to ensure that path-
> name is absolute.)
>
> > +
> > + error = user_path_at(dfd, filename, lookup_flags, &filepath);
> ^^^ And user_path_at() isn't quite what you need either
> because with AT_EMPTY_PATH we also want to allow for filename to be NULL
> (not just empty string) and user_path_at() does not support that. That's
> why I in my previous replies suggested you should follow what setxattrat()
> does and that sadly it is more painful than it should be. You need
> something like:
>
> name = getname_maybe_null(filename, at_flags);
> if (!name) {
> CLASS(fd, f)(dfd);
>
> if (fd_empty(f))
> return -EBADF;
> error = vfs_fileattr_get(file_dentry(fd_file(f)), &fa);
> } else {
> error = filename_lookup(dfd, filename, lookup_flags, &filepath,
> NULL);
> if (error)
> goto out;
> error = vfs_fileattr_get(filepath.dentry, &fa);
> path_put(&filepath);
> }
> if (!error)
> error = copy_fsxattr_to_user(&fa, fsx);
> out:
> putname(name);
> return error;
>
> Longer term, we need to provide user_path_maybe_null_at() for this but I
> don't want to drag you into this cleanup :)
Oh, I missed that, thanks for pointing this out, I will change it as
suggested.
--
- Andrey
^ permalink raw reply
* [PATCH v2 00/20] mm: MM owner tracking for large folios (!hugetlb) + CONFIG_NO_PAGE_MAPCOUNT
From: David Hildenbrand @ 2025-02-24 16:55 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
David Hildenbrand, Andrew Morton, Matthew Wilcox (Oracle),
Tejun Heo, Zefan Li, Johannes Weiner, Michal Koutný,
Jonathan Corbet, Andy Lutomirski, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, Muchun Song, Liam R. Howlett,
Lorenzo Stoakes, Vlastimil Babka, Jann Horn
This patch series is about to haunt me in my dreams, and the time spent
on this is ridiculous ... anyhow, here goes a new version that now also
supports 32bit (I wish we would have dynamically allocated "struct folio"
already ...) and always enables the new tracking with
CONFIG_TRANSPARENT_HUGEPAGE.
Let's add an "easy" way to decide -- without false positives, without
page-mapcounts and without page table/rmap scanning -- whether a large
folio is "certainly mapped exclusively" into a single MM, or whether it
"maybe mapped shared" into multiple MMs.
Use that information to implement Copy-on-Write reuse, to convert
folio_likely_mapped_shared() to folio_maybe_mapped_share(), and to
introduce a kernel config option that let's us not use+maintain
per-page mapcounts in large folios anymore.
The bigger picture was presented at LSF/MM [1].
This series is effectively a follow-up on my early work [2], which
implemented a more precise, but also more complicated, way to identify
whether a large folio is "mapped shared" into multiple MMs or
"mapped exclusively" into a single MM.
1 Patch Organization
====================
Patch #1 -> #6: make more room in order-1 folios, so we have two
"unsigned long" available for our purposes
Patch #7 -> #11: preparations
Patch #12: MM owner tracking for large folios
Patch #13: COW reuse for PTE-mapped anon THP
Patch #14: folio_maybe_mapped_shared()
Patch #15 -> #20: introduce and implement CONFIG_NO_PAGE_MAPCOUNT
2 MM owner tracking
===================
We assign each MM a unique ID ("MM ID"), to be able to squeeze more
information in our folios. On 32bit we use 15-bit IDs, on 64bit we use
31-bit IDs.
For each large folios, we now store two MM-ID+mapcount ("slot")
combinations:
* mm0_id + mm0_mapcount
* mm1_id + mm1_mapcount
On 32bit, we use a 16-bit per-MM mapcount, on 64bit an ordinary 32bit
mapcount. This way, we require 2x "unsigned long" on 32bit and 64bit for
both slots.
Paired with the large mapcount, we can reliably identify whether one
of these MMs is the current owner (-> owns all mappings) or even holds
all folio references (-> owns all mappings, and all references are from
mappings).
As long as only two MMs map folio pages at a time, we can reliably and
precisely identify whether a large folio is "mapped shared" or
"mapped exclusively".
Any additional MM that starts mapping the folio while there are no free
slots becomes an "untracked MM". If one such "untracked MM" is the last
one mapping a folio exclusively, we will *not* detect the folio as
"mapped exclusively" but instead as "maybe mapped shared". (exception:
only a single mapping remains)
So that's where the approach gets imprecise.
For now, we use a bit-spinlock to sync the large mapcount + slots, and
make sure we do keep the machinery fast, to not degrade (un)map performance
drastically: for example, we make sure to only use a single atomic (when
grabbing the bit-spinlock), like we would already perform when updating
the large mapcount.
3 CONFIG_NO_PAGE_MAPCOUNT
=========================
patch #15 -> #20 spell out and document what exactly is affected when
not maintaining the per-page mapcounts in large folios anymore.
Most importantly, as we cannot maintain folio->_nr_pages_mapped anymore when
(un)mapping pages, we'll account a complete folio as mapped if a
single page is mapped. In addition, we'll not detect partially mapped
anonymous folios as such in all cases yet.
Likely less relevant changes include that we might now under-estimate the
USS (Unique Set Size) of a process, but never over-estimate it.
The goal is to make CONFIG_NO_PAGE_MAPCOUNT the default at some point,
to then slowly make it the only option, as we learn about real-life
impacts and possible ways to mitigate them.
4 Performance
=============
Detailed performance numbers were included in v1 [4], and not that much
changed between v1 and v2.
I did plenty of measurements on different systems in the meantime, that
all revealed slightly different results.
The pte-mapped-folio micro-benchmarks are fairly sensitive to code layout
changes on some systems. Especially the fork() benchmark started being
more-shaky-than-before on recent kernels for some reason.
In summary, with my micro-benchmarks:
* Small folios are not impacted.
* CoW performance seems to be mostly unchanged across all folios sizes.
* CoW reuse performance of large folios now matches CoW reuse performance
of small folios, because we now actually implement the CoW reuse
optimization. On an Intel Xeon Silver 4210R I measured a ~65% reduction
in runtime, on an arm64 system I measured ~54% reduction.
* munmap() performance improves with CONFIG_NO_PAGE_MAPCOUNT. I saw
double-digit % reduction (up to ~30% on an Intel Xeon Silver 4210R
and up to ~70% on an AmpereOne A192-32X) with larger folios. The
larger the folios, the larger the performance improvement.
* munmao() performance very slightly (couple percent) degrades without
CONFIG_NO_PAGE_MAPCOUNT for smaller folios. For larger folios, there
seems to be no change at all.
* fork() performance improves with CONFIG_NO_PAGE_MAPCOUNT. I saw
double-digit % reduction (up to ~20% on an Intel Xeon Silver 4210R
and up to ~10% on an AmpereOne A192-32X) with larger folios. The larger
the folios, the larger the performance improvement.
* While fork() performance without CONFIG_NO_PAGE_MAPCOUNT seems to be
almost unchanged on some systems, I saw some degradation for
smaller folios on the AmpereOne A192-32X. I did not investigate the
details yet, but I suspect code layout changes or suboptimal code
placement / inlining.
I'm not to worried about the fork() micro-benchmarks for smaller folios
given how shaky the results are lately and by how much we improved fork()
performance recently.
I also ran case-anon-cow-rand and case-anon-cow-seq part of vm-scalability,
to assess the scalability and the impact of the bit-spinlock.
My measurements on a two 2-socket 10-core Intel Xeon Silver 4210R CPU
revealed no significant changes.
Similarly, running these benchmarks with 2 MiB THPs enabled on the
AmpereOne A192-32X with 192 cores, I got < 1% difference with < 1% stdev,
which is nice.
So far, I did not get my hands on a similarly large system with multiple
sockets.
I found no other fitting scalability benchmarks that seem to really hammer
on concurrent mapping/unmapping of large folio pages like case-anon-cow-seq
does.
5 Concerns
==========
5.1 Bit spinlock
----------------
I'm not quite happy about the bit-spinlock, but so far it does not seem to
affect scalability in my measurements.
If it ever becomes a problem we could either investigate improving the
locking, or simply stopping the MM tracking once there are "too many
mappings" and simply assume that the folio is "mapped shared" until it
was freed.
This would be similar (but slightly different) to the "0,1,2,stopped"
counting idea Willy had at some point. Adding that logic to "stop tracking"
adds more code to the hot path, so I avoided that for now.
5.2 folio_maybe_mapped_shared()
-------------------------------
I documented the change from folio_likely_mapped_shared() to
folio_maybe_mapped_shared() quite extensively. If we run into surprises,
I have some ideas on how to resolve them. For now, I think we should
be fine.
5.3 Added code to map/unmap hot path
------------------------------------
So far, it looks like the added code on the rmap hot path does not
really seem to matter much in the bigger picture. I'd like to further
reduce it (and possibly improve fork() performance further), but I don't
easily see how right now. Well, and I am out of puff :)
Having that said, alternatives I considered (e.g., per-MM per-folio
mapcount) would add a lot more overhead to these hot paths.
6 Future Work
=============
6.1 Large mapcount
------------------
It would be very handy if the large mapcount would count how often folio
pages are actually mapped into page tables: a PMD on x86-64 would count
512 times. Calculating the average per-page mapcount will be easy, and
remapping (PMD->PTE) folios would get even faster.
That would also remove the need for the entire mapcount (except for
PMD-sized folios for memory statistics reasons ...), and allow for mapping
folios larger than PMDs (e.g., 4 MiB) easily.
The downside is that we maybe would also have to take the same number of
folio references to make our folio_mapcount() == folio_ref_count() work. I
think it should be possible (user space could trigger many PTE mappings
already), but we be a bit more careful about possible mapcount/refcount
overflows. (e.g., fail mapping a folio if the mapcount exceeds a certain
threshold)
Maybe some day we'll have a 64bit refcount+mapcount.
6.2 hugetlb
-----------
I'd love to make use of the same tracking also for hugetlb.
The real problem is PMD table sharing: getting a page mapped by MM X and
unmapped by MM Y will not work. With mshare, that problem should not exist
(all mapping/unmapping will be routed through the mshare MM).
7 Version Updates
=================
I did a bunch of cross-compiles and quite some testing on i386, x86-64 and
arm64. The build bots were very helpful as well.
To keep the CC list short, adding only relevant subsystem maintainers
(CCed on all patches, sorry :) ).
v1 -> v2:
* 32bit support. It would all be easier if we would already allocate
"struct folio" dynamically, but fortunately when we manage to do that,
it will just clean that part up again. For now, we have to relocate in
"struct folio" the _pincount and _entire_mapcount on 32bit, and the
hugetlb data unconditionally.
* "mm/rmap: basic MM owner tracking for large folios (!hugetlb)"
-> Now unconditionally enabled with CONFIG_TRANSPARENT_HUGEPAGE
-> Some changes to slot handling to handle some edge cases in a better
way.
-> Reworked the way flags are stored, in light of 32bit support.
* "mm: convert folio_likely_mapped_shared() to folio_maybe_mapped_shared()"
-> Use the new logic always such that we can rename the function
* A bunch of cleanups/simplifications
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Zefan Li <lizefan.x@bytedance.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: "Michal Koutný" <mkoutny@suse.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Muchun Song <muchun.song@linux.dev>
Cc: "Liam R. Howlett" <Liam.Howlett@oracle.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Jann Horn <jannh@google.com>
David Hildenbrand (20):
mm: factor out large folio handling from folio_order() into
folio_large_order()
mm: factor out large folio handling from folio_nr_pages() into
folio_large_nr_pages()
mm: let _folio_nr_pages overlay memcg_data in first tail page
mm: move hugetlb specific things in folio to page[3]
mm: move _pincount in folio to page[2] on 32bit
mm: move _entire_mapcount in folio to page[2] on 32bit
mm/rmap: pass dst_vma to folio_dup_file_rmap_pte() and friends
mm/rmap: pass vma to __folio_add_rmap()
mm/rmap: abstract large mapcount operations for large folios
(!hugetlb)
bit_spinlock: __always_inline (un)lock functions
mm/rmap: use folio_large_nr_pages() in add/remove functions
mm/rmap: basic MM owner tracking for large folios (!hugetlb)
mm: Copy-on-Write (COW) reuse support for PTE-mapped THP
mm: convert folio_likely_mapped_shared() to
folio_maybe_mapped_shared()
mm: CONFIG_NO_PAGE_MAPCOUNT to prepare for not maintain per-page
mapcounts in large folios
fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount
(CONFIG_NO_PAGE_MAPCOUNT)
fs/proc/task_mmu: remove per-page mapcount dependency for
PM_MMAP_EXCLUSIVE (CONFIG_NO_PAGE_MAPCOUNT)
fs/proc/task_mmu: remove per-page mapcount dependency for "mapmax"
(CONFIG_NO_PAGE_MAPCOUNT)
fs/proc/task_mmu: remove per-page mapcount dependency for
smaps/smaps_rollup (CONFIG_NO_PAGE_MAPCOUNT)
mm: stop maintaining the per-page mapcount of large folios
(CONFIG_NO_PAGE_MAPCOUNT)
.../admin-guide/cgroup-v1/memory.rst | 4 +
Documentation/admin-guide/cgroup-v2.rst | 10 +-
Documentation/admin-guide/mm/pagemap.rst | 16 +-
Documentation/filesystems/proc.rst | 28 +-
Documentation/mm/transhuge.rst | 39 ++-
fs/proc/internal.h | 39 +++
fs/proc/page.c | 19 +-
fs/proc/task_mmu.c | 39 ++-
include/linux/bit_spinlock.h | 8 +-
include/linux/mm.h | 93 +++---
include/linux/mm_types.h | 110 +++++--
include/linux/page-flags.h | 4 +
include/linux/rmap.h | 270 ++++++++++++++++--
kernel/fork.c | 36 +++
mm/Kconfig | 22 ++
mm/debug.c | 10 +-
mm/gup.c | 8 +-
mm/huge_memory.c | 20 +-
mm/hugetlb.c | 1 -
mm/internal.h | 20 +-
mm/khugepaged.c | 8 +-
mm/madvise.c | 6 +-
mm/memory.c | 96 ++++++-
mm/mempolicy.c | 8 +-
mm/migrate.c | 7 +-
mm/mprotect.c | 2 +-
mm/page_alloc.c | 50 +++-
mm/page_owner.c | 2 +-
mm/rmap.c | 118 ++++++--
29 files changed, 903 insertions(+), 190 deletions(-)
base-commit: f7ed46277aaa8f848f18959ff68469f5186ba87c
--
2.48.1
^ 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