* Re: [PATCH v3 08/12] man/man2/move_mount.2: document "new" mount API
From: Askar Safin @ 2025-08-12 10:00 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alejandro Colomar, Michael T. Kerrisk, Alexander Viro, Jan Kara,
G. Branden Robinson, linux-man, linux-api, linux-fsdevel,
linux-kernel, David Howells, Christian Brauner
In-Reply-To: <20250809-new-mount-api-v3-8-f61405c80f34@cyphar.com>
move_mount for v2 contained this:
> Mounts cannot be moved beneath the rootfs
In v3 you changed this to this:
> Mount objects cannot be attached beneath the filesystem root
You made this phrase worse.
"Filesystem root" can be understood as "root of superblock".
So, please, change this to "root directory" or something.
> This would create a new bind-mount of /home/cyphar as attached mount object, and then attach
You meant "as detached mount object"
Also: I see that you addressed all my v2 comments. Thank you!
--
Askar Safin
https://types.pl/@safinaskar
^ permalink raw reply
* Re: [PATCH v3 07/12] man/man2/fsmount.2: document "new" mount API
From: Aleksa Sarai @ 2025-08-12 14:33 UTC (permalink / raw)
To: Askar Safin
Cc: Alejandro Colomar, Michael T. Kerrisk, Alexander Viro, Jan Kara,
G. Branden Robinson, linux-man, linux-api, linux-fsdevel,
linux-kernel, David Howells, Christian Brauner
In-Reply-To: <1989d90de76.d3b8b3cc73065.2447955224950374755@zohomail.com>
[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]
On 2025-08-12, Askar Safin <safinaskar@zohomail.com> wrote:
> fsmount:
> > Unlike open_tree(2) with OPEN_TREE_CLONE, fsmount() can only be called once in the lifetime of a filesystem instance to produce a mount object.
>
> I don't understand what you meant here. This phrase in its current form is wrong.
> Consider this scenario: we did this:
> fsopen(...)
> fsconfig(..., FSCONFIG_SET_STRING, "source", ...)
> fsconfig(..., FSCONFIG_CMD_CREATE, ...)
> fsmount(...)
> fsopen(...)
> fsconfig(..., FSCONFIG_SET_STRING, "source", ...)
> fsconfig(..., FSCONFIG_CMD_CREATE, ...)
> fsmount(...)
>
> We used FSCONFIG_CMD_CREATE here as opposed to FSCONFIG_CMD_CREATE_EXCL, thus
> it is possible that second fsmount will return mount for the same superblock.
> Thus that statement "fsmount() can only be called once in the lifetime of a filesystem instance to produce a mount object"
> is not true.
Yeah, the superblock reuse behaviour makes this description less
coherent than what I was going for. My thinking was that a reused
superblock is (to userspace) conceptually a new filesystem instance
because they create it the same way as any other filesystem instance.
(In fact, the rest of the VFS treats them the same way too -- only
sget_fc() knows about superblock reuse.)
But yeah, "filesystem context" is more accurate here, so probably just:
Unlike open_tree(2) with OPEN_TREE_CLONE, fsmount() can only be called
once in the lifetime of a filesystem context.
Though maybe we should mention that it's fsopen(2)-only (even though
it's mentioned earlier in the DESCRIPTION)? If you read the sentence in
isolation you might get the wrong impression. Do you have any
alternative suggestions?
FWIW, superblock reuse is one of those things that is a fairly hairy
implementation detail of the VFS, and as such it has quite odd
semantics. I probably wouldn't have documented it as heavily if it
wasn't for the addition of FSCONFIG_CMD_CREATE_EXCL (maybe an entry in
BUGS or CAVEATS at most -- this behaviour has an even worse impact on
mount(2) but it's completely undocumented there).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* Re: [PATCH v3 08/12] man/man2/move_mount.2: document "new" mount API
From: Aleksa Sarai @ 2025-08-12 14:36 UTC (permalink / raw)
To: Askar Safin
Cc: Alejandro Colomar, Michael T. Kerrisk, Alexander Viro, Jan Kara,
G. Branden Robinson, linux-man, linux-api, linux-fsdevel,
linux-kernel, David Howells, Christian Brauner
In-Reply-To: <1989db97e30.b71849c573511.8013418925525314426@zohomail.com>
[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]
On 2025-08-12, Askar Safin <safinaskar@zohomail.com> wrote:
> move_mount for v2 contained this:
> > Mounts cannot be moved beneath the rootfs
>
> In v3 you changed this to this:
> > Mount objects cannot be attached beneath the filesystem root
>
> You made this phrase worse.
>
> "Filesystem root" can be understood as "root of superblock".
> So, please, change this to "root directory" or something.
Maybe I should borrow the "root mount" terminology from pivot_root(2)?
(Though they use "root mount in the mount namespace of the calling
process", which is a little wordy.) I didn't like using "rootfs" as
shorthand in a man-page.
> > This would create a new bind-mount of /home/cyphar as attached mount object, and then attach
> You meant "as detached mount object"
Thanks, I have already fixed this in my branch (and the two other
misuses of "attach" in fsopen(2)). FWIW, open_tree(2) was the first
man-page in this series that I wrote, so I hadn't settled on the wording
the first draft.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* Re: [PATCH v3 07/12] man/man2/fsmount.2: document "new" mount API
From: Aleksa Sarai @ 2025-08-12 16:18 UTC (permalink / raw)
To: Askar Safin
Cc: Alejandro Colomar, Michael T. Kerrisk, Alexander Viro, Jan Kara,
G. Branden Robinson, linux-man, linux-api, linux-fsdevel,
linux-kernel, David Howells, Christian Brauner
In-Reply-To: <2025-08-12.1755007445-rural-feudal-spacebar-forehead-28QkCN@cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 481 bytes --]
On 2025-08-13, Aleksa Sarai <cyphar@cyphar.com> wrote:
> But yeah, "filesystem context" is more accurate here, so probably just:
Oops, I meant to include
> Unlike open_tree(2) with OPEN_TREE_CLONE, fsmount() can only be called
> once in the lifetime of a filesystem context.
to create a mount object.
at the end.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* Re: [PATCH v3 06/12] man/man2/fsconfig.2: document "new" mount API
From: Aleksa Sarai @ 2025-08-12 18:25 UTC (permalink / raw)
To: Alejandro Colomar
Cc: Michael T. Kerrisk, Alexander Viro, Jan Kara, Askar Safin,
G. Branden Robinson, linux-man, linux-api, linux-fsdevel,
linux-kernel, David Howells, Christian Brauner
In-Reply-To: <20250809-new-mount-api-v3-6-f61405c80f34@cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
On 2025-08-09, Aleksa Sarai <cyphar@cyphar.com> wrote:
> +Note that the Linux kernel reuses filesystem instances
> +for many filesystems,
> +so (depending on the filesystem being configured and parameters used)
> +it is possible for the filesystem instance "created" by
> +.B \%FSCONFIG_CMD_CREATE
> +to, in fact, be a reference
> +to an existing filesystem instance in the kernel.
> +The kernel will attempt to merge the specified parameters
> +of this filesystem configuration context
> +with those of the filesystem instance being reused,
> +but some parameters may be
> +.IR "silently ignored" .
While looking at this again, I realised this explanation is almost
certainly incorrect in a few places (and was based on a misunderstanding
of how sget_fc() works and how it interacts with vfs_get_tree()).
I'll rewrite this in the next version.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]
^ permalink raw reply
* [PATCH util-linux v2] fallocate: add FALLOC_FL_WRITE_ZEROES support
From: Zhang Yi @ 2025-08-13 2:40 UTC (permalink / raw)
To: linux-fsdevel, linux-block, dm-devel, linux-nvme, linux-scsi
Cc: linux-kernel, linux-api, hch, tytso, djwong, bmarzins, chaitanyak,
shinichiro.kawasaki, brauner, martin.petersen, yi.zhang, yi.zhang,
chengzhihao1, yukuai3, yangerkun
From: Zhang Yi <yi.zhang@huawei.com>
The Linux kernel (since version 6.17) supports FALLOC_FL_WRITE_ZEROES in
fallocate(2). Add support for FALLOC_FL_WRITE_ZEROES to the fallocate
utility by introducing a new option -w|--write-zeroes.
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=278c7d9b5e0c
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
v1->v2:
- Minor description modification to align with the kernel.
sys-utils/fallocate.1.adoc | 11 +++++++++--
sys-utils/fallocate.c | 20 ++++++++++++++++----
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/sys-utils/fallocate.1.adoc b/sys-utils/fallocate.1.adoc
index 44ee0ef4c..0ec9ff9a9 100644
--- a/sys-utils/fallocate.1.adoc
+++ b/sys-utils/fallocate.1.adoc
@@ -12,7 +12,7 @@ fallocate - preallocate or deallocate space to a file
== SYNOPSIS
-*fallocate* [*-c*|*-p*|*-z*] [*-o* _offset_] *-l* _length_ [*-n*] _filename_
+*fallocate* [*-c*|*-p*|*-z*|*-w*] [*-o* _offset_] *-l* _length_ [*-n*] _filename_
*fallocate* *-d* [*-o* _offset_] [*-l* _length_] _filename_
@@ -28,7 +28,7 @@ The exit status returned by *fallocate* is 0 on success and 1 on failure.
The _length_ and _offset_ arguments may be followed by the multiplicative suffixes KiB (=1024), MiB (=1024*1024), and so on for GiB, TiB, PiB, EiB, ZiB, and YiB (the "iB" is optional, e.g., "K" has the same meaning as "KiB") or the suffixes KB (=1000), MB (=1000*1000), and so on for GB, TB, PB, EB, ZB, and YB.
-The options *--collapse-range*, *--dig-holes*, *--punch-hole*, *--zero-range* and *--posix* are mutually exclusive.
+The options *--collapse-range*, *--dig-holes*, *--punch-hole*, *--zero-range*, *--write-zeroes* and *--posix* are mutually exclusive.
*-c*, *--collapse-range*::
Removes a byte range from a file, without leaving a hole. The byte range to be collapsed starts at _offset_ and continues for _length_ bytes. At the completion of the operation, the contents of the file starting at the location __offset__+_length_ will be appended at the location _offset_, and the file will be _length_ bytes smaller. The option *--keep-size* may not be specified for the collapse-range operation.
@@ -76,6 +76,13 @@ Option *--keep-size* can be specified to prevent file length modification.
+
Available since Linux 3.14 for ext4 (only for extent-based files) and XFS.
+*-w*, *--write-zeroes*::
+Zeroes space in the byte range starting at _offset_ and continuing for _length_ bytes. Within the specified range, blocks are preallocated for the regions that span the holes in the file. After a successful call, subsequent reads from this range will return zeroes, subsequent writes to that range do not require further changes to the file mapping metadata.
++
+Zeroing is done within the filesystem by preferably submitting write zeores commands, the alternative way is submitting actual zeroed data, the specified range will be converted into written extents. The write zeroes command is typically faster than write actual data if the device supports unmap write zeroes, the specified range will not be physically zeroed out on the device.
++
+Options *--keep-size* can not be specified for the write-zeroes operation.
+
include::man-common/help-version.adoc[]
== AUTHORS
diff --git a/sys-utils/fallocate.c b/sys-utils/fallocate.c
index 13bf52915..8d37fdad7 100644
--- a/sys-utils/fallocate.c
+++ b/sys-utils/fallocate.c
@@ -40,7 +40,7 @@
#if defined(HAVE_LINUX_FALLOC_H) && \
(!defined(FALLOC_FL_KEEP_SIZE) || !defined(FALLOC_FL_PUNCH_HOLE) || \
!defined(FALLOC_FL_COLLAPSE_RANGE) || !defined(FALLOC_FL_ZERO_RANGE) || \
- !defined(FALLOC_FL_INSERT_RANGE))
+ !defined(FALLOC_FL_INSERT_RANGE) || !defined(FALLOC_FL_WRITE_ZEROES))
# include <linux/falloc.h> /* non-libc fallback for FALLOC_FL_* flags */
#endif
@@ -65,6 +65,10 @@
# define FALLOC_FL_INSERT_RANGE 0x20
#endif
+#ifndef FALLOC_FL_WRITE_ZEROES
+# define FALLOC_FL_WRITE_ZEROES 0x80
+#endif
+
#include "nls.h"
#include "strutils.h"
#include "c.h"
@@ -94,6 +98,7 @@ static void __attribute__((__noreturn__)) usage(void)
fputs(_(" -o, --offset <num> offset for range operations, in bytes\n"), out);
fputs(_(" -p, --punch-hole replace a range with a hole (implies -n)\n"), out);
fputs(_(" -z, --zero-range zero and ensure allocation of a range\n"), out);
+ fputs(_(" -w, --write-zeroes write zeroes and ensure allocation of a range\n"), out);
#ifdef HAVE_POSIX_FALLOCATE
fputs(_(" -x, --posix use posix_fallocate(3) instead of fallocate(2)\n"), out);
#endif
@@ -304,6 +309,7 @@ int main(int argc, char **argv)
{ "dig-holes", no_argument, NULL, 'd' },
{ "insert-range", no_argument, NULL, 'i' },
{ "zero-range", no_argument, NULL, 'z' },
+ { "write-zeroes", no_argument, NULL, 'w' },
{ "offset", required_argument, NULL, 'o' },
{ "length", required_argument, NULL, 'l' },
{ "posix", no_argument, NULL, 'x' },
@@ -312,8 +318,8 @@ int main(int argc, char **argv)
};
static const ul_excl_t excl[] = { /* rows and cols in ASCII order */
- { 'c', 'd', 'i', 'p', 'x', 'z'},
- { 'c', 'i', 'n', 'x' },
+ { 'c', 'd', 'i', 'p', 'w', 'x', 'z'},
+ { 'c', 'i', 'n', 'w', 'x' },
{ 0 }
};
int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
@@ -323,7 +329,7 @@ int main(int argc, char **argv)
textdomain(PACKAGE);
close_stdout_atexit();
- while ((c = getopt_long(argc, argv, "hvVncpdizxl:o:", longopts, NULL))
+ while ((c = getopt_long(argc, argv, "hvVncpdizwxl:o:", longopts, NULL))
!= -1) {
err_exclusive_options(c, longopts, excl, excl_st);
@@ -353,6 +359,9 @@ int main(int argc, char **argv)
case 'z':
mode |= FALLOC_FL_ZERO_RANGE;
break;
+ case 'w':
+ mode |= FALLOC_FL_WRITE_ZEROES;
+ break;
case 'x':
#ifdef HAVE_POSIX_FALLOCATE
posix = 1;
@@ -429,6 +438,9 @@ int main(int argc, char **argv)
else if (mode & FALLOC_FL_ZERO_RANGE)
fprintf(stdout, _("%s: %s (%ju bytes) zeroed.\n"),
filename, str, length);
+ else if (mode & FALLOC_FL_WRITE_ZEROES)
+ fprintf(stdout, _("%s: %s (%ju bytes) write zeroed.\n"),
+ filename, str, length);
else
fprintf(stdout, _("%s: %s (%ju bytes) allocated.\n"),
filename, str, length);
--
2.39.2
^ permalink raw reply related
* [PATCH xfsprogs v2] xfs_io: add FALLOC_FL_WRITE_ZEROES support
From: Zhang Yi @ 2025-08-13 2:42 UTC (permalink / raw)
To: linux-fsdevel, linux-block, dm-devel, linux-nvme, linux-scsi,
linux-xfs
Cc: linux-kernel, linux-api, hch, tytso, djwong, bmarzins, chaitanyak,
shinichiro.kawasaki, brauner, martin.petersen, yi.zhang, yi.zhang,
chengzhihao1, yukuai3, yangerkun
From: Zhang Yi <yi.zhang@huawei.com>
The Linux kernel (since version 6.17) supports FALLOC_FL_WRITE_ZEROES in
fallocate(2). Add support for FALLOC_FL_WRITE_ZEROES support to the
fallocate utility by introducing a new 'fwzero' command in the xfs_io
tool.
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=278c7d9b5e0c
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
v1->v2:
- Minor description modification to align with the kernel.
io/prealloc.c | 36 ++++++++++++++++++++++++++++++++++++
man/man8/xfs_io.8 | 6 ++++++
2 files changed, 42 insertions(+)
diff --git a/io/prealloc.c b/io/prealloc.c
index 8e968c9f..9a64bf53 100644
--- a/io/prealloc.c
+++ b/io/prealloc.c
@@ -30,6 +30,10 @@
#define FALLOC_FL_UNSHARE_RANGE 0x40
#endif
+#ifndef FALLOC_FL_WRITE_ZEROES
+#define FALLOC_FL_WRITE_ZEROES 0x80
+#endif
+
static cmdinfo_t allocsp_cmd;
static cmdinfo_t freesp_cmd;
static cmdinfo_t resvsp_cmd;
@@ -41,6 +45,7 @@ static cmdinfo_t fcollapse_cmd;
static cmdinfo_t finsert_cmd;
static cmdinfo_t fzero_cmd;
static cmdinfo_t funshare_cmd;
+static cmdinfo_t fwzero_cmd;
static int
offset_length(
@@ -377,6 +382,27 @@ funshare_f(
return 0;
}
+static int
+fwzero_f(
+ int argc,
+ char **argv)
+{
+ xfs_flock64_t segment;
+ int mode = FALLOC_FL_WRITE_ZEROES;
+
+ if (!offset_length(argv[1], argv[2], &segment)) {
+ exitcode = 1;
+ return 0;
+ }
+
+ if (fallocate(file->fd, mode, segment.l_start, segment.l_len)) {
+ perror("fallocate");
+ exitcode = 1;
+ return 0;
+ }
+ return 0;
+}
+
void
prealloc_init(void)
{
@@ -489,4 +515,14 @@ prealloc_init(void)
funshare_cmd.oneline =
_("unshares shared blocks within the range");
add_command(&funshare_cmd);
+
+ fwzero_cmd.name = "fwzero";
+ fwzero_cmd.cfunc = fwzero_f;
+ fwzero_cmd.argmin = 2;
+ fwzero_cmd.argmax = 2;
+ fwzero_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+ fwzero_cmd.args = _("off len");
+ fwzero_cmd.oneline =
+ _("zeroes space and eliminates holes by allocating and submitting write zeroes");
+ add_command(&fwzero_cmd);
}
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index b0dcfdb7..0a673322 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -550,6 +550,12 @@ With the
.B -k
option, use the FALLOC_FL_KEEP_SIZE flag as well.
.TP
+.BI fwzero " offset length"
+Call fallocate with FALLOC_FL_WRITE_ZEROES flag as described in the
+.BR fallocate (2)
+manual page to allocate and zero blocks within the range by submitting write
+zeroes.
+.TP
.BI zero " offset length"
Call xfsctl with
.B XFS_IOC_ZERO_RANGE
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Vipin Sharma @ 2025-08-13 6:34 UTC (permalink / raw)
To: Pasha Tatashin
Cc: pratyush, jasonmiu, graf, changyuanl, rppt, dmatlack, rientjes,
corbet, rdunlap, ilpo.jarvinen, kanie, ojeda, aliceryhl,
masahiroy, akpm, tj, yoann.congal, mmaurer, roman.gushchin,
chenridong, axboe, mark.rutland, jannh, vincent.guittot, hannes,
dan.j.williams, david, joel.granados, rostedt, anna.schumaker,
song, zhangguopeng, linux, linux-kernel, linux-doc, linux-mm,
gregkh, tglx, mingo, bp, dave.hansen, x86, hpa, rafael, dakr,
bartosz.golaszewski, cw00.choi, myungjoo.ham, yesanishhere,
Jonathan.Cameron, quic_zijuhu, aleksander.lobakin, ira.weiny,
andriy.shevchenko, leon, lukas, bhelgaas, wagi, djeffery,
stuart.w.hayes, ptyadav, lennart, brauner, linux-api,
linux-fsdevel, saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <20250807014442.3829950-30-pasha.tatashin@soleen.com>
On 2025-08-07 01:44:35, Pasha Tatashin wrote:
> From: Pratyush Yadav <ptyadav@amazon.de>
> +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
> + unsigned int nr_folios)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < nr_folios; i++) {
> + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
> + struct folio *folio;
> +
> + if (!pfolio->foliodesc)
> + continue;
> +
> + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
> +
> + kho_unpreserve_folio(folio);
This one is missing WARN_ON_ONCE() similar to the one in
memfd_luo_preserve_folios().
> + unpin_folio(folio);
> + }
> +}
> +
> +static void *memfd_luo_create_fdt(unsigned long size)
> +{
> + unsigned int order = get_order(size);
> + struct folio *fdt_folio;
> + int err = 0;
> + void *fdt;
> +
> + if (order > MAX_PAGE_ORDER)
> + return NULL;
> +
> + fdt_folio = folio_alloc(GFP_KERNEL, order);
__GFP_ZERO should also be used here. Otherwise this can lead to
unintentional passing of old kernel memory.
> +static int memfd_luo_prepare(struct liveupdate_file_handler *handler,
> + struct file *file, u64 *data)
> +{
> + struct memfd_luo_preserved_folio *preserved_folios;
> + struct inode *inode = file_inode(file);
> + unsigned int max_folios, nr_folios = 0;
> + int err = 0, preserved_size;
> + struct folio **folios;
> + long size, nr_pinned;
> + pgoff_t offset;
> + void *fdt;
> + u64 pos;
> +
> + if (WARN_ON_ONCE(!shmem_file(file)))
> + return -EINVAL;
This one is only check for shmem_file, whereas in
memfd_luo_can_preserve() there is check for inode->i_nlink also. Is that
not needed here?
> +
> + inode_lock(inode);
> + shmem_i_mapping_freeze(inode, true);
> +
> + size = i_size_read(inode);
> + if ((PAGE_ALIGN(size) / PAGE_SIZE) > UINT_MAX) {
> + err = -E2BIG;
> + goto err_unlock;
> + }
> +
> + /*
> + * Guess the number of folios based on inode size. Real number might end
> + * up being smaller if there are higher order folios.
> + */
> + max_folios = PAGE_ALIGN(size) / PAGE_SIZE;
> + folios = kvmalloc_array(max_folios, sizeof(*folios), GFP_KERNEL);
__GFP_ZERO?
> +static int memfd_luo_freeze(struct liveupdate_file_handler *handler,
> + struct file *file, u64 *data)
> +{
> + u64 pos = file->f_pos;
> + void *fdt;
> + int err;
> +
> + if (WARN_ON_ONCE(!*data))
> + return -EINVAL;
> +
> + fdt = phys_to_virt(*data);
> +
> + /*
> + * The pos or size might have changed since prepare. Everything else
> + * stays the same.
> + */
> + err = fdt_setprop(fdt, 0, "pos", &pos, sizeof(pos));
> + if (err)
> + return err;
Comment is talking about pos and size but code is only updating pos.
> +static int memfd_luo_retrieve(struct liveupdate_file_handler *handler, u64 data,
> + struct file **file_p)
> +{
> + const struct memfd_luo_preserved_folio *pfolios;
> + int nr_pfolios, len, ret = 0, i = 0;
> + struct address_space *mapping;
> + struct folio *folio, *fdt_folio;
> + const u64 *pos, *size;
> + struct inode *inode;
> + struct file *file;
> + const void *fdt;
> +
> + fdt_folio = memfd_luo_get_fdt(data);
> + if (!fdt_folio)
> + return -ENOENT;
> +
> + fdt = page_to_virt(folio_page(fdt_folio, 0));
> +
> + pfolios = fdt_getprop(fdt, 0, "folios", &len);
> + if (!pfolios || len % sizeof(*pfolios)) {
> + pr_err("invalid 'folios' property\n");
Print should clearly state that error is because fields is not found or
len is not multiple of sizeof(*pfolios).
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Greg KH @ 2025-08-13 7:09 UTC (permalink / raw)
To: Vipin Sharma
Cc: Pasha Tatashin, pratyush, jasonmiu, graf, changyuanl, rppt,
dmatlack, rientjes, corbet, rdunlap, ilpo.jarvinen, kanie, ojeda,
aliceryhl, masahiroy, akpm, tj, yoann.congal, mmaurer,
roman.gushchin, chenridong, axboe, mark.rutland, jannh,
vincent.guittot, hannes, dan.j.williams, david, joel.granados,
rostedt, anna.schumaker, song, zhangguopeng, linux, linux-kernel,
linux-doc, linux-mm, tglx, mingo, bp, dave.hansen, x86, hpa,
rafael, dakr, bartosz.golaszewski, cw00.choi, myungjoo.ham,
yesanishhere, Jonathan.Cameron, quic_zijuhu, aleksander.lobakin,
ira.weiny, andriy.shevchenko, leon, lukas, bhelgaas, wagi,
djeffery, stuart.w.hayes, ptyadav, lennart, brauner, linux-api,
linux-fsdevel, saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <20250813063407.GA3182745.vipinsh@google.com>
On Tue, Aug 12, 2025 at 11:34:37PM -0700, Vipin Sharma wrote:
> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
> > From: Pratyush Yadav <ptyadav@amazon.de>
> > +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
> > + unsigned int nr_folios)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < nr_folios; i++) {
> > + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
> > + struct folio *folio;
> > +
> > + if (!pfolio->foliodesc)
> > + continue;
> > +
> > + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
> > +
> > + kho_unpreserve_folio(folio);
>
> This one is missing WARN_ON_ONCE() similar to the one in
> memfd_luo_preserve_folios().
So you really want to cause a machine to reboot and get a CVE issued for
this, if it could be triggered? That's bold :)
Please don't. If that can happen, handle the issue and move on, don't
crash boxes.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pratyush Yadav @ 2025-08-13 12:02 UTC (permalink / raw)
To: Greg KH
Cc: Vipin Sharma, Pasha Tatashin, pratyush, jasonmiu, graf,
changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, jgg,
parav, leonro, witu
In-Reply-To: <2025081310-custodian-ashamed-3104@gregkh>
On Wed, Aug 13 2025, Greg KH wrote:
> On Tue, Aug 12, 2025 at 11:34:37PM -0700, Vipin Sharma wrote:
>> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
>> > From: Pratyush Yadav <ptyadav@amazon.de>
>> > +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
>> > + unsigned int nr_folios)
>> > +{
>> > + unsigned int i;
>> > +
>> > + for (i = 0; i < nr_folios; i++) {
>> > + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
>> > + struct folio *folio;
>> > +
>> > + if (!pfolio->foliodesc)
>> > + continue;
>> > +
>> > + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
>> > +
>> > + kho_unpreserve_folio(folio);
>>
>> This one is missing WARN_ON_ONCE() similar to the one in
>> memfd_luo_preserve_folios().
>
> So you really want to cause a machine to reboot and get a CVE issued for
> this, if it could be triggered? That's bold :)
>
> Please don't. If that can happen, handle the issue and move on, don't
> crash boxes.
Why would a WARN() crash the machine? That is what BUG() does, not
WARN().
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Greg KH @ 2025-08-13 12:14 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Vipin Sharma, Pasha Tatashin, jasonmiu, graf, changyuanl, rppt,
dmatlack, rientjes, corbet, rdunlap, ilpo.jarvinen, kanie, ojeda,
aliceryhl, masahiroy, akpm, tj, yoann.congal, mmaurer,
roman.gushchin, chenridong, axboe, mark.rutland, jannh,
vincent.guittot, hannes, dan.j.williams, david, joel.granados,
rostedt, anna.schumaker, song, zhangguopeng, linux, linux-kernel,
linux-doc, linux-mm, tglx, mingo, bp, dave.hansen, x86, hpa,
rafael, dakr, bartosz.golaszewski, cw00.choi, myungjoo.ham,
yesanishhere, Jonathan.Cameron, quic_zijuhu, aleksander.lobakin,
ira.weiny, andriy.shevchenko, leon, lukas, bhelgaas, wagi,
djeffery, stuart.w.hayes, lennart, brauner, linux-api,
linux-fsdevel, saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <mafs01ppfxwe8.fsf@kernel.org>
On Wed, Aug 13, 2025 at 02:02:07PM +0200, Pratyush Yadav wrote:
> On Wed, Aug 13 2025, Greg KH wrote:
>
> > On Tue, Aug 12, 2025 at 11:34:37PM -0700, Vipin Sharma wrote:
> >> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
> >> > From: Pratyush Yadav <ptyadav@amazon.de>
> >> > +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
> >> > + unsigned int nr_folios)
> >> > +{
> >> > + unsigned int i;
> >> > +
> >> > + for (i = 0; i < nr_folios; i++) {
> >> > + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
> >> > + struct folio *folio;
> >> > +
> >> > + if (!pfolio->foliodesc)
> >> > + continue;
> >> > +
> >> > + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
> >> > +
> >> > + kho_unpreserve_folio(folio);
> >>
> >> This one is missing WARN_ON_ONCE() similar to the one in
> >> memfd_luo_preserve_folios().
> >
> > So you really want to cause a machine to reboot and get a CVE issued for
> > this, if it could be triggered? That's bold :)
> >
> > Please don't. If that can happen, handle the issue and move on, don't
> > crash boxes.
>
> Why would a WARN() crash the machine? That is what BUG() does, not
> WARN().
See 'panic_on_warn' which is enabled in a few billion Linux systems
these days :(
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pratyush Yadav @ 2025-08-13 12:29 UTC (permalink / raw)
To: Vipin Sharma
Cc: Pasha Tatashin, pratyush, jasonmiu, graf, changyuanl, rppt,
dmatlack, rientjes, corbet, rdunlap, ilpo.jarvinen, kanie, ojeda,
aliceryhl, masahiroy, akpm, tj, yoann.congal, mmaurer,
roman.gushchin, chenridong, axboe, mark.rutland, jannh,
vincent.guittot, hannes, dan.j.williams, david, joel.granados,
rostedt, anna.schumaker, song, zhangguopeng, linux, linux-kernel,
linux-doc, linux-mm, gregkh, tglx, mingo, bp, dave.hansen, x86,
hpa, rafael, dakr, bartosz.golaszewski, cw00.choi, myungjoo.ham,
yesanishhere, Jonathan.Cameron, quic_zijuhu, aleksander.lobakin,
ira.weiny, andriy.shevchenko, leon, lukas, bhelgaas, wagi,
djeffery, stuart.w.hayes, lennart, brauner, linux-api,
linux-fsdevel, saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <20250813063407.GA3182745.vipinsh@google.com>
Hi Vipin,
Thanks for the review.
On Tue, Aug 12 2025, Vipin Sharma wrote:
> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
>> From: Pratyush Yadav <ptyadav@amazon.de>
>> +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
>> + unsigned int nr_folios)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < nr_folios; i++) {
>> + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
>> + struct folio *folio;
>> +
>> + if (!pfolio->foliodesc)
>> + continue;
>> +
>> + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
>> +
>> + kho_unpreserve_folio(folio);
>
> This one is missing WARN_ON_ONCE() similar to the one in
> memfd_luo_preserve_folios().
Right, will add.
>
>> + unpin_folio(folio);
Looking at this code caught my eye. This can also be called from LUO's
finish callback if no one claimed the memfd after live update. In that
case, unpin_folio() is going to underflow the pincount or refcount on
the folio since after the kexec, the folio is no longer pinned. We
should only be doing folio_put().
I think this function should take a argument to specify which of these
cases it is dealing with.
>> + }
>> +}
>> +
>> +static void *memfd_luo_create_fdt(unsigned long size)
>> +{
>> + unsigned int order = get_order(size);
>> + struct folio *fdt_folio;
>> + int err = 0;
>> + void *fdt;
>> +
>> + if (order > MAX_PAGE_ORDER)
>> + return NULL;
>> +
>> + fdt_folio = folio_alloc(GFP_KERNEL, order);
>
> __GFP_ZERO should also be used here. Otherwise this can lead to
> unintentional passing of old kernel memory.
fdt_create() zeroes out the buffer so this should not be a problem.
>
>> +static int memfd_luo_prepare(struct liveupdate_file_handler *handler,
>> + struct file *file, u64 *data)
>> +{
>> + struct memfd_luo_preserved_folio *preserved_folios;
>> + struct inode *inode = file_inode(file);
>> + unsigned int max_folios, nr_folios = 0;
>> + int err = 0, preserved_size;
>> + struct folio **folios;
>> + long size, nr_pinned;
>> + pgoff_t offset;
>> + void *fdt;
>> + u64 pos;
>> +
>> + if (WARN_ON_ONCE(!shmem_file(file)))
>> + return -EINVAL;
>
> This one is only check for shmem_file, whereas in
> memfd_luo_can_preserve() there is check for inode->i_nlink also. Is that
> not needed here?
Actually, this should never happen since the LUO can_preserve() callback
should make sure of this. I think it would be perfectly fine to just
drop this check. I only added it because I was being extra careful.
>
>> +
>> + inode_lock(inode);
>> + shmem_i_mapping_freeze(inode, true);
>> +
>> + size = i_size_read(inode);
>> + if ((PAGE_ALIGN(size) / PAGE_SIZE) > UINT_MAX) {
>> + err = -E2BIG;
>> + goto err_unlock;
>> + }
>> +
>> + /*
>> + * Guess the number of folios based on inode size. Real number might end
>> + * up being smaller if there are higher order folios.
>> + */
>> + max_folios = PAGE_ALIGN(size) / PAGE_SIZE;
>> + folios = kvmalloc_array(max_folios, sizeof(*folios), GFP_KERNEL);
>
> __GFP_ZERO?
Why? This is only used in this function and gets freed on return. And
the function only looks at the elements that get initialized by
memfd_pin_folios().
>
>> +static int memfd_luo_freeze(struct liveupdate_file_handler *handler,
>> + struct file *file, u64 *data)
>> +{
>> + u64 pos = file->f_pos;
>> + void *fdt;
>> + int err;
>> +
>> + if (WARN_ON_ONCE(!*data))
>> + return -EINVAL;
>> +
>> + fdt = phys_to_virt(*data);
>> +
>> + /*
>> + * The pos or size might have changed since prepare. Everything else
>> + * stays the same.
>> + */
>> + err = fdt_setprop(fdt, 0, "pos", &pos, sizeof(pos));
>> + if (err)
>> + return err;
>
> Comment is talking about pos and size but code is only updating pos.
Right. Comment is out of date. size can no longer change since prepare.
So will update the comment.
>
>> +static int memfd_luo_retrieve(struct liveupdate_file_handler *handler, u64 data,
>> + struct file **file_p)
>> +{
>> + const struct memfd_luo_preserved_folio *pfolios;
>> + int nr_pfolios, len, ret = 0, i = 0;
>> + struct address_space *mapping;
>> + struct folio *folio, *fdt_folio;
>> + const u64 *pos, *size;
>> + struct inode *inode;
>> + struct file *file;
>> + const void *fdt;
>> +
>> + fdt_folio = memfd_luo_get_fdt(data);
>> + if (!fdt_folio)
>> + return -ENOENT;
>> +
>> + fdt = page_to_virt(folio_page(fdt_folio, 0));
>> +
>> + pfolios = fdt_getprop(fdt, 0, "folios", &len);
>> + if (!pfolios || len % sizeof(*pfolios)) {
>> + pr_err("invalid 'folios' property\n");
>
> Print should clearly state that error is because fields is not found or
> len is not multiple of sizeof(*pfolios).
Eh, there is already too much boilerplate one has to write (and read)
for parsing the FDT. Is there really a need for an extra 3-4 lines of
code for _each_ property that is parsed?
Long term, I think we shouldn't be doing this manually anyway. I think
the maintainable path forward is to define a schema for the serialized
data and have a parser that takes in the schema and gives out a parsed
struct, doing all sorts of checks in the process.
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Jason Gunthorpe @ 2025-08-13 12:41 UTC (permalink / raw)
To: Greg KH
Cc: Pratyush Yadav, Vipin Sharma, Pasha Tatashin, jasonmiu, graf,
changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <2025081351-tinsel-sprinkler-af77@gregkh>
On Wed, Aug 13, 2025 at 02:14:23PM +0200, Greg KH wrote:
> On Wed, Aug 13, 2025 at 02:02:07PM +0200, Pratyush Yadav wrote:
> > On Wed, Aug 13 2025, Greg KH wrote:
> >
> > > On Tue, Aug 12, 2025 at 11:34:37PM -0700, Vipin Sharma wrote:
> > >> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
> > >> > From: Pratyush Yadav <ptyadav@amazon.de>
> > >> > +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
> > >> > + unsigned int nr_folios)
> > >> > +{
> > >> > + unsigned int i;
> > >> > +
> > >> > + for (i = 0; i < nr_folios; i++) {
> > >> > + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
> > >> > + struct folio *folio;
> > >> > +
> > >> > + if (!pfolio->foliodesc)
> > >> > + continue;
> > >> > +
> > >> > + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
> > >> > +
> > >> > + kho_unpreserve_folio(folio);
> > >>
> > >> This one is missing WARN_ON_ONCE() similar to the one in
> > >> memfd_luo_preserve_folios().
> > >
> > > So you really want to cause a machine to reboot and get a CVE issued for
> > > this, if it could be triggered? That's bold :)
> > >
> > > Please don't. If that can happen, handle the issue and move on, don't
> > > crash boxes.
> >
> > Why would a WARN() crash the machine? That is what BUG() does, not
> > WARN().
>
> See 'panic_on_warn' which is enabled in a few billion Linux systems
> these days :(
This has been discussed so many times already:
https://lwn.net/Articles/969923/
When someone tried to formalize this "don't use WARN_ON" position
in the coding-style.rst it was NAK'd:
https://lwn.net/ml/linux-kernel/10af93f8-83f2-48ce-9bc3-80fe4c60082c@redhat.com/
Based on Linus's opposition to the idea:
https://lore.kernel.org/all/CAHk-=wgF7K2gSSpy=m_=K3Nov4zaceUX9puQf1TjkTJLA2XC_g@mail.gmail.com/
Use the warn ons. Make sure they can't be triggered by userspace. Use
them to detect corruption/malfunction in the kernel.
In this case if kho_unpreserve_folio() fails in this call chain it
means some error unwind is wrongly happening out of sequence, and we
are now forced to leak memory. Unwind is not something that userspace
should be controlling, so of course we want a WARN_ON here.
Jason
^ permalink raw reply
* Re: [PATCH v3 26/30] mm: shmem: use SHMEM_F_* flags instead of VM_* flags
From: Pratyush Yadav @ 2025-08-13 12:42 UTC (permalink / raw)
To: Vipin Sharma
Cc: Pasha Tatashin, pratyush, jasonmiu, graf, changyuanl, rppt,
dmatlack, rientjes, corbet, rdunlap, ilpo.jarvinen, kanie, ojeda,
aliceryhl, masahiroy, akpm, tj, yoann.congal, mmaurer,
roman.gushchin, chenridong, axboe, mark.rutland, jannh,
vincent.guittot, hannes, dan.j.williams, david, joel.granados,
rostedt, anna.schumaker, song, zhangguopeng, linux, linux-kernel,
linux-doc, linux-mm, gregkh, tglx, mingo, bp, dave.hansen, x86,
hpa, rafael, dakr, bartosz.golaszewski, cw00.choi, myungjoo.ham,
yesanishhere, Jonathan.Cameron, quic_zijuhu, aleksander.lobakin,
ira.weiny, andriy.shevchenko, leon, lukas, bhelgaas, wagi,
djeffery, stuart.w.hayes, lennart, brauner, linux-api,
linux-fsdevel, saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <20250811231107.GA2328988.vipinsh@google.com>
On Mon, Aug 11 2025, Vipin Sharma wrote:
> On 2025-08-07 01:44:32, Pasha Tatashin wrote:
>> From: Pratyush Yadav <ptyadav@amazon.de>
>> @@ -3123,7 +3123,9 @@ static struct inode *__shmem_get_inode(struct mnt_idmap *idmap,
>> spin_lock_init(&info->lock);
>> atomic_set(&info->stop_eviction, 0);
>> info->seals = F_SEAL_SEAL;
>> - info->flags = flags & VM_NORESERVE;
>> + info->flags = 0;
>
> This is not needed as the 'info' is being set to 0 just above
> spin_lock_init.
>
>> + if (flags & VM_NORESERVE)
>> + info->flags |= SHMEM_F_NORESERVE;
>
> As info->flags will be 0, this can be just direct assignment '='.
I think it is a bit more readable this way.
Anyway, I don't have a strong opinion, so if you insist, I'll change
this.
>
>> info->i_crtime = inode_get_mtime(inode);
>> info->fsflags = (dir == NULL) ? 0 :
>> SHMEM_I(dir)->fsflags & SHMEM_FL_INHERITED;
>> @@ -5862,8 +5864,10 @@ static inline struct inode *shmem_get_inode(struct mnt_idmap *idmap,
>> /* common code */
>>
>> static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name,
>> - loff_t size, unsigned long flags, unsigned int i_flags)
>> + loff_t size, unsigned long vm_flags,
>> + unsigned int i_flags)
>
> Nit: Might be just my editor, but this alignment seems off.
Looks fine for me:
https://gist.github.com/prati0100/a06229ca99cac5aae795fb962bb24ac5
Checkpatch also doesn't complain. Can you double-check? And if it still
looks off, can you describe what's wrong?
>
>> {
>> + unsigned long flags = (vm_flags & VM_NORESERVE) ? SHMEM_F_NORESERVE : 0;
>> struct inode *inode;
>> struct file *res;
>>
>> @@ -5880,7 +5884,7 @@ static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name,
>> return ERR_PTR(-ENOMEM);
>>
>> inode = shmem_get_inode(&nop_mnt_idmap, mnt->mnt_sb, NULL,
>> - S_IFREG | S_IRWXUGO, 0, flags);
>> + S_IFREG | S_IRWXUGO, 0, vm_flags);
>> if (IS_ERR(inode)) {
>> shmem_unacct_size(flags, size);
>> return ERR_CAST(inode);
>> --
>> 2.50.1.565.gc32cd1483b-goog
>>
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pratyush Yadav @ 2025-08-13 12:44 UTC (permalink / raw)
To: Pasha Tatashin
Cc: pratyush, jasonmiu, graf, changyuanl, rppt, dmatlack, rientjes,
corbet, rdunlap, ilpo.jarvinen, kanie, ojeda, aliceryhl,
masahiroy, akpm, tj, yoann.congal, mmaurer, roman.gushchin,
chenridong, axboe, mark.rutland, jannh, vincent.guittot, hannes,
dan.j.williams, david, joel.granados, rostedt, anna.schumaker,
song, zhangguopeng, linux, linux-kernel, linux-doc, linux-mm,
gregkh, tglx, mingo, bp, dave.hansen, x86, hpa, rafael, dakr,
bartosz.golaszewski, cw00.choi, myungjoo.ham, yesanishhere,
Jonathan.Cameron, quic_zijuhu, aleksander.lobakin, ira.weiny,
andriy.shevchenko, leon, lukas, bhelgaas, wagi, djeffery,
stuart.w.hayes, lennart, brauner, linux-api, linux-fsdevel,
saeedm, ajayachandra, jgg, parav, leonro, witu, jrhilke
In-Reply-To: <CA+CK2bAP-PWkYtZbw8ofhTgDaW3qoQkNob30wWSjidxEUTV4pg@mail.gmail.com>
On Fri, Aug 08 2025, Pasha Tatashin wrote:
>> +static int memfd_luo_preserve_folios(struct memfd_luo_preserved_folio *pfolios,
>> + struct folio **folios,
>> + unsigned int nr_folios)
>> +{
>> + unsigned int i;
>
> Should be 'long i'
>
> Otherwise in err_unpreserve we get into an infinite loop. Thank you
> Josh Hilke for noticing this.
Good catch! Will fix.
>
>> + int err;
>> +
>> + for (i = 0; i < nr_folios; i++) {
>> + struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
>> + struct folio *folio = folios[i];
>> + unsigned int flags = 0;
>> + unsigned long pfn;
>> +
>> + err = kho_preserve_folio(folio);
>> + if (err)
>> + goto err_unpreserve;
>> +
>> + pfn = folio_pfn(folio);
>> + if (folio_test_dirty(folio))
>> + flags |= PRESERVED_FLAG_DIRTY;
>> + if (folio_test_uptodate(folio))
>> + flags |= PRESERVED_FLAG_UPTODATE;
>> +
>> + pfolio->foliodesc = PRESERVED_FOLIO_MKDESC(pfn, flags);
>> + pfolio->index = folio->index;
>> + }
>> +
>> + return 0;
>> +
>> +err_unpreserve:
>> + i--;
>> + for (; i >= 0; i--)
>> + WARN_ON_ONCE(kho_unpreserve_folio(folios[i]));
>> + return err;
>> +}
>> +
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Greg KH @ 2025-08-13 13:00 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Pratyush Yadav, Vipin Sharma, Pasha Tatashin, jasonmiu, graf,
changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <20250813124140.GA699432@nvidia.com>
On Wed, Aug 13, 2025 at 09:41:40AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 13, 2025 at 02:14:23PM +0200, Greg KH wrote:
> > On Wed, Aug 13, 2025 at 02:02:07PM +0200, Pratyush Yadav wrote:
> > > On Wed, Aug 13 2025, Greg KH wrote:
> > >
> > > > On Tue, Aug 12, 2025 at 11:34:37PM -0700, Vipin Sharma wrote:
> > > >> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
> > > >> > From: Pratyush Yadav <ptyadav@amazon.de>
> > > >> > +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
> > > >> > + unsigned int nr_folios)
> > > >> > +{
> > > >> > + unsigned int i;
> > > >> > +
> > > >> > + for (i = 0; i < nr_folios; i++) {
> > > >> > + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
> > > >> > + struct folio *folio;
> > > >> > +
> > > >> > + if (!pfolio->foliodesc)
> > > >> > + continue;
> > > >> > +
> > > >> > + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
> > > >> > +
> > > >> > + kho_unpreserve_folio(folio);
> > > >>
> > > >> This one is missing WARN_ON_ONCE() similar to the one in
> > > >> memfd_luo_preserve_folios().
> > > >
> > > > So you really want to cause a machine to reboot and get a CVE issued for
> > > > this, if it could be triggered? That's bold :)
> > > >
> > > > Please don't. If that can happen, handle the issue and move on, don't
> > > > crash boxes.
> > >
> > > Why would a WARN() crash the machine? That is what BUG() does, not
> > > WARN().
> >
> > See 'panic_on_warn' which is enabled in a few billion Linux systems
> > these days :(
>
> This has been discussed so many times already:
>
> https://lwn.net/Articles/969923/
>
> When someone tried to formalize this "don't use WARN_ON" position
> in the coding-style.rst it was NAK'd:
>
> https://lwn.net/ml/linux-kernel/10af93f8-83f2-48ce-9bc3-80fe4c60082c@redhat.com/
>
> Based on Linus's opposition to the idea:
>
> https://lore.kernel.org/all/CAHk-=wgF7K2gSSpy=m_=K3Nov4zaceUX9puQf1TjkTJLA2XC_g@mail.gmail.com/
>
> Use the warn ons. Make sure they can't be triggered by userspace. Use
> them to detect corruption/malfunction in the kernel.
>
> In this case if kho_unpreserve_folio() fails in this call chain it
> means some error unwind is wrongly happening out of sequence, and we
> are now forced to leak memory. Unwind is not something that userspace
> should be controlling, so of course we want a WARN_ON here.
"should be" is the key here. And it's not obvious from this patch if
that's true or not, which is why I mentioned it.
I will keep bringing this up, given the HUGE number of CVEs I keep
assigning each week for when userspace hits WARN_ON() calls until that
flow starts to die out either because we don't keep adding new calls, OR
we finally fix them all. Both would be good...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pratyush Yadav @ 2025-08-13 13:31 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Greg KH, Pratyush Yadav, Vipin Sharma, Pasha Tatashin, jasonmiu,
graf, changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <20250813124140.GA699432@nvidia.com>
On Wed, Aug 13 2025, Jason Gunthorpe wrote:
> On Wed, Aug 13, 2025 at 02:14:23PM +0200, Greg KH wrote:
>> On Wed, Aug 13, 2025 at 02:02:07PM +0200, Pratyush Yadav wrote:
>> > On Wed, Aug 13 2025, Greg KH wrote:
>> >
>> > > On Tue, Aug 12, 2025 at 11:34:37PM -0700, Vipin Sharma wrote:
>> > >> On 2025-08-07 01:44:35, Pasha Tatashin wrote:
>> > >> > From: Pratyush Yadav <ptyadav@amazon.de>
>> > >> > +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
>> > >> > + unsigned int nr_folios)
>> > >> > +{
>> > >> > + unsigned int i;
>> > >> > +
>> > >> > + for (i = 0; i < nr_folios; i++) {
>> > >> > + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
>> > >> > + struct folio *folio;
>> > >> > +
>> > >> > + if (!pfolio->foliodesc)
>> > >> > + continue;
>> > >> > +
>> > >> > + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
>> > >> > +
>> > >> > + kho_unpreserve_folio(folio);
>> > >>
>> > >> This one is missing WARN_ON_ONCE() similar to the one in
>> > >> memfd_luo_preserve_folios().
>> > >
>> > > So you really want to cause a machine to reboot and get a CVE issued for
>> > > this, if it could be triggered? That's bold :)
>> > >
>> > > Please don't. If that can happen, handle the issue and move on, don't
>> > > crash boxes.
>> >
>> > Why would a WARN() crash the machine? That is what BUG() does, not
>> > WARN().
>>
>> See 'panic_on_warn' which is enabled in a few billion Linux systems
>> these days :(
>
> This has been discussed so many times already:
>
> https://lwn.net/Articles/969923/
>
> When someone tried to formalize this "don't use WARN_ON" position
> in the coding-style.rst it was NAK'd:
>
> https://lwn.net/ml/linux-kernel/10af93f8-83f2-48ce-9bc3-80fe4c60082c@redhat.com/
>
> Based on Linus's opposition to the idea:
>
> https://lore.kernel.org/all/CAHk-=wgF7K2gSSpy=m_=K3Nov4zaceUX9puQf1TjkTJLA2XC_g@mail.gmail.com/
>
> Use the warn ons. Make sure they can't be triggered by userspace. Use
> them to detect corruption/malfunction in the kernel.
>
> In this case if kho_unpreserve_folio() fails in this call chain it
> means some error unwind is wrongly happening out of sequence, and we
> are now forced to leak memory. Unwind is not something that userspace
> should be controlling, so of course we want a WARN_ON here.
Yep. And if we are saying WARN() should never be used then doesn't that
make panic_on_warn a no-op? What is even the point of that option then?
Here, we are unable to unpreserve a folio that we have preserved. This
isn't a normal error that we expect to happen. This should _not_ happen
unless something has gone horribly wrong.
For example, the calls to kho_preserve_folio() don't WARN(), since that
can fail for various reasons. They just return the error up the call
chain. As an analogy, allocating a page can fail, and it is quite
reasonable to expect the code to not throw out WARN()s for that. But if
for some reason you can't free a page that you allocated, this is very
unexpected and should WARN(). Of course, in Linux the page free APIs
don't even return a status, but I hope you get my point.
If I were a system administrator who sets panic_on_warn, I would _want_
the system to crash so no further damage happens and I can collect
logs/crash dumps to investigate later. Without the WARN(), I never get a
chance to debug and my system breaks silently. For all others, the
kernel goes on with some possibly corrupted/broken state.
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pratyush Yadav @ 2025-08-13 13:37 UTC (permalink / raw)
To: Greg KH
Cc: Jason Gunthorpe, Pratyush Yadav, Vipin Sharma, Pasha Tatashin,
jasonmiu, graf, changyuanl, rppt, dmatlack, rientjes, corbet,
rdunlap, ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm,
tj, yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <2025081334-rotten-visible-517a@gregkh>
On Wed, Aug 13 2025, Greg KH wrote:
> On Wed, Aug 13, 2025 at 09:41:40AM -0300, Jason Gunthorpe wrote:
[...]
>> Use the warn ons. Make sure they can't be triggered by userspace. Use
>> them to detect corruption/malfunction in the kernel.
>>
>> In this case if kho_unpreserve_folio() fails in this call chain it
>> means some error unwind is wrongly happening out of sequence, and we
>> are now forced to leak memory. Unwind is not something that userspace
>> should be controlling, so of course we want a WARN_ON here.
>
> "should be" is the key here. And it's not obvious from this patch if
> that's true or not, which is why I mentioned it.
>
> I will keep bringing this up, given the HUGE number of CVEs I keep
> assigning each week for when userspace hits WARN_ON() calls until that
> flow starts to die out either because we don't keep adding new calls, OR
> we finally fix them all. Both would be good...
Out of curiosity, why is hitting a WARN_ON() considered a vulnerability?
I'd guess one reason is overwhelming system console which can cause a
denial of service, but what about WARN_ON_ONCE() or WARN_RATELIMIT()?
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pasha Tatashin @ 2025-08-13 13:41 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Greg KH, Jason Gunthorpe, Vipin Sharma, jasonmiu, graf,
changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <mafs07bz7wdfk.fsf@kernel.org>
On Wed, Aug 13, 2025 at 1:37 PM Pratyush Yadav <pratyush@kernel.org> wrote:
>
> On Wed, Aug 13 2025, Greg KH wrote:
>
> > On Wed, Aug 13, 2025 at 09:41:40AM -0300, Jason Gunthorpe wrote:
> [...]
> >> Use the warn ons. Make sure they can't be triggered by userspace. Use
> >> them to detect corruption/malfunction in the kernel.
> >>
> >> In this case if kho_unpreserve_folio() fails in this call chain it
> >> means some error unwind is wrongly happening out of sequence, and we
> >> are now forced to leak memory. Unwind is not something that userspace
> >> should be controlling, so of course we want a WARN_ON here.
> >
> > "should be" is the key here. And it's not obvious from this patch if
> > that's true or not, which is why I mentioned it.
> >
> > I will keep bringing this up, given the HUGE number of CVEs I keep
> > assigning each week for when userspace hits WARN_ON() calls until that
> > flow starts to die out either because we don't keep adding new calls, OR
> > we finally fix them all. Both would be good...
>
> Out of curiosity, why is hitting a WARN_ON() considered a vulnerability?
> I'd guess one reason is overwhelming system console which can cause a
> denial of service, but what about WARN_ON_ONCE() or WARN_RATELIMIT()?
My understanding that it is vulnerability only if it can be triggered
from userspace, otherwise it is a preferred method to give a notice
that something is very wrong.
Given the large number of machines that have panic_on_warn, a reliable
kernel crash that is triggered from userspace is a vulnerability(?).
Pasha
>
> --
> Regards,
> Pratyush Yadav
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pasha Tatashin @ 2025-08-13 13:49 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Vipin Sharma, jasonmiu, graf, changyuanl, rppt, dmatlack,
rientjes, corbet, rdunlap, ilpo.jarvinen, kanie, ojeda, aliceryhl,
masahiroy, akpm, tj, yoann.congal, mmaurer, roman.gushchin,
chenridong, axboe, mark.rutland, jannh, vincent.guittot, hannes,
dan.j.williams, david, joel.granados, rostedt, anna.schumaker,
song, zhangguopeng, linux, linux-kernel, linux-doc, linux-mm,
gregkh, tglx, mingo, bp, dave.hansen, x86, hpa, rafael, dakr,
bartosz.golaszewski, cw00.choi, myungjoo.ham, yesanishhere,
Jonathan.Cameron, quic_zijuhu, aleksander.lobakin, ira.weiny,
andriy.shevchenko, leon, lukas, bhelgaas, wagi, djeffery,
stuart.w.hayes, lennart, brauner, linux-api, linux-fsdevel,
saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <mafs0wm77wgjx.fsf@kernel.org>
On Wed, Aug 13, 2025 at 12:29 PM Pratyush Yadav <pratyush@kernel.org> wrote:
>
> Hi Vipin,
>
> Thanks for the review.
>
> On Tue, Aug 12 2025, Vipin Sharma wrote:
>
> > On 2025-08-07 01:44:35, Pasha Tatashin wrote:
> >> From: Pratyush Yadav <ptyadav@amazon.de>
> >> +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
> >> + unsigned int nr_folios)
> >> +{
> >> + unsigned int i;
> >> +
> >> + for (i = 0; i < nr_folios; i++) {
> >> + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
> >> + struct folio *folio;
> >> +
> >> + if (!pfolio->foliodesc)
> >> + continue;
> >> +
> >> + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
> >> +
> >> + kho_unpreserve_folio(folio);
> >
> > This one is missing WARN_ON_ONCE() similar to the one in
> > memfd_luo_preserve_folios().
>
> Right, will add.
>
> >
> >> + unpin_folio(folio);
>
> Looking at this code caught my eye. This can also be called from LUO's
> finish callback if no one claimed the memfd after live update. In that
> case, unpin_folio() is going to underflow the pincount or refcount on
> the folio since after the kexec, the folio is no longer pinned. We
> should only be doing folio_put().
>
> I think this function should take a argument to specify which of these
> cases it is dealing with.
>
> >> + }
> >> +}
> >> +
> >> +static void *memfd_luo_create_fdt(unsigned long size)
> >> +{
> >> + unsigned int order = get_order(size);
> >> + struct folio *fdt_folio;
> >> + int err = 0;
> >> + void *fdt;
> >> +
> >> + if (order > MAX_PAGE_ORDER)
> >> + return NULL;
> >> +
> >> + fdt_folio = folio_alloc(GFP_KERNEL, order);
> >
> > __GFP_ZERO should also be used here. Otherwise this can lead to
> > unintentional passing of old kernel memory.
>
> fdt_create() zeroes out the buffer so this should not be a problem.
You are right, fdt_create() zeroes the whole buffer, however, I wonder
if it could be `optimized` to only clear only the header part of FDT,
not the rest and this could potentially lead us to send an FDT buffer
that contains both a valid FDT and the trailing bits contain data from
old kernel.
Pasha
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Greg KH @ 2025-08-13 13:53 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Jason Gunthorpe, Vipin Sharma, Pasha Tatashin, jasonmiu, graf,
changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <mafs07bz7wdfk.fsf@kernel.org>
On Wed, Aug 13, 2025 at 03:37:03PM +0200, Pratyush Yadav wrote:
> On Wed, Aug 13 2025, Greg KH wrote:
>
> > On Wed, Aug 13, 2025 at 09:41:40AM -0300, Jason Gunthorpe wrote:
> [...]
> >> Use the warn ons. Make sure they can't be triggered by userspace. Use
> >> them to detect corruption/malfunction in the kernel.
> >>
> >> In this case if kho_unpreserve_folio() fails in this call chain it
> >> means some error unwind is wrongly happening out of sequence, and we
> >> are now forced to leak memory. Unwind is not something that userspace
> >> should be controlling, so of course we want a WARN_ON here.
> >
> > "should be" is the key here. And it's not obvious from this patch if
> > that's true or not, which is why I mentioned it.
> >
> > I will keep bringing this up, given the HUGE number of CVEs I keep
> > assigning each week for when userspace hits WARN_ON() calls until that
> > flow starts to die out either because we don't keep adding new calls, OR
> > we finally fix them all. Both would be good...
>
> Out of curiosity, why is hitting a WARN_ON() considered a vulnerability?
> I'd guess one reason is overwhelming system console which can cause a
> denial of service, but what about WARN_ON_ONCE() or WARN_RATELIMIT()?
If panic_on_warn is set, this will cause the machine to crash/reboot,
which is considered a "vulnerability" by the CVE.org definition. If a
user can trigger this, it gets a CVE assigned to it.
hope this helps,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Greg KH @ 2025-08-13 13:53 UTC (permalink / raw)
To: Pasha Tatashin
Cc: Pratyush Yadav, Jason Gunthorpe, Vipin Sharma, jasonmiu, graf,
changyuanl, rppt, dmatlack, rientjes, corbet, rdunlap,
ilpo.jarvinen, kanie, ojeda, aliceryhl, masahiroy, akpm, tj,
yoann.congal, mmaurer, roman.gushchin, chenridong, axboe,
mark.rutland, jannh, vincent.guittot, hannes, dan.j.williams,
david, joel.granados, rostedt, anna.schumaker, song, zhangguopeng,
linux, linux-kernel, linux-doc, linux-mm, tglx, mingo, bp,
dave.hansen, x86, hpa, rafael, dakr, bartosz.golaszewski,
cw00.choi, myungjoo.ham, yesanishhere, Jonathan.Cameron,
quic_zijuhu, aleksander.lobakin, ira.weiny, andriy.shevchenko,
leon, lukas, bhelgaas, wagi, djeffery, stuart.w.hayes, lennart,
brauner, linux-api, linux-fsdevel, saeedm, ajayachandra, parav,
leonro, witu
In-Reply-To: <CA+CK2bDs9prKNSo=Ris-L7T43ZFU7ji3cBH3KD1=FxXg7hFbFA@mail.gmail.com>
On Wed, Aug 13, 2025 at 01:41:51PM +0000, Pasha Tatashin wrote:
> On Wed, Aug 13, 2025 at 1:37 PM Pratyush Yadav <pratyush@kernel.org> wrote:
> >
> > On Wed, Aug 13 2025, Greg KH wrote:
> >
> > > On Wed, Aug 13, 2025 at 09:41:40AM -0300, Jason Gunthorpe wrote:
> > [...]
> > >> Use the warn ons. Make sure they can't be triggered by userspace. Use
> > >> them to detect corruption/malfunction in the kernel.
> > >>
> > >> In this case if kho_unpreserve_folio() fails in this call chain it
> > >> means some error unwind is wrongly happening out of sequence, and we
> > >> are now forced to leak memory. Unwind is not something that userspace
> > >> should be controlling, so of course we want a WARN_ON here.
> > >
> > > "should be" is the key here. And it's not obvious from this patch if
> > > that's true or not, which is why I mentioned it.
> > >
> > > I will keep bringing this up, given the HUGE number of CVEs I keep
> > > assigning each week for when userspace hits WARN_ON() calls until that
> > > flow starts to die out either because we don't keep adding new calls, OR
> > > we finally fix them all. Both would be good...
> >
> > Out of curiosity, why is hitting a WARN_ON() considered a vulnerability?
> > I'd guess one reason is overwhelming system console which can cause a
> > denial of service, but what about WARN_ON_ONCE() or WARN_RATELIMIT()?
>
> My understanding that it is vulnerability only if it can be triggered
> from userspace, otherwise it is a preferred method to give a notice
> that something is very wrong.
>
> Given the large number of machines that have panic_on_warn, a reliable
> kernel crash that is triggered from userspace is a vulnerability(?).
Yes, and so is a unreliable one :)
^ permalink raw reply
* Re: [PATCH v3 29/30] luo: allow preserving memfd
From: Pratyush Yadav @ 2025-08-13 13:55 UTC (permalink / raw)
To: Pasha Tatashin
Cc: Pratyush Yadav, Vipin Sharma, jasonmiu, graf, changyuanl, rppt,
dmatlack, rientjes, corbet, rdunlap, ilpo.jarvinen, kanie, ojeda,
aliceryhl, masahiroy, akpm, tj, yoann.congal, mmaurer,
roman.gushchin, chenridong, axboe, mark.rutland, jannh,
vincent.guittot, hannes, dan.j.williams, david, joel.granados,
rostedt, anna.schumaker, song, zhangguopeng, linux, linux-kernel,
linux-doc, linux-mm, gregkh, tglx, mingo, bp, dave.hansen, x86,
hpa, rafael, dakr, bartosz.golaszewski, cw00.choi, myungjoo.ham,
yesanishhere, Jonathan.Cameron, quic_zijuhu, aleksander.lobakin,
ira.weiny, andriy.shevchenko, leon, lukas, bhelgaas, wagi,
djeffery, stuart.w.hayes, lennart, brauner, linux-api,
linux-fsdevel, saeedm, ajayachandra, jgg, parav, leonro, witu
In-Reply-To: <CA+CK2bCmQ3hY+ACnLrVZ1qwiTiVvxEBCDNFmAHn_uVRagvshhw@mail.gmail.com>
On Wed, Aug 13 2025, Pasha Tatashin wrote:
> On Wed, Aug 13, 2025 at 12:29 PM Pratyush Yadav <pratyush@kernel.org> wrote:
>>
>> Hi Vipin,
>>
>> Thanks for the review.
>>
>> On Tue, Aug 12 2025, Vipin Sharma wrote:
>>
>> > On 2025-08-07 01:44:35, Pasha Tatashin wrote:
>> >> From: Pratyush Yadav <ptyadav@amazon.de>
>> >> +static void memfd_luo_unpreserve_folios(const struct memfd_luo_preserved_folio *pfolios,
>> >> + unsigned int nr_folios)
>> >> +{
>> >> + unsigned int i;
>> >> +
>> >> + for (i = 0; i < nr_folios; i++) {
>> >> + const struct memfd_luo_preserved_folio *pfolio = &pfolios[i];
>> >> + struct folio *folio;
>> >> +
>> >> + if (!pfolio->foliodesc)
>> >> + continue;
>> >> +
>> >> + folio = pfn_folio(PRESERVED_FOLIO_PFN(pfolio->foliodesc));
>> >> +
>> >> + kho_unpreserve_folio(folio);
>> >
>> > This one is missing WARN_ON_ONCE() similar to the one in
>> > memfd_luo_preserve_folios().
>>
>> Right, will add.
>>
>> >
>> >> + unpin_folio(folio);
>>
>> Looking at this code caught my eye. This can also be called from LUO's
>> finish callback if no one claimed the memfd after live update. In that
>> case, unpin_folio() is going to underflow the pincount or refcount on
>> the folio since after the kexec, the folio is no longer pinned. We
>> should only be doing folio_put().
>>
>> I think this function should take a argument to specify which of these
>> cases it is dealing with.
>>
>> >> + }
>> >> +}
>> >> +
>> >> +static void *memfd_luo_create_fdt(unsigned long size)
>> >> +{
>> >> + unsigned int order = get_order(size);
>> >> + struct folio *fdt_folio;
>> >> + int err = 0;
>> >> + void *fdt;
>> >> +
>> >> + if (order > MAX_PAGE_ORDER)
>> >> + return NULL;
>> >> +
>> >> + fdt_folio = folio_alloc(GFP_KERNEL, order);
>> >
>> > __GFP_ZERO should also be used here. Otherwise this can lead to
>> > unintentional passing of old kernel memory.
>>
>> fdt_create() zeroes out the buffer so this should not be a problem.
>
> You are right, fdt_create() zeroes the whole buffer, however, I wonder
> if it could be `optimized` to only clear only the header part of FDT,
> not the rest and this could potentially lead us to send an FDT buffer
> that contains both a valid FDT and the trailing bits contain data from
> old kernel.
Fair enough. At least the API documentation does not say anything about
the state of the buffer. My main concern was around performance since
the FDT can be multiple megabytes long for big memfds. Anyway, this
isn't in the blackout window so perhaps we can live with it. Will add
the GFP_ZERO.
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: do_change_type(): refuse to operate on unmounted/not ours mounts
From: Al Viro @ 2025-08-13 18:56 UTC (permalink / raw)
To: Andrei Vagin
Cc: Andrei Vagin, Christian Brauner, linux-fsdevel, LKML, criu,
Linux API, stable
In-Reply-To: <CAEWA0a6jgj8vQhrijSJXUHBnCTtz0HEV66tmaVKPe83ng=3feQ@mail.gmail.com>
On Sat, Jul 26, 2025 at 02:01:20PM -0700, Andrei Vagin wrote:
> > For a very mild example of fun to be had there:
> > mount("none", "/mnt", "tmpfs", 0, "");
> > chdir("/mnt");
> > umount2(".", MNT_DETACH);
> > mount(NULL, ".", NULL, MS_SHARED, NULL);
> > Repeat in a loop, watch mount group id leak. That's a trivial example
> > of violating the assertion ("a mount that had been through umount_tree()
> > is out of propagation graph and related data structures for good").
>
> I wasn't referring to detached mounts. CRIU modifies mounts from
> non-current namespaces.
>
> >
> > As for the "CAP_SYS_ADMIN within the mount user namespace" - which
> > userns do you have in mind?
> >
>
> The user namespace of the target mount:
> ns_capable(mnt->mnt_ns->user_ns, CAP_SYS_ADMIN)
To bring that thread back: how about the following? If nobody objects,
I'm going to throw it into viro/vfs.git #fixes...
[PATCH] use uniform permission checks for all mount propagation changes
do_change_type() and do_set_group() are operating on different
aspects of the same thing - propagation graph. The latter
asks for mounts involved to be mounted in namespace(s) the caller
has CAP_SYS_ADMIN for. The former is a mess - originally it
didn't even check that mount *is* mounted. That got fixed,
but the resulting check turns out to be too strict for userland -
in effect, we check that mount is in our namespace, having already
checked that we have CAP_SYS_ADMIN there.
What we really need (in both cases) is
* we only touch mounts that are mounted. Hard requirement,
data corruption if that's get violated.
* we don't allow to mess with a namespace unless you already
have enough permissions to do so (i.e. CAP_SYS_ADMIN in its userns).
That's an equivalent of what do_set_group() does; let's extract that
into a helper (may_change_propagation()) and use it in both
do_set_group() and do_change_type().
Fixes: 12f147ddd6de "do_change_type(): refuse to operate on unmounted/not ours mounts"
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/namespace.c b/fs/namespace.c
index ddfd4457d338..e7d9b23f1e9e 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2862,6 +2862,19 @@ static int graft_tree(struct mount *mnt, struct mount *p, struct mountpoint *mp)
return attach_recursive_mnt(mnt, p, mp);
}
+static int may_change_propagation(const struct mount *m)
+{
+ struct mnt_namespace *ns = m->mnt_ns;
+
+ // it must be mounted in some namespace
+ if (IS_ERR_OR_NULL(ns)) // is_mounted()
+ return -EINVAL;
+ // and the caller must be admin in userns of that namespace
+ if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN))
+ return -EPERM;
+ return 0;
+}
+
/*
* Sanity check the flags to change_mnt_propagation.
*/
@@ -2898,10 +2911,10 @@ static int do_change_type(struct path *path, int ms_flags)
return -EINVAL;
namespace_lock();
- if (!check_mnt(mnt)) {
- err = -EINVAL;
+ err = may_change_propagation(mnt);
+ if (err)
goto out_unlock;
- }
+
if (type == MS_SHARED) {
err = invent_group_ids(mnt, recurse);
if (err)
@@ -3347,18 +3360,11 @@ static int do_set_group(struct path *from_path, struct path *to_path)
namespace_lock();
- err = -EINVAL;
- /* To and From must be mounted */
- if (!is_mounted(&from->mnt))
- goto out;
- if (!is_mounted(&to->mnt))
- goto out;
-
- err = -EPERM;
- /* We should be allowed to modify mount namespaces of both mounts */
- if (!ns_capable(from->mnt_ns->user_ns, CAP_SYS_ADMIN))
+ err = may_change_propagation(from);
+ if (err)
goto out;
- if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
+ err = may_change_propagation(from);
+ if (err)
goto out;
err = -EINVAL;
^ permalink raw reply related
* Re: do_change_type(): refuse to operate on unmounted/not ours mounts
From: Tycho Andersen @ 2025-08-13 19:09 UTC (permalink / raw)
To: Al Viro
Cc: Andrei Vagin, Andrei Vagin, Christian Brauner, linux-fsdevel,
LKML, criu, Linux API, stable
In-Reply-To: <20250813185601.GJ222315@ZenIV>
On Wed, Aug 13, 2025 at 07:56:01PM +0100, Al Viro wrote:
> @@ -3347,18 +3360,11 @@ static int do_set_group(struct path *from_path, struct path *to_path)
>
> namespace_lock();
>
> - err = -EINVAL;
> - /* To and From must be mounted */
> - if (!is_mounted(&from->mnt))
> - goto out;
> - if (!is_mounted(&to->mnt))
> - goto out;
> -
> - err = -EPERM;
> - /* We should be allowed to modify mount namespaces of both mounts */
> - if (!ns_capable(from->mnt_ns->user_ns, CAP_SYS_ADMIN))
> + err = may_change_propagation(from);
> + if (err)
> goto out;
> - if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
> + err = may_change_propagation(from);
Just driving by, but I guess you mean "to" here.
Tycho
^ 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