* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-02-28 8:30 UTC (permalink / raw)
To: Amir Goldstein
Cc: 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,
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: <CAOQ4uxgyYBFqkq6cQsso4LxJsPJ4uECOdskXmz-nmGhhV5BQWg@mail.gmail.com>
On 2025-02-21 20:15:24, 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.
> >
>
> 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/
>
I'm fine with making Pali's patchset a dependency for this syscall,
as if vfs_fileattr_set() will start returning EINVAL on unsupported
flags this syscall will pass it through (ioctls will need to ignore
it). And as these syscalls use fsxattr anyway the fsx_xflags_mask
field will be here.
--
- Andrey
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Pali Rohár @ 2025-02-25 20:34 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Christian Brauner, Arnd Bergmann, Amir Goldstein,
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,
Jan Kara, Mickaël Salaün, Günther Noack,
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, Theodore Ts'o
In-Reply-To: <20250225155926.GD6265@frogsfrogsfrogs>
On Tuesday 25 February 2025 07:59:26 Darrick J. Wong wrote:
> On Tue, Feb 25, 2025 at 12:24:08PM +0100, Christian Brauner wrote:
> > On Tue, Feb 25, 2025 at 11:40:51AM +0100, Arnd Bergmann wrote:
> > > On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> > > > On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> > > >> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> > > >>
> > > >> The ioctl interface relies on the existing behavior, see
> > > >> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> > > >> CoW extent size hint") for how it was previously extended
> > > >> with an optional flag/word. I think that is fine for the syscall
> > > >> as well, but should be properly documented since it is different
> > > >> from how most syscalls work.
> > > >
> > > > If we're doing a new system call I see no reason to limit us to a
> > > > pre-existing structure or structure layout.
> > >
> > > Obviously we could create a new structure, but I also see no
> > > reason to do so. The existing ioctl interface was added in
> > > in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
> > > of which have been used so far.
> > >
> > > If this structure works for another 23 years before we run out
> > > of spare bytes, I think that's good enough. Building in an
> > > incompatible way to handle potential future contents would
> > > just make it harder to use for any userspace that wants to
> > > use the new syscalls but still needs a fallback to the
> > > ioctl version.
> >
> > The fact that this structure has existed since the dawn of time doesn't
> > mean it needs to be retained when adding a completely new system call.
> >
> > People won't mix both. They either switch to the new interface because
> > they want to get around the limitations of the old interface or they
> > keep using the old interface and the associated workarounds.
> >
> > In another thread they keep arguing about new extensions for Windows
> > that are going to be added to the ioctl interface and how to make it fit
> > into this. That just shows that it's very hard to predict from the
> > amount of past changes how many future changes are going to happen. And
> > if an interface is easy to extend it might well invite new changes that
> > people didn't want to or couldn't make using the old interface.
>
> Agreed, I don't think it's hard to enlarge struct fsxattr in the
> existing ioctl interface; either we figure out how to make the kernel
> fill out the "missing" bytes with an internal getfsxattr call, or we
> make it return some errno if we would be truncating real output due to
> struct size limits and leave a note in the manpage that "EL3HLT means
> use a bigger structure definition"
>
> Then both interfaces can plod along for another 30 years. :)
>
> --D
For Windows attributes, there are for sure needed new 11 bits for
attributes which can be both get and set, additional 4 bits for get-only
attributes, and plus there are 9 reserved bits (which Windows can start
using it and exporting over NTFS or SMB). And it is possible that
Windows can reuse some bits which were previously assigned for things
which today does not appear on NTFS.
I think that fsx_xflags does not have enough free bits for all these
attributes. So it would be really nice to design API/ABI in away which
can be extended for new fields.
Also another two points, for this new syscalls. I have not looked at the
current changes (I was added to CC just recently), but it would be nice:
1) If syscall API allows to operate on the symlink itself. This is
because NTFS and also SMB symlink also contains attributes. ioctl
interface currently does not support to get/set these symlink
attributes.
2) If syscall API contains ability to just change subset of attributes.
And provide an error reporting to userspace if userspace application
is trying to set attribute which is not supported by the filesystem.
This error reporting is needed for possible "cp -a" or possible
"rsync" implementation which informs when some metadata cannot be
backup/restored. There are more filesystems which supports only
subset of attributes, this applies also for windows attributes.
For example UDF fs supports only "hidden" attribute.
^ permalink raw reply
* [PATCH v4 3/5] futex: Wire up set_robust_list2 syscall
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
Wire up the new set_robust_list2 syscall in all available architectures.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
kernel/sys_ni.c | 1 +
scripts/syscall.tbl | 1 +
17 files changed, 17 insertions(+)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index c59d53d6d3f3..d1193a7f948e 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -506,3 +506,4 @@
574 common getxattrat sys_getxattrat
575 common listxattrat sys_listxattrat
576 common removexattrat sys_removexattrat
+577 common set_robust_list2 sys_robust_list2
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 49eeb2ad8dbd..269721f54a5c 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -481,3 +481,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index f5ed71f1910d..75a387585b3a 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -466,3 +466,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 680f568b77f2..176f84b79c1c 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -472,3 +472,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 0b9b7e25b69a..47e28d67ca8a 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -405,3 +405,4 @@
464 n32 getxattrat sys_getxattrat
465 n32 listxattrat sys_listxattrat
466 n32 removexattrat sys_removexattrat
+467 n32 set_robust_list2 sys_set_robust_list2
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index c844cd5cda62..488c1bca7715 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -381,3 +381,4 @@
464 n64 getxattrat sys_getxattrat
465 n64 listxattrat sys_listxattrat
466 n64 removexattrat sys_removexattrat
+467 n64 set_robust_list2 sys_set_robust_list2
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 349b8aad1159..f983086695a8 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -454,3 +454,4 @@
464 o32 getxattrat sys_getxattrat
465 o32 listxattrat sys_listxattrat
466 o32 removexattrat sys_removexattrat
+467 o32 set_robust_list2 sys_set_robust_list2
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index d9fc94c86965..f8735cb8046b 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -465,3 +465,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index d8b4ab78bef0..1da55a6a3bb5 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -557,3 +557,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index e9115b4d8b63..93bda0d6580b 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -469,3 +469,4 @@
464 common getxattrat sys_getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2 sys_set_robust_list2
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index c8cad33bf250..dd591da98af5 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -470,3 +470,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 727f99d333b3..a4ee76e234a3 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -512,3 +512,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 4d0fb2fba7e2..8d609abda75b 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -472,3 +472,4 @@
464 i386 getxattrat sys_getxattrat
465 i386 listxattrat sys_listxattrat
466 i386 removexattrat sys_removexattrat
+467 i386 set_robust_list2 sys_set_robust_list2
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 5eb708bff1c7..2c6461df154b 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -390,6 +390,7 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 37effc1b134e..fa46635d7380 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -437,3 +437,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index c00a86931f8c..71fbac6176c8 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -195,6 +195,7 @@ COND_SYSCALL(move_pages);
COND_SYSCALL(set_mempolicy_home_node);
COND_SYSCALL(cachestat);
COND_SYSCALL(mseal);
+COND_SYSCALL(set_robust_list2);
COND_SYSCALL(perf_event_open);
COND_SYSCALL(accept4);
diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
index ebbdb3c42e9f..615a3043c982 100644
--- a/scripts/syscall.tbl
+++ b/scripts/syscall.tbl
@@ -407,3 +407,4 @@
464 common getxattrat sys_getxattrat
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
+467 common set_robust_list2 sys_set_robust_list2
--
2.48.1
^ permalink raw reply related
* [PATCH v4 4/5] futex: Remove the limit of elements for sys_set_robust_list2 lists
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
Remove the limit of ROBUST_LIST_LIMIT elements that a robust list can
have, for the ones created with the new interface. This is done by
overwritten the list as it's proceeded in a way that we avoid circular
lists.
For the old interface, we keep the limited behavior to avoid changing
the API.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
kernel/futex/core.c | 50 ++++++++++++++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 07a7e5e9bc8d..bfd4443208ea 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -777,7 +777,8 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
* We silently return on any sign of list-walking problem.
*/
static void exit_robust_list64(struct task_struct *curr,
- struct robust_list_head __user *head)
+ struct robust_list_head __user *head,
+ bool destroyable)
{
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
@@ -821,13 +822,17 @@ static void exit_robust_list64(struct task_struct *curr,
}
if (rc)
return;
- entry = next_entry;
- pi = next_pi;
+
/*
* Avoid excessively long or circular lists:
*/
- if (!--limit)
+ if (!destroyable && !--limit)
break;
+ else
+ put_user(&head->list, &entry->next);
+
+ entry = next_entry;
+ pi = next_pi;
cond_resched();
}
@@ -839,7 +844,8 @@ static void exit_robust_list64(struct task_struct *curr,
}
#else
static void exit_robust_list64(struct task_struct *curr,
- struct robust_list_head __user *head)
+ struct robust_list_head __user *head,
+ bool destroyable)
{
pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
}
@@ -877,7 +883,8 @@ fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
* We silently return on any sign of list-walking problem.
*/
static void exit_robust_list32(struct task_struct *curr,
- struct robust_list_head32 __user *head)
+ struct robust_list_head32 __user *head,
+ bool destroyable)
{
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
@@ -926,14 +933,17 @@ static void exit_robust_list32(struct task_struct *curr,
}
if (rc)
return;
- uentry = next_uentry;
- entry = next_entry;
- pi = next_pi;
/*
* Avoid excessively long or circular lists:
*/
- if (!--limit)
+ if (!destroyable && !--limit)
break;
+ else
+ put_user((struct robust_list __user *) &head->list, &entry->next);
+
+ uentry = next_uentry;
+ entry = next_entry;
+ pi = next_pi;
cond_resched();
}
@@ -1087,26 +1097,38 @@ static void exit_pi_state_list(struct task_struct *curr)
static inline void exit_pi_state_list(struct task_struct *curr) { }
#endif
+/*
+ * futex_cleanup - After the task exists, process the robust lists
+ *
+ * Walk through the linked list, parsing robust lists and freeing the
+ * allocated lists. Lists created with the set_robust_list2 don't have a limit
+ * for sizing and can be destroyed while we walk on it to avoid circular list.
+ */
static void futex_cleanup(struct task_struct *tsk)
{
struct robust_list2_entry *curr, *n;
struct list_head *list2 = &tsk->robust_list2;
+ bool destroyable = true;
+ int i = 0;
/*
- * Walk through the linked list, parsing robust lists and freeing the
- * allocated lists
*/
if (unlikely(!list_empty(list2))) {
list_for_each_entry_safe(curr, n, list2, list) {
+ destroyable = true;
+ if (tsk->robust_list_index == i)
+ destroyable = false;
+
if (curr->head != NULL) {
if (curr->list_type == ROBUST_LIST_64BIT)
- exit_robust_list64(tsk, curr->head);
+ exit_robust_list64(tsk, curr->head, destroyable);
else if (curr->list_type == ROBUST_LIST_32BIT)
- exit_robust_list32(tsk, curr->head);
+ exit_robust_list32(tsk, curr->head, destroyable);
curr->head = NULL;
}
list_del_init(&curr->list);
kfree(curr);
+ i++;
}
}
--
2.48.1
^ permalink raw reply related
* [PATCH v4 2/5] futex: Create set_robust_list2
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
Create a new robust_list() syscall. The current syscall can't be
expanded to cover the following use case, so a new one is needed. This
new syscall allows users to set multiple robust lists per process and to
have either 32bit or 64bit pointers in the list.
* Interface
This is the proposed interface:
long set_robust_list2(void *head, int index, unsigned int flags)
`head` is the head of the userspace struct robust_list_head, just as old
set_robust_list(). It needs to be a void pointer since it can point to a
normal robust_list_head or a compat_robust_list_head.
`flags` can be used for defining the list type:
enum robust_list_type {
ROBUST_LIST_32BIT,
ROBUST_LIST_64BIT,
};
`index` is the index in the internal robust_list's linked list (the
naming starts to get confusing, I reckon). If `index == -1`, that means
that user wants to set a new robust_list, and the kernel will append it
in the end of the list, assign a new index and return this index to the
user. If `index >= 0`, that means that user wants to re-set `*head` of
an already existing list (similarly to what happens when you call
set_robust_list() twice with different `*head`).
If `index` is out of range, or it points to a non-existing robust_list,
or if the internal list is full, an error is returned.
Unaligned `head` addresses are refused by the kernel with -EINVAL.
User cannot remove lists.
* Implementation
The old syscall's set/get_robust_list() are converted to use the linked
list as well. When using only the old syscalls user shouldn't any
difference as the internal code will handle the linked list insertion as
usual. When mixing old and new interfaces users should be aware that one
of the elements of the list was created by another syscall and they
should have special care handling this element index.
On exit, the linked list is parsed and all robust lists regardless of
which interface it was used to create them are handled.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/linux/futex.h | 5 +-
include/linux/sched.h | 5 +-
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/futex.h | 24 +++++++
kernel/futex/core.c | 111 ++++++++++++++++++++++++------
kernel/futex/futex.h | 5 ++
kernel/futex/syscalls.c | 81 ++++++++++++++++++++--
7 files changed, 205 insertions(+), 30 deletions(-)
diff --git a/include/linux/futex.h b/include/linux/futex.h
index 8217b5ebdd9c..39335f21aea6 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -72,10 +72,11 @@ enum {
static inline void futex_init_task(struct task_struct *tsk)
{
- tsk->robust_list = NULL;
+ tsk->robust_list_index = -1;
#ifdef CONFIG_COMPAT
- tsk->compat_robust_list = NULL;
+ tsk->compat_robust_list_index = -1;
#endif
+ INIT_LIST_HEAD(&tsk->robust_list2);
INIT_LIST_HEAD(&tsk->pi_state_list);
tsk->pi_state_cache = NULL;
tsk->futex_state = FUTEX_STATE_OK;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 29e500d8d19d..903c1aedbe07 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1297,10 +1297,11 @@ struct task_struct {
u32 rmid;
#endif
#ifdef CONFIG_FUTEX
- struct robust_list_head __user *robust_list;
+ int robust_list_index;
#ifdef CONFIG_COMPAT
- struct robust_list_head32 __user *compat_robust_list;
+ int compat_robust_list_index;
#endif
+ struct list_head robust_list2;
struct list_head pi_state_list;
struct futex_pi_state *pi_state_cache;
struct mutex futex_exit_mutex;
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 88dc393c2bca..477cce02ed72 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -850,8 +850,10 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
#define __NR_removexattrat 466
__SYSCALL(__NR_removexattrat, sys_removexattrat)
+#define __NR_set_robust_list2 467
+
#undef __NR_syscalls
-#define __NR_syscalls 467
+#define __NR_syscalls 468
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index d2ee625ea189..13903a278b71 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -146,6 +146,30 @@ struct robust_list_head {
struct robust_list __user *list_op_pending;
};
+#define ROBUST_LISTS_PER_TASK 10
+
+enum robust_list2_type {
+ ROBUST_LIST_32BIT,
+ ROBUST_LIST_64BIT,
+};
+
+#define ROBUST_LIST_TYPE_MASK (ROBUST_LIST_32BIT | ROBUST_LIST_64BIT)
+
+/*
+ * This is an entry of a linked list of robust lists.
+ *
+ * @head: can point to a 64bit list or a 32bit list
+ * @list_type: determine the size of the futex pointers in the list
+ * @index: the index of this entry in the list
+ * @list: linked list element
+ */
+struct robust_list2_entry {
+ void __user *head;
+ enum robust_list2_type list_type;
+ unsigned int index;
+ struct list_head list;
+};
+
/*
* Are there any waiters for this robust futex:
*/
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 3d81a53c114c..07a7e5e9bc8d 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -776,9 +776,9 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list64(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr,
+ struct robust_list_head __user *head)
{
- struct robust_list_head __user *head = curr->robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
@@ -838,7 +838,8 @@ static void exit_robust_list64(struct task_struct *curr)
}
}
#else
-static void exit_robust_list64(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr,
+ struct robust_list_head __user *head)
{
pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
}
@@ -875,9 +876,9 @@ fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list32(struct task_struct *curr)
+static void exit_robust_list32(struct task_struct *curr,
+ struct robust_list_head32 __user *head)
{
- struct robust_list_head32 __user *head = curr->compat_robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
@@ -943,6 +944,70 @@ static void exit_robust_list32(struct task_struct *curr)
}
}
+long do_set_robust_list2(struct robust_list_head __user *head,
+ int index, unsigned int type)
+{
+ struct list_head *list2 = ¤t->robust_list2;
+ struct robust_list2_entry *prev, *new = NULL;
+
+ if (index == -1) {
+ if (list_empty(list2)) {
+ index = 0;
+ } else {
+ prev = list_last_entry(list2, struct robust_list2_entry, list);
+ index = prev->index + 1;
+ }
+
+ if (index >= ROBUST_LISTS_PER_TASK)
+ return -EINVAL;
+
+ new = kmalloc(sizeof(struct robust_list2_entry), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ list_add_tail(&new->list, list2);
+ new->index = index;
+
+ } else if (index >= 0) {
+ struct robust_list2_entry *curr;
+
+ if (list_empty(list2))
+ return -ENOENT;
+
+ list_for_each_entry(curr, list2, list) {
+ if (index == curr->index) {
+ new = curr;
+ break;
+ }
+ }
+
+ if (!new)
+ return -ENOENT;
+ }
+
+ BUG_ON(!new);
+ new->head = head;
+ new->list_type = type;
+
+ return index;
+}
+
+struct robust_list_head __user *get_robust_list2(int index, struct task_struct *task)
+{
+ struct list_head *list2 = &task->robust_list2;
+ struct robust_list2_entry *curr;
+
+ if (list_empty(list2) || index == -1)
+ return NULL;
+
+ list_for_each_entry(curr, list2, list) {
+ if (index == curr->index)
+ return curr->head;
+ }
+
+ return NULL;
+}
+
#ifdef CONFIG_FUTEX_PI
/*
@@ -1024,24 +1089,28 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
static void futex_cleanup(struct task_struct *tsk)
{
-#ifdef CONFIG_64BIT
- if (unlikely(tsk->robust_list)) {
- exit_robust_list64(tsk);
- tsk->robust_list = NULL;
- }
-#else
- if (unlikely(tsk->robust_list)) {
- exit_robust_list32(tsk);
- tsk->robust_list = NULL;
- }
-#endif
+ struct robust_list2_entry *curr, *n;
+ struct list_head *list2 = &tsk->robust_list2;
-#ifdef CONFIG_COMPAT
- if (unlikely(tsk->compat_robust_list)) {
- exit_robust_list32(tsk);
- tsk->compat_robust_list = NULL;
+ /*
+ * Walk through the linked list, parsing robust lists and freeing the
+ * allocated lists
+ */
+ if (unlikely(!list_empty(list2))) {
+ list_for_each_entry_safe(curr, n, list2, list) {
+ if (curr->head != NULL) {
+ if (curr->list_type == ROBUST_LIST_64BIT)
+ exit_robust_list64(tsk, curr->head);
+ else if (curr->list_type == ROBUST_LIST_32BIT)
+ exit_robust_list32(tsk, curr->head);
+ curr->head = NULL;
+ }
+ list_del_init(&curr->list);
+ kfree(curr);
+ }
}
-#endif
+
+ tsk->robust_list_index = -1;
if (unlikely(!list_empty(&tsk->pi_state_list)))
exit_pi_state_list(tsk);
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 6b2f4c7eb720..b8c20deb5552 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -409,6 +409,11 @@ extern int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
extern int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
ktime_t *abs_time, u32 bitset);
+extern long do_set_robust_list2(struct robust_list_head __user *head,
+ int index, unsigned int type);
+
+extern struct robust_list_head __user *get_robust_list2(int index, struct task_struct *task);
+
/**
* struct futex_vector - Auxiliary struct for futex_waitv()
* @w: Userspace provided data
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index dba193dfd216..56ee1123cbd8 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -20,6 +20,18 @@
* the list. There can only be one such pending lock.
*/
+#ifdef CONFIG_64BIT
+static inline int robust_list_native_type(void)
+{
+ return ROBUST_LIST_64BIT;
+}
+#else
+static inline int robust_list_native_type(void)
+{
+ return ROBUST_LIST_32BIT;
+}
+#endif
+
/**
* sys_set_robust_list() - Set the robust-futex list head of a task
* @head: pointer to the list-head
@@ -28,17 +40,63 @@
SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
size_t, len)
{
+ unsigned int type = robust_list_native_type();
+ int ret;
+
/*
* The kernel knows only one size for now:
*/
if (unlikely(len != sizeof(*head)))
return -EINVAL;
- current->robust_list = head;
+ ret = do_set_robust_list2(head, current->robust_list_index, type);
+ if (ret < 0)
+ return ret;
+
+ current->robust_list_index = ret;
return 0;
}
+#define ROBUST_LIST_FLAGS ROBUST_LIST_TYPE_MASK
+
+/*
+ * sys_set_robust_list2()
+ *
+ * When index == -1, create a new list for user. When index >= 0, try to find
+ * the corresponding list and re-set the head there.
+ *
+ * Return values:
+ * >= 0: success, index of the robust list
+ * -EINVAL: invalid flags, invalid index
+ * -ENOENT: requested index no where to be found
+ * -ENOMEM: error allocating new list
+ * -ESRCH: too many allocated lists
+ */
+SYSCALL_DEFINE3(set_robust_list2, struct robust_list_head __user *, head,
+ int, index, unsigned int, flags)
+{
+ unsigned int type;
+
+ type = flags & ROBUST_LIST_TYPE_MASK;
+
+ if (index < -1 || index >= ROBUST_LISTS_PER_TASK)
+ return -EINVAL;
+
+ if ((flags & ~ROBUST_LIST_FLAGS) != 0)
+ return -EINVAL;
+
+ if (((uintptr_t) head % sizeof(u32)) != 0)
+ return -EINVAL;
+
+#ifndef CONFIG_64BIT
+ if (type == ROBUST_LIST_64BIT)
+ return -EINVAL;
+#endif
+
+ return do_set_robust_list2(head, index, type);
+}
+
/**
* sys_get_robust_list() - Get the robust-futex list head of a task
* @pid: pid of the process [zero for current task]
@@ -52,6 +110,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head __user *head;
unsigned long ret;
struct task_struct *p;
+ int index;
rcu_read_lock();
@@ -68,9 +127,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->robust_list;
+ index = p->robust_list_index;
rcu_read_unlock();
+ head = get_robust_list2(index, p);
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
@@ -443,10 +504,19 @@ COMPAT_SYSCALL_DEFINE2(set_robust_list,
struct robust_list_head32 __user *, head,
compat_size_t, len)
{
+ unsigned int type = ROBUST_LIST_32BIT;
+ int ret;
+
if (unlikely(len != sizeof(*head)))
return -EINVAL;
- current->compat_robust_list = head;
+ ret = do_set_robust_list2((struct robust_list_head __user *) head,
+ current->robust_list_index, type);
+ if (ret < 0)
+ return ret;
+
+ current->robust_list_index = ret;
+
return 0;
}
@@ -458,6 +528,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
struct robust_list_head32 __user *head;
unsigned long ret;
struct task_struct *p;
+ int index;
rcu_read_lock();
@@ -474,9 +545,11 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
goto err_unlock;
- head = p->compat_robust_list;
+ index = p->compat_robust_list_index;
rcu_read_unlock();
+ head = (struct robust_list_head32 __user *) get_robust_list2(index, p);
+
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(ptr_to_compat(head), head_ptr);
--
2.48.1
^ permalink raw reply related
* [PATCH v4 0/5] futex: Create set_robust_list2
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
This patch adds a new robust_list() syscall. The current syscall
can't be expanded to cover the following use case, so a new one is
needed. This new syscall allows users to set multiple robust lists per
process and to have either 32bit or 64bit pointers in the list.
* Use case
FEX-Emu[1] is an application that runs x86 and x86-64 binaries on an
AArch64 Linux host. One of the tasks of FEX-Emu is to translate syscalls
from one platform to another. Existing set_robust_list() can't be easily
translated because of two limitations:
1) x86 apps can have 32bit pointers robust lists. For a x86-64 kernel
this is not a problem, because of the compat entry point. But there's
no such compat entry point for AArch64, so the kernel would do the
pointer arithmetic wrongly. Is also unviable to userspace to keep
track every addition/removal to the robust list and keep a 64bit
version of it somewhere else to feed the kernel. Thus, the new
interface has an option of telling the kernel if the list is filled
with 32bit or 64bit pointers.
2) Apps can set just one robust list (in theory, x86-64 can set two if
they also use the compat entry point). That means that when a x86 app
asks FEX-Emu to call set_robust_list(), FEX have two options: to
overwrite their own robust list pointer and make the app robust, or
to ignore the app robust list and keep the emulator robust. The new
interface allows for multiple robust lists per application, solving
this.
* Interface
This is the proposed interface:
long set_robust_list2(void *head, int index, unsigned int flags)
`head` is the head of the userspace struct robust_list_head, just as old
set_robust_list(). It needs to be a void pointer since it can point to a normal
robust_list_head or a compat_robust_list_head.
`flags` can be used for defining the list type:
enum robust_list_type {
ROBUST_LIST_32BIT,
ROBUST_LIST_64BIT,
};
`index` is the index in the internal robust_list's linked list (the naming
starts to get confusing, I reckon). If `index == -1`, that means that user wants
to set a new robust_list, and the kernel will append it in the end of the list,
assign a new index and return this index to the user. If `index >= 0`, that
means that user wants to re-set `*head` of an already existing list (similarly
to what happens when you call set_robust_list() twice with different `*head`).
If `index` is out of range, or it points to a non-existing robust_list, or if
the internal list is full, an error is returned.
* Implementation
The implementation re-uses most of the existing robust list interface as
possible. The new task_struct member `struct list_head robust_list2` is just a
linked list where new lists are appended as the user requests more lists, and by
futex_cleanup(), the kernel walks through the internal list feeding
exit_robust_list() with the robust_list's.
This implementation supports up to 10 lists (defined at ROBUST_LISTS_PER_TASK),
but it was an arbitrary number for this RFC. For the described use case above, 4
should be enough, I'm not sure which should be the limit.
It doesn't support list removal (should it support?). It doesn't have a proper
get_robust_list2() yet as well, but I can add it in a next revision. We could
also have a generic robust_list() syscall that can be used to set/get and be
controlled by flags.
The new interface has a `unsigned int flags` argument, making it
extensible for future use cases as well.
It refuses unaligned `head` addresses. It doesn't have a limit for elements in a
single list (like ROBUST_LIST_LIMIT), it destroys the list as it is parsed to be
safe against circular lists.
* Testing
This patcheset has a selftest patch that expands this one:
https://lore.kernel.org/lkml/20250212131123.37431-1-andrealmeid@igalia.com/
Also, FEX-Emu added support for this interface to validate it:
https://github.com/FEX-Emu/FEX/pull/3966
Feedback is very welcomed!
Thanks,
André
[1] https://github.com/FEX-Emu/FEX
Changelog:
- Refuse unaligned head pointers
- Ignore ROBUST_LIST_LIMIT for lists created with this interface and make it
robust against circular lists
- Fix a get_robust_list() syscall bug for getting the list from another thread
- Adapt selftest to use the new interface
v3: https://lore.kernel.org/lkml/20241217174958.477692-1-andrealmeid@igalia.com/
- Old syscall set_robust_list() adds new head to the internal linked list of
robust lists pointers, instead of having a field just for them. Remove
tsk->robust_list and use only tsk->robust_list2
v2: https://lore.kernel.org/lkml/20241101162147.284993-1-andrealmeid@igalia.com/
- Added a patch to properly deal with exit_robust_list() in 64bit vs 32bit
- Wired-up syscall for all archs
- Added more of the cover letter to the commit message
v1: https://lore.kernel.org/lkml/20241024145735.162090-1-andrealmeid@igalia.com/
André Almeida (5):
futex: Use explicit sizes for compat_exit_robust_list
futex: Create set_robust_list2
futex: Wire up set_robust_list2 syscall
futex: Remove the limit of elements for sys_set_robust_list2 lists
selftests: futex: Expand robust list test for the new interface
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/compat.h | 12 +-
include/linux/futex.h | 16 +-
include/linux/sched.h | 5 +-
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/futex.h | 24 +++
kernel/futex/core.c | 165 ++++++++++++++----
kernel/futex/futex.h | 5 +
kernel/futex/syscalls.c | 85 ++++++++-
kernel/sys_ni.c | 1 +
scripts/syscall.tbl | 1 +
.../selftests/futex/functional/robust_list.c | 160 ++++++++++++++++-
26 files changed, 436 insertions(+), 57 deletions(-)
--
2.48.1
^ permalink raw reply
* [PATCH v4 5/5] selftests: futex: Expand robust list test for the new interface
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
Expand the current robust list test for the new set_robust_list2
syscall. Create an option to make it possible to run the same tests
using the new syscall, and also add two new relevant test: test long
lists (bigger than ROBUST_LIST_LIMIT) and for unaligned addresses.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
.../selftests/futex/functional/robust_list.c | 160 +++++++++++++++++-
1 file changed, 156 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/futex/functional/robust_list.c b/tools/testing/selftests/futex/functional/robust_list.c
index 42690b2440fd..201acbeeac5a 100644
--- a/tools/testing/selftests/futex/functional/robust_list.c
+++ b/tools/testing/selftests/futex/functional/robust_list.c
@@ -35,16 +35,45 @@
#include <stddef.h>
#include <sys/mman.h>
#include <sys/wait.h>
+#include <stdint.h>
#define STACK_SIZE (1024 * 1024)
#define FUTEX_TIMEOUT 3
+#define SYS_set_robust_list2 467
+
+enum robust_list2_type {
+ ROBUST_LIST_32BIT,
+ ROBUST_LIST_64BIT,
+};
+
static pthread_barrier_t barrier, barrier2;
+bool robust2 = false;
+
int set_robust_list(struct robust_list_head *head, size_t len)
{
- return syscall(SYS_set_robust_list, head, len);
+ int ret, flags;
+
+ if (!robust2) {
+ return syscall(SYS_set_robust_list, head, len);
+ }
+
+ if (sizeof(head) == 8)
+ flags = ROBUST_LIST_64BIT;
+ else
+ flags = ROBUST_LIST_32BIT;
+
+ /*
+ * We act as we have just one list here. We try to use the first slot,
+ * but if it hasn't been alocated yet we allocate it.
+ */
+ ret = syscall(SYS_set_robust_list2, head, 0, flags);
+ if (ret == -1 && errno == ENOENT)
+ ret = syscall(SYS_set_robust_list2, head, -1, flags);
+
+ return ret;
}
int get_robust_list(int pid, struct robust_list_head **head, size_t *len_ptr)
@@ -246,6 +275,11 @@ static void test_set_robust_list_invalid_size(void)
size_t head_size = sizeof(struct robust_list_head);
int ret;
+ if (robust2) {
+ ksft_test_result_skip("This test is only for old robust interface\n");
+ return;
+ }
+
ret = set_robust_list(&head, head_size);
ASSERT_EQ(ret, 0);
@@ -321,6 +355,11 @@ static void test_get_robust_list_child(void)
struct robust_list_head head, *get_head;
size_t len_ptr;
+ if (robust2) {
+ ksft_test_result_skip("Not implemented in the new robust interface\n");
+ return;
+ }
+
ret = pthread_barrier_init(&barrier, NULL, 2);
ret = pthread_barrier_init(&barrier2, NULL, 2);
ASSERT_EQ(ret, 0);
@@ -332,7 +371,7 @@ static void test_get_robust_list_child(void)
ret = get_robust_list(tid, &get_head, &len_ptr);
ASSERT_EQ(ret, 0);
- ASSERT_EQ(&head, get_head);
+ ASSERT_EQ(get_head, &head);
pthread_barrier_wait(&barrier2);
@@ -507,11 +546,119 @@ static void test_circular_list(void)
ksft_test_result_pass("%s\n", __func__);
}
+#define ROBUST_LIST_LIMIT 2048
+#define CHILD_LIST_LIMIT (ROBUST_LIST_LIMIT + 10)
+
+static int child_robust_list_limit(void *arg)
+{
+ struct lock_struct *locks;
+ struct robust_list *list;
+ struct robust_list_head head;
+ int ret, i;
+
+ locks = (struct lock_struct *) arg;
+
+ ret = set_list(&head);
+ if (ret)
+ ksft_test_result_fail("set_list error\n");
+
+ /*
+ * Create a very long list of locks
+ */
+ head.list.next = &locks[0].list;
+
+ list = head.list.next;
+ for (i = 0; i < CHILD_LIST_LIMIT - 1; i++) {
+ list->next = &locks[i+1].list;
+ list = list->next;
+ }
+ list->next = &head.list;
+
+ /*
+ * Grab the lock in the last one, and die without releasing it
+ */
+ mutex_lock(&locks[CHILD_LIST_LIMIT], &head, false);
+ pthread_barrier_wait(&barrier);
+
+ sleep(1);
+
+ return 0;
+}
+
+/*
+ * The old robust list used to have a limit of 2048 items from the kernel side.
+ * After this limit the kernel stops walking the list and ignore the other
+ * futexes, causing deadlocks.
+ *
+ * For the new interface, test if we can wait for a list of more than 2048
+ * elements.
+ */
+static void test_robust_list_limit(void)
+{
+ struct lock_struct locks[CHILD_LIST_LIMIT + 1];
+ _Atomic(unsigned int) *futex = &locks[CHILD_LIST_LIMIT].futex;
+ struct robust_list_head head;
+ int ret;
+
+ if (!robust2) {
+ ksft_test_result_skip("This test is only for new robust interface\n");
+ return;
+ }
+
+ *futex = 0;
+
+ ret = set_list(&head);
+ ASSERT_EQ(ret, 0);
+
+ ret = pthread_barrier_init(&barrier, NULL, 2);
+ ASSERT_EQ(ret, 0);
+
+ create_child(child_robust_list_limit, locks);
+
+ /*
+ * After the child thread creates the very long list of locks, wait on
+ * the last one.
+ */
+ pthread_barrier_wait(&barrier);
+ ret = mutex_lock(&locks[CHILD_LIST_LIMIT], &head, false);
+
+ if (ret != 0)
+ printf("futex wait returned %d\n", errno);
+ ASSERT_EQ(ret, 0);
+
+ ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
+
+ wait(NULL);
+ pthread_barrier_destroy(&barrier);
+
+ ksft_test_result_pass("%s\n", __func__);
+}
+
+/*
+ * The kernel should refuse an unaligned head pointer
+ */
+static void test_unaligned_address(void)
+{
+ struct robust_list_head head, *h;
+ int ret;
+
+ if (!robust2) {
+ ksft_test_result_skip("This test is only for new robust interface\n");
+ return;
+ }
+
+ h = (struct robust_list_head *) ((uintptr_t) &head + 1);
+ ret = set_list(h);
+ ASSERT_EQ(ret, -1);
+ ASSERT_EQ(errno, EINVAL);
+}
+
void usage(char *prog)
{
printf("Usage: %s\n", prog);
printf(" -c Use color\n");
printf(" -h Display this help message\n");
+ printf(" -n Use robust2 syscall\n");
printf(" -v L Verbosity level: %d=QUIET %d=CRITICAL %d=INFO\n",
VQUIET, VCRITICAL, VINFO);
}
@@ -520,7 +667,7 @@ int main(int argc, char *argv[])
{
int c;
- while ((c = getopt(argc, argv, "cht:v:")) != -1) {
+ while ((c = getopt(argc, argv, "chnt:v:")) != -1) {
switch (c) {
case 'c':
log_color(1);
@@ -531,6 +678,9 @@ int main(int argc, char *argv[])
case 'v':
log_verbosity(atoi(optarg));
break;
+ case 'n':
+ robust2 = true;
+ break;
default:
usage(basename(argv[0]));
exit(1);
@@ -538,7 +688,7 @@ int main(int argc, char *argv[])
}
ksft_print_header();
- ksft_set_plan(7);
+ ksft_set_plan(8);
test_robustness();
@@ -548,6 +698,8 @@ int main(int argc, char *argv[])
test_set_list_op_pending();
test_robust_list_multiple_elements();
test_circular_list();
+ test_robust_list_limit();
+ test_unaligned_address();
ksft_print_cnts();
return 0;
--
2.48.1
^ permalink raw reply related
* [PATCH v4 1/5] futex: Use explicit sizes for compat_exit_robust_list
From: André Almeida @ 2025-02-25 18:35 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Arnd Bergmann, sonicadvance1
Cc: linux-kernel, kernel-dev, linux-api, Vinicius Peixoto,
Sebastian Andrzej Siewior, André Almeida
In-Reply-To: <20250225183531.682556-1-andrealmeid@igalia.com>
There are two functions for handling robust lists during the task
exit: exit_robust_list() and compat_exit_robust_list(). The first one
handles either 64bit or 32bit lists, depending if it's a 64bit or 32bit
kernel. The compat_exit_robust_list() only exists in 64bit kernels that
supports 32bit syscalls, and handles 32bit lists.
For the new syscall set_robust_list2(), 64bit kernels need to be able to
handle 32bit lists despite having or not support for 32bit syscalls, so
make compat_exit_robust_list() exist regardless of compat_ config.
Also, use explicitly sizing, otherwise in a 32bit kernel both
exit_robust_list() and compat_exit_robust_list() would be the exactly
same function, with none of them dealing with 64bit robust lists.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/linux/compat.h | 12 +----------
include/linux/futex.h | 11 +++++++++++
include/linux/sched.h | 2 +-
kernel/futex/core.c | 44 ++++++++++++++++++++++++++---------------
kernel/futex/syscalls.c | 4 ++--
5 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 56cebaff0c91..968a9135ff48 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -385,16 +385,6 @@ struct compat_ifconf {
compat_caddr_t ifcbuf;
};
-struct compat_robust_list {
- compat_uptr_t next;
-};
-
-struct compat_robust_list_head {
- struct compat_robust_list list;
- compat_long_t futex_offset;
- compat_uptr_t list_op_pending;
-};
-
#ifdef CONFIG_COMPAT_OLD_SIGACTION
struct compat_old_sigaction {
compat_uptr_t sa_handler;
@@ -672,7 +662,7 @@ asmlinkage long compat_sys_waitid(int, compat_pid_t,
struct compat_siginfo __user *, int,
struct compat_rusage __user *);
asmlinkage long
-compat_sys_set_robust_list(struct compat_robust_list_head __user *head,
+compat_sys_set_robust_list(struct robust_list_head32 __user *head,
compat_size_t len);
asmlinkage long
compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
diff --git a/include/linux/futex.h b/include/linux/futex.h
index b70df27d7e85..8217b5ebdd9c 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -53,6 +53,17 @@ union futex_key {
#define FUTEX_KEY_INIT (union futex_key) { .both = { .ptr = 0ULL } }
#ifdef CONFIG_FUTEX
+
+struct robust_list32 {
+ u32 next;
+};
+
+struct robust_list_head32 {
+ struct robust_list32 list;
+ s32 futex_offset;
+ u32 list_op_pending;
+};
+
enum {
FUTEX_STATE_OK,
FUTEX_STATE_EXITING,
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9632e3318e0d..29e500d8d19d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1299,7 +1299,7 @@ struct task_struct {
#ifdef CONFIG_FUTEX
struct robust_list_head __user *robust_list;
#ifdef CONFIG_COMPAT
- struct compat_robust_list_head __user *compat_robust_list;
+ struct robust_list_head32 __user *compat_robust_list;
#endif
struct list_head pi_state_list;
struct futex_pi_state *pi_state_cache;
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 3db8567f5a44..3d81a53c114c 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -769,13 +769,14 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
return 0;
}
+#ifdef CONFIG_64BIT
/*
* Walk curr->robust_list (very carefully, it's a userspace list!)
* and mark any locks found there dead, and notify any waiters.
*
* We silently return on any sign of list-walking problem.
*/
-static void exit_robust_list(struct task_struct *curr)
+static void exit_robust_list64(struct task_struct *curr)
{
struct robust_list_head __user *head = curr->robust_list;
struct robust_list __user *entry, *next_entry, *pending;
@@ -836,8 +837,13 @@ static void exit_robust_list(struct task_struct *curr)
curr, pip, HANDLE_DEATH_PENDING);
}
}
+#else
+static void exit_robust_list64(struct task_struct *curr)
+{
+ pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
+}
+#endif
-#ifdef CONFIG_COMPAT
static void __user *futex_uaddr(struct robust_list __user *entry,
compat_long_t futex_offset)
{
@@ -851,13 +857,13 @@ static void __user *futex_uaddr(struct robust_list __user *entry,
* Fetch a robust-list pointer. Bit 0 signals PI futexes:
*/
static inline int
-compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **entry,
- compat_uptr_t __user *head, unsigned int *pi)
+fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
+ u32 __user *head, unsigned int *pi)
{
if (get_user(*uentry, head))
return -EFAULT;
- *entry = compat_ptr((*uentry) & ~1);
+ *entry = (void __user *)(unsigned long)((*uentry) & ~1);
*pi = (unsigned int)(*uentry) & 1;
return 0;
@@ -869,21 +875,21 @@ compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **ent
*
* We silently return on any sign of list-walking problem.
*/
-static void compat_exit_robust_list(struct task_struct *curr)
+static void exit_robust_list32(struct task_struct *curr)
{
- struct compat_robust_list_head __user *head = curr->compat_robust_list;
+ struct robust_list_head32 __user *head = curr->compat_robust_list;
struct robust_list __user *entry, *next_entry, *pending;
unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
unsigned int next_pi;
- compat_uptr_t uentry, next_uentry, upending;
- compat_long_t futex_offset;
+ u32 uentry, next_uentry, upending;
+ s32 futex_offset;
int rc;
/*
* Fetch the list head (which was registered earlier, via
* sys_set_robust_list()):
*/
- if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
+ if (fetch_robust_entry32((u32 *)&uentry, &entry, (u32 *)&head->list.next, &pi))
return;
/*
* Fetch the relative futex offset:
@@ -894,7 +900,7 @@ static void compat_exit_robust_list(struct task_struct *curr)
* Fetch any possibly pending lock-add first, and handle it
* if it exists:
*/
- if (compat_fetch_robust_entry(&upending, &pending,
+ if (fetch_robust_entry32(&upending, &pending,
&head->list_op_pending, &pip))
return;
@@ -904,8 +910,8 @@ static void compat_exit_robust_list(struct task_struct *curr)
* Fetch the next entry in the list before calling
* handle_futex_death:
*/
- rc = compat_fetch_robust_entry(&next_uentry, &next_entry,
- (compat_uptr_t __user *)&entry->next, &next_pi);
+ rc = fetch_robust_entry32(&next_uentry, &next_entry,
+ (u32 __user *)&entry->next, &next_pi);
/*
* A pending lock might already be on the list, so
* dont process it twice:
@@ -936,7 +942,6 @@ static void compat_exit_robust_list(struct task_struct *curr)
handle_futex_death(uaddr, curr, pip, HANDLE_DEATH_PENDING);
}
}
-#endif
#ifdef CONFIG_FUTEX_PI
@@ -1019,14 +1024,21 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
static void futex_cleanup(struct task_struct *tsk)
{
+#ifdef CONFIG_64BIT
if (unlikely(tsk->robust_list)) {
- exit_robust_list(tsk);
+ exit_robust_list64(tsk);
tsk->robust_list = NULL;
}
+#else
+ if (unlikely(tsk->robust_list)) {
+ exit_robust_list32(tsk);
+ tsk->robust_list = NULL;
+ }
+#endif
#ifdef CONFIG_COMPAT
if (unlikely(tsk->compat_robust_list)) {
- compat_exit_robust_list(tsk);
+ exit_robust_list32(tsk);
tsk->compat_robust_list = NULL;
}
#endif
diff --git a/kernel/futex/syscalls.c b/kernel/futex/syscalls.c
index 4b6da9116aa6..dba193dfd216 100644
--- a/kernel/futex/syscalls.c
+++ b/kernel/futex/syscalls.c
@@ -440,7 +440,7 @@ SYSCALL_DEFINE4(futex_requeue,
#ifdef CONFIG_COMPAT
COMPAT_SYSCALL_DEFINE2(set_robust_list,
- struct compat_robust_list_head __user *, head,
+ struct robust_list_head32 __user *, head,
compat_size_t, len)
{
if (unlikely(len != sizeof(*head)))
@@ -455,7 +455,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
compat_uptr_t __user *, head_ptr,
compat_size_t __user *, len_ptr)
{
- struct compat_robust_list_head __user *head;
+ struct robust_list_head32 __user *head;
unsigned long ret;
struct task_struct *p;
--
2.48.1
^ permalink raw reply related
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-25 16:48 UTC (permalink / raw)
To: Lorenzo Stoakes, Vlastimil Babka
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <3102ab3b-67b6-4047-9db7-e83c3b9c1c53@lucifer.local>
>> As for compatibility with VM_LOCKONFAULT, do we need a new
>> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
>> that it can be just retrofitted (like you retrofit file backed mappings)?
>> AFAIU the only risk would be breaking somebody that already relies on a
>> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
>>
>>
>
> Hmm yeah I suppose. I guess just to be consistent with the other _LOCKED
> variants? (which seem to be... undocumented at least in man pages :P, and yes I
> realise this is me semi-volunteering to do that obviously...).
>
> But on the other hand, we could also expand this if you and I see also Dave feel
> this makes sense and wouldn't be confusing.
Just my 2 cents: one thing that came to mind: an existing library would
have to be updated to use the _LOCKED variant if the app would be using
mlockall(future), which is a bit unfortunate -- and if it could be
avoided, it would be great.
But yeah, devil is in the detail ...
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Lorenzo Stoakes @ 2025-02-25 16:37 UTC (permalink / raw)
To: Vlastimil Babka
Cc: David Hildenbrand, Andrew Morton, Suren Baghdasaryan,
Liam R . Howlett, Matthew Wilcox, Paul E . McKenney, Jann Horn,
linux-mm, linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <e0954e13-2c7d-447c-ba86-19875c74bc3b@suse.cz>
On Tue, Feb 25, 2025 at 04:54:22PM +0100, Vlastimil Babka wrote:
> On 2/18/25 18:28, Lorenzo Stoakes wrote:
> > On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
> >>
> >> > > >
> >> > > > It fails because it tries to 'touch' the memory, but 'touching' guard
> >> > > > region memory causes a segfault. This kind of breaks the idea of
> >> > > > mlock()'ing guard regions.
> >> > > >
> >> > > > I think adding workarounds to make this possible in any way is not really
> >> > > > worth it (and would probably be pretty gross).
> >> > > >
> >> > > > We already document that 'mlock()ing lightweight guard regions will fail'
> >> > > > as per man page so this is all in line with that.
> >> > >
> >> > > Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
> >> > > allowing install/remove of guard regions when that flag is set.
> >> >
> >> > We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
> >> > disallow.
> >>
> >>
> >> See mlock2();
> >>
> >> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
> >> {
> >> vm_flags_t vm_flags = VM_LOCKED;
> >>
> >> if (flags & ~MLOCK_ONFAULT)
> >> return -EINVAL;
> >>
> >> if (flags & MLOCK_ONFAULT)
> >> vm_flags |= VM_LOCKONFAULT;
> >>
> >> return do_mlock(start, len, vm_flags);
> >> }
> >>
> >>
> >> VM_LOCKONFAULT always as VM_LOCKED set as well.
> >
> > OK cool, that makes sense.
> >
> > As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
> > again, then... :P if only somebody would write it down in a book...
> >
> > Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
> > in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
> > excluded right now.
>
> Sorry for the late reply. So AFAIU from your conversations, guards can't be
> compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
> guards for stacks will soon discover that mlockall() users exist and are
> broken by this, and the attempts will fail? That's a bummer.
>
Yeah damn, this pushes up the priority on this.
Yeah unfortunately we cannot support this with guard regions being installed
_after_ the mlockall() but can for before.
Let me write this on my eternal whiteboard of doom (TM), because that ups the
priority on this. I want to have a good think and see if it might after all be
possible to find a way to make things work here for sake of this case.
This thing is already shipped now, so it is inevitably going to be an add-on.
I will try some experiments when I get a sec.
Thanks very much for bringing this point up! This is pretty key.
> As for compatibility with VM_LOCKONFAULT, do we need a new
> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
> that it can be just retrofitted (like you retrofit file backed mappings)?
> AFAIU the only risk would be breaking somebody that already relies on a
> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
>
>
Hmm yeah I suppose. I guess just to be consistent with the other _LOCKED
variants? (which seem to be... undocumented at least in man pages :P, and yes I
realise this is me semi-volunteering to do that obviously...).
But on the other hand, we could also expand this if you and I see also Dave feel
this makes sense and wouldn't be confusing.
Agreed entirely that it'd be very very odd for a user to rely on that so I think
we'll be fine.
I shall return to this topic later, in the form of a series, probably!
Cheers!
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: David Hildenbrand @ 2025-02-25 16:31 UTC (permalink / raw)
To: Vlastimil Babka, Lorenzo Stoakes
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <e0954e13-2c7d-447c-ba86-19875c74bc3b@suse.cz>
On 25.02.25 16:54, Vlastimil Babka wrote:
> On 2/18/25 18:28, Lorenzo Stoakes wrote:
>> On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
>>>
>>>>>>
>>>>>> It fails because it tries to 'touch' the memory, but 'touching' guard
>>>>>> region memory causes a segfault. This kind of breaks the idea of
>>>>>> mlock()'ing guard regions.
>>>>>>
>>>>>> I think adding workarounds to make this possible in any way is not really
>>>>>> worth it (and would probably be pretty gross).
>>>>>>
>>>>>> We already document that 'mlock()ing lightweight guard regions will fail'
>>>>>> as per man page so this is all in line with that.
>>>>>
>>>>> Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
>>>>> allowing install/remove of guard regions when that flag is set.
>>>>
>>>> We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
>>>> disallow.
>>>
>>>
>>> See mlock2();
>>>
>>> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>>> {
>>> vm_flags_t vm_flags = VM_LOCKED;
>>>
>>> if (flags & ~MLOCK_ONFAULT)
>>> return -EINVAL;
>>>
>>> if (flags & MLOCK_ONFAULT)
>>> vm_flags |= VM_LOCKONFAULT;
>>>
>>> return do_mlock(start, len, vm_flags);
>>> }
>>>
>>>
>>> VM_LOCKONFAULT always as VM_LOCKED set as well.
>>
>> OK cool, that makes sense.
>>
>> As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
>> again, then... :P if only somebody would write it down in a book...
>>
>> Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
>> in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
>> excluded right now.
>
> Sorry for the late reply. So AFAIU from your conversations, guards can't be
> compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
> guards for stacks will soon discover that mlockall() users exist and are
> broken by this, and the attempts will fail? That's a bummer.
>
> As for compatibility with VM_LOCKONFAULT, do we need a new
> MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
> that it can be just retrofitted (like you retrofit file backed mappings)?
> AFAIU the only risk would be breaking somebody that already relies on a
> failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
Exactly my thinking I didn't have time to phrase during that
conversation. IMHO, we were careful with MADV_DONTNEED because it was
... around for a little bit longer :)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Darrick J. Wong @ 2025-02-25 15:59 UTC (permalink / raw)
To: Christian Brauner
Cc: Arnd Bergmann, Amir Goldstein, 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,
Jan Kara, Mickaël Salaün, Günther Noack,
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 Ts'o
In-Reply-To: <20250225-testfahrt-seilwinde-64e6f44c01ce@brauner>
On Tue, Feb 25, 2025 at 12:24:08PM +0100, Christian Brauner wrote:
> On Tue, Feb 25, 2025 at 11:40:51AM +0100, Arnd Bergmann wrote:
> > On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> > > On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> > >> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> > >>
> > >> The ioctl interface relies on the existing behavior, see
> > >> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> > >> CoW extent size hint") for how it was previously extended
> > >> with an optional flag/word. I think that is fine for the syscall
> > >> as well, but should be properly documented since it is different
> > >> from how most syscalls work.
> > >
> > > If we're doing a new system call I see no reason to limit us to a
> > > pre-existing structure or structure layout.
> >
> > Obviously we could create a new structure, but I also see no
> > reason to do so. The existing ioctl interface was added in
> > in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
> > of which have been used so far.
> >
> > If this structure works for another 23 years before we run out
> > of spare bytes, I think that's good enough. Building in an
> > incompatible way to handle potential future contents would
> > just make it harder to use for any userspace that wants to
> > use the new syscalls but still needs a fallback to the
> > ioctl version.
>
> The fact that this structure has existed since the dawn of time doesn't
> mean it needs to be retained when adding a completely new system call.
>
> People won't mix both. They either switch to the new interface because
> they want to get around the limitations of the old interface or they
> keep using the old interface and the associated workarounds.
>
> In another thread they keep arguing about new extensions for Windows
> that are going to be added to the ioctl interface and how to make it fit
> into this. That just shows that it's very hard to predict from the
> amount of past changes how many future changes are going to happen. And
> if an interface is easy to extend it might well invite new changes that
> people didn't want to or couldn't make using the old interface.
Agreed, I don't think it's hard to enlarge struct fsxattr in the
existing ioctl interface; either we figure out how to make the kernel
fill out the "missing" bytes with an internal getfsxattr call, or we
make it return some errno if we would be truncating real output due to
struct size limits and leave a note in the manpage that "EL3HLT means
use a bigger structure definition"
Then both interfaces can plod along for another 30 years. :)
--D
^ permalink raw reply
* Re: [PATCH 0/4] mm: permit guard regions for file-backed/shmem mappings
From: Vlastimil Babka @ 2025-02-25 15:54 UTC (permalink / raw)
To: Lorenzo Stoakes, David Hildenbrand
Cc: Andrew Morton, Suren Baghdasaryan, Liam R . Howlett,
Matthew Wilcox, Paul E . McKenney, Jann Horn, linux-mm,
linux-kernel, Shuah Khan, linux-kselftest, linux-api,
John Hubbard, Juan Yescas, Kalesh Singh
In-Reply-To: <c0e079bd-a840-4240-93ae-0ee2755d425a@lucifer.local>
On 2/18/25 18:28, Lorenzo Stoakes wrote:
> On Tue, Feb 18, 2025 at 06:25:35PM +0100, David Hildenbrand wrote:
>>
>> > > >
>> > > > It fails because it tries to 'touch' the memory, but 'touching' guard
>> > > > region memory causes a segfault. This kind of breaks the idea of
>> > > > mlock()'ing guard regions.
>> > > >
>> > > > I think adding workarounds to make this possible in any way is not really
>> > > > worth it (and would probably be pretty gross).
>> > > >
>> > > > We already document that 'mlock()ing lightweight guard regions will fail'
>> > > > as per man page so this is all in line with that.
>> > >
>> > > Right, and I claim that supporting VM_LOCKONFAULT might likely be as easy as
>> > > allowing install/remove of guard regions when that flag is set.
>> >
>> > We already allow this flag! VM_LOCKED and VM_HUGETLB are the only flags we
>> > disallow.
>>
>>
>> See mlock2();
>>
>> SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>> {
>> vm_flags_t vm_flags = VM_LOCKED;
>>
>> if (flags & ~MLOCK_ONFAULT)
>> return -EINVAL;
>>
>> if (flags & MLOCK_ONFAULT)
>> vm_flags |= VM_LOCKONFAULT;
>>
>> return do_mlock(start, len, vm_flags);
>> }
>>
>>
>> VM_LOCKONFAULT always as VM_LOCKED set as well.
>
> OK cool, that makes sense.
>
> As with much kernel stuff, I knew this in the past. Then I forgot. Then I knew
> again, then... :P if only somebody would write it down in a book...
>
> Yeah then that makes sense to check explicitly for (VM_LOCKED | VM_LOCKONFAULT)
> in any MADV_GUARD_INSTALL_LOCKED variant as obviously this would be passively
> excluded right now.
Sorry for the late reply. So AFAIU from your conversations, guards can't be
compatible with VM_LOCKED, which means e.g. any attempts of glibc to use
guards for stacks will soon discover that mlockall() users exist and are
broken by this, and the attempts will fail? That's a bummer.
As for compatibility with VM_LOCKONFAULT, do we need a new
MADV_GUARD_INSTALL_LOCKED or can we say MADV_GUARD_INSTALL is new enough
that it can be just retrofitted (like you retrofit file backed mappings)?
AFAIU the only risk would be breaking somebody that already relies on a
failure for VM_LOCKONFAULT, and it's unlikely there's such a somebody now.
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-02-25 11:24 UTC (permalink / raw)
To: Arnd Bergmann
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,
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 Ts'o
In-Reply-To: <3c860dc0-ba8d-4324-b286-c160b7d8d2c4@app.fastmail.com>
On Tue, Feb 25, 2025 at 11:40:51AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> > On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> >> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
> >>
> >> The ioctl interface relies on the existing behavior, see
> >> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> >> CoW extent size hint") for how it was previously extended
> >> with an optional flag/word. I think that is fine for the syscall
> >> as well, but should be properly documented since it is different
> >> from how most syscalls work.
> >
> > If we're doing a new system call I see no reason to limit us to a
> > pre-existing structure or structure layout.
>
> Obviously we could create a new structure, but I also see no
> reason to do so. The existing ioctl interface was added in
> in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
> of which have been used so far.
>
> If this structure works for another 23 years before we run out
> of spare bytes, I think that's good enough. Building in an
> incompatible way to handle potential future contents would
> just make it harder to use for any userspace that wants to
> use the new syscalls but still needs a fallback to the
> ioctl version.
The fact that this structure has existed since the dawn of time doesn't
mean it needs to be retained when adding a completely new system call.
People won't mix both. They either switch to the new interface because
they want to get around the limitations of the old interface or they
keep using the old interface and the associated workarounds.
In another thread they keep arguing about new extensions for Windows
that are going to be added to the ioctl interface and how to make it fit
into this. That just shows that it's very hard to predict from the
amount of past changes how many future changes are going to happen. And
if an interface is easy to extend it might well invite new changes that
people didn't want to or couldn't make using the old interface.
^ permalink raw reply
* Re: [PATCH v2 17/20] fs/proc/task_mmu: remove per-page mapcount dependency for PM_MMAP_EXCLUSIVE (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-25 10:56 UTC (permalink / raw)
To: linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
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
In-Reply-To: <20250224165603.1434404-18-david@redhat.com>
On 24.02.25 17:55, David Hildenbrand wrote:
> Let's implement an alternative when per-page mapcounts in large folios are
> no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>
> PM_MMAP_EXCLUSIVE will now be set if folio_likely_mapped_shared() is
> true -- when the folio is considered "mapped shared", including when
> it once was "mapped shared" but no longer is, as documented.
>
> This might result in and under-indication of "exclusively mapped", which
> is considered better than over-indicating it: under-estimating the USS
> (Unique Set Size) is better than over-estimating it.
>
> As an alternative, we could simply remove that flag with
> CONFIG_NO_PAGE_MAPCOUNT completely, but there might be value to it. So,
> let's keep it like that and document the behavior.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> Documentation/admin-guide/mm/pagemap.rst | 9 +++++++++
> fs/proc/task_mmu.c | 11 +++++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
> index 49590306c61a0..131c86574c39a 100644
> --- a/Documentation/admin-guide/mm/pagemap.rst
> +++ b/Documentation/admin-guide/mm/pagemap.rst
> @@ -37,6 +37,15 @@ There are four components to pagemap:
> precisely which pages are mapped (or in swap) and comparing mapped
> pages between processes.
>
> + Note that in some kernel configurations, all pages part of a larger
> + allocation (e.g., THP) might be considered "mapped shared" if the large
> + allocation is considered "mapped shared": if not all pages are exclusive to
> + the same process. Further, some kernel configurations might consider larger
> + allocations "mapped shared", if they were at one point considered
> + "mapped shared", even if they would now be considered "exclusively mapped".
> + Consequently, in these kernel configurations, bit 56 might be set although
> + the page is actually "exclusively mapped"
I rewrote this yet another time to maybe make it clearer ...
+ Traditionally, bit 56 indicates that a page is mapped exactly once and bit
+ 56 is clear when a page is mapped multiple times, even when mapped in the
+ same process multiple times. In some kernel configurations, the semantics
+ for pages part of a larger allocation (e.g., THP) differ: bit 56 is set if
+ all pages part of the corresponding large allocation are *certainly* mapped
+ in the same process, even if the page is mapped multiple times in that
+ process. Bit 56 is clear when any page page of the larger allocation
+ is *maybe* mapped in a different process. In some cases, a large allocation
+ might be treated as "maybe mapped by multiple processes" even though this
+ is no longer the case.
(talking about "process" is not completely correct, it's actually "MMs"; but
that might add more confusion here)
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Arnd Bergmann @ 2025-02-25 10:40 UTC (permalink / raw)
To: Christian Brauner
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,
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 Ts'o
In-Reply-To: <20250225-strom-kopflos-32062347cd13@brauner>
On Tue, Feb 25, 2025, at 11:22, Christian Brauner wrote:
> On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
>> On Mon, Feb 24, 2025, at 12:32, Christian Brauner wrote:
>>
>> The ioctl interface relies on the existing behavior, see
>> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
>> CoW extent size hint") for how it was previously extended
>> with an optional flag/word. I think that is fine for the syscall
>> as well, but should be properly documented since it is different
>> from how most syscalls work.
>
> If we're doing a new system call I see no reason to limit us to a
> pre-existing structure or structure layout.
Obviously we could create a new structure, but I also see no
reason to do so. The existing ioctl interface was added in
in 2002 as part of linux-2.5.35 with 16 bytes of padding, half
of which have been used so far.
If this structure works for another 23 years before we run out
of spare bytes, I think that's good enough. Building in an
incompatible way to handle potential future contents would
just make it harder to use for any userspace that wants to
use the new syscalls but still needs a fallback to the
ioctl version.
Arnd
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-02-25 10:22 UTC (permalink / raw)
To: Arnd Bergmann
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,
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 Ts'o
In-Reply-To: <6b51ffa2-9d67-4466-865e-e703c1243352@app.fastmail.com>
On Tue, Feb 25, 2025 at 09:02:04AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 24, 2025, at 12:32, 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:
>
> >> > > @@ -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.
>
> I don't think that works in this case, because 'struct fsxattr'
> is an existing structure that is defined with a fixed size of
> 28 bytes. If we ever need more than 8 extra bytes, then the
> existing ioctl commands are also broken.
>
> Replacing fsxattr with an extensible structure of the same contents
> would work, but I feel that just adds more complication for little
> gain.
>
> On the other hand, there is an open question about how unknown
> flags and fields in this structure. FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR
> treats them as optional and just ignores anything it doesn't
> understand, while copy_struct_from_user() would treat any unknown
> but set bytes as -E2BIG.
>
> The ioctl interface relies on the existing behavior, see
> 0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
> CoW extent size hint") for how it was previously extended
> with an optional flag/word. I think that is fine for the syscall
> as well, but should be properly documented since it is different
> from how most syscalls work.
If we're doing a new system call I see no reason to limit us to a
pre-existing structure or structure layout.
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Arnd Bergmann @ 2025-02-25 8:02 UTC (permalink / raw)
To: Christian Brauner
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,
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 Ts'o
In-Reply-To: <20250224-klinke-hochdekoriert-3f6be89005a8@brauner>
On Mon, Feb 24, 2025, at 12:32, 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:
>> > > @@ -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.
I don't think that works in this case, because 'struct fsxattr'
is an existing structure that is defined with a fixed size of
28 bytes. If we ever need more than 8 extra bytes, then the
existing ioctl commands are also broken.
Replacing fsxattr with an extensible structure of the same contents
would work, but I feel that just adds more complication for little
gain.
On the other hand, there is an open question about how unknown
flags and fields in this structure. FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR
treats them as optional and just ignores anything it doesn't
understand, while copy_struct_from_user() would treat any unknown
but set bytes as -E2BIG.
The ioctl interface relies on the existing behavior, see
0a6eab8bd4e0 ("vfs: support FS_XFLAG_COWEXTSIZE and get/set of
CoW extent size hint") for how it was previously extended
with an optional flag/word. I think that is fine for the syscall
as well, but should be properly documented since it is different
from how most syscalls work.
Arnd
^ permalink raw reply
* Re: [PATCH v3] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Paul Moore @ 2025-02-25 2:37 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Mickaël Salaün, 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: <jvo6uj7ro5czlo5ukw3vtf5mpqgrbuksqq4j63s2i6gwrjpz4m@kghpcqyi7gwb>
On Mon, Feb 24, 2025 at 11:00 AM Andrey Albershteyn <aalbersh@redhat.com> wrote:
> 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:
...
> > > +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
A valid point. While these are two different operations, with
different structs/types, I suspect that most LSMs will consider them
to be roughly equivalent from an access control perspective, which is
why I felt the existing security_inode_{set,get}attr() hooks seemed
appropriate. However, there likely is value in keeping the ATTR and
FSX operations separate; those LSMs that wish to treat them the same
can easily do so in their respective LSM callbacks.
With all this in mind, I think it probably makes sense to create two
new LSM hooks, security_inode_{get,set}fsxattr(). The get hook should
probably be placed inside vfs_fileattr_get() just before the call to
the inode's fileattr_get() method, and the set hook should probably be
placed inside vfs_fileattr_set(), inside the inode lock and after a
successful call to fileattr_set_prepare().
Does that sound better to everyone?
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:53 UTC (permalink / raw)
To: Zi Yan
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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
In-Reply-To: <1FAD9E31-3D11-4759-9363-4B76BE96002A@nvidia.com>
On 24.02.25 22:44, Zi Yan wrote:
> On 24 Feb 2025, at 16:42, David Hildenbrand wrote:
>
>> On 24.02.25 22:23, Zi Yan wrote:
>>> On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
>>>
>>>> On 24.02.25 22:10, Zi Yan wrote:
>>>>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>>>>
>>>>>> On 24.02.25 21:40, Zi Yan wrote:
>>>>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>>
>>>>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>>>>> return 1.
>>>>>>>>
>>>>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>>>>> into all address spaces, there is no change.
>>>>>>>>
>>>>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>>
>>>>>>>> But the information exposed by this interface can still be valuable, and
>>>>>>>> frequently we deal with fully-mapped large folios where the average
>>>>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>>>>> now and document the new behavior.
>>>>>>>>
>>>>>>>> Note: this interface is likely not very relevant for performance. If
>>>>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>>>>> precisely how often this folio page is mapped.
>>>>>>>>
>>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>>> ---
>>>>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>>>>> skip over unmapped regions.
>>>>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>>>>> - times each page is mapped, indexed by PFN.
>>>>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>>>>> + not track the precise number of times a page part of a larger allocation
>>>>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>>>>> + be at least 1.
>>>>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>>>>> number of times a page is mapped.
>>>>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>>>>> --- a/fs/proc/internal.h
>>>>>>>> +++ b/fs/proc/internal.h
>>>>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>>>>> return mapcount;
>>>>>>>> }
>>>>>>>> +/**
>>>>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>>>>> + * folio
>>>>>>>> + * @folio: The folio.
>>>>>>>> + *
>>>>>>>> + * The average number of present user page table entries that reference each
>>>>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>>>>> + *
>>>>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>>>>> + * (e.g., shared zeropage).
>>>>>>>> + */
>>>>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>>>>> +{
>>>>>>>> + int mapcount, entire_mapcount;
>>>>>>>> + unsigned int adjust;
>>>>>>>> +
>>>>>>>> + if (!folio_test_large(folio))
>>>>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>>>>> +
>>>>>>>> + mapcount = folio_large_mapcount(folio);
>>>>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>>>>> + if (mapcount <= entire_mapcount)
>>>>>>>> + return entire_mapcount;
>>>>>>>> + mapcount -= entire_mapcount;
>>>>>>>> +
>>>>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>>>>
>>>>>> Thanks for the review!
>>>>>>
>>>>>>>
>>>>>>> Is there any reason for choosing this adjust number? A comment might be
>>>>>>> helpful in case people want to change it later, either with some reasoning
>>>>>>> or just saying it is chosen empirically.
>>>>>>
>>>>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>>>>
>>>>>> So this should be "ordinary" rounding.
>>>>>
>>>>> I thought the rounding would be (mapcount + 511) / 512.
>>>>
>>>> Yes, that's "rounding up".
>>>>
>>>>> But
>>>>> that means if one subpage is mapped, the average will be 1.
>>>>> Your rounding means if at least half of the subpages is mapped,
>>>>> the average will be 1. Others might think 1/3 is mapped,
>>>>> the average will be 1. That is why I think adjust looks like
>>>>> a magic number.
>>>>
>>>> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>>>>
>>>> There was a reason why I decided to round to the nearest integer instead.
>>>>
>>>> Let me think about this once more, I went back and forth a couple of times on this.
>>>
>>> Sure. Your current choice might be good enough for now. My intend of
>>> adding a comment here is just to let people know the adjust can be
>>> changed in the future. :)
>>
>> The following will make the callers easier to read, while keeping
>> the rounding to the next integer for the other cases untouched.
>>
>> +/**
>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>> + * folio
>> + * @folio: The folio.
>> + *
>> + * The average number of present user page table entries that reference each
>> + * page in this folio as tracked via the RMAP: either referenced directly
>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>> + *
>> + * The average is calculated by rounding to the nearest integer; however,
>> + * if at least a single page is mapped, the average will be at least 1.
>> + *
>> + * Returns: The average number of mappings per page in this folio.
>> + */
>> +static inline int folio_average_page_mapcount(struct folio *folio)
>> +{
>> + int mapcount, entire_mapcount, avg;
>> +
>> + if (!folio_test_large(folio))
>> + return atomic_read(&folio->_mapcount) + 1;
>> +
>> + mapcount = folio_large_mapcount(folio);
>> + if (unlikely(mapcount <= 0))
>> + return 0;
>> + entire_mapcount = folio_entire_mapcount(folio);
>> + if (mapcount <= entire_mapcount)
>> + return entire_mapcount;
>> + mapcount -= entire_mapcount;
>> +
>> + /* Round to closest integer ... */
>> + avg = (mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
>> + avg += entire_mapcount;
>> + /* ... but return at least 1. */
>> + return max_t(int, avg, 1);
>> +}
>
> LGTM. Thanks.
Thanks! BTW, I think I chose the "round to closest integer" primarily
to make the PSS estimate a bit better. But that is indeed something
that can be adjusted easily later.
BTW, as commented in the cover letter, being able to calculate the
avg without the entire_mapcount could clean this function up quite a bit
(and make it completely atomic), but that will require more work.
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: Zi Yan @ 2025-02-24 21:44 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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
In-Reply-To: <3f6b7e66-3412-4af2-97d9-6d31d6373079@redhat.com>
On 24 Feb 2025, at 16:42, David Hildenbrand wrote:
> On 24.02.25 22:23, Zi Yan wrote:
>> On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
>>
>>> On 24.02.25 22:10, Zi Yan wrote:
>>>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>>>
>>>>> On 24.02.25 21:40, Zi Yan wrote:
>>>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>
>>>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>>>> return 1.
>>>>>>>
>>>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>>>> into all address spaces, there is no change.
>>>>>>>
>>>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>>
>>>>>>> But the information exposed by this interface can still be valuable, and
>>>>>>> frequently we deal with fully-mapped large folios where the average
>>>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>>>> now and document the new behavior.
>>>>>>>
>>>>>>> Note: this interface is likely not very relevant for performance. If
>>>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>>>> precisely how often this folio page is mapped.
>>>>>>>
>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>> ---
>>>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>>>> skip over unmapped regions.
>>>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>>>> - times each page is mapped, indexed by PFN.
>>>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>>>> + not track the precise number of times a page part of a larger allocation
>>>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>>>> + be at least 1.
>>>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>>>> number of times a page is mapped.
>>>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>>>> --- a/fs/proc/internal.h
>>>>>>> +++ b/fs/proc/internal.h
>>>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>>>> return mapcount;
>>>>>>> }
>>>>>>> +/**
>>>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>>>> + * folio
>>>>>>> + * @folio: The folio.
>>>>>>> + *
>>>>>>> + * The average number of present user page table entries that reference each
>>>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>>>> + *
>>>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>>>> + * (e.g., shared zeropage).
>>>>>>> + */
>>>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>>>> +{
>>>>>>> + int mapcount, entire_mapcount;
>>>>>>> + unsigned int adjust;
>>>>>>> +
>>>>>>> + if (!folio_test_large(folio))
>>>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>>>> +
>>>>>>> + mapcount = folio_large_mapcount(folio);
>>>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>>>> + if (mapcount <= entire_mapcount)
>>>>>>> + return entire_mapcount;
>>>>>>> + mapcount -= entire_mapcount;
>>>>>>> +
>>>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>>>
>>>>>> Is there any reason for choosing this adjust number? A comment might be
>>>>>> helpful in case people want to change it later, either with some reasoning
>>>>>> or just saying it is chosen empirically.
>>>>>
>>>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>>>
>>>>> So this should be "ordinary" rounding.
>>>>
>>>> I thought the rounding would be (mapcount + 511) / 512.
>>>
>>> Yes, that's "rounding up".
>>>
>>>> But
>>>> that means if one subpage is mapped, the average will be 1.
>>>> Your rounding means if at least half of the subpages is mapped,
>>>> the average will be 1. Others might think 1/3 is mapped,
>>>> the average will be 1. That is why I think adjust looks like
>>>> a magic number.
>>>
>>> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>>>
>>> There was a reason why I decided to round to the nearest integer instead.
>>>
>>> Let me think about this once more, I went back and forth a couple of times on this.
>>
>> Sure. Your current choice might be good enough for now. My intend of
>> adding a comment here is just to let people know the adjust can be
>> changed in the future. :)
>
> The following will make the callers easier to read, while keeping
> the rounding to the next integer for the other cases untouched.
>
> +/**
> + * folio_average_page_mapcount() - Average number of mappings per page in this
> + * folio
> + * @folio: The folio.
> + *
> + * The average number of present user page table entries that reference each
> + * page in this folio as tracked via the RMAP: either referenced directly
> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
> + *
> + * The average is calculated by rounding to the nearest integer; however,
> + * if at least a single page is mapped, the average will be at least 1.
> + *
> + * Returns: The average number of mappings per page in this folio.
> + */
> +static inline int folio_average_page_mapcount(struct folio *folio)
> +{
> + int mapcount, entire_mapcount, avg;
> +
> + if (!folio_test_large(folio))
> + return atomic_read(&folio->_mapcount) + 1;
> +
> + mapcount = folio_large_mapcount(folio);
> + if (unlikely(mapcount <= 0))
> + return 0;
> + entire_mapcount = folio_entire_mapcount(folio);
> + if (mapcount <= entire_mapcount)
> + return entire_mapcount;
> + mapcount -= entire_mapcount;
> +
> + /* Round to closest integer ... */
> + avg = (mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
> + avg += entire_mapcount;
> + /* ... but return at least 1. */
> + return max_t(int, avg, 1);
> +}
LGTM. Thanks.
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:42 UTC (permalink / raw)
To: Zi Yan
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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
In-Reply-To: <30C2A030-7438-4298-87D8-287BED1EA473@nvidia.com>
On 24.02.25 22:23, Zi Yan wrote:
> On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
>
>> On 24.02.25 22:10, Zi Yan wrote:
>>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>>
>>>> On 24.02.25 21:40, Zi Yan wrote:
>>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>
>>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>>> return 1.
>>>>>>
>>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>>> into all address spaces, there is no change.
>>>>>>
>>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>>
>>>>>> But the information exposed by this interface can still be valuable, and
>>>>>> frequently we deal with fully-mapped large folios where the average
>>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>>> now and document the new behavior.
>>>>>>
>>>>>> Note: this interface is likely not very relevant for performance. If
>>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>>> precisely how often this folio page is mapped.
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>>> skip over unmapped regions.
>>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>>> - times each page is mapped, indexed by PFN.
>>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>>> + not track the precise number of times a page part of a larger allocation
>>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>>> + be at least 1.
>>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>>> number of times a page is mapped.
>>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>>> --- a/fs/proc/internal.h
>>>>>> +++ b/fs/proc/internal.h
>>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>>> return mapcount;
>>>>>> }
>>>>>> +/**
>>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>>> + * folio
>>>>>> + * @folio: The folio.
>>>>>> + *
>>>>>> + * The average number of present user page table entries that reference each
>>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>>> + *
>>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>>> + * (e.g., shared zeropage).
>>>>>> + */
>>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>>> +{
>>>>>> + int mapcount, entire_mapcount;
>>>>>> + unsigned int adjust;
>>>>>> +
>>>>>> + if (!folio_test_large(folio))
>>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>>> +
>>>>>> + mapcount = folio_large_mapcount(folio);
>>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>>> + if (mapcount <= entire_mapcount)
>>>>>> + return entire_mapcount;
>>>>>> + mapcount -= entire_mapcount;
>>>>>> +
>>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>>
>>>> Thanks for the review!
>>>>
>>>>>
>>>>> Is there any reason for choosing this adjust number? A comment might be
>>>>> helpful in case people want to change it later, either with some reasoning
>>>>> or just saying it is chosen empirically.
>>>>
>>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>>
>>>> So this should be "ordinary" rounding.
>>>
>>> I thought the rounding would be (mapcount + 511) / 512.
>>
>> Yes, that's "rounding up".
>>
>>> But
>>> that means if one subpage is mapped, the average will be 1.
>>> Your rounding means if at least half of the subpages is mapped,
>>> the average will be 1. Others might think 1/3 is mapped,
>>> the average will be 1. That is why I think adjust looks like
>>> a magic number.
>>
>> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>>
>> There was a reason why I decided to round to the nearest integer instead.
>>
>> Let me think about this once more, I went back and forth a couple of times on this.
>
> Sure. Your current choice might be good enough for now. My intend of
> adding a comment here is just to let people know the adjust can be
> changed in the future. :)
The following will make the callers easier to read, while keeping
the rounding to the next integer for the other cases untouched.
+/**
+ * folio_average_page_mapcount() - Average number of mappings per page in this
+ * folio
+ * @folio: The folio.
+ *
+ * The average number of present user page table entries that reference each
+ * page in this folio as tracked via the RMAP: either referenced directly
+ * (PTE) or as part of a larger area that covers this page (e.g., PMD).
+ *
+ * The average is calculated by rounding to the nearest integer; however,
+ * if at least a single page is mapped, the average will be at least 1.
+ *
+ * Returns: The average number of mappings per page in this folio.
+ */
+static inline int folio_average_page_mapcount(struct folio *folio)
+{
+ int mapcount, entire_mapcount, avg;
+
+ if (!folio_test_large(folio))
+ return atomic_read(&folio->_mapcount) + 1;
+
+ mapcount = folio_large_mapcount(folio);
+ if (unlikely(mapcount <= 0))
+ return 0;
+ entire_mapcount = folio_entire_mapcount(folio);
+ if (mapcount <= entire_mapcount)
+ return entire_mapcount;
+ mapcount -= entire_mapcount;
+
+ /* Round to closest integer ... */
+ avg = (mapcount + folio_large_nr_pages(folio) / 2) >> folio_large_order(folio);
+ avg += entire_mapcount;
+ /* ... but return at least 1. */
+ return max_t(int, avg, 1);
+}
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: Zi Yan @ 2025-02-24 21:23 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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
In-Reply-To: <567b02b0-3e39-4e3c-ba41-1bc59217a421@redhat.com>
On 24 Feb 2025, at 16:15, David Hildenbrand wrote:
> On 24.02.25 22:10, Zi Yan wrote:
>> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>>
>>> On 24.02.25 21:40, Zi Yan wrote:
>>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>
>>>>> For large folios, we'll return the per-page average mapcount within the
>>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>>> return 1.
>>>>>
>>>>> For hugetlb folios and for large folios that are fully mapped
>>>>> into all address spaces, there is no change.
>>>>>
>>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>>
>>>>> But the information exposed by this interface can still be valuable, and
>>>>> frequently we deal with fully-mapped large folios where the average
>>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>>> now and document the new behavior.
>>>>>
>>>>> Note: this interface is likely not very relevant for performance. If
>>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>>> precisely how often this folio page is mapped.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>>> fs/proc/page.c | 19 ++++++++++++---
>>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>>> index caba0f52dd36c..49590306c61a0 100644
>>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>>> skip over unmapped regions.
>>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>>> - times each page is mapped, indexed by PFN.
>>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>>> + not track the precise number of times a page part of a larger allocation
>>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>>> + mappings per page in this larger allocation is returned instead. However,
>>>>> + if any page of the large allocation is mapped, the returned value will
>>>>> + be at least 1.
>>>>> The page-types tool in the tools/mm directory can be used to query the
>>>>> number of times a page is mapped.
>>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>>> index 1695509370b88..16aa1fd260771 100644
>>>>> --- a/fs/proc/internal.h
>>>>> +++ b/fs/proc/internal.h
>>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>>> return mapcount;
>>>>> }
>>>>> +/**
>>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>>> + * folio
>>>>> + * @folio: The folio.
>>>>> + *
>>>>> + * The average number of present user page table entries that reference each
>>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>>> + *
>>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>>> + * (e.g., shared zeropage).
>>>>> + */
>>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>>> +{
>>>>> + int mapcount, entire_mapcount;
>>>>> + unsigned int adjust;
>>>>> +
>>>>> + if (!folio_test_large(folio))
>>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>>> +
>>>>> + mapcount = folio_large_mapcount(folio);
>>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>>> + if (mapcount <= entire_mapcount)
>>>>> + return entire_mapcount;
>>>>> + mapcount -= entire_mapcount;
>>>>> +
>>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>>
>>> Thanks for the review!
>>>
>>>>
>>>> Is there any reason for choosing this adjust number? A comment might be
>>>> helpful in case people want to change it later, either with some reasoning
>>>> or just saying it is chosen empirically.
>>>
>>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>>
>>> So this should be "ordinary" rounding.
>>
>> I thought the rounding would be (mapcount + 511) / 512.
>
> Yes, that's "rounding up".
>
>> But
>> that means if one subpage is mapped, the average will be 1.
>> Your rounding means if at least half of the subpages is mapped,
>> the average will be 1. Others might think 1/3 is mapped,
>> the average will be 1. That is why I think adjust looks like
>> a magic number.
>
> I think all callers could tolerate (or benefit) from folio_average_page_mapcount() returning at least 1 in case any page is mapped.
>
> There was a reason why I decided to round to the nearest integer instead.
>
> Let me think about this once more, I went back and forth a couple of times on this.
Sure. Your current choice might be good enough for now. My intend of
adding a comment here is just to let people know the adjust can be
changed in the future. :)
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCH v2 19/20] fs/proc/task_mmu: remove per-page mapcount dependency for smaps/smaps_rollup (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:17 UTC (permalink / raw)
To: Zi Yan, linux-kernel
Cc: linux-doc, cgroups, linux-mm, linux-fsdevel, linux-api,
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
In-Reply-To: <D80Z3H6NZARU.1HP5EKXOJ68QH@nvidia.com>
On 24.02.25 21:53, Zi Yan wrote:
> On Mon Feb 24, 2025 at 11:56 AM EST, David Hildenbrand wrote:
>> Let's implement an alternative when per-page mapcounts in large folios are
>> no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>
>> When computing the output for smaps / smaps_rollups, in particular when
>> calculating the USS (Unique Set Size) and the PSS (Proportional Set Size),
>> we still rely on per-page mapcounts.
>>
>> To determine private vs. shared, we'll use folio_likely_mapped_shared(),
>> similar to how we handle PM_MMAP_EXCLUSIVE. Similarly, we might now
>> under-estimate the USS and count pages towards "shared" that are
>> actually "private" ("exclusively mapped").
>>
>> When calculating the PSS, we'll now also use the average per-page
>> mapcount for large folios: this can result in both, an over-estimation
>> and an under-estimation of the PSS. The difference is not expected to
>> matter much in practice, but we'll have to learn as we go.
>>
>> We can now provide folio_precise_page_mapcount() only with
>> CONFIG_PAGE_MAPCOUNT, and remove one of the last users of per-page
>> mapcounts when CONFIG_NO_PAGE_MAPCOUNT is enabled.
>>
>> Document the new behavior.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> Documentation/filesystems/proc.rst | 13 +++++++++++++
>> fs/proc/internal.h | 8 ++++++++
>> fs/proc/task_mmu.c | 17 +++++++++++++++--
>> 3 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>> index 1aa190017f796..57d55274a1f42 100644
>> --- a/Documentation/filesystems/proc.rst
>> +++ b/Documentation/filesystems/proc.rst
>> @@ -506,6 +506,19 @@ Note that even a page which is part of a MAP_SHARED mapping, but has only
>> a single pte mapped, i.e. is currently used by only one process, is accounted
>> as private and not as shared.
>>
>> +Note that in some kernel configurations, all pages part of a larger allocation
>> +(e.g., THP) might be considered "shared" if the large allocation is
>> +considered "shared": if not all pages are exclusive to the same process.
>> +Further, some kernel configurations might consider larger allocations "shared",
>> +if they were at one point considered "shared", even if they would now be
>> +considered "exclusive".
>> +
>> +Some kernel configurations do not track the precise number of times a page part
>> +of a larger allocation is mapped. In this case, when calculating the PSS, the
>> +average number of mappings per page in this larger allocation might be used
>> +as an approximation for the number of mappings of a page. The PSS calculation
>> +will be imprecise in this case.
>> +
>> "Referenced" indicates the amount of memory currently marked as referenced or
>> accessed.
>>
>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>> index 16aa1fd260771..70205425a2daa 100644
>> --- a/fs/proc/internal.h
>> +++ b/fs/proc/internal.h
>> @@ -143,6 +143,7 @@ unsigned name_to_int(const struct qstr *qstr);
>> /* Worst case buffer size needed for holding an integer. */
>> #define PROC_NUMBUF 13
>>
>> +#ifdef CONFIG_PAGE_MAPCOUNT
>> /**
>> * folio_precise_page_mapcount() - Number of mappings of this folio page.
>> * @folio: The folio.
>> @@ -173,6 +174,13 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>
>> return mapcount;
>> }
>> +#else /* !CONFIG_PAGE_MAPCOUNT */
>> +static inline int folio_precise_page_mapcount(struct folio *folio,
>> + struct page *page)
>> +{
>> + BUILD_BUG();
>> +}
>> +#endif /* CONFIG_PAGE_MAPCOUNT */
>>
>> /**
>> * folio_average_page_mapcount() - Average number of mappings per page in this
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index d7ee842367f0f..7ca0bc3bf417d 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -707,6 +707,8 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
>> struct folio *folio = page_folio(page);
>> int i, nr = compound ? compound_nr(page) : 1;
>> unsigned long size = nr * PAGE_SIZE;
>> + bool exclusive;
>> + int mapcount;
>>
>> /*
>> * First accumulate quantities that depend only on |size| and the type
>> @@ -747,18 +749,29 @@ static void smaps_account(struct mem_size_stats *mss, struct page *page,
>> dirty, locked, present);
>> return;
>> }
>> +
>> + if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) {
>> + mapcount = folio_average_page_mapcount(folio);
>
> This seems inconsistent with how folio_average_page_mapcount() is used
> in patch 16 and 18.
It only looks that way. Note that the code below only does
>> if (mapcount >= 2)
>> pss /= mapcount;
Having it as 0 or 1 doesn't matter in that regard.
Thanks!
--
Cheers,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v2 16/20] fs/proc/page: remove per-page mapcount dependency for /proc/kpagecount (CONFIG_NO_PAGE_MAPCOUNT)
From: David Hildenbrand @ 2025-02-24 21:15 UTC (permalink / raw)
To: Zi Yan
Cc: linux-kernel, linux-doc, cgroups, linux-mm, linux-fsdevel,
linux-api, 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
In-Reply-To: <9010E213-9FC5-4900-B971-D032CB879F2E@nvidia.com>
On 24.02.25 22:10, Zi Yan wrote:
> On 24 Feb 2025, at 16:02, David Hildenbrand wrote:
>
>> On 24.02.25 21:40, Zi Yan wrote:
>>> On Mon Feb 24, 2025 at 11:55 AM EST, David Hildenbrand wrote:
>>>> Let's implement an alternative when per-page mapcounts in large folios
>>>> are no longer maintained -- soon with CONFIG_NO_PAGE_MAPCOUNT.
>>>>
>>>> For large folios, we'll return the per-page average mapcount within the
>>>> folio, except when the average is 0 but the folio is mapped: then we
>>>> return 1.
>>>>
>>>> For hugetlb folios and for large folios that are fully mapped
>>>> into all address spaces, there is no change.
>>>>
>>>> As an alternative, we could simply return 0 for non-hugetlb large folios,
>>>> or disable this legacy interface with CONFIG_NO_PAGE_MAPCOUNT.
>>>>
>>>> But the information exposed by this interface can still be valuable, and
>>>> frequently we deal with fully-mapped large folios where the average
>>>> corresponds to the actual page mapcount. So we'll leave it like this for
>>>> now and document the new behavior.
>>>>
>>>> Note: this interface is likely not very relevant for performance. If
>>>> ever required, we could try doing a rather expensive rmap walk to collect
>>>> precisely how often this folio page is mapped.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>> Documentation/admin-guide/mm/pagemap.rst | 7 +++++-
>>>> fs/proc/internal.h | 31 ++++++++++++++++++++++++
>>>> fs/proc/page.c | 19 ++++++++++++---
>>>> 3 files changed, 53 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
>>>> index caba0f52dd36c..49590306c61a0 100644
>>>> --- a/Documentation/admin-guide/mm/pagemap.rst
>>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
>>>> @@ -42,7 +42,12 @@ There are four components to pagemap:
>>>> skip over unmapped regions.
>>>> * ``/proc/kpagecount``. This file contains a 64-bit count of the number of
>>>> - times each page is mapped, indexed by PFN.
>>>> + times each page is mapped, indexed by PFN. Some kernel configurations do
>>>> + not track the precise number of times a page part of a larger allocation
>>>> + (e.g., THP) is mapped. In these configurations, the average number of
>>>> + mappings per page in this larger allocation is returned instead. However,
>>>> + if any page of the large allocation is mapped, the returned value will
>>>> + be at least 1.
>>>> The page-types tool in the tools/mm directory can be used to query the
>>>> number of times a page is mapped.
>>>> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
>>>> index 1695509370b88..16aa1fd260771 100644
>>>> --- a/fs/proc/internal.h
>>>> +++ b/fs/proc/internal.h
>>>> @@ -174,6 +174,37 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
>>>> return mapcount;
>>>> }
>>>> +/**
>>>> + * folio_average_page_mapcount() - Average number of mappings per page in this
>>>> + * folio
>>>> + * @folio: The folio.
>>>> + *
>>>> + * The average number of present user page table entries that reference each
>>>> + * page in this folio as tracked via the RMAP: either referenced directly
>>>> + * (PTE) or as part of a larger area that covers this page (e.g., PMD).
>>>> + *
>>>> + * Returns: The average number of mappings per page in this folio. 0 for
>>>> + * folios that are not mapped to user space or are not tracked via the RMAP
>>>> + * (e.g., shared zeropage).
>>>> + */
>>>> +static inline int folio_average_page_mapcount(struct folio *folio)
>>>> +{
>>>> + int mapcount, entire_mapcount;
>>>> + unsigned int adjust;
>>>> +
>>>> + if (!folio_test_large(folio))
>>>> + return atomic_read(&folio->_mapcount) + 1;
>>>> +
>>>> + mapcount = folio_large_mapcount(folio);
>>>> + entire_mapcount = folio_entire_mapcount(folio);
>>>> + if (mapcount <= entire_mapcount)
>>>> + return entire_mapcount;
>>>> + mapcount -= entire_mapcount;
>>>> +
>>>> + adjust = folio_large_nr_pages(folio) / 2;
>>
>> Thanks for the review!
>>
>>>
>>> Is there any reason for choosing this adjust number? A comment might be
>>> helpful in case people want to change it later, either with some reasoning
>>> or just saying it is chosen empirically.
>>
>> We're dividing by folio_large_nr_pages(folio) (shifting by folio_large_order(folio)), so this is not a magic number at all.
>>
>> So this should be "ordinary" rounding.
>
> I thought the rounding would be (mapcount + 511) / 512.
Yes, that's "rounding up".
> But
> that means if one subpage is mapped, the average will be 1.
> Your rounding means if at least half of the subpages is mapped,
> the average will be 1. Others might think 1/3 is mapped,
> the average will be 1. That is why I think adjust looks like
> a magic number.
I think all callers could tolerate (or benefit) from
folio_average_page_mapcount() returning at least 1 in case any page is
mapped.
There was a reason why I decided to round to the nearest integer instead.
Let me think about this once more, I went back and forth a couple of
times on this.
--
Cheers,
David / dhildenb
^ 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