* [PATCH RFC 0/8] extensible syscalls: CHECK_FIELDS to allow for easier feature detection
From: Aleksa Sarai @ 2024-09-02 7:06 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai, stable
This is something that I've been thinking about for a while. We had a
discussion at LPC 2020 about this[1] but the proposals suggested there
never materialised.
In short, it is quite difficult for userspace to detect the feature
capability of syscalls at runtime. This is something a lot of programs
want to do, but they are forced to create elaborate scenarios to try to
figure out if a feature is supported without causing damage to the
system. For the vast majority of cases, each individual feature also
needs to be tested individually (because syscall results are
all-or-nothing), so testing even a single syscall's feature set can
easily inflate the startup time of programs.
This patchset implements the fairly minimal design I proposed in this
talk[2] and in some old LKML threads (though I can't find the exact
references ATM). The general flow looks like:
1. Userspace will indicate to the kernel that a syscall should a be
no-op by setting the top bit of the extensible struct size argument.
We will almost certainly never support exabyte sized structs, so the
top bits are free for us to use as makeshift flag bits. This is
preferable to using the per-syscall flag field inside the structure
because seccomp can easily detect the bit in the flag and allow the
probe or forcefully return -EEXTSYS_NOOP.
2. The kernel will then fill the provided structure with every valid
bit pattern that the current kernel understands.
For flags or other bitflag-like fields, this is the set of valid
flags or bits. For pointer fields or fields that take an arbitrary
value, the field has every bit set (0xFF... to fill the field) to
indicate that any value is valid in the field.
3. The syscall then returns -EEXTSYS_NOOP which is an errno that will
only ever be used for this purpose (so userspace can be sure that
the request succeeded).
On older kernels, the syscall will return a different error (usually
-E2BIG or -EFAULT) and userspace can do their old-fashioned checks.
4. Userspace can then check which flags and fields are supported by
looking at the fields in the returned structure. Flags are checked
by doing an AND with the flags field, and field support can checked
by comparing to 0. In principle you could just AND the entire
structure if you wanted to do this check generically without caring
about the structure contents (this is what libraries might consider
doing).
Userspace can even find out the internal kernel structure size by
passing a PAGE_SIZE buffer and seeing how many bytes are non-zero.
As with copy_struct_from_user(), this is designed to be forward- and
backwards- compatible.
This allows programas to get a one-shot understanding of what features a
syscall supports without having to do any elaborate setups or tricks to
detect support for destructive features. Flags can simply be ANDed to
check if they are in the supported set, and fields can just be checked
to see if they are non-zero.
This patchset is IMHO the simplest way we can add the ability to
introspect the feature set of extensible struct (copy_struct_from_user)
syscalls. It doesn't preclude the chance of a more generic mechanism
being added later.
The intended way of using this interface to get feature information
looks something like the following (imagine that openat2 has gained a
new field and a new flag in the future):
static bool openat2_no_automount_supported;
static bool openat2_cwd_fd_supported;
int check_openat2_support(void)
{
int err;
struct open_how how = {};
err = openat2(AT_FDCWD, ".", &how, CHECK_FIELDS | sizeof(how));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
openat2_no_automount_supported = (how.flags & RESOLVE_NO_AUTOMOUNT);
openat2_cwd_fd_supported = (how.cwd_fd != 0);
break;
}
}
[1]: https://lwn.net/Articles/830666/
[2]: https://youtu.be/ggD-eb3yPVs
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Aleksa Sarai (8):
uaccess: add copy_struct_to_user helper
sched_getattr: port to copy_struct_to_user
openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
openat2: add CHECK_FIELDS flag to usize argument
clone3: add CHECK_FIELDS flag to usize argument
selftests: openat2: add 0xFF poisoned data after misaligned struct
selftests: openat2: add CHECK_FIELDS selftests
selftests: clone3: add CHECK_FIELDS selftests
fs/open.c | 17 ++
include/linux/uaccess.h | 98 +++++++++
include/uapi/asm-generic/errno.h | 3 +
include/uapi/linux/openat2.h | 2 +
kernel/fork.c | 33 ++-
kernel/sched/syscalls.c | 42 +---
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 2 +-
.../testing/selftests/clone3/clone3_check_fields.c | 229 +++++++++++++++++++++
tools/testing/selftests/openat2/openat2_test.c | 126 +++++++++++-
10 files changed, 504 insertions(+), 49 deletions(-)
---
base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
change-id: 20240803-extensible-structs-check_fields-a47e94cef691
Best regards,
--
Aleksa Sarai <cyphar@cyphar.com>
^ permalink raw reply
* Re: [PATCH xfstests v1 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-01 12:57 UTC (permalink / raw)
To: Amir Goldstein
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <CAOQ4uxjzpoUtH9OGYmj8K4FF0V4J8vi1W6Ry0Po1RoZ70vQ_fA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 12532 bytes --]
On 2024-08-30, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Aug 28, 2024 at 12:37 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> > make sure they match properly as part of the regular open_by_handle
> > tests.
> >
> > Link: https://lore.kernel.org/all/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > src/open_by_handle.c | 123 ++++++++++++++++++++++++++++++++-----------
> > tests/generic/426 | 1 +
> > 2 files changed, 93 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..cbd68aeadac1 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,15 @@ Examples:
> > #include <errno.h>
> > #include <linux/limits.h>
> > #include <libgen.h>
> > +#include <stdint.h>
> >
> > #include <sys/stat.h>
> > #include "statx.h"
> >
> > +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> > +# define AT_HANDLE_MNT_ID_UNIQUE 0x001
> > +#endif
> > +
> > #define MAXFILES 1024
> >
> > struct handle {
> > @@ -99,7 +104,7 @@ struct handle {
> >
> > void usage(void)
> > {
> > - fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> > + fprintf(stderr, "usage: open_by_handle [-cludMmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> > fprintf(stderr, "\n");
> > fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> > fprintf(stderr, "open_by_handle <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
> > @@ -111,6 +116,7 @@ void usage(void)
> > fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
> > fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> > fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> > + fprintf(stderr, "open_by_handle -M <test_dir> [N] - confirm that the mount id returned by name_to_handle_at matches the mount id in statx\n");
> > fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
> > fprintf(stderr, "open_by_handle -p <test_dir> - create/delete and try to open by handle also test_dir itself\n");
> > fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
> > @@ -120,6 +126,81 @@ void usage(void)
> > exit(EXIT_FAILURE);
> > }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz,
> > + int checkmountid)
> > +{
> > + int ret;
> > + int mntid_short;
> > +
> > + uint64_t mntid_unique;
> > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > + struct handle dummy_fh;
> > +
> > + if (checkmountid) {
> > + struct statx statxbuf;
> > +
> > + /* Get both the short and unique mount id. */
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + statx_mntid_short = statxbuf.stx_mnt_id;
> > +
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> > + return EXIT_FAILURE;
>
> This failure will break the test on LTS kernels - we don't want that.
> Instead I think you should:
> - drop the -M option
> - get statx_mntid_unique here IF kernel supports STATX_MNT_ID_UNIQUE
> and then...
Ah okay, I wasn't sure if the xfstests policy was like selftests where
only the latest kernel matters. I'll send a v2 with the suggestions you
mentioned.
However, presumably this means we would also not do the
STATX_MNT_ID_UNIQUE check if name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE)
returns -EINVAL, right?
> > + }
> > + if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> > + fprintf(stderr, "%s: no STATX_MNT_ID_UNIQUE in stx_mask\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > + statx_mntid_unique = statxbuf.stx_mnt_id;
> > + }
> > +
> > + fh->handle_bytes = bufsz;
> > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > + if (bufsz < fh->handle_bytes) {
> > + /* Query the filesystem required bufsz and the file handle */
> > + if (ret != -1 || errno != EOVERFLOW) {
> > + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> > + return EXIT_FAILURE;
> > + }
> > + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> > + }
> > + if (ret < 0) {
> > + fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + if (checkmountid) {
> > + if (mntid_short != (int) statx_mntid_short) {
> > + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + /*
> > + * Get the unique mount ID. We don't need to get another copy of the
> > + * handle so store it in a dummy struct.
> > + */
> > + dummy_fh.fh.handle_bytes = fh->handle_bytes;
> > + if (name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE) < 0) {
> > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > +
> > + if (mntid_unique != statx_mntid_unique) {
> > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> > + return EXIT_FAILURE;
> > + }
>
> - check statx_mntid_unique here IFF statx_mntid_unique is set
> - always check statx_mntid_short (what could be a reason to not check it?)
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > int i, c;
> > @@ -132,19 +213,20 @@ int main(int argc, char **argv)
> > char fname2[PATH_MAX];
> > char *test_dir;
> > char *mount_dir;
> > - int mount_fd, mount_id;
> > + int mount_fd;
> > char *infile = NULL, *outfile = NULL;
> > int in_fd = 0, out_fd = 0;
> > int numfiles = 1;
> > int create = 0, delete = 0, nlink = 1, move = 0;
> > int rd = 0, wr = 0, wrafter = 0, parent = 0;
> > int keepopen = 0, drop_caches = 1, sleep_loop = 0;
> > + int checkmountid = 0;
> > int bufsz = MAX_HANDLE_SZ;
> >
> > if (argc < 2)
> > usage();
> >
> > - while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> > + while ((c = getopt(argc, argv, "cludMmrwapknhi:o:sz")) != -1) {
> > switch (c) {
> > case 'c':
> > create = 1;
> > @@ -172,6 +254,9 @@ int main(int argc, char **argv)
> > delete = 1;
> > nlink = 0;
> > break;
> > + case 'M':
> > + checkmountid = 1;
> > + break;
> > case 'm':
> > move = 1;
> > break;
> > @@ -307,21 +392,9 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> > }
> > } else {
> > - handle[i].fh.handle_bytes = bufsz;
> > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > - if (bufsz < handle[i].fh.handle_bytes) {
> > - /* Query the filesystem required bufsz and the file handle */
> > - if (ret != -1 || errno != EOVERFLOW) {
> > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> > - return EXIT_FAILURE;
> > - }
> > - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> > - }
> > - if (ret < 0) {
> > - strcat(fname, ": name_to_handle");
> > - perror(fname);
> > + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, checkmountid);
> > + if (ret < 0)
> > return EXIT_FAILURE;
> > - }
> > }
> > if (keepopen) {
> > /* Open without close to keep unlinked files around */
> > @@ -349,21 +422,9 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> > }
> > } else {
> > - dir_handle.fh.handle_bytes = bufsz;
> > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > - if (bufsz < dir_handle.fh.handle_bytes) {
> > - /* Query the filesystem required bufsz and the file handle */
> > - if (ret != -1 || errno != EOVERFLOW) {
> > - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> > - return EXIT_FAILURE;
> > - }
> > - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> > - }
> > - if (ret < 0) {
> > - strcat(dname, ": name_to_handle");
> > - perror(dname);
> > + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, checkmountid);
> > + if (ret < 0)
> > return EXIT_FAILURE;
> > - }
> > }
> > if (out_fd) {
> > ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> > diff --git a/tests/generic/426 b/tests/generic/426
> > index 25909f220e1e..df481c58562c 100755
> > --- a/tests/generic/426
> > +++ b/tests/generic/426
> > @@ -51,6 +51,7 @@ test_file_handles $testdir -d
> > # Check non-stale handles to linked files
> > create_test_files $testdir
> > test_file_handles $testdir
> > +test_file_handles $testdir -M
>
> I see no reason to add option -M and add a second invocation.
>
> Something I am missing?
Given how many other custom modes there were, I assumed that providing
it as an additional flag would've been preferred. I'll just make it
automatic.
Thanks!
>
> Thanks,
> Amir.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [musl] AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: Rich Felker @ 2024-08-31 15:41 UTC (permalink / raw)
To: H.J. Lu; +Cc: Linux Kernel Mailing List, linux-api, libc-alpha, musl
In-Reply-To: <CAMe9rOqSSX_YP7dq5WK7vDyrQ5RP6nUNrim-8FjJi1X_8NfAvg@mail.gmail.com>
On Sat, Aug 31, 2024 at 08:09:49AM -0700, H.J. Lu wrote:
> On Sat, Aug 31, 2024 at 8:03 AM Rich Felker <dalias@libc.org> wrote:
> >
> > On Sat, Aug 31, 2024 at 11:29:02AM +0200, Szabolcs Nagy wrote:
> > > * Rich Felker <dalias@libc.org> [2024-08-29 16:54:38 -0400]:
> > > > As I understand it, the AT_MINSIGSTKSZ auxv value is supposed to be a
> > > > suitable runtime value for MINSIGSTKSZ (sysconf(_SC_MINSIGSTKSZ)),
> > > > such that it's safe to pass as a size to sigaltstack. However, this is
> > > > not how the kernel actually implements it. At least on x86 and
> > > > powerpc, the kernel fills it via get_sigframe_size, which computes the
> > > > size of the sigcontext/siginfo/etc to be pushed and uses that
> > > > directly, without allowing any space for actual execution, and without
> > > > ensuring the value is at least as large as the legacy constant
> > > > MINSIGSTKSZ. This leads to two problems:
> > > >
> > > > 1. If userspace uses the value without clamping it not-below
> > > > MINSIGSTKSZ, sigaltstack will fail with ENOMEM.
> > > >
> > > > 2. If the kernel needs more space than MINSIGSTKSZ just for the signal
> > > > frame structures, userspace that trusts AT_MINSIGSTKSZ will only
> > > > allocate enough for the frame, and the program will immediately
> > > > crash/stack-overflow once execution passes to userspace.
> > > >
> > > > Since existing kernels in the wild can't be fixed, and since it looks
> > > > like the problem is just that the kernel chose a poor definition of
> > > > AT_MINSIGSTKSZ, I think userspace (glibc, musl, etc.) need to work
> > > > around the problem, adding a per-arch correction term to
> > > > AT_MINSIGSTKSZ that's basically equal to:
> > > >
> > > > legacy_MINSIGSTKSZ - AT_MINSIGSTKSZ as returned on legacy hw
> > > >
> > > > such that adding the correction term would reproduce the expected
> > > > value MINSIGSTKSZ.
> > > >
> > > > The only question is whether the kernel will commit to keeping this
> > > > behavior, or whether it would be "fixed" to include all the needed
> > > > working space when they eventually decide they want bigger stacks for
> > > > some new register file bloat. I think keeping the current behavior, so
> > > > we can just add a fixed offset, is probably the best thing to do.
> > >
> > > i think it makes sense that the kernel sets AT_MINSIGSTKSZ
> > > according to what the kernel needs (signal frame size)
> > > anything beyond that is up to userspace requirements (e.g.
> > > the kernel cannot know if the libc wraps signal handlers)
> > >
> > > it's up to the libc to adjust sysconf(_SC_MINSIGSTKSZ)
> > > according to posix or backward compat requirements.
> >
> > I think this is a reasonable viea and means the aux key was just very
> > poorly named. It should have been called something like
> > AT_SIGFRAMESIZE to indicate to the userspace-side consumer that it's
> > not a suitable value for MINSIGSTKSZ, only a contributing term for it.
> >
> > Rich
>
> glibc manual has
>
> ‘_SC_MINSIGSTKSZ’
>
> Inquire about the minimum number of bytes of free stack space
> required in order to guarantee successful, non-nested handling of a
> single signal whose handler is an empty function.
>
> ‘MINSIGSTKSZ’
> This is the amount of signal stack space the operating
> system needs just to implement signal delivery. The size
> of a signal stack *must* be greater than this.
>
> For most cases, just using ‘SIGSTKSZ’ for ‘ss_size’ is
> sufficient. But if you know how much stack space your
> program's signal handlers will need, you may want to use
> a different size. In this case, you should allocate
> ‘MINSIGSTKSZ’ additional bytes for the signal stack and
> increase ‘ss_size’ accordingly.
This is ambiguously worded (does "operating system" mean kernel?) and
does not agree with POSIX, which defines it as:
Minimum stack size for a signal handler.
And otherwise just specifies that sigaltstack shall fail if given a
smaller size.
The POSIX definition is also underspecified but it's clear that it
should be possible to execute at least a do-nothing signal handler
(like one which immediately returns and whose sole purpose is to
induce EINTR when intalled without SA_RESTART), or even a minimal one
that does something like storing to a global variable, with such a
small stack. Allowing a size where even a do-nothing signal handler
results in a memory-clobbering overflow or access fault seems
non-conforming to me.
The historical sizes all allowed for 1k of execution space on top of
what the historical signal frames consumed, and more for some archs. I
don't think there's a POSIX contract to include that much, but I think
there is a backwards-compatibility motivation to do so. Otherwise
there will be application that were working when
sysconf(_SC_MINSIGSTKSZ) historical_val as the result of
max(value_from_aux,historial_val), but which break catastrophically as
soon as value_from_aux is bigger than historical_val-sigframe_size.
Rich
^ permalink raw reply
* Re: [musl] AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: H.J. Lu @ 2024-08-31 15:09 UTC (permalink / raw)
To: Rich Felker; +Cc: Linux Kernel Mailing List, linux-api, libc-alpha, musl
In-Reply-To: <20240831150241.GP10433@brightrain.aerifal.cx>
On Sat, Aug 31, 2024 at 8:03 AM Rich Felker <dalias@libc.org> wrote:
>
> On Sat, Aug 31, 2024 at 11:29:02AM +0200, Szabolcs Nagy wrote:
> > * Rich Felker <dalias@libc.org> [2024-08-29 16:54:38 -0400]:
> > > As I understand it, the AT_MINSIGSTKSZ auxv value is supposed to be a
> > > suitable runtime value for MINSIGSTKSZ (sysconf(_SC_MINSIGSTKSZ)),
> > > such that it's safe to pass as a size to sigaltstack. However, this is
> > > not how the kernel actually implements it. At least on x86 and
> > > powerpc, the kernel fills it via get_sigframe_size, which computes the
> > > size of the sigcontext/siginfo/etc to be pushed and uses that
> > > directly, without allowing any space for actual execution, and without
> > > ensuring the value is at least as large as the legacy constant
> > > MINSIGSTKSZ. This leads to two problems:
> > >
> > > 1. If userspace uses the value without clamping it not-below
> > > MINSIGSTKSZ, sigaltstack will fail with ENOMEM.
> > >
> > > 2. If the kernel needs more space than MINSIGSTKSZ just for the signal
> > > frame structures, userspace that trusts AT_MINSIGSTKSZ will only
> > > allocate enough for the frame, and the program will immediately
> > > crash/stack-overflow once execution passes to userspace.
> > >
> > > Since existing kernels in the wild can't be fixed, and since it looks
> > > like the problem is just that the kernel chose a poor definition of
> > > AT_MINSIGSTKSZ, I think userspace (glibc, musl, etc.) need to work
> > > around the problem, adding a per-arch correction term to
> > > AT_MINSIGSTKSZ that's basically equal to:
> > >
> > > legacy_MINSIGSTKSZ - AT_MINSIGSTKSZ as returned on legacy hw
> > >
> > > such that adding the correction term would reproduce the expected
> > > value MINSIGSTKSZ.
> > >
> > > The only question is whether the kernel will commit to keeping this
> > > behavior, or whether it would be "fixed" to include all the needed
> > > working space when they eventually decide they want bigger stacks for
> > > some new register file bloat. I think keeping the current behavior, so
> > > we can just add a fixed offset, is probably the best thing to do.
> >
> > i think it makes sense that the kernel sets AT_MINSIGSTKSZ
> > according to what the kernel needs (signal frame size)
> > anything beyond that is up to userspace requirements (e.g.
> > the kernel cannot know if the libc wraps signal handlers)
> >
> > it's up to the libc to adjust sysconf(_SC_MINSIGSTKSZ)
> > according to posix or backward compat requirements.
>
> I think this is a reasonable viea and means the aux key was just very
> poorly named. It should have been called something like
> AT_SIGFRAMESIZE to indicate to the userspace-side consumer that it's
> not a suitable value for MINSIGSTKSZ, only a contributing term for it.
>
> Rich
glibc manual has
‘_SC_MINSIGSTKSZ’
Inquire about the minimum number of bytes of free stack space
required in order to guarantee successful, non-nested handling of a
single signal whose handler is an empty function.
‘MINSIGSTKSZ’
This is the amount of signal stack space the operating
system needs just to implement signal delivery. The size
of a signal stack *must* be greater than this.
For most cases, just using ‘SIGSTKSZ’ for ‘ss_size’ is
sufficient. But if you know how much stack space your
program's signal handlers will need, you may want to use
a different size. In this case, you should allocate
‘MINSIGSTKSZ’ additional bytes for the signal stack and
increase ‘ss_size’ accordingly.
--
H.J.
^ permalink raw reply
* Re: [musl] AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: Rich Felker @ 2024-08-31 15:02 UTC (permalink / raw)
To: Linux Kernel Mailing List, linux-api, libc-alpha, musl
In-Reply-To: <20240831092902.GA2724612@port70.net>
On Sat, Aug 31, 2024 at 11:29:02AM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2024-08-29 16:54:38 -0400]:
> > As I understand it, the AT_MINSIGSTKSZ auxv value is supposed to be a
> > suitable runtime value for MINSIGSTKSZ (sysconf(_SC_MINSIGSTKSZ)),
> > such that it's safe to pass as a size to sigaltstack. However, this is
> > not how the kernel actually implements it. At least on x86 and
> > powerpc, the kernel fills it via get_sigframe_size, which computes the
> > size of the sigcontext/siginfo/etc to be pushed and uses that
> > directly, without allowing any space for actual execution, and without
> > ensuring the value is at least as large as the legacy constant
> > MINSIGSTKSZ. This leads to two problems:
> >
> > 1. If userspace uses the value without clamping it not-below
> > MINSIGSTKSZ, sigaltstack will fail with ENOMEM.
> >
> > 2. If the kernel needs more space than MINSIGSTKSZ just for the signal
> > frame structures, userspace that trusts AT_MINSIGSTKSZ will only
> > allocate enough for the frame, and the program will immediately
> > crash/stack-overflow once execution passes to userspace.
> >
> > Since existing kernels in the wild can't be fixed, and since it looks
> > like the problem is just that the kernel chose a poor definition of
> > AT_MINSIGSTKSZ, I think userspace (glibc, musl, etc.) need to work
> > around the problem, adding a per-arch correction term to
> > AT_MINSIGSTKSZ that's basically equal to:
> >
> > legacy_MINSIGSTKSZ - AT_MINSIGSTKSZ as returned on legacy hw
> >
> > such that adding the correction term would reproduce the expected
> > value MINSIGSTKSZ.
> >
> > The only question is whether the kernel will commit to keeping this
> > behavior, or whether it would be "fixed" to include all the needed
> > working space when they eventually decide they want bigger stacks for
> > some new register file bloat. I think keeping the current behavior, so
> > we can just add a fixed offset, is probably the best thing to do.
>
> i think it makes sense that the kernel sets AT_MINSIGSTKSZ
> according to what the kernel needs (signal frame size)
> anything beyond that is up to userspace requirements (e.g.
> the kernel cannot know if the libc wraps signal handlers)
>
> it's up to the libc to adjust sysconf(_SC_MINSIGSTKSZ)
> according to posix or backward compat requirements.
I think this is a reasonable viea and means the aux key was just very
poorly named. It should have been called something like
AT_SIGFRAMESIZE to indicate to the userspace-side consumer that it's
not a suitable value for MINSIGSTKSZ, only a contributing term for it.
Rich
^ permalink raw reply
* Re: [musl] AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: Szabolcs Nagy @ 2024-08-31 9:29 UTC (permalink / raw)
To: Rich Felker; +Cc: Linux Kernel Mailing List, linux-api, libc-alpha, musl
In-Reply-To: <20240829205436.GA14562@brightrain.aerifal.cx>
* Rich Felker <dalias@libc.org> [2024-08-29 16:54:38 -0400]:
> As I understand it, the AT_MINSIGSTKSZ auxv value is supposed to be a
> suitable runtime value for MINSIGSTKSZ (sysconf(_SC_MINSIGSTKSZ)),
> such that it's safe to pass as a size to sigaltstack. However, this is
> not how the kernel actually implements it. At least on x86 and
> powerpc, the kernel fills it via get_sigframe_size, which computes the
> size of the sigcontext/siginfo/etc to be pushed and uses that
> directly, without allowing any space for actual execution, and without
> ensuring the value is at least as large as the legacy constant
> MINSIGSTKSZ. This leads to two problems:
>
> 1. If userspace uses the value without clamping it not-below
> MINSIGSTKSZ, sigaltstack will fail with ENOMEM.
>
> 2. If the kernel needs more space than MINSIGSTKSZ just for the signal
> frame structures, userspace that trusts AT_MINSIGSTKSZ will only
> allocate enough for the frame, and the program will immediately
> crash/stack-overflow once execution passes to userspace.
>
> Since existing kernels in the wild can't be fixed, and since it looks
> like the problem is just that the kernel chose a poor definition of
> AT_MINSIGSTKSZ, I think userspace (glibc, musl, etc.) need to work
> around the problem, adding a per-arch correction term to
> AT_MINSIGSTKSZ that's basically equal to:
>
> legacy_MINSIGSTKSZ - AT_MINSIGSTKSZ as returned on legacy hw
>
> such that adding the correction term would reproduce the expected
> value MINSIGSTKSZ.
>
> The only question is whether the kernel will commit to keeping this
> behavior, or whether it would be "fixed" to include all the needed
> working space when they eventually decide they want bigger stacks for
> some new register file bloat. I think keeping the current behavior, so
> we can just add a fixed offset, is probably the best thing to do.
i think it makes sense that the kernel sets AT_MINSIGSTKSZ
according to what the kernel needs (signal frame size)
anything beyond that is up to userspace requirements (e.g.
the kernel cannot know if the libc wraps signal handlers)
it's up to the libc to adjust sysconf(_SC_MINSIGSTKSZ)
according to posix or backward compat requirements.
^ permalink raw reply
* Re: [PATCH xfstests v1 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-08-30 17:10 UTC (permalink / raw)
To: Aleksa Sarai
Cc: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240828103706.2393267-2-cyphar@cyphar.com>
On Wed, Aug 28, 2024 at 12:37 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
> make sure they match properly as part of the regular open_by_handle
> tests.
>
> Link: https://lore.kernel.org/all/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> src/open_by_handle.c | 123 ++++++++++++++++++++++++++++++++-----------
> tests/generic/426 | 1 +
> 2 files changed, 93 insertions(+), 31 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..cbd68aeadac1 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,15 @@ Examples:
> #include <errno.h>
> #include <linux/limits.h>
> #include <libgen.h>
> +#include <stdint.h>
>
> #include <sys/stat.h>
> #include "statx.h"
>
> +#ifndef AT_HANDLE_MNT_ID_UNIQUE
> +# define AT_HANDLE_MNT_ID_UNIQUE 0x001
> +#endif
> +
> #define MAXFILES 1024
>
> struct handle {
> @@ -99,7 +104,7 @@ struct handle {
>
> void usage(void)
> {
> - fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> + fprintf(stderr, "usage: open_by_handle [-cludMmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
> fprintf(stderr, "\n");
> fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
> fprintf(stderr, "open_by_handle <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
> @@ -111,6 +116,7 @@ void usage(void)
> fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
> fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
> fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
> + fprintf(stderr, "open_by_handle -M <test_dir> [N] - confirm that the mount id returned by name_to_handle_at matches the mount id in statx\n");
> fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
> fprintf(stderr, "open_by_handle -p <test_dir> - create/delete and try to open by handle also test_dir itself\n");
> fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
> @@ -120,6 +126,81 @@ void usage(void)
> exit(EXIT_FAILURE);
> }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz,
> + int checkmountid)
> +{
> + int ret;
> + int mntid_short;
> +
> + uint64_t mntid_unique;
> + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> + struct handle dummy_fh;
> +
> + if (checkmountid) {
> + struct statx statxbuf;
> +
> + /* Get both the short and unique mount id. */
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> + fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
> + return EXIT_FAILURE;
> + }
> + statx_mntid_short = statxbuf.stx_mnt_id;
> +
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
> + return EXIT_FAILURE;
This failure will break the test on LTS kernels - we don't want that.
Instead I think you should:
- drop the -M option
- get statx_mntid_unique here IF kernel supports STATX_MNT_ID_UNIQUE
and then...
> + }
> + if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> + fprintf(stderr, "%s: no STATX_MNT_ID_UNIQUE in stx_mask\n", fname);
> + return EXIT_FAILURE;
> + }
> + statx_mntid_unique = statxbuf.stx_mnt_id;
> + }
> +
> + fh->handle_bytes = bufsz;
> + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> + if (bufsz < fh->handle_bytes) {
> + /* Query the filesystem required bufsz and the file handle */
> + if (ret != -1 || errno != EOVERFLOW) {
> + fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
> + return EXIT_FAILURE;
> + }
> + ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
> + }
> + if (ret < 0) {
> + fprintf(stderr, "%s: name_to_handle: %m\n", fname);
> + return EXIT_FAILURE;
> + }
> +
> + if (checkmountid) {
> + if (mntid_short != (int) statx_mntid_short) {
> + fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
> + return EXIT_FAILURE;
> + }
> +
> + /*
> + * Get the unique mount ID. We don't need to get another copy of the
> + * handle so store it in a dummy struct.
> + */
> + dummy_fh.fh.handle_bytes = fh->handle_bytes;
> + if (name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE) < 0) {
> + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> +
> + if (mntid_unique != statx_mntid_unique) {
> + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
> + return EXIT_FAILURE;
> + }
- check statx_mntid_unique here IFF statx_mntid_unique is set
- always check statx_mntid_short (what could be a reason to not check it?)
> + }
> +
> + return 0;
> +}
> +
> int main(int argc, char **argv)
> {
> int i, c;
> @@ -132,19 +213,20 @@ int main(int argc, char **argv)
> char fname2[PATH_MAX];
> char *test_dir;
> char *mount_dir;
> - int mount_fd, mount_id;
> + int mount_fd;
> char *infile = NULL, *outfile = NULL;
> int in_fd = 0, out_fd = 0;
> int numfiles = 1;
> int create = 0, delete = 0, nlink = 1, move = 0;
> int rd = 0, wr = 0, wrafter = 0, parent = 0;
> int keepopen = 0, drop_caches = 1, sleep_loop = 0;
> + int checkmountid = 0;
> int bufsz = MAX_HANDLE_SZ;
>
> if (argc < 2)
> usage();
>
> - while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> + while ((c = getopt(argc, argv, "cludMmrwapknhi:o:sz")) != -1) {
> switch (c) {
> case 'c':
> create = 1;
> @@ -172,6 +254,9 @@ int main(int argc, char **argv)
> delete = 1;
> nlink = 0;
> break;
> + case 'M':
> + checkmountid = 1;
> + break;
> case 'm':
> move = 1;
> break;
> @@ -307,21 +392,9 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
> } else {
> - handle[i].fh.handle_bytes = bufsz;
> - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> - if (bufsz < handle[i].fh.handle_bytes) {
> - /* Query the filesystem required bufsz and the file handle */
> - if (ret != -1 || errno != EOVERFLOW) {
> - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
> - return EXIT_FAILURE;
> - }
> - ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
> - }
> - if (ret < 0) {
> - strcat(fname, ": name_to_handle");
> - perror(fname);
> + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, checkmountid);
> + if (ret < 0)
> return EXIT_FAILURE;
> - }
> }
> if (keepopen) {
> /* Open without close to keep unlinked files around */
> @@ -349,21 +422,9 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
> } else {
> - dir_handle.fh.handle_bytes = bufsz;
> - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> - if (bufsz < dir_handle.fh.handle_bytes) {
> - /* Query the filesystem required bufsz and the file handle */
> - if (ret != -1 || errno != EOVERFLOW) {
> - fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
> - return EXIT_FAILURE;
> - }
> - ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
> - }
> - if (ret < 0) {
> - strcat(dname, ": name_to_handle");
> - perror(dname);
> + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, checkmountid);
> + if (ret < 0)
> return EXIT_FAILURE;
> - }
> }
> if (out_fd) {
> ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> diff --git a/tests/generic/426 b/tests/generic/426
> index 25909f220e1e..df481c58562c 100755
> --- a/tests/generic/426
> +++ b/tests/generic/426
> @@ -51,6 +51,7 @@ test_file_handles $testdir -d
> # Check non-stale handles to linked files
> create_test_files $testdir
> test_file_handles $testdir
> +test_file_handles $testdir -M
I see no reason to add option -M and add a second invocation.
Something I am missing?
Thanks,
Amir.
^ permalink raw reply
* AT_MINSIGSTKSZ mismatched interpretation kernel vs libc
From: Rich Felker @ 2024-08-29 20:54 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: linux-api, libc-alpha, musl
As I understand it, the AT_MINSIGSTKSZ auxv value is supposed to be a
suitable runtime value for MINSIGSTKSZ (sysconf(_SC_MINSIGSTKSZ)),
such that it's safe to pass as a size to sigaltstack. However, this is
not how the kernel actually implements it. At least on x86 and
powerpc, the kernel fills it via get_sigframe_size, which computes the
size of the sigcontext/siginfo/etc to be pushed and uses that
directly, without allowing any space for actual execution, and without
ensuring the value is at least as large as the legacy constant
MINSIGSTKSZ. This leads to two problems:
1. If userspace uses the value without clamping it not-below
MINSIGSTKSZ, sigaltstack will fail with ENOMEM.
2. If the kernel needs more space than MINSIGSTKSZ just for the signal
frame structures, userspace that trusts AT_MINSIGSTKSZ will only
allocate enough for the frame, and the program will immediately
crash/stack-overflow once execution passes to userspace.
Since existing kernels in the wild can't be fixed, and since it looks
like the problem is just that the kernel chose a poor definition of
AT_MINSIGSTKSZ, I think userspace (glibc, musl, etc.) need to work
around the problem, adding a per-arch correction term to
AT_MINSIGSTKSZ that's basically equal to:
legacy_MINSIGSTKSZ - AT_MINSIGSTKSZ as returned on legacy hw
such that adding the correction term would reproduce the expected
value MINSIGSTKSZ.
The only question is whether the kernel will commit to keeping this
behavior, or whether it would be "fixed" to include all the needed
working space when they eventually decide they want bigger stacks for
some new register file bloat. I think keeping the current behavior, so
we can just add a fixed offset, is probably the best thing to do.
Rich
^ permalink raw reply
* Re: [PATCH RFC v2 0/4] mm: Introduce MAP_BELOW_HINT
From: Palmer Dabbelt @ 2024-08-29 17:00 UTC (permalink / raw)
To: vbabka
Cc: Charlie Jenkins, Arnd Bergmann, Richard Henderson, ink, mattst88,
vgupta, linux, guoren, chenhuacai, kernel, tsbogend,
James.Bottomley, deller, mpe, npiggin, christophe.leroy, naveen,
agordeev, gerald.schaefer, hca, gor, borntraeger, svens, ysato,
dalias, glaubitz, davem, andreas, tglx, mingo, bp, dave.hansen,
x86, hpa, luto, peterz, muchun.song, akpm, Liam.Howlett,
lorenzo.stoakes, shuah, Linus Torvalds, linux-arch, linux-kernel,
linux-alpha, linux-snps-arc, linux-arm-kernel, linux-csky,
loongarch, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-mm, linux-kselftest, linux-api
In-Reply-To: <a048515e-f6e4-4d77-920b-6742529f3ca4@suse.cz>
On Thu, 29 Aug 2024 02:02:34 PDT (-0700), vbabka@suse.cz wrote:
> Such a large recipient list and no linux-api. CC'd, please include it on
> future postings.
>
> On 8/29/24 09:15, Charlie Jenkins wrote:
>> Some applications rely on placing data in free bits addresses allocated
>> by mmap. Various architectures (eg. x86, arm64, powerpc) restrict the
>> address returned by mmap to be less than the 48-bit address space,
>> unless the hint address uses more than 47 bits (the 48th bit is reserved
>> for the kernel address space).
>>
>> The riscv architecture needs a way to similarly restrict the virtual
>> address space. On the riscv port of OpenJDK an error is thrown if
>> attempted to run on the 57-bit address space, called sv57 [1]. golang
>> has a comment that sv57 support is not complete, but there are some
>> workarounds to get it to mostly work [2].
>>
>> These applications work on x86 because x86 does an implicit 47-bit
>> restriction of mmap() address that contain a hint address that is less
>> than 48 bits.
>>
>> Instead of implicitly restricting the address space on riscv (or any
>> current/future architecture), a flag would allow users to opt-in to this
>> behavior rather than opt-out as is done on other architectures. This is
>> desirable because it is a small class of applications that do pointer
>> masking.
>
> I doubt it's desirable to have different behavior depending on architecture.
> Also you could say it's a small class of applications that need more than 47
> bits.
We're sort of stuck with the architeture-depending behavior here: for
the first few years RISC-V only had 39-bit VAs, so the defato uABI ended
up being that userspace can ignore way more bits. While 48 bits might
be enough for everyone, 39 doesn't seem to be -- or at least IIRC when
we tried restricting the default to that, we broke stuff. There's also
some other wrinkles like arbitrary bit boundaries in pointer masking and
vendor-specific paging formats, but at some point we just end up down a
rabbit hole of insanity there...
FWIW, I think that userspace depending on just tossing some VA bits
because some kernels happened to never allocate from them is just
broken, but it seems like other ports worked around the 48->57 bit
transition and we're trying to do something similar for 39->48 (and that
works with 49->57, as we'll have to deal with that eventually).
So that's basically how we ended up with this sort of thing: trying to
do something similar without a flag broke userspace because we were
trying to jam too much into the hints. I couldn't really figure out a
way to satisfy all the userspace constraints by just implicitly
retrofitting behavior based on the hints, so we figured having an
explicit flag to control the behavior would be the sanest way to go.
That said: I'm not opposed to just saying "depending on 39-bit VAs is
broken" and just forcing people to fix it.
>> This flag will also allow seemless compatibility between all
>> architectures, so applications like Go and OpenJDK that use bits in a
>> virtual address can request the exact number of bits they need in a
>> generic way. The flag can be checked inside of vm_unmapped_area() so
>> that this flag does not have to be handled individually by each
>> architecture.
>>
>> Link:
>> https://github.com/openjdk/jdk/blob/f080b4bb8a75284db1b6037f8c00ef3b1ef1add1/src/hotspot/cpu/riscv/vm_version_riscv.cpp#L79
>> [1]
>> Link:
>> https://github.com/golang/go/blob/9e8ea567c838574a0f14538c0bbbd83c3215aa55/src/runtime/tagptr_64bit.go#L47
>> [2]
>>
>> To: Arnd Bergmann <arnd@arndb.de>
>> To: Richard Henderson <richard.henderson@linaro.org>
>> To: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
>> To: Matt Turner <mattst88@gmail.com>
>> To: Vineet Gupta <vgupta@kernel.org>
>> To: Russell King <linux@armlinux.org.uk>
>> To: Guo Ren <guoren@kernel.org>
>> To: Huacai Chen <chenhuacai@kernel.org>
>> To: WANG Xuerui <kernel@xen0n.name>
>> To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>> To: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
>> To: Helge Deller <deller@gmx.de>
>> To: Michael Ellerman <mpe@ellerman.id.au>
>> To: Nicholas Piggin <npiggin@gmail.com>
>> To: Christophe Leroy <christophe.leroy@csgroup.eu>
>> To: Naveen N Rao <naveen@kernel.org>
>> To: Alexander Gordeev <agordeev@linux.ibm.com>
>> To: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
>> To: Heiko Carstens <hca@linux.ibm.com>
>> To: Vasily Gorbik <gor@linux.ibm.com>
>> To: Christian Borntraeger <borntraeger@linux.ibm.com>
>> To: Sven Schnelle <svens@linux.ibm.com>
>> To: Yoshinori Sato <ysato@users.sourceforge.jp>
>> To: Rich Felker <dalias@libc.org>
>> To: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>> To: David S. Miller <davem@davemloft.net>
>> To: Andreas Larsson <andreas@gaisler.com>
>> To: Thomas Gleixner <tglx@linutronix.de>
>> To: Ingo Molnar <mingo@redhat.com>
>> To: Borislav Petkov <bp@alien8.de>
>> To: Dave Hansen <dave.hansen@linux.intel.com>
>> To: x86@kernel.org
>> To: H. Peter Anvin <hpa@zytor.com>
>> To: Andy Lutomirski <luto@kernel.org>
>> To: Peter Zijlstra <peterz@infradead.org>
>> To: Muchun Song <muchun.song@linux.dev>
>> To: Andrew Morton <akpm@linux-foundation.org>
>> To: Liam R. Howlett <Liam.Howlett@oracle.com>
>> To: Vlastimil Babka <vbabka@suse.cz>
>> To: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> To: Shuah Khan <shuah@kernel.org>
>> Cc: linux-arch@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-alpha@vger.kernel.org
>> Cc: linux-snps-arc@lists.infradead.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-csky@vger.kernel.org
>> Cc: loongarch@lists.linux.dev
>> Cc: linux-mips@vger.kernel.org
>> Cc: linux-parisc@vger.kernel.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-s390@vger.kernel.org
>> Cc: linux-sh@vger.kernel.org
>> Cc: sparclinux@vger.kernel.org
>> Cc: linux-mm@kvack.org
>> Cc: linux-kselftest@vger.kernel.org
>> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>>
>> Changes in v2:
>> - Added much greater detail to cover letter
>> - Removed all code that touched architecture specific code and was able
>> to factor this out into all generic functions, except for flags that
>> needed to be added to vm_unmapped_area_info
>> - Made this an RFC since I have only tested it on riscv and x86
>> - Link to v1: https://lore.kernel.org/r/20240827-patches-below_hint_mmap-v1-0-46ff2eb9022d@rivosinc.com
>>
>> ---
>> Charlie Jenkins (4):
>> mm: Add MAP_BELOW_HINT
>> mm: Add hint and mmap_flags to struct vm_unmapped_area_info
>> mm: Support MAP_BELOW_HINT in vm_unmapped_area()
>> selftests/mm: Create MAP_BELOW_HINT test
>>
>> arch/alpha/kernel/osf_sys.c | 2 ++
>> arch/arc/mm/mmap.c | 3 +++
>> arch/arm/mm/mmap.c | 7 ++++++
>> arch/csky/abiv1/mmap.c | 3 +++
>> arch/loongarch/mm/mmap.c | 3 +++
>> arch/mips/mm/mmap.c | 3 +++
>> arch/parisc/kernel/sys_parisc.c | 3 +++
>> arch/powerpc/mm/book3s64/slice.c | 7 ++++++
>> arch/s390/mm/hugetlbpage.c | 4 ++++
>> arch/s390/mm/mmap.c | 6 ++++++
>> arch/sh/mm/mmap.c | 6 ++++++
>> arch/sparc/kernel/sys_sparc_32.c | 3 +++
>> arch/sparc/kernel/sys_sparc_64.c | 6 ++++++
>> arch/sparc/mm/hugetlbpage.c | 4 ++++
>> arch/x86/kernel/sys_x86_64.c | 6 ++++++
>> arch/x86/mm/hugetlbpage.c | 4 ++++
>> fs/hugetlbfs/inode.c | 4 ++++
>> include/linux/mm.h | 2 ++
>> include/uapi/asm-generic/mman-common.h | 1 +
>> mm/mmap.c | 9 ++++++++
>> tools/include/uapi/asm-generic/mman-common.h | 1 +
>> tools/testing/selftests/mm/Makefile | 1 +
>> tools/testing/selftests/mm/map_below_hint.c | 32 ++++++++++++++++++++++++++++
>> 23 files changed, 120 insertions(+)
>> ---
>> base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
>> change-id: 20240827-patches-below_hint_mmap-b13d79ae1c55
^ permalink raw reply
* Re: [PATCH RFC v2 0/4] mm: Introduce MAP_BELOW_HINT
From: Vlastimil Babka @ 2024-08-29 9:02 UTC (permalink / raw)
To: Charlie Jenkins, Arnd Bergmann, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Vineet Gupta, Russell King, Guo Ren,
Huacai Chen, WANG Xuerui, Thomas Bogendoerfer,
James E.J. Bottomley, Helge Deller, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Alexander Gordeev, Gerald Schaefer, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Sven Schnelle, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz, David S. Miller, Andreas Larsson,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Muchun Song,
Andrew Morton, Liam R. Howlett, Lorenzo Stoakes, Shuah Khan,
Linus Torvalds
Cc: linux-arch, linux-kernel, linux-alpha, linux-snps-arc,
linux-arm-kernel, linux-csky, loongarch, linux-mips, linux-parisc,
linuxppc-dev, linux-s390, linux-sh, sparclinux, linux-mm,
linux-kselftest, Linux API
In-Reply-To: <20240829-patches-below_hint_mmap-v2-0-638a28d9eae0@rivosinc.com>
Such a large recipient list and no linux-api. CC'd, please include it on
future postings.
On 8/29/24 09:15, Charlie Jenkins wrote:
> Some applications rely on placing data in free bits addresses allocated
> by mmap. Various architectures (eg. x86, arm64, powerpc) restrict the
> address returned by mmap to be less than the 48-bit address space,
> unless the hint address uses more than 47 bits (the 48th bit is reserved
> for the kernel address space).
>
> The riscv architecture needs a way to similarly restrict the virtual
> address space. On the riscv port of OpenJDK an error is thrown if
> attempted to run on the 57-bit address space, called sv57 [1]. golang
> has a comment that sv57 support is not complete, but there are some
> workarounds to get it to mostly work [2].
>
> These applications work on x86 because x86 does an implicit 47-bit
> restriction of mmap() address that contain a hint address that is less
> than 48 bits.
>
> Instead of implicitly restricting the address space on riscv (or any
> current/future architecture), a flag would allow users to opt-in to this
> behavior rather than opt-out as is done on other architectures. This is
> desirable because it is a small class of applications that do pointer
> masking.
I doubt it's desirable to have different behavior depending on architecture.
Also you could say it's a small class of applications that need more than 47
bits.
> This flag will also allow seemless compatibility between all
> architectures, so applications like Go and OpenJDK that use bits in a
> virtual address can request the exact number of bits they need in a
> generic way. The flag can be checked inside of vm_unmapped_area() so
> that this flag does not have to be handled individually by each
> architecture.
>
> Link:
> https://github.com/openjdk/jdk/blob/f080b4bb8a75284db1b6037f8c00ef3b1ef1add1/src/hotspot/cpu/riscv/vm_version_riscv.cpp#L79
> [1]
> Link:
> https://github.com/golang/go/blob/9e8ea567c838574a0f14538c0bbbd83c3215aa55/src/runtime/tagptr_64bit.go#L47
> [2]
>
> To: Arnd Bergmann <arnd@arndb.de>
> To: Richard Henderson <richard.henderson@linaro.org>
> To: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> To: Matt Turner <mattst88@gmail.com>
> To: Vineet Gupta <vgupta@kernel.org>
> To: Russell King <linux@armlinux.org.uk>
> To: Guo Ren <guoren@kernel.org>
> To: Huacai Chen <chenhuacai@kernel.org>
> To: WANG Xuerui <kernel@xen0n.name>
> To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> To: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
> To: Helge Deller <deller@gmx.de>
> To: Michael Ellerman <mpe@ellerman.id.au>
> To: Nicholas Piggin <npiggin@gmail.com>
> To: Christophe Leroy <christophe.leroy@csgroup.eu>
> To: Naveen N Rao <naveen@kernel.org>
> To: Alexander Gordeev <agordeev@linux.ibm.com>
> To: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> To: Heiko Carstens <hca@linux.ibm.com>
> To: Vasily Gorbik <gor@linux.ibm.com>
> To: Christian Borntraeger <borntraeger@linux.ibm.com>
> To: Sven Schnelle <svens@linux.ibm.com>
> To: Yoshinori Sato <ysato@users.sourceforge.jp>
> To: Rich Felker <dalias@libc.org>
> To: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> To: David S. Miller <davem@davemloft.net>
> To: Andreas Larsson <andreas@gaisler.com>
> To: Thomas Gleixner <tglx@linutronix.de>
> To: Ingo Molnar <mingo@redhat.com>
> To: Borislav Petkov <bp@alien8.de>
> To: Dave Hansen <dave.hansen@linux.intel.com>
> To: x86@kernel.org
> To: H. Peter Anvin <hpa@zytor.com>
> To: Andy Lutomirski <luto@kernel.org>
> To: Peter Zijlstra <peterz@infradead.org>
> To: Muchun Song <muchun.song@linux.dev>
> To: Andrew Morton <akpm@linux-foundation.org>
> To: Liam R. Howlett <Liam.Howlett@oracle.com>
> To: Vlastimil Babka <vbabka@suse.cz>
> To: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> To: Shuah Khan <shuah@kernel.org>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-alpha@vger.kernel.org
> Cc: linux-snps-arc@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-csky@vger.kernel.org
> Cc: loongarch@lists.linux.dev
> Cc: linux-mips@vger.kernel.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: sparclinux@vger.kernel.org
> Cc: linux-mm@kvack.org
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
>
> Changes in v2:
> - Added much greater detail to cover letter
> - Removed all code that touched architecture specific code and was able
> to factor this out into all generic functions, except for flags that
> needed to be added to vm_unmapped_area_info
> - Made this an RFC since I have only tested it on riscv and x86
> - Link to v1: https://lore.kernel.org/r/20240827-patches-below_hint_mmap-v1-0-46ff2eb9022d@rivosinc.com
>
> ---
> Charlie Jenkins (4):
> mm: Add MAP_BELOW_HINT
> mm: Add hint and mmap_flags to struct vm_unmapped_area_info
> mm: Support MAP_BELOW_HINT in vm_unmapped_area()
> selftests/mm: Create MAP_BELOW_HINT test
>
> arch/alpha/kernel/osf_sys.c | 2 ++
> arch/arc/mm/mmap.c | 3 +++
> arch/arm/mm/mmap.c | 7 ++++++
> arch/csky/abiv1/mmap.c | 3 +++
> arch/loongarch/mm/mmap.c | 3 +++
> arch/mips/mm/mmap.c | 3 +++
> arch/parisc/kernel/sys_parisc.c | 3 +++
> arch/powerpc/mm/book3s64/slice.c | 7 ++++++
> arch/s390/mm/hugetlbpage.c | 4 ++++
> arch/s390/mm/mmap.c | 6 ++++++
> arch/sh/mm/mmap.c | 6 ++++++
> arch/sparc/kernel/sys_sparc_32.c | 3 +++
> arch/sparc/kernel/sys_sparc_64.c | 6 ++++++
> arch/sparc/mm/hugetlbpage.c | 4 ++++
> arch/x86/kernel/sys_x86_64.c | 6 ++++++
> arch/x86/mm/hugetlbpage.c | 4 ++++
> fs/hugetlbfs/inode.c | 4 ++++
> include/linux/mm.h | 2 ++
> include/uapi/asm-generic/mman-common.h | 1 +
> mm/mmap.c | 9 ++++++++
> tools/include/uapi/asm-generic/mman-common.h | 1 +
> tools/testing/selftests/mm/Makefile | 1 +
> tools/testing/selftests/mm/map_below_hint.c | 32 ++++++++++++++++++++++++++++
> 23 files changed, 120 insertions(+)
> ---
> base-commit: 5be63fc19fcaa4c236b307420483578a56986a37
> change-id: 20240827-patches-below_hint_mmap-b13d79ae1c55
^ permalink raw reply
* [PATCH xfstests v1 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-08-28 10:37 UTC (permalink / raw)
To: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan
Cc: Aleksa Sarai, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240828103706.2393267-1-cyphar@cyphar.com>
Now that open_by_handle_at(2) can return u64 mount IDs, do some tests to
make sure they match properly as part of the regular open_by_handle
tests.
Link: https://lore.kernel.org/all/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
src/open_by_handle.c | 123 ++++++++++++++++++++++++++++++++-----------
tests/generic/426 | 1 +
2 files changed, 93 insertions(+), 31 deletions(-)
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index d9c802ca9bd1..cbd68aeadac1 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -86,10 +86,15 @@ Examples:
#include <errno.h>
#include <linux/limits.h>
#include <libgen.h>
+#include <stdint.h>
#include <sys/stat.h>
#include "statx.h"
+#ifndef AT_HANDLE_MNT_ID_UNIQUE
+# define AT_HANDLE_MNT_ID_UNIQUE 0x001
+#endif
+
#define MAXFILES 1024
struct handle {
@@ -99,7 +104,7 @@ struct handle {
void usage(void)
{
- fprintf(stderr, "usage: open_by_handle [-cludmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
+ fprintf(stderr, "usage: open_by_handle [-cludMmrwapknhs] [<-i|-o> <handles_file>] <test_dir> [num_files]\n");
fprintf(stderr, "\n");
fprintf(stderr, "open_by_handle -c <test_dir> [N] - create N test files under test_dir, try to get file handles and exit\n");
fprintf(stderr, "open_by_handle <test_dir> [N] - get file handles of test files, drop caches and try to open by handle\n");
@@ -111,6 +116,7 @@ void usage(void)
fprintf(stderr, "open_by_handle -l <test_dir> [N] - create hardlinks to test files, drop caches and try to open by handle\n");
fprintf(stderr, "open_by_handle -u <test_dir> [N] - unlink (hardlinked) test files, drop caches and try to open by handle\n");
fprintf(stderr, "open_by_handle -d <test_dir> [N] - unlink test files and hardlinks, drop caches and try to open by handle\n");
+ fprintf(stderr, "open_by_handle -M <test_dir> [N] - confirm that the mount id returned by name_to_handle_at matches the mount id in statx\n");
fprintf(stderr, "open_by_handle -m <test_dir> [N] - rename test files, drop caches and try to open by handle\n");
fprintf(stderr, "open_by_handle -p <test_dir> - create/delete and try to open by handle also test_dir itself\n");
fprintf(stderr, "open_by_handle -i <handles_file> <test_dir> [N] - read test files handles from file and try to open by handle\n");
@@ -120,6 +126,81 @@ void usage(void)
exit(EXIT_FAILURE);
}
+int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz,
+ int checkmountid)
+{
+ int ret;
+ int mntid_short;
+
+ uint64_t mntid_unique;
+ uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+ struct handle dummy_fh;
+
+ if (checkmountid) {
+ struct statx statxbuf;
+
+ /* Get both the short and unique mount id. */
+ if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+ fprintf(stderr, "%s: no STATX_MNT_ID in stx_mask\n", fname);
+ return EXIT_FAILURE;
+ }
+ statx_mntid_short = statxbuf.stx_mnt_id;
+
+ if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID_UNIQUE, &statxbuf) < 0) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
+ fprintf(stderr, "%s: no STATX_MNT_ID_UNIQUE in stx_mask\n", fname);
+ return EXIT_FAILURE;
+ }
+ statx_mntid_unique = statxbuf.stx_mnt_id;
+ }
+
+ fh->handle_bytes = bufsz;
+ ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+ if (bufsz < fh->handle_bytes) {
+ /* Query the filesystem required bufsz and the file handle */
+ if (ret != -1 || errno != EOVERFLOW) {
+ fprintf(stderr, "%s: unexpected result from name_to_handle_at: %d (%m)\n", fname, ret);
+ return EXIT_FAILURE;
+ }
+ ret = name_to_handle_at(AT_FDCWD, fname, fh, &mntid_short, 0);
+ }
+ if (ret < 0) {
+ fprintf(stderr, "%s: name_to_handle: %m\n", fname);
+ return EXIT_FAILURE;
+ }
+
+ if (checkmountid) {
+ if (mntid_short != (int) statx_mntid_short) {
+ fprintf(stderr, "%s: name_to_handle_at returned a different mount ID to STATX_MNT_ID: %u != %lu\n", fname, mntid_short, statx_mntid_short);
+ return EXIT_FAILURE;
+ }
+
+ /*
+ * Get the unique mount ID. We don't need to get another copy of the
+ * handle so store it in a dummy struct.
+ */
+ dummy_fh.fh.handle_bytes = fh->handle_bytes;
+ if (name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE) < 0) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+
+ if (mntid_unique != statx_mntid_unique) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) returned a different mount ID to STATX_MNT_ID_UNIQUE: %lu != %lu\n", fname, mntid_unique, statx_mntid_unique);
+ return EXIT_FAILURE;
+ }
+ }
+
+ return 0;
+}
+
int main(int argc, char **argv)
{
int i, c;
@@ -132,19 +213,20 @@ int main(int argc, char **argv)
char fname2[PATH_MAX];
char *test_dir;
char *mount_dir;
- int mount_fd, mount_id;
+ int mount_fd;
char *infile = NULL, *outfile = NULL;
int in_fd = 0, out_fd = 0;
int numfiles = 1;
int create = 0, delete = 0, nlink = 1, move = 0;
int rd = 0, wr = 0, wrafter = 0, parent = 0;
int keepopen = 0, drop_caches = 1, sleep_loop = 0;
+ int checkmountid = 0;
int bufsz = MAX_HANDLE_SZ;
if (argc < 2)
usage();
- while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
+ while ((c = getopt(argc, argv, "cludMmrwapknhi:o:sz")) != -1) {
switch (c) {
case 'c':
create = 1;
@@ -172,6 +254,9 @@ int main(int argc, char **argv)
delete = 1;
nlink = 0;
break;
+ case 'M':
+ checkmountid = 1;
+ break;
case 'm':
move = 1;
break;
@@ -307,21 +392,9 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- handle[i].fh.handle_bytes = bufsz;
- ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
- if (bufsz < handle[i].fh.handle_bytes) {
- /* Query the filesystem required bufsz and the file handle */
- if (ret != -1 || errno != EOVERFLOW) {
- fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", fname);
- return EXIT_FAILURE;
- }
- ret = name_to_handle_at(AT_FDCWD, fname, &handle[i].fh, &mount_id, 0);
- }
- if (ret < 0) {
- strcat(fname, ": name_to_handle");
- perror(fname);
+ ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, checkmountid);
+ if (ret < 0)
return EXIT_FAILURE;
- }
}
if (keepopen) {
/* Open without close to keep unlinked files around */
@@ -349,21 +422,9 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- dir_handle.fh.handle_bytes = bufsz;
- ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
- if (bufsz < dir_handle.fh.handle_bytes) {
- /* Query the filesystem required bufsz and the file handle */
- if (ret != -1 || errno != EOVERFLOW) {
- fprintf(stderr, "Unexpected result from name_to_handle_at(%s)\n", dname);
- return EXIT_FAILURE;
- }
- ret = name_to_handle_at(AT_FDCWD, test_dir, &dir_handle.fh, &mount_id, 0);
- }
- if (ret < 0) {
- strcat(dname, ": name_to_handle");
- perror(dname);
+ ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, checkmountid);
+ if (ret < 0)
return EXIT_FAILURE;
- }
}
if (out_fd) {
ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
diff --git a/tests/generic/426 b/tests/generic/426
index 25909f220e1e..df481c58562c 100755
--- a/tests/generic/426
+++ b/tests/generic/426
@@ -51,6 +51,7 @@ test_file_handles $testdir -d
# Check non-stale handles to linked files
create_test_files $testdir
test_file_handles $testdir
+test_file_handles $testdir -M
# Check non-stale handles to files that were hardlinked and original deleted
create_test_files $testdir
--
2.46.0
^ permalink raw reply related
* [PATCH xfstests v1 1/2] statx: update headers to include newer statx fields
From: Aleksa Sarai @ 2024-08-28 10:37 UTC (permalink / raw)
To: fstests, Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Liang, Kan
Cc: Aleksa Sarai, Christoph Hellwig, Josef Bacik, linux-fsdevel,
linux-nfs, linux-kernel, linux-api, linux-perf-users
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com>
These come from Linux v6.11-rc5.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
src/open_by_handle.c | 4 +++-
src/statx.h | 22 ++++++++++++++++++++--
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..d9c802ca9bd1 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -82,12 +82,14 @@ Examples:
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
-#include <sys/stat.h>
#include <sys/types.h>
#include <errno.h>
#include <linux/limits.h>
#include <libgen.h>
+#include <sys/stat.h>
+#include "statx.h"
+
#define MAXFILES 1024
struct handle {
diff --git a/src/statx.h b/src/statx.h
index 3f239d791dfe..935cb2ed415e 100644
--- a/src/statx.h
+++ b/src/statx.h
@@ -102,7 +102,7 @@ struct statx {
__u64 stx_ino; /* Inode number */
__u64 stx_size; /* File size */
__u64 stx_blocks; /* Number of 512-byte blocks allocated */
- __u64 __spare1[1];
+ __u64 stx_attributes_mask; /* Mask to show what's supported in stx_attributes */
/* 0x40 */
struct statx_timestamp stx_atime; /* Last access time */
struct statx_timestamp stx_btime; /* File creation time */
@@ -114,7 +114,18 @@ struct statx {
__u32 stx_dev_major; /* ID of device containing file [uncond] */
__u32 stx_dev_minor;
/* 0x90 */
- __u64 __spare2[14]; /* Spare space for future expansion */
+ __u64 stx_mnt_id;
+ __u32 stx_dio_mem_align; /* Memory buffer alignment for direct I/O */
+ __u32 stx_dio_offset_align; /* File offset alignment for direct I/O */
+ /* 0xa0 */
+ __u64 stx_subvol; /* Subvolume identifier */
+ __u32 stx_atomic_write_unit_min; /* Min atomic write unit in bytes */
+ __u32 stx_atomic_write_unit_max; /* Max atomic write unit in bytes */
+ /* 0xb0 */
+ __u32 stx_atomic_write_segments_max; /* Max atomic write segment count */
+ __u32 __spare1[1];
+ /* 0xb8 */
+ __u64 __spare3[9]; /* Spare space for future expansion */
/* 0x100 */
};
@@ -139,6 +150,13 @@ struct statx {
#define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */
#define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */
#define STATX_BTIME 0x00000800U /* Want/got stx_btime */
+#define STATX_MNT_ID 0x00001000U /* Got stx_mnt_id */
+#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */
+#define STATX_MNT_ID_UNIQUE 0x00004000U /* Want/got extended stx_mount_id */
+#define STATX_SUBVOL 0x00008000U /* Want/got stx_subvol */
+#define STATX_WRITE_ATOMIC 0x00010000U /* Want/got atomic_write_* fields */
+
+#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
#define STATX_ALL 0x00000fffU /* All currently supported flags */
/*
--
2.46.0
^ permalink raw reply related
* [PATCH RESEND v3 2/2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Aleksa Sarai @ 2024-08-28 10:19 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter
Cc: Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-nfs,
linux-kernel, linux-api, linux-perf-users, Aleksa Sarai
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com>
Now that we provide a unique 64-bit mount ID interface in statx(2), we
can now provide a race-free way for name_to_handle_at(2) to provide a
file handle and corresponding mount without needing to worry about
racing with /proc/mountinfo parsing or having to open a file just to do
statx(2).
While this is not necessary if you are using AT_EMPTY_PATH and don't
care about an extra statx(2) call, users that pass full paths into
name_to_handle_at(2) need to know which mount the file handle comes from
(to make sure they don't try to open_by_handle_at a file handle from a
different filesystem) and switching to AT_EMPTY_PATH would require
allocating a file for every name_to_handle_at(2) call, turning
err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
AT_HANDLE_MNT_ID_UNIQUE);
into
int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
mntid = statxbuf.stx_mnt_id;
close(fd);
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fhandle.c | 29 ++++++++++++++++------
include/linux/syscalls.h | 2 +-
include/uapi/linux/fcntl.h | 1 +
tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 1 +
4 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/fs/fhandle.c b/fs/fhandle.c
index 6e8cea16790e..8cb665629f4a 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -16,7 +16,8 @@
static long do_sys_name_to_handle(const struct path *path,
struct file_handle __user *ufh,
- int __user *mnt_id, int fh_flags)
+ void __user *mnt_id, bool unique_mntid,
+ int fh_flags)
{
long retval;
struct file_handle f_handle;
@@ -69,9 +70,19 @@ static long do_sys_name_to_handle(const struct path *path,
} else
retval = 0;
/* copy the mount id */
- if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
- copy_to_user(ufh, handle,
- struct_size(handle, f_handle, handle_bytes)))
+ if (unique_mntid) {
+ if (put_user(real_mount(path->mnt)->mnt_id_unique,
+ (u64 __user *) mnt_id))
+ retval = -EFAULT;
+ } else {
+ if (put_user(real_mount(path->mnt)->mnt_id,
+ (int __user *) mnt_id))
+ retval = -EFAULT;
+ }
+ /* copy the handle */
+ if (retval != -EFAULT &&
+ copy_to_user(ufh, handle,
+ struct_size(handle, f_handle, handle_bytes)))
retval = -EFAULT;
kfree(handle);
return retval;
@@ -83,6 +94,7 @@ static long do_sys_name_to_handle(const struct path *path,
* @name: name that should be converted to handle.
* @handle: resulting file handle
* @mnt_id: mount id of the file system containing the file
+ * (u64 if AT_HANDLE_MNT_ID_UNIQUE, otherwise int)
* @flag: flag value to indicate whether to follow symlink or not
* and whether a decodable file handle is required.
*
@@ -92,7 +104,7 @@ static long do_sys_name_to_handle(const struct path *path,
* value required.
*/
SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
- struct file_handle __user *, handle, int __user *, mnt_id,
+ struct file_handle __user *, handle, void __user *, mnt_id,
int, flag)
{
struct path path;
@@ -100,7 +112,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
int fh_flags;
int err;
- if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
+ if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
+ AT_HANDLE_MNT_ID_UNIQUE))
return -EINVAL;
lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
@@ -109,7 +122,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
lookup_flags |= LOOKUP_EMPTY;
err = user_path_at(dfd, name, lookup_flags, &path);
if (!err) {
- err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
+ err = do_sys_name_to_handle(&path, handle, mnt_id,
+ flag & AT_HANDLE_MNT_ID_UNIQUE,
+ fh_flags);
path_put(&path);
}
return err;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 4bcf6754738d..5758104921e6 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -870,7 +870,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
#endif
asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
struct file_handle __user *handle,
- int __user *mnt_id, int flag);
+ void __user *mnt_id, int flag);
asmlinkage long sys_open_by_handle_at(int mountdirfd,
struct file_handle __user *handle,
int flags);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 38a6d66d9e88..87e2dec79fea 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -152,6 +152,7 @@
#define AT_HANDLE_FID 0x200 /* File handle is needed to compare
object identity and may not be
usable with open_by_handle_at(2). */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
index 38a6d66d9e88..87e2dec79fea 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
@@ -152,6 +152,7 @@
#define AT_HANDLE_FID 0x200 /* File handle is needed to compare
object identity and may not be
usable with open_by_handle_at(2). */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
--
2.46.0
^ permalink raw reply related
* [PATCH RESEND v3 1/2] uapi: explain how per-syscall AT_* flags should be allocated
From: Aleksa Sarai @ 2024-08-28 10:19 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter
Cc: Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-nfs,
linux-kernel, linux-api, linux-perf-users, Aleksa Sarai
In-Reply-To: <20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com>
Unfortunately, the way we have gone about adding new AT_* flags has
been a little messy. In the beginning, all of the AT_* flags had generic
meanings and so it made sense to share the flag bits indiscriminately.
However, we inevitably ran into syscalls that needed their own
syscall-specific flags. Due to the lack of a planned out policy, we
ended up with the following situations:
* Existing syscalls adding new features tended to use new AT_* bits,
with some effort taken to try to re-use bits for flags that were so
obviously syscall specific that they only make sense for a single
syscall (such as the AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID triplet).
Given the constraints of bitflags, this works well in practice, but
ideally (to avoid future confusion) we would plan ahead and define a
set of "per-syscall bits" ahead of time so that when allocating new
bits we don't end up with a complete mish-mash of which bits are
supposed to be per-syscall and which aren't.
* New syscalls dealt with this in several ways:
- Some syscalls (like renameat2(2), move_mount(2), fsopen(2), and
fspick(2)) created their separate own flag spaces that have no
overlap with the AT_* flags. Most of these ended up allocating
their bits sequentually.
In the case of move_mount(2) and fspick(2), several flags have
identical meanings to AT_* flags but were allocated in their own
flag space.
This makes sense for syscalls that will never share AT_* flags, but
for some syscalls this leads to duplication with AT_* flags in a
way that could cause confusion (if renameat2(2) grew a
RENAME_EMPTY_PATH it seems likely that users could mistake it for
AT_EMPTY_PATH since it is an *at(2) syscall).
- Some syscalls unfortunately ended up both creating their own flag
space while also using bits from other flag spaces. The most
obvious example is open_tree(2), where the standard usage ends up
using flags from *THREE* separate flag spaces:
open_tree(AT_FDCWD, "/foo", OPEN_TREE_CLONE|O_CLOEXEC|AT_RECURSIVE);
(Note that O_CLOEXEC is also platform-specific, so several future
OPEN_TREE_* bits are also made unusable in one fell swoop.)
It's not entirely clear to me what the "right" choice is for new
syscalls. Just saying that all future VFS syscalls should use AT_* flags
doesn't seem practical. openat2(2) has RESOLVE_* flags (many of which
don't make much sense to burn generic AT_* flags for) and move_mount(2)
has separate AT_*-like flags for both the source and target so separate
flags are needed anyway (though it seems possible that renameat2(2)
could grow *_EMPTY_PATH flags at some point, and it's a bit of a shame
they can't be reused).
But at least for syscalls that _do_ choose to use AT_* flags, we should
explicitly state the policy that 0x2ff is currently intended for
per-syscall flags and that new flags should err on the side of
overlapping with existing flag bits (so we can extend the scope of
generic flags in the future if necessary).
And add AT_* aliases for the RENAME_* flags to further cement that
renameat2(2) is an *at(2) flag, just with its own per-syscall flags.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
include/uapi/linux/fcntl.h | 80 ++++++++++++++-------
tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 83 +++++++++++++++-------
2 files changed, 115 insertions(+), 48 deletions(-)
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index e55a3314bcb0..38a6d66d9e88 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -90,37 +90,69 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#define AT_FDCWD -100 /* Special value for dirfd used to
+ indicate openat should use the
+ current working directory. */
+
+
+/* Generic flags for the *at(2) family of syscalls. */
+
+/* Reserved for per-syscall flags 0xff. */
+#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic
+ links. */
+/* Reserved for per-syscall flags 0x200 */
+#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
+#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount
+ traversal. */
+#define AT_EMPTY_PATH 0x1000 /* Allow empty relative
+ pathname to operate on dirfd
+ directly. */
/*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
- * unlinkat. The two functions do completely different things and therefore,
- * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
- * faccessat would be undefined behavior and thus treating it equivalent to
- * AT_EACCESS is valid undefined behavior.
+ * These flags are currently statx(2)-specific, but they could be made generic
+ * in the future and so they should not be used for other per-syscall flags.
*/
-#define AT_FDCWD -100 /* Special value used to indicate
- openat should use the current
- working directory. */
-#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
+#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
+#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
+#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
+#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+
+#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+
+/*
+ * Per-syscall flags for the *at(2) family of syscalls.
+ *
+ * These are flags that are so syscall-specific that a user passing these flags
+ * to the wrong syscall is so "clearly wrong" that we can safely call such
+ * usage "undefined behaviour".
+ *
+ * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
+ * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
+ * only to unlinkat. The two functions do completely different things and
+ * therefore, the flags can be allowed to overlap. For example, passing
+ * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
+ * equivalent to AT_EACCESS is valid undefined behavior.
+ *
+ * Note for implementers: When picking a new per-syscall AT_* flag, try to
+ * reuse already existing flags first. This leaves us with as many unused bits
+ * as possible, so we can use them for generic bits in the future if necessary.
+ */
+
+/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
+#define AT_RENAME_NOREPLACE 0x0001
+#define AT_RENAME_EXCHANGE 0x0002
+#define AT_RENAME_WHITEOUT 0x0004
+
+/* Flag for faccessat(2). */
#define AT_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
+/* Flag for unlinkat(2). */
#define AT_REMOVEDIR 0x200 /* Remove directory instead of
unlinking file. */
-#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
-#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
-#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
-
-#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
-#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
-#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
-#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
-
-#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+/* Flags for name_to_handle_at(2). */
+#define AT_HANDLE_FID 0x200 /* File handle is needed to compare
+ object identity and may not be
+ usable with open_by_handle_at(2). */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
-#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
- compare object identity and may not
- be usable to open_by_handle_at(2) */
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
diff --git a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
index c0bcc185fa48..38a6d66d9e88 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/fcntl.h
@@ -16,6 +16,9 @@
#define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3)
+/* Was the file just created? */
+#define F_CREATED_QUERY (F_LINUX_SPECIFIC_BASE + 4)
+
/*
* Cancel a blocking posix lock; internal use only until we expose an
* asynchronous lock api to userspace:
@@ -87,37 +90,69 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#define AT_FDCWD -100 /* Special value for dirfd used to
+ indicate openat should use the
+ current working directory. */
+
+
+/* Generic flags for the *at(2) family of syscalls. */
+
+/* Reserved for per-syscall flags 0xff. */
+#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic
+ links. */
+/* Reserved for per-syscall flags 0x200 */
+#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
+#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount
+ traversal. */
+#define AT_EMPTY_PATH 0x1000 /* Allow empty relative
+ pathname to operate on dirfd
+ directly. */
+/*
+ * These flags are currently statx(2)-specific, but they could be made generic
+ * in the future and so they should not be used for other per-syscall flags.
+ */
+#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
+#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
+#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
+#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+
+#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+
/*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
- * unlinkat. The two functions do completely different things and therefore,
- * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
- * faccessat would be undefined behavior and thus treating it equivalent to
- * AT_EACCESS is valid undefined behavior.
+ * Per-syscall flags for the *at(2) family of syscalls.
+ *
+ * These are flags that are so syscall-specific that a user passing these flags
+ * to the wrong syscall is so "clearly wrong" that we can safely call such
+ * usage "undefined behaviour".
+ *
+ * For example, the constants AT_REMOVEDIR and AT_EACCESS have the same value.
+ * AT_EACCESS is meaningful only to faccessat, while AT_REMOVEDIR is meaningful
+ * only to unlinkat. The two functions do completely different things and
+ * therefore, the flags can be allowed to overlap. For example, passing
+ * AT_REMOVEDIR to faccessat would be undefined behavior and thus treating it
+ * equivalent to AT_EACCESS is valid undefined behavior.
+ *
+ * Note for implementers: When picking a new per-syscall AT_* flag, try to
+ * reuse already existing flags first. This leaves us with as many unused bits
+ * as possible, so we can use them for generic bits in the future if necessary.
*/
-#define AT_FDCWD -100 /* Special value used to indicate
- openat should use the current
- working directory. */
-#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
+
+/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
+#define AT_RENAME_NOREPLACE 0x0001
+#define AT_RENAME_EXCHANGE 0x0002
+#define AT_RENAME_WHITEOUT 0x0004
+
+/* Flag for faccessat(2). */
#define AT_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
+/* Flag for unlinkat(2). */
#define AT_REMOVEDIR 0x200 /* Remove directory instead of
unlinking file. */
-#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
-#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
-#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
-
-#define AT_STATX_SYNC_TYPE 0x6000 /* Type of synchronisation required from statx() */
-#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
-#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
-#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
-
-#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
+/* Flags for name_to_handle_at(2). */
+#define AT_HANDLE_FID 0x200 /* File handle is needed to compare
+ object identity and may not be
+ usable with open_by_handle_at(2). */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
-#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
- compare object identity and may not
- be usable to open_by_handle_at(2) */
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
--
2.46.0
^ permalink raw reply related
* [PATCH RESEND v3 0/2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Aleksa Sarai @ 2024-08-28 10:19 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, Peter Zijlstra,
Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter
Cc: Christoph Hellwig, Josef Bacik, linux-fsdevel, linux-nfs,
linux-kernel, linux-api, linux-perf-users, Aleksa Sarai
Now that we provide a unique 64-bit mount ID interface in statx(2), we
can now provide a race-free way for name_to_handle_at(2) to provide a
file handle and corresponding mount without needing to worry about
racing with /proc/mountinfo parsing or having to open a file just to do
statx(2).
While this is not necessary if you are using AT_EMPTY_PATH and don't
care about an extra statx(2) call, users that pass full paths into
name_to_handle_at(2) need to know which mount the file handle comes from
(to make sure they don't try to open_by_handle_at a file handle from a
different filesystem) and switching to AT_EMPTY_PATH would require
allocating a file for every name_to_handle_at(2) call, turning
err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
AT_HANDLE_MNT_ID_UNIQUE);
into
int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
mntid = statxbuf.stx_mnt_id;
close(fd);
Also, this series adds a patch to clarify how AT_* flag allocation
should work going forwards.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Resend:
- v3: <https://lore.kernel.org/r/20240801-exportfs-u64-mount-id-v3-0-be5d6283144a@cyphar.com>
Changes in v3:
- Added a patch describing how AT_* flags should be allocated in the
future, based on Amir's suggestions.
- Included AT_* aliases for RENAME_* flags to further indicate that
renameat2(2) is an *at(2) syscall and to indicate that those flags
have been allocated already in the per-syscall range.
- Switched AT_HANDLE_MNT_ID_UNIQUE to use 0x01 (to reuse
(AT_)RENAME_NOREPLACE).
- v2: <https://lore.kernel.org/r/20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
Changes in v2:
- Fixed a few minor compiler warnings and a buggy copy_to_user() check.
- Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
- Switched to using an AT_* bit from 0xFF and defining that range as
being "per-syscall" for future usage.
- Sync tools/ copy of <linux/fcntl.h> to include changes.
- v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>
---
Aleksa Sarai (2):
uapi: explain how per-syscall AT_* flags should be allocated
fhandle: expose u64 mount id to name_to_handle_at(2)
fs/fhandle.c | 29 ++++++--
include/linux/syscalls.h | 2 +-
include/uapi/linux/fcntl.h | 81 ++++++++++++++-------
tools/perf/trace/beauty/include/uapi/linux/fcntl.h | 84 +++++++++++++++-------
4 files changed, 140 insertions(+), 56 deletions(-)
---
base-commit: 766508e7e2c5075eb744cb29b8cef6fa835b0344
change-id: 20240515-exportfs-u64-mount-id-9ebb5c58b53c
Best regards,
--
Aleksa Sarai <cyphar@cyphar.com>
^ permalink raw reply
* Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available
From: Charlie Jenkins @ 2024-08-26 16:30 UTC (permalink / raw)
To: Yangyu Chen
Cc: Palmer Dabbelt, rsworktech, Alexandre Ghiti, Paul Walmsley,
Albert Ou, Shuah Khan, Jonathan Corbet, linux-mm, linux-riscv,
Linux Kernel Mailing List, linux-kselftest, linux-doc, linux-api
In-Reply-To: <tencent_0D422E1A03F8C005B3B4331D39709B636809@qq.com>
On Fri, Aug 23, 2024 at 02:55:15PM +0800, Yangyu Chen wrote:
>
>
> > On Aug 23, 2024, at 13:57, Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Fri, Aug 23, 2024 at 01:28:18PM +0800, Yangyu Chen wrote:
> >>
> >>
> >>> On Aug 23, 2024, at 12:39, Charlie Jenkins <charlie@rivosinc.com> wrote:
> >>>
> >>> On Thu, Aug 22, 2024 at 10:51:54AM +0800, Yangyu Chen wrote:
> >>>>
> >>>>
> >>>>> On Aug 22, 2024, at 06:17, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>>>>
> >>>>> On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworktech@outlook.com wrote:
> >>>>>> On 2024-08-20 01:00, Charlie Jenkins wrote:
> >>>>>>> On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
> >>>>>>>> On 2024-03-22 22:06, Palmer Dabbelt wrote:
> >>>>>>>>> On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
> >>>>>>>>>> On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
> >>>>>>>>>>> On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
> >>>>>>>>>>>> On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
> >>>>>>>>>>>>> On riscv it is guaranteed that the address returned by mmap is less
> >>>>>>>>>>>>> than
> >>>>>>>>>>>>> the hint address. Allow mmap to return an address all the way up to
> >>>>>>>>>>>>> addr, if provided, rather than just up to the lower address space.
> >>>>>>>>>>>>>>> This provides a performance benefit as well, allowing
> >>>>>>>>>>> mmap to exit
> >>>>>>>>>>>>> after
> >>>>>>>>>>>>> checking that the address is in range rather than searching for a
> >>>>>>>>>>>>> valid
> >>>>>>>>>>>>> address.
> >>>>>>>>>>>>>>> It is possible to provide an address that uses at most the same
> >>>>>>>>>>>>> number
> >>>>>>>>>>>>> of bits, however it is significantly more computationally expensive
> >>>>>>>>>>>>> to
> >>>>>>>>>>>>> provide that number rather than setting the max to be the hint
> >>>>>>>>>>>>> address.
> >>>>>>>>>>>>> There is the instruction clz/clzw in Zbb that returns the highest
> >>>>>>>>>>>>> set
> >>>>>>>>>>>>> bit
> >>>>>>>>>>>>> which could be used to performantly implement this, but it would
> >>>>>>>>>>>>> still
> >>>>>>>>>>>>> be slower than the current implementation. At worst case, half of
> >>>>>>>>>>>>> the
> >>>>>>>>>>>>> address would not be able to be allocated when a hint address is
> >>>>>>>>>>>>> provided.
> >>>>>>>>>>>>>>> Signed-off-by: Charlie Jenkins<charlie@rivosinc.com>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>> arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
> >>>>>>>>>>>>> -
> >>>>>>>>>>>>> 1 file changed, 11 insertions(+), 16 deletions(-)
> >>>>>>>>>>>>>>> diff --git a/arch/riscv/include/asm/processor.h
> >>>>>>>>>>>>> b/arch/riscv/include/asm/processor.h
> >>>>>>>>>>>>> index f19f861cda54..8ece7a8f0e18 100644
> >>>>>>>>>>>>> --- a/arch/riscv/include/asm/processor.h
> >>>>>>>>>>>>> +++ b/arch/riscv/include/asm/processor.h
> >>>>>>>>>>>>> @@ -14,22 +14,16 @@
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> #include <asm/ptrace.h>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> -#ifdef CONFIG_64BIT
> >>>>>>>>>>>>> -#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
> >>>>>>>>>>>>> -#define STACK_TOP_MAX TASK_SIZE_64
> >>>>>>>>>>>>> -
> >>>>>>>>>>>>> #define arch_get_mmap_end(addr, len, flags) \
> >>>>>>>>>>>>> ({ \
> >>>>>>>>>>>>> unsigned long
> >>>>>>>>>>>>> mmap_end; \
> >>>>>>>>>>>>> typeof(addr) _addr = (addr); \
> >>>>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> >>>>>>>>>>>>> is_compat_task())) \
> >>>>>>>>>>>>> + if ((_addr) == 0 || \
> >>>>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> >>>>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
> >>>>>>>>>>>>> 1))) \
> >>>>>>>>>>>>> mmap_end = STACK_TOP_MAX; \
> >>>>>>>>>>>>> - else if ((_addr) >= VA_USER_SV57) \
> >>>>>>>>>>>>> - mmap_end = STACK_TOP_MAX; \
> >>>>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> >>>>>>>>>>>>> VA_BITS_SV48)) \
> >>>>>>>>>>>>> - mmap_end = VA_USER_SV48; \
> >>>>>>>>>>>>> else \
> >>>>>>>>>>>>> - mmap_end = VA_USER_SV39; \
> >>>>>>>>>>>>> + mmap_end = (_addr + len); \
> >>>>>>>>>>>>> mmap_end; \
> >>>>>>>>>>>>> })
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> @@ -39,17 +33,18 @@
> >>>>>>>>>>>>> typeof(addr) _addr = (addr); \
> >>>>>>>>>>>>> typeof(base) _base = (base); \
> >>>>>>>>>>>>> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
> >>>>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> >>>>>>>>>>>>> is_compat_task())) \
> >>>>>>>>>>>>> + if ((_addr) == 0 || \
> >>>>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> >>>>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
> >>>>>>>>>>>>> 1))) \
> >>>>>>>>>>>>> mmap_base = (_base); \
> >>>>>>>>>>>>> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
> >>>>>>>>>>>>> VA_BITS_SV57)) \
> >>>>>>>>>>>>> - mmap_base = VA_USER_SV57 - rnd_gap; \
> >>>>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> >>>>>>>>>>>>> VA_BITS_SV48)) \
> >>>>>>>>>>>>> - mmap_base = VA_USER_SV48 - rnd_gap; \
> >>>>>>>>>>>>> else \
> >>>>>>>>>>>>> - mmap_base = VA_USER_SV39 - rnd_gap; \
> >>>>>>>>>>>>> + mmap_base = (_addr + len) - rnd_gap; \
> >>>>>>>>>>>>> mmap_base; \
> >>>>>>>>>>>>> })
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +#ifdef CONFIG_64BIT
> >>>>>>>>>>>>> +#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
> >>>>>>>>>>>>> +#define STACK_TOP_MAX TASK_SIZE_64
> >>>>>>>>>>>>> #else
> >>>>>>>>>>>>> #define DEFAULT_MAP_WINDOW TASK_SIZE
> >>>>>>>>>>>>> #define STACK_TOP_MAX TASK_SIZE
> >>>>>>>>>>>>>>> I have carefully tested your patch on qemu with sv57. A
> >>>>>>>>>>> bug that
> >>>>>>>>>>>> needs
> >>>>>>>>>>>> to be solved is that mmap with the same hint address without
> >>>>>>>>>>>> MAP_FIXED
> >>>>>>>>>>>> set will fail the second time.
> >>>>>>>>>>>>> Userspace code to reproduce the bug:
> >>>>>>>>>>>>> #include <sys/mman.h>
> >>>>>>>>>>>> #include <stdio.h>
> >>>>>>>>>>>> #include <stdint.h>
> >>>>>>>>>>>>> void test(char *addr) {
> >>>>>>>>>>>> char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
> >>>>>>>>>>>> MAP_ANONYMOUS
> >>>>>>>>>>>>> MAP_PRIVATE, -1, 0);
> >>>>>>>>>>>> printf("hint %p got %p.\n", addr, res);
> >>>>>>>>>>>> }
> >>>>>>>>>>>>> int main (void) {
> >>>>>>>>>>>> test(1<<30);
> >>>>>>>>>>>> test(1<<30);
> >>>>>>>>>>>> test(1<<30);
> >>>>>>>>>>>> return 0;
> >>>>>>>>>>>> }
> >>>>>>>>>>>>> output:
> >>>>>>>>>>>>> hint 0x40000000 got 0x40000000.
> >>>>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
> >>>>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
> >>>>>>>>>>>>> output on x86:
> >>>>>>>>>>>>> hint 0x40000000 got 0x40000000.
> >>>>>>>>>>>> hint 0x40000000 got 0x7f9171363000.
> >>>>>>>>>>>> hint 0x40000000 got 0x7f9171362000.
> >>>>>>>>>>>>> It may need to implement a special arch_get_unmapped_area and
> >>>>>>>>>>>> arch_get_unmapped_area_topdown function.
> >>>>>>>>>>>>
> >>>>>>>>>>> This is because hint address < rnd_gap. I have tried to let mmap_base =
> >>>>>>>>>>> min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
> >>>>>>>>>>> does not work for bottom-up while ulimit -s is unlimited. You said this
> >>>>>>>>>>> behavior is expected from patch v2 review. However it brings a new
> >>>>>>>>>>> regression even on sv39 systems.
> >>>>>>>>>>>
> >>>>>>>>>>> I still don't know the reason why use addr+len as the upper-bound. I
> >>>>>>>>>>> think solution like x86/arm64/powerpc provide two address space switch
> >>>>>>>>>>> based on whether hint address above the default map window is enough.
> >>>>>>>>>>>
> >>>>>>>>>> Yep this is expected. It is up to the maintainers to decide.
> >>>>>>>>> Sorry I forgot to reply to this, I had a buffer sitting around somewhere
> >>>>>>>>> but I must have lost it.
> >>>>>>>>>
> >>>>>>>>> I think Charlie's approach is the right way to go. Putting my userspace
> >>>>>>>>> hat on, I'd much rather have my allocations fail rather than silently
> >>>>>>>>> ignore the hint when there's memory pressure.
> >>>>>>>>>
> >>>>>>>>> If there's some real use case that needs these low hints to be silently
> >>>>>>>>> ignored under VA pressure then we can try and figure something out that
> >>>>>>>>> makes those applications work.
> >>>>>>>> I could confirm that this patch has broken chromium's partition allocator on
> >>>>>>>> riscv64. The minimal reproduction I use is chromium-mmap.c:
> >>>>>>>>
> >>>>>>>> #include <stdio.h>
> >>>>>>>> #include <sys/mman.h>
> >>>>>>>>
> >>>>>>>> int main() {
> >>>>>>>> void* expected = (void*)0x400000000;
> >>>>>>>> void* addr = mmap(expected, 17179869184, PROT_NONE,
> >>>>>>>> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> >>>>>>>> if (addr != expected) {
> >>>>>>> It is not valid to assume that the address returned by mmap will be the
> >>>>>>> hint address. If the hint address is not available, mmap will return a
> >>>>>>> different address.
> >>>>>>
> >>>>>> Oh, sorry I didn't make it clear what is the expected behavior.
> >>>>>> The printf here is solely for debugging purpose and I don't mean that
> >>>>>> chromium expect it will get the hint address. The expected behavior is
> >>>>>> that both the two mmap calls will succeed.
> >>>>>>
> >>>>>>>> printf("Not expected address: %p != %p\n", addr, expected);
> >>>>>>>> }
> >>>>>>>> expected = (void*)0x3fffff000;
> >>>>>>>> addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
> >>>>>>>> -1, 0);
> >>>>>>>> if (addr != expected) {
> >>>>>>>> printf("Not expected address: %p != %p\n", addr, expected);
> >>>>>>>> }
> >>>>>>>> return 0;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> The second mmap fails with ENOMEM. Manually reverting this commit fixes the
> >>>>>>>> issue for me. So I think it's clearly a regression and breaks userspace.
> >>>>>>>>
> >>>>>>> The issue here is that overlapping memory is being requested. This
> >>>>>>> second mmap will never be able to provide an address at 0x3fffff000 with
> >>>>>>> a size of 0x400001000 since mmap just provided an address at 0x400000000
> >>>>>>> with a size of 0x400000000.
> >>>>>>>
> >>>>>>> Before this patch, this request causes mmap to return a completely
> >>>>>>> arbitrary value. There is no reason to use a hint address in this manner
> >>>>>>> because the hint can never be respected. Since an arbitrary address is
> >>>>>>> desired, a hint of zero should be used.
> >>>>>>>
> >>>>>>> This patch causes the behavior to be more deterministic. Instead of
> >>>>>>> providing an arbitrary address, it causes the address to be less than or
> >>>>>>> equal to the hint address. This allows for applications to make
> >>>>>>> assumptions about the returned address.
> >>>>>>
> >>>>>> About the overlap, of course the partition allocator's request for
> >>>>>> overlapped vma seems unreasonable.
> >>>>>>
> >>>>>> But I still don't quite understand why mmap cannot use an address higher
> >>>>>> than the hint address.
> >>>>>> The hint address, after all, is a hint, not a requirement.
> >>>>>>
> >>>>>> Quoting the man page:
> >>>>>>
> >>>>>>> If another mapping already exists there, the kernel picks
> >>>>>>> a new address that may or may not depend on the hint. The
> >>>>>>> address of the new mapping is returned as the result of the call.
> >>>>>>
> >>>>>> So for casual programmers that only reads man page but not architecture
> >>>>>> specific kernel documentation, the current behavior of mmap on riscv64
> >>>>>> failing on overlapped address ranges are quite surprising IMO.
> >>>>>>
> >>>>>> And quoting the man page again about the errno:
> >>>>>>
> >>>>>>> ENOMEM No memory is available.
> >>>>>>>
> >>>>>>> ENOMEM The process's maximum number of mappings would have been
> >>>>>>> exceeded. This error can also occur for munmap(), when
> >>>>>>> unmapping a region in the middle of an existing mapping,
> >>>>>>> since this results in two smaller mappings on either side
> >>>>>>> of the region being unmapped.
> >>>>>>>
> >>>>>>> ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
> >>>>>>> described in getrlimit(2), would have been exceeded.
> >>>>>>>
> >>>>>>> ENOMEM We don't like addr, because it exceeds the virtual address
> >>>>>>> space of the CPU.
> >>>>>>>
> >>>>>>
> >>>>>> There's no matching description for the ENOMEM returned here.
> >>>>>> I would suggest removing "because it exceeds the virtual address
> >>>>>> space of the CPU." from the last item if the ENOMEM behavior here
> >>>>>> is expected.
> >>>>>>
> >>>>>>> This code is unfortunately relying on the previously mostly undefined
> >>>>>>> behavior of the hint address in mmap.
> >>>>>>
> >>>>>> Although I haven't read the code of chromium's partition allocator to
> >>>>>> judge whether it should
> >>>>>> be improved or fixed for riscv64, I do know that the kernel "don't break
> >>>>>> userspace" and "never EVER blame the user programs".
> >>>>>
> >>>>> Ya, sorry for breaking stuff.
> >>>>>
> >>>>> The goal here was to move to the mmap flag behavor similar to what arm64 and x86 have, as that was done in a way that didn't appear to break userspace -- or at least any real userspace programs. IIRC that first test was pretty broken (it actually depended on the hint address), but sounds like that's not the case.
> >>>>>
> >>>>> I think maybe this is just luck: we didn't chunk the address space up, we're just hinting on every bit, so we're just more likely to hit the exhaustion. Doesn't really matter, though, as if it's breaking stuff so we've got to deal with it.
> >>>>>
> >>>>> Charlie and I are just talking, and best we can come up with is to move to the behavior where we fall back to larger allocation regions when there's no space in the smaller allocation region.
> >>>>
> >>>>
> >>>> For this solution, the only difference from the mmap behavior of
> >>>> x86 and aarch64 is that we will first try to allocate some memory
> >>>> from an address less or equal to the request address + size. But
> >>>> for most cases, I think there is no need to do that, especially for
> >>>> those addresses < BIT(47), as most program works fine on x86-64,
> >>>> which has 47bit available userspace address space to use. And for
> >>>> that program that wants an address < BIT(32), we already have
> >>>> MAP_32BIT now.
> >>>>
> >>>> I think we can just fix like that patch:
> >>>> https://lore.kernel.org/lkml/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
> >>>
> >>> This patch does not satisfy the requirement of having the ability to guarantee
> >>> that mmap returns an address that is less than the hint address.
> >>
> >> Indeed. My intuition is to remove it and align it with x86 and aarch64.
> >>
> >>> This
> >>> patch only allows an address to be less than the DEFAULT_MAP_WINDOW
> >>> which is 32 bits on sv32, 39 bits on sv39, and 48 bits on sv48 or sv57.
> >>>
> >>> This patch also again falls into the trap of using the hint address to
> >>> forcefully restrict the address space.
> >>
> >> Indeed. However, x86 and aarch64 also use this behavior to restrict
> >> va >= BIT(47) by default unless we have the hint address larger
> >> than BIT(47).
> >>
> >>> I agree with Levi that it is not
> >>> very good behavior to have a "hint" cause mmap to fail if conforming to
> >>> the hint isn't possible. Instead, I believe it to be more logical to try
> >>> to allocate at the hint address, otherwise give a random address.
> >>>
> >>
> >> I also agree with this.
> >>
> >>> The current behavior can then be maintained through the flag
> >>> MAP_BELOW_HINT. This way the user explicitly selects that they want mmap
> >>> to fail if an address could not be found within the hint address
> >>> constraints.
> >>>
> >>
> >> I think restricting the addresses with the MAP_BELOW_HINT flag
> >> would be the best choice. However, it remains a problem: What should
> >> the behavior be when there is no MAP_BELOW_HINT? I think we can
> >> fallback to Sv48 on the Sv57 machine by default to align with x86
> >> and aarch64.
> >
> > Although that is the behavior on other architectures, I am hesitant to
> > follow it because it is a somewhat arbitrary restriction. With a generic
> > flag that can force mmap to provide exactly the number of bits that an
> > application needs, there is no need for this restriction on riscv. It
> > may cause problems for applications running on sv57 hardware, however:
> >
> > 1. sv57 hardware does not exist yet
> >
>
> Note that we have QEMU, which uses Sv57 by default. If the mmap
> returns an address >= BIT(47) with hint address == NULL, many QEMU
> users, such as some distro package builders, may need to deal with
> some problems.
Yes that is true. However that was an existing problem before any of
these patches went in. I think the best solution is to have an explicit
flag as you have suggested with similar behavor to MAP_32BIT to solve
this.
- Charlie
>
> > 2. A hint address would still be required if following the same behavior
> > as other architectures.
> > a. It would aid in the porting of an application to sv57
> > hardware, but I am not sure that forcing this restriction is
> > worth having this one piece of parity. Applications using the
> > proposed generic flag would work as expected on all
> > architectures as well.
> >
> >>
> >>> - Charlie
> >>>
> >>>>
> >>>>> Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
> >>>>>
> >>>>>>> The goal of this patch is to help
> >>>>>>> developers have more consistent mmap behavior, but maybe it is necessary
> >>>>>>> to hide this behavior behind an mmap flag.
> >>>>>>
> >>>>>> Thank you for helping to shape a more consistent mmap behavior.
> >>>>>> I think this should be fixed ASAP either by allowing the hint address to
> >>>>>> be ignored
> >>>>>> (as suggested by the Linux man page), or hide this behavior behind an
> >>>>>> mmap flag as you said.
> >>>>>>
> >>>>>>> - Charlie
> >>>>>>>
> >>>>>>>> See alsohttps://github.com/riscv-forks/electron/issues/4
> >>>>>>>>
> >>>>>>>>>> - Charlie
> >>>>>>>> Sincerely,
> >>>>>>>> Levi
> >>>>>>>>
> >>>>>>
> >>>>>> I accidentally introduced some HTML into this reply so this reply is
> >>>>>> resent as plain text.
> >>>>>>
> >>>>>> Sincerely,
> >>>>>> Levi
> >>
> >>
>
^ permalink raw reply
* Re: Recommended version handshake for FUSE clients
From: Miklos Szeredi @ 2024-08-26 12:26 UTC (permalink / raw)
To: Florian Weimer; +Cc: fuse-devel, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <87seurv5hb.fsf@oldenburg.str.redhat.com>
On Mon, 26 Aug 2024 at 14:22, Florian Weimer <fweimer@redhat.com> wrote:
> Or perhaps I should bundle some older version of <linux/fuse.h> with my
> sources?
Bundling the fuse header with your sources is the only sane thing to
do. Libfuse does this, and so should you.
Thanks,
Miklos
^ permalink raw reply
* Recommended version handshake for FUSE clients
From: Florian Weimer @ 2024-08-26 12:22 UTC (permalink / raw)
To: fuse-devel, linux-fsdevel, linux-api, linux-kernel
I cannot use libfuse, so I'm writing my own mini-wrapper directly on top
of the kernel interface. For the major version handshake, I understand
that I need to reply with version 7 if a later major version is
required. But what to do about the minor version?
Should I use the minimum version that is expected to work (because there
have not been any struct layout changes since)? I think this could end
up problematic if some struct sizes have grown between the known-good
minimum version and the <linux/fuse.h> version, something that a future
<linux/fuse.h> version might bring.
Or should I just send back FUSE_KERNEL_MINOR_VERSION from the system
version of <linux/fuse.h>? I think this assumes that merely increasing
the minor version does not result in behavioral changes (such as
additonal FUSE events being generated that the current code knows
nothing about).
The code only has to run on the system that it was compiled on and
possibly newer kernels. Going back to older kernels is not needed. But
I also expect people using a wide range of kernel header versions to
compile this code.
Or perhaps I should bundle some older version of <linux/fuse.h> with my
sources?
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] random: vDSO getrandom() must reject invalid flag
From: Jason A. Donenfeld @ 2024-08-26 6:53 UTC (permalink / raw)
To: Yann Droneaud
Cc: linux-kernel, linux-crypto, linux-api, Theodore Ts'o,
Andy Lutomirski, Thomas Gleixner, Vincenzo Frascino,
Linus Torvalds, Greg Kroah-Hartman, Adhemerval Zanella Netto,
Carlos O'Donell, Florian Weimer, Arnd Bergmann, Jann Horn,
Christian Brauner, David Hildenbrand
In-Reply-To: <20240825144758.325298-1-yann@droneaud.fr>
Hi Yann,
On Sun, Aug 25, 2024 at 04:47:50PM +0200, Yann Droneaud wrote:
> Like getrandom() syscall, vDSO getrandom() must not let
> unknown flags unnoticed [1].
>
> It could be possible to return -EINVAL from vDSO, but
> in the likely case a new flag is added to getrandom()
> syscall in the future, it would be nicer to get the
> behavior from the syscall, instead of an error until
> the vDSO is extended to support the new flag.
Thanks, that seems right to me.
Currently the @flags only matter if the RNG isn't initialized yet, so we
fallback if it's not initialized. But if it is initialized, all of the
flags behave the same way, so it didn't bother checking them. But that
doesn't account for invalid flags, and you're right to point out that
accepting them silently is an API problem.
I've applied this here, and I'll send it in as a fix soon:
https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/commit/?id=ed9fbbeb29
Thanks for the patch,
Jason
^ permalink raw reply
* [PATCH] random: vDSO getrandom() must reject invalid flag
From: Yann Droneaud @ 2024-08-25 14:47 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Yann Droneaud, linux-kernel, linux-crypto, linux-api,
Theodore Ts'o, Andy Lutomirski, Thomas Gleixner,
Vincenzo Frascino, Linus Torvalds, Greg Kroah-Hartman,
Adhemerval Zanella Netto, Carlos O'Donell, Florian Weimer,
Arnd Bergmann, Jann Horn, Christian Brauner, David Hildenbrand
In-Reply-To: <20240712014009.281406-3-Jason@zx2c4.com>
Like getrandom() syscall, vDSO getrandom() must not let
unknown flags unnoticed [1].
It could be possible to return -EINVAL from vDSO, but
in the likely case a new flag is added to getrandom()
syscall in the future, it would be nicer to get the
behavior from the syscall, instead of an error until
the vDSO is extended to support the new flag.
[1] Designing the API: Planning for Extension
https://docs.kernel.org/process/adding-syscalls.html#designing-the-api-planning-for-extension
Signed-off-by: Yann Droneaud <yann@droneaud.fr>
---
lib/vdso/getrandom.c | 4 ++++
1 file changed, 4 insertions(+)
Hi Jason,
Please indulge me as I'm a bit late to add some junk to the conversation[2].
[2] Re: [RFC PATCH 0/4] random: a simple vDSO mechanism for reseeding userspace CSPRNGs
https://lore.kernel.org/all/CAHmME9oXB8=jUz98tv6k1xS+ELaRmgartoT6go_1axhH1L-HJg@mail.gmail.com/
Bye.
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index b230f0b10832..be9db42c8309 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c
@@ -89,6 +89,10 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
if (unlikely(opaque_len != sizeof(*state)))
goto fallback_syscall;
+ /* Unexpected flags are to be handled by the kernel */
+ if (unlikely(flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE)))
+ goto fallback_syscall;
+
/*
* If the kernel's RNG is not yet ready, then it's not possible to provide random bytes from
* userspace, because A) the various @flags require this to block, or not, depending on
--
2.46.0
^ permalink raw reply related
* Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available
From: Yangyu Chen @ 2024-08-23 6:55 UTC (permalink / raw)
To: Charlie Jenkins
Cc: Palmer Dabbelt, rsworktech, Alexandre Ghiti, Paul Walmsley,
Albert Ou, Shuah Khan, Jonathan Corbet, linux-mm, linux-riscv,
Linux Kernel Mailing List, linux-kselftest, linux-doc, linux-api
In-Reply-To: <ZsgkuO5qbnC+79H1@ghost>
> On Aug 23, 2024, at 13:57, Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Aug 23, 2024 at 01:28:18PM +0800, Yangyu Chen wrote:
>>
>>
>>> On Aug 23, 2024, at 12:39, Charlie Jenkins <charlie@rivosinc.com> wrote:
>>>
>>> On Thu, Aug 22, 2024 at 10:51:54AM +0800, Yangyu Chen wrote:
>>>>
>>>>
>>>>> On Aug 22, 2024, at 06:17, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>>>
>>>>> On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworktech@outlook.com wrote:
>>>>>> On 2024-08-20 01:00, Charlie Jenkins wrote:
>>>>>>> On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
>>>>>>>> On 2024-03-22 22:06, Palmer Dabbelt wrote:
>>>>>>>>> On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
>>>>>>>>>> On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
>>>>>>>>>>> On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
>>>>>>>>>>>> On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
>>>>>>>>>>>>> On riscv it is guaranteed that the address returned by mmap is less
>>>>>>>>>>>>> than
>>>>>>>>>>>>> the hint address. Allow mmap to return an address all the way up to
>>>>>>>>>>>>> addr, if provided, rather than just up to the lower address space.
>>>>>>>>>>>>>>> This provides a performance benefit as well, allowing
>>>>>>>>>>> mmap to exit
>>>>>>>>>>>>> after
>>>>>>>>>>>>> checking that the address is in range rather than searching for a
>>>>>>>>>>>>> valid
>>>>>>>>>>>>> address.
>>>>>>>>>>>>>>> It is possible to provide an address that uses at most the same
>>>>>>>>>>>>> number
>>>>>>>>>>>>> of bits, however it is significantly more computationally expensive
>>>>>>>>>>>>> to
>>>>>>>>>>>>> provide that number rather than setting the max to be the hint
>>>>>>>>>>>>> address.
>>>>>>>>>>>>> There is the instruction clz/clzw in Zbb that returns the highest
>>>>>>>>>>>>> set
>>>>>>>>>>>>> bit
>>>>>>>>>>>>> which could be used to performantly implement this, but it would
>>>>>>>>>>>>> still
>>>>>>>>>>>>> be slower than the current implementation. At worst case, half of
>>>>>>>>>>>>> the
>>>>>>>>>>>>> address would not be able to be allocated when a hint address is
>>>>>>>>>>>>> provided.
>>>>>>>>>>>>>>> Signed-off-by: Charlie Jenkins<charlie@rivosinc.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
>>>>>>>>>>>>> -
>>>>>>>>>>>>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>>>>>>>>>>>>>> diff --git a/arch/riscv/include/asm/processor.h
>>>>>>>>>>>>> b/arch/riscv/include/asm/processor.h
>>>>>>>>>>>>> index f19f861cda54..8ece7a8f0e18 100644
>>>>>>>>>>>>> --- a/arch/riscv/include/asm/processor.h
>>>>>>>>>>>>> +++ b/arch/riscv/include/asm/processor.h
>>>>>>>>>>>>> @@ -14,22 +14,16 @@
>>>>>>>>>>>>>
>>>>>>>>>>>>> #include <asm/ptrace.h>
>>>>>>>>>>>>>
>>>>>>>>>>>>> -#ifdef CONFIG_64BIT
>>>>>>>>>>>>> -#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
>>>>>>>>>>>>> -#define STACK_TOP_MAX TASK_SIZE_64
>>>>>>>>>>>>> -
>>>>>>>>>>>>> #define arch_get_mmap_end(addr, len, flags) \
>>>>>>>>>>>>> ({ \
>>>>>>>>>>>>> unsigned long
>>>>>>>>>>>>> mmap_end; \
>>>>>>>>>>>>> typeof(addr) _addr = (addr); \
>>>>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
>>>>>>>>>>>>> is_compat_task())) \
>>>>>>>>>>>>> + if ((_addr) == 0 || \
>>>>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
>>>>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
>>>>>>>>>>>>> 1))) \
>>>>>>>>>>>>> mmap_end = STACK_TOP_MAX; \
>>>>>>>>>>>>> - else if ((_addr) >= VA_USER_SV57) \
>>>>>>>>>>>>> - mmap_end = STACK_TOP_MAX; \
>>>>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
>>>>>>>>>>>>> VA_BITS_SV48)) \
>>>>>>>>>>>>> - mmap_end = VA_USER_SV48; \
>>>>>>>>>>>>> else \
>>>>>>>>>>>>> - mmap_end = VA_USER_SV39; \
>>>>>>>>>>>>> + mmap_end = (_addr + len); \
>>>>>>>>>>>>> mmap_end; \
>>>>>>>>>>>>> })
>>>>>>>>>>>>>
>>>>>>>>>>>>> @@ -39,17 +33,18 @@
>>>>>>>>>>>>> typeof(addr) _addr = (addr); \
>>>>>>>>>>>>> typeof(base) _base = (base); \
>>>>>>>>>>>>> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
>>>>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
>>>>>>>>>>>>> is_compat_task())) \
>>>>>>>>>>>>> + if ((_addr) == 0 || \
>>>>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
>>>>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
>>>>>>>>>>>>> 1))) \
>>>>>>>>>>>>> mmap_base = (_base); \
>>>>>>>>>>>>> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
>>>>>>>>>>>>> VA_BITS_SV57)) \
>>>>>>>>>>>>> - mmap_base = VA_USER_SV57 - rnd_gap; \
>>>>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
>>>>>>>>>>>>> VA_BITS_SV48)) \
>>>>>>>>>>>>> - mmap_base = VA_USER_SV48 - rnd_gap; \
>>>>>>>>>>>>> else \
>>>>>>>>>>>>> - mmap_base = VA_USER_SV39 - rnd_gap; \
>>>>>>>>>>>>> + mmap_base = (_addr + len) - rnd_gap; \
>>>>>>>>>>>>> mmap_base; \
>>>>>>>>>>>>> })
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#ifdef CONFIG_64BIT
>>>>>>>>>>>>> +#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
>>>>>>>>>>>>> +#define STACK_TOP_MAX TASK_SIZE_64
>>>>>>>>>>>>> #else
>>>>>>>>>>>>> #define DEFAULT_MAP_WINDOW TASK_SIZE
>>>>>>>>>>>>> #define STACK_TOP_MAX TASK_SIZE
>>>>>>>>>>>>>>> I have carefully tested your patch on qemu with sv57. A
>>>>>>>>>>> bug that
>>>>>>>>>>>> needs
>>>>>>>>>>>> to be solved is that mmap with the same hint address without
>>>>>>>>>>>> MAP_FIXED
>>>>>>>>>>>> set will fail the second time.
>>>>>>>>>>>>> Userspace code to reproduce the bug:
>>>>>>>>>>>>> #include <sys/mman.h>
>>>>>>>>>>>> #include <stdio.h>
>>>>>>>>>>>> #include <stdint.h>
>>>>>>>>>>>>> void test(char *addr) {
>>>>>>>>>>>> char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
>>>>>>>>>>>> MAP_ANONYMOUS
>>>>>>>>>>>>> MAP_PRIVATE, -1, 0);
>>>>>>>>>>>> printf("hint %p got %p.\n", addr, res);
>>>>>>>>>>>> }
>>>>>>>>>>>>> int main (void) {
>>>>>>>>>>>> test(1<<30);
>>>>>>>>>>>> test(1<<30);
>>>>>>>>>>>> test(1<<30);
>>>>>>>>>>>> return 0;
>>>>>>>>>>>> }
>>>>>>>>>>>>> output:
>>>>>>>>>>>>> hint 0x40000000 got 0x40000000.
>>>>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
>>>>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
>>>>>>>>>>>>> output on x86:
>>>>>>>>>>>>> hint 0x40000000 got 0x40000000.
>>>>>>>>>>>> hint 0x40000000 got 0x7f9171363000.
>>>>>>>>>>>> hint 0x40000000 got 0x7f9171362000.
>>>>>>>>>>>>> It may need to implement a special arch_get_unmapped_area and
>>>>>>>>>>>> arch_get_unmapped_area_topdown function.
>>>>>>>>>>>>
>>>>>>>>>>> This is because hint address < rnd_gap. I have tried to let mmap_base =
>>>>>>>>>>> min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
>>>>>>>>>>> does not work for bottom-up while ulimit -s is unlimited. You said this
>>>>>>>>>>> behavior is expected from patch v2 review. However it brings a new
>>>>>>>>>>> regression even on sv39 systems.
>>>>>>>>>>>
>>>>>>>>>>> I still don't know the reason why use addr+len as the upper-bound. I
>>>>>>>>>>> think solution like x86/arm64/powerpc provide two address space switch
>>>>>>>>>>> based on whether hint address above the default map window is enough.
>>>>>>>>>>>
>>>>>>>>>> Yep this is expected. It is up to the maintainers to decide.
>>>>>>>>> Sorry I forgot to reply to this, I had a buffer sitting around somewhere
>>>>>>>>> but I must have lost it.
>>>>>>>>>
>>>>>>>>> I think Charlie's approach is the right way to go. Putting my userspace
>>>>>>>>> hat on, I'd much rather have my allocations fail rather than silently
>>>>>>>>> ignore the hint when there's memory pressure.
>>>>>>>>>
>>>>>>>>> If there's some real use case that needs these low hints to be silently
>>>>>>>>> ignored under VA pressure then we can try and figure something out that
>>>>>>>>> makes those applications work.
>>>>>>>> I could confirm that this patch has broken chromium's partition allocator on
>>>>>>>> riscv64. The minimal reproduction I use is chromium-mmap.c:
>>>>>>>>
>>>>>>>> #include <stdio.h>
>>>>>>>> #include <sys/mman.h>
>>>>>>>>
>>>>>>>> int main() {
>>>>>>>> void* expected = (void*)0x400000000;
>>>>>>>> void* addr = mmap(expected, 17179869184, PROT_NONE,
>>>>>>>> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
>>>>>>>> if (addr != expected) {
>>>>>>> It is not valid to assume that the address returned by mmap will be the
>>>>>>> hint address. If the hint address is not available, mmap will return a
>>>>>>> different address.
>>>>>>
>>>>>> Oh, sorry I didn't make it clear what is the expected behavior.
>>>>>> The printf here is solely for debugging purpose and I don't mean that
>>>>>> chromium expect it will get the hint address. The expected behavior is
>>>>>> that both the two mmap calls will succeed.
>>>>>>
>>>>>>>> printf("Not expected address: %p != %p\n", addr, expected);
>>>>>>>> }
>>>>>>>> expected = (void*)0x3fffff000;
>>>>>>>> addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
>>>>>>>> -1, 0);
>>>>>>>> if (addr != expected) {
>>>>>>>> printf("Not expected address: %p != %p\n", addr, expected);
>>>>>>>> }
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> The second mmap fails with ENOMEM. Manually reverting this commit fixes the
>>>>>>>> issue for me. So I think it's clearly a regression and breaks userspace.
>>>>>>>>
>>>>>>> The issue here is that overlapping memory is being requested. This
>>>>>>> second mmap will never be able to provide an address at 0x3fffff000 with
>>>>>>> a size of 0x400001000 since mmap just provided an address at 0x400000000
>>>>>>> with a size of 0x400000000.
>>>>>>>
>>>>>>> Before this patch, this request causes mmap to return a completely
>>>>>>> arbitrary value. There is no reason to use a hint address in this manner
>>>>>>> because the hint can never be respected. Since an arbitrary address is
>>>>>>> desired, a hint of zero should be used.
>>>>>>>
>>>>>>> This patch causes the behavior to be more deterministic. Instead of
>>>>>>> providing an arbitrary address, it causes the address to be less than or
>>>>>>> equal to the hint address. This allows for applications to make
>>>>>>> assumptions about the returned address.
>>>>>>
>>>>>> About the overlap, of course the partition allocator's request for
>>>>>> overlapped vma seems unreasonable.
>>>>>>
>>>>>> But I still don't quite understand why mmap cannot use an address higher
>>>>>> than the hint address.
>>>>>> The hint address, after all, is a hint, not a requirement.
>>>>>>
>>>>>> Quoting the man page:
>>>>>>
>>>>>>> If another mapping already exists there, the kernel picks
>>>>>>> a new address that may or may not depend on the hint. The
>>>>>>> address of the new mapping is returned as the result of the call.
>>>>>>
>>>>>> So for casual programmers that only reads man page but not architecture
>>>>>> specific kernel documentation, the current behavior of mmap on riscv64
>>>>>> failing on overlapped address ranges are quite surprising IMO.
>>>>>>
>>>>>> And quoting the man page again about the errno:
>>>>>>
>>>>>>> ENOMEM No memory is available.
>>>>>>>
>>>>>>> ENOMEM The process's maximum number of mappings would have been
>>>>>>> exceeded. This error can also occur for munmap(), when
>>>>>>> unmapping a region in the middle of an existing mapping,
>>>>>>> since this results in two smaller mappings on either side
>>>>>>> of the region being unmapped.
>>>>>>>
>>>>>>> ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
>>>>>>> described in getrlimit(2), would have been exceeded.
>>>>>>>
>>>>>>> ENOMEM We don't like addr, because it exceeds the virtual address
>>>>>>> space of the CPU.
>>>>>>>
>>>>>>
>>>>>> There's no matching description for the ENOMEM returned here.
>>>>>> I would suggest removing "because it exceeds the virtual address
>>>>>> space of the CPU." from the last item if the ENOMEM behavior here
>>>>>> is expected.
>>>>>>
>>>>>>> This code is unfortunately relying on the previously mostly undefined
>>>>>>> behavior of the hint address in mmap.
>>>>>>
>>>>>> Although I haven't read the code of chromium's partition allocator to
>>>>>> judge whether it should
>>>>>> be improved or fixed for riscv64, I do know that the kernel "don't break
>>>>>> userspace" and "never EVER blame the user programs".
>>>>>
>>>>> Ya, sorry for breaking stuff.
>>>>>
>>>>> The goal here was to move to the mmap flag behavor similar to what arm64 and x86 have, as that was done in a way that didn't appear to break userspace -- or at least any real userspace programs. IIRC that first test was pretty broken (it actually depended on the hint address), but sounds like that's not the case.
>>>>>
>>>>> I think maybe this is just luck: we didn't chunk the address space up, we're just hinting on every bit, so we're just more likely to hit the exhaustion. Doesn't really matter, though, as if it's breaking stuff so we've got to deal with it.
>>>>>
>>>>> Charlie and I are just talking, and best we can come up with is to move to the behavior where we fall back to larger allocation regions when there's no space in the smaller allocation region.
>>>>
>>>>
>>>> For this solution, the only difference from the mmap behavior of
>>>> x86 and aarch64 is that we will first try to allocate some memory
>>>> from an address less or equal to the request address + size. But
>>>> for most cases, I think there is no need to do that, especially for
>>>> those addresses < BIT(47), as most program works fine on x86-64,
>>>> which has 47bit available userspace address space to use. And for
>>>> that program that wants an address < BIT(32), we already have
>>>> MAP_32BIT now.
>>>>
>>>> I think we can just fix like that patch:
>>>> https://lore.kernel.org/lkml/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
>>>
>>> This patch does not satisfy the requirement of having the ability to guarantee
>>> that mmap returns an address that is less than the hint address.
>>
>> Indeed. My intuition is to remove it and align it with x86 and aarch64.
>>
>>> This
>>> patch only allows an address to be less than the DEFAULT_MAP_WINDOW
>>> which is 32 bits on sv32, 39 bits on sv39, and 48 bits on sv48 or sv57.
>>>
>>> This patch also again falls into the trap of using the hint address to
>>> forcefully restrict the address space.
>>
>> Indeed. However, x86 and aarch64 also use this behavior to restrict
>> va >= BIT(47) by default unless we have the hint address larger
>> than BIT(47).
>>
>>> I agree with Levi that it is not
>>> very good behavior to have a "hint" cause mmap to fail if conforming to
>>> the hint isn't possible. Instead, I believe it to be more logical to try
>>> to allocate at the hint address, otherwise give a random address.
>>>
>>
>> I also agree with this.
>>
>>> The current behavior can then be maintained through the flag
>>> MAP_BELOW_HINT. This way the user explicitly selects that they want mmap
>>> to fail if an address could not be found within the hint address
>>> constraints.
>>>
>>
>> I think restricting the addresses with the MAP_BELOW_HINT flag
>> would be the best choice. However, it remains a problem: What should
>> the behavior be when there is no MAP_BELOW_HINT? I think we can
>> fallback to Sv48 on the Sv57 machine by default to align with x86
>> and aarch64.
>
> Although that is the behavior on other architectures, I am hesitant to
> follow it because it is a somewhat arbitrary restriction. With a generic
> flag that can force mmap to provide exactly the number of bits that an
> application needs, there is no need for this restriction on riscv. It
> may cause problems for applications running on sv57 hardware, however:
>
> 1. sv57 hardware does not exist yet
>
Note that we have QEMU, which uses Sv57 by default. If the mmap
returns an address >= BIT(47) with hint address == NULL, many QEMU
users, such as some distro package builders, may need to deal with
some problems.
> 2. A hint address would still be required if following the same behavior
> as other architectures.
> a. It would aid in the porting of an application to sv57
> hardware, but I am not sure that forcing this restriction is
> worth having this one piece of parity. Applications using the
> proposed generic flag would work as expected on all
> architectures as well.
>
>>
>>> - Charlie
>>>
>>>>
>>>>> Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
>>>>>
>>>>>>> The goal of this patch is to help
>>>>>>> developers have more consistent mmap behavior, but maybe it is necessary
>>>>>>> to hide this behavior behind an mmap flag.
>>>>>>
>>>>>> Thank you for helping to shape a more consistent mmap behavior.
>>>>>> I think this should be fixed ASAP either by allowing the hint address to
>>>>>> be ignored
>>>>>> (as suggested by the Linux man page), or hide this behavior behind an
>>>>>> mmap flag as you said.
>>>>>>
>>>>>>> - Charlie
>>>>>>>
>>>>>>>> See alsohttps://github.com/riscv-forks/electron/issues/4
>>>>>>>>
>>>>>>>>>> - Charlie
>>>>>>>> Sincerely,
>>>>>>>> Levi
>>>>>>>>
>>>>>>
>>>>>> I accidentally introduced some HTML into this reply so this reply is
>>>>>> resent as plain text.
>>>>>>
>>>>>> Sincerely,
>>>>>> Levi
>>
>>
^ permalink raw reply
* Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available
From: Charlie Jenkins @ 2024-08-23 5:57 UTC (permalink / raw)
To: Yangyu Chen
Cc: Palmer Dabbelt, rsworktech, Alexandre Ghiti, Paul Walmsley,
Albert Ou, Shuah Khan, Jonathan Corbet, linux-mm, linux-riscv,
Linux Kernel Mailing List, linux-kselftest, linux-doc, linux-api
In-Reply-To: <tencent_B65111F737A62A64BBD1900F5F1040DBC805@qq.com>
On Fri, Aug 23, 2024 at 01:28:18PM +0800, Yangyu Chen wrote:
>
>
> > On Aug 23, 2024, at 12:39, Charlie Jenkins <charlie@rivosinc.com> wrote:
> >
> > On Thu, Aug 22, 2024 at 10:51:54AM +0800, Yangyu Chen wrote:
> >>
> >>
> >>> On Aug 22, 2024, at 06:17, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>>
> >>> On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworktech@outlook.com wrote:
> >>>> On 2024-08-20 01:00, Charlie Jenkins wrote:
> >>>>> On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
> >>>>>> On 2024-03-22 22:06, Palmer Dabbelt wrote:
> >>>>>>> On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
> >>>>>>>> On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
> >>>>>>>>> On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
> >>>>>>>>>> On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
> >>>>>>>>>>> On riscv it is guaranteed that the address returned by mmap is less
> >>>>>>>>>>> than
> >>>>>>>>>>> the hint address. Allow mmap to return an address all the way up to
> >>>>>>>>>>> addr, if provided, rather than just up to the lower address space.
> >>>>>>>>>>>>> This provides a performance benefit as well, allowing
> >>>>>>>>> mmap to exit
> >>>>>>>>>>> after
> >>>>>>>>>>> checking that the address is in range rather than searching for a
> >>>>>>>>>>> valid
> >>>>>>>>>>> address.
> >>>>>>>>>>>>> It is possible to provide an address that uses at most the same
> >>>>>>>>>>> number
> >>>>>>>>>>> of bits, however it is significantly more computationally expensive
> >>>>>>>>>>> to
> >>>>>>>>>>> provide that number rather than setting the max to be the hint
> >>>>>>>>>>> address.
> >>>>>>>>>>> There is the instruction clz/clzw in Zbb that returns the highest
> >>>>>>>>>>> set
> >>>>>>>>>>> bit
> >>>>>>>>>>> which could be used to performantly implement this, but it would
> >>>>>>>>>>> still
> >>>>>>>>>>> be slower than the current implementation. At worst case, half of
> >>>>>>>>>>> the
> >>>>>>>>>>> address would not be able to be allocated when a hint address is
> >>>>>>>>>>> provided.
> >>>>>>>>>>>>> Signed-off-by: Charlie Jenkins<charlie@rivosinc.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>> arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
> >>>>>>>>>>> -
> >>>>>>>>>>> 1 file changed, 11 insertions(+), 16 deletions(-)
> >>>>>>>>>>>>> diff --git a/arch/riscv/include/asm/processor.h
> >>>>>>>>>>> b/arch/riscv/include/asm/processor.h
> >>>>>>>>>>> index f19f861cda54..8ece7a8f0e18 100644
> >>>>>>>>>>> --- a/arch/riscv/include/asm/processor.h
> >>>>>>>>>>> +++ b/arch/riscv/include/asm/processor.h
> >>>>>>>>>>> @@ -14,22 +14,16 @@
> >>>>>>>>>>>
> >>>>>>>>>>> #include <asm/ptrace.h>
> >>>>>>>>>>>
> >>>>>>>>>>> -#ifdef CONFIG_64BIT
> >>>>>>>>>>> -#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
> >>>>>>>>>>> -#define STACK_TOP_MAX TASK_SIZE_64
> >>>>>>>>>>> -
> >>>>>>>>>>> #define arch_get_mmap_end(addr, len, flags) \
> >>>>>>>>>>> ({ \
> >>>>>>>>>>> unsigned long
> >>>>>>>>>>> mmap_end; \
> >>>>>>>>>>> typeof(addr) _addr = (addr); \
> >>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> >>>>>>>>>>> is_compat_task())) \
> >>>>>>>>>>> + if ((_addr) == 0 || \
> >>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> >>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
> >>>>>>>>>>> 1))) \
> >>>>>>>>>>> mmap_end = STACK_TOP_MAX; \
> >>>>>>>>>>> - else if ((_addr) >= VA_USER_SV57) \
> >>>>>>>>>>> - mmap_end = STACK_TOP_MAX; \
> >>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> >>>>>>>>>>> VA_BITS_SV48)) \
> >>>>>>>>>>> - mmap_end = VA_USER_SV48; \
> >>>>>>>>>>> else \
> >>>>>>>>>>> - mmap_end = VA_USER_SV39; \
> >>>>>>>>>>> + mmap_end = (_addr + len); \
> >>>>>>>>>>> mmap_end; \
> >>>>>>>>>>> })
> >>>>>>>>>>>
> >>>>>>>>>>> @@ -39,17 +33,18 @@
> >>>>>>>>>>> typeof(addr) _addr = (addr); \
> >>>>>>>>>>> typeof(base) _base = (base); \
> >>>>>>>>>>> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
> >>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> >>>>>>>>>>> is_compat_task())) \
> >>>>>>>>>>> + if ((_addr) == 0 || \
> >>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> >>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
> >>>>>>>>>>> 1))) \
> >>>>>>>>>>> mmap_base = (_base); \
> >>>>>>>>>>> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
> >>>>>>>>>>> VA_BITS_SV57)) \
> >>>>>>>>>>> - mmap_base = VA_USER_SV57 - rnd_gap; \
> >>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> >>>>>>>>>>> VA_BITS_SV48)) \
> >>>>>>>>>>> - mmap_base = VA_USER_SV48 - rnd_gap; \
> >>>>>>>>>>> else \
> >>>>>>>>>>> - mmap_base = VA_USER_SV39 - rnd_gap; \
> >>>>>>>>>>> + mmap_base = (_addr + len) - rnd_gap; \
> >>>>>>>>>>> mmap_base; \
> >>>>>>>>>>> })
> >>>>>>>>>>>
> >>>>>>>>>>> +#ifdef CONFIG_64BIT
> >>>>>>>>>>> +#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
> >>>>>>>>>>> +#define STACK_TOP_MAX TASK_SIZE_64
> >>>>>>>>>>> #else
> >>>>>>>>>>> #define DEFAULT_MAP_WINDOW TASK_SIZE
> >>>>>>>>>>> #define STACK_TOP_MAX TASK_SIZE
> >>>>>>>>>>>>> I have carefully tested your patch on qemu with sv57. A
> >>>>>>>>> bug that
> >>>>>>>>>> needs
> >>>>>>>>>> to be solved is that mmap with the same hint address without
> >>>>>>>>>> MAP_FIXED
> >>>>>>>>>> set will fail the second time.
> >>>>>>>>>>> Userspace code to reproduce the bug:
> >>>>>>>>>>> #include <sys/mman.h>
> >>>>>>>>>> #include <stdio.h>
> >>>>>>>>>> #include <stdint.h>
> >>>>>>>>>>> void test(char *addr) {
> >>>>>>>>>> char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
> >>>>>>>>>> MAP_ANONYMOUS
> >>>>>>>>>>> MAP_PRIVATE, -1, 0);
> >>>>>>>>>> printf("hint %p got %p.\n", addr, res);
> >>>>>>>>>> }
> >>>>>>>>>>> int main (void) {
> >>>>>>>>>> test(1<<30);
> >>>>>>>>>> test(1<<30);
> >>>>>>>>>> test(1<<30);
> >>>>>>>>>> return 0;
> >>>>>>>>>> }
> >>>>>>>>>>> output:
> >>>>>>>>>>> hint 0x40000000 got 0x40000000.
> >>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
> >>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
> >>>>>>>>>>> output on x86:
> >>>>>>>>>>> hint 0x40000000 got 0x40000000.
> >>>>>>>>>> hint 0x40000000 got 0x7f9171363000.
> >>>>>>>>>> hint 0x40000000 got 0x7f9171362000.
> >>>>>>>>>>> It may need to implement a special arch_get_unmapped_area and
> >>>>>>>>>> arch_get_unmapped_area_topdown function.
> >>>>>>>>>>
> >>>>>>>>> This is because hint address < rnd_gap. I have tried to let mmap_base =
> >>>>>>>>> min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
> >>>>>>>>> does not work for bottom-up while ulimit -s is unlimited. You said this
> >>>>>>>>> behavior is expected from patch v2 review. However it brings a new
> >>>>>>>>> regression even on sv39 systems.
> >>>>>>>>>
> >>>>>>>>> I still don't know the reason why use addr+len as the upper-bound. I
> >>>>>>>>> think solution like x86/arm64/powerpc provide two address space switch
> >>>>>>>>> based on whether hint address above the default map window is enough.
> >>>>>>>>>
> >>>>>>>> Yep this is expected. It is up to the maintainers to decide.
> >>>>>>> Sorry I forgot to reply to this, I had a buffer sitting around somewhere
> >>>>>>> but I must have lost it.
> >>>>>>>
> >>>>>>> I think Charlie's approach is the right way to go. Putting my userspace
> >>>>>>> hat on, I'd much rather have my allocations fail rather than silently
> >>>>>>> ignore the hint when there's memory pressure.
> >>>>>>>
> >>>>>>> If there's some real use case that needs these low hints to be silently
> >>>>>>> ignored under VA pressure then we can try and figure something out that
> >>>>>>> makes those applications work.
> >>>>>> I could confirm that this patch has broken chromium's partition allocator on
> >>>>>> riscv64. The minimal reproduction I use is chromium-mmap.c:
> >>>>>>
> >>>>>> #include <stdio.h>
> >>>>>> #include <sys/mman.h>
> >>>>>>
> >>>>>> int main() {
> >>>>>> void* expected = (void*)0x400000000;
> >>>>>> void* addr = mmap(expected, 17179869184, PROT_NONE,
> >>>>>> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> >>>>>> if (addr != expected) {
> >>>>> It is not valid to assume that the address returned by mmap will be the
> >>>>> hint address. If the hint address is not available, mmap will return a
> >>>>> different address.
> >>>>
> >>>> Oh, sorry I didn't make it clear what is the expected behavior.
> >>>> The printf here is solely for debugging purpose and I don't mean that
> >>>> chromium expect it will get the hint address. The expected behavior is
> >>>> that both the two mmap calls will succeed.
> >>>>
> >>>>>> printf("Not expected address: %p != %p\n", addr, expected);
> >>>>>> }
> >>>>>> expected = (void*)0x3fffff000;
> >>>>>> addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
> >>>>>> -1, 0);
> >>>>>> if (addr != expected) {
> >>>>>> printf("Not expected address: %p != %p\n", addr, expected);
> >>>>>> }
> >>>>>> return 0;
> >>>>>> }
> >>>>>>
> >>>>>> The second mmap fails with ENOMEM. Manually reverting this commit fixes the
> >>>>>> issue for me. So I think it's clearly a regression and breaks userspace.
> >>>>>>
> >>>>> The issue here is that overlapping memory is being requested. This
> >>>>> second mmap will never be able to provide an address at 0x3fffff000 with
> >>>>> a size of 0x400001000 since mmap just provided an address at 0x400000000
> >>>>> with a size of 0x400000000.
> >>>>>
> >>>>> Before this patch, this request causes mmap to return a completely
> >>>>> arbitrary value. There is no reason to use a hint address in this manner
> >>>>> because the hint can never be respected. Since an arbitrary address is
> >>>>> desired, a hint of zero should be used.
> >>>>>
> >>>>> This patch causes the behavior to be more deterministic. Instead of
> >>>>> providing an arbitrary address, it causes the address to be less than or
> >>>>> equal to the hint address. This allows for applications to make
> >>>>> assumptions about the returned address.
> >>>>
> >>>> About the overlap, of course the partition allocator's request for
> >>>> overlapped vma seems unreasonable.
> >>>>
> >>>> But I still don't quite understand why mmap cannot use an address higher
> >>>> than the hint address.
> >>>> The hint address, after all, is a hint, not a requirement.
> >>>>
> >>>> Quoting the man page:
> >>>>
> >>>>> If another mapping already exists there, the kernel picks
> >>>>> a new address that may or may not depend on the hint. The
> >>>>> address of the new mapping is returned as the result of the call.
> >>>>
> >>>> So for casual programmers that only reads man page but not architecture
> >>>> specific kernel documentation, the current behavior of mmap on riscv64
> >>>> failing on overlapped address ranges are quite surprising IMO.
> >>>>
> >>>> And quoting the man page again about the errno:
> >>>>
> >>>>> ENOMEM No memory is available.
> >>>>>
> >>>>> ENOMEM The process's maximum number of mappings would have been
> >>>>> exceeded. This error can also occur for munmap(), when
> >>>>> unmapping a region in the middle of an existing mapping,
> >>>>> since this results in two smaller mappings on either side
> >>>>> of the region being unmapped.
> >>>>>
> >>>>> ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
> >>>>> described in getrlimit(2), would have been exceeded.
> >>>>>
> >>>>> ENOMEM We don't like addr, because it exceeds the virtual address
> >>>>> space of the CPU.
> >>>>>
> >>>>
> >>>> There's no matching description for the ENOMEM returned here.
> >>>> I would suggest removing "because it exceeds the virtual address
> >>>> space of the CPU." from the last item if the ENOMEM behavior here
> >>>> is expected.
> >>>>
> >>>>> This code is unfortunately relying on the previously mostly undefined
> >>>>> behavior of the hint address in mmap.
> >>>>
> >>>> Although I haven't read the code of chromium's partition allocator to
> >>>> judge whether it should
> >>>> be improved or fixed for riscv64, I do know that the kernel "don't break
> >>>> userspace" and "never EVER blame the user programs".
> >>>
> >>> Ya, sorry for breaking stuff.
> >>>
> >>> The goal here was to move to the mmap flag behavor similar to what arm64 and x86 have, as that was done in a way that didn't appear to break userspace -- or at least any real userspace programs. IIRC that first test was pretty broken (it actually depended on the hint address), but sounds like that's not the case.
> >>>
> >>> I think maybe this is just luck: we didn't chunk the address space up, we're just hinting on every bit, so we're just more likely to hit the exhaustion. Doesn't really matter, though, as if it's breaking stuff so we've got to deal with it.
> >>>
> >>> Charlie and I are just talking, and best we can come up with is to move to the behavior where we fall back to larger allocation regions when there's no space in the smaller allocation region.
> >>
> >>
> >> For this solution, the only difference from the mmap behavior of
> >> x86 and aarch64 is that we will first try to allocate some memory
> >> from an address less or equal to the request address + size. But
> >> for most cases, I think there is no need to do that, especially for
> >> those addresses < BIT(47), as most program works fine on x86-64,
> >> which has 47bit available userspace address space to use. And for
> >> that program that wants an address < BIT(32), we already have
> >> MAP_32BIT now.
> >>
> >> I think we can just fix like that patch:
> >> https://lore.kernel.org/lkml/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
> >
> > This patch does not satisfy the requirement of having the ability to guarantee
> > that mmap returns an address that is less than the hint address.
>
> Indeed. My intuition is to remove it and align it with x86 and aarch64.
>
> > This
> > patch only allows an address to be less than the DEFAULT_MAP_WINDOW
> > which is 32 bits on sv32, 39 bits on sv39, and 48 bits on sv48 or sv57.
> >
> > This patch also again falls into the trap of using the hint address to
> > forcefully restrict the address space.
>
> Indeed. However, x86 and aarch64 also use this behavior to restrict
> va >= BIT(47) by default unless we have the hint address larger
> than BIT(47).
>
> > I agree with Levi that it is not
> > very good behavior to have a "hint" cause mmap to fail if conforming to
> > the hint isn't possible. Instead, I believe it to be more logical to try
> > to allocate at the hint address, otherwise give a random address.
> >
>
> I also agree with this.
>
> > The current behavior can then be maintained through the flag
> > MAP_BELOW_HINT. This way the user explicitly selects that they want mmap
> > to fail if an address could not be found within the hint address
> > constraints.
> >
>
> I think restricting the addresses with the MAP_BELOW_HINT flag
> would be the best choice. However, it remains a problem: What should
> the behavior be when there is no MAP_BELOW_HINT? I think we can
> fallback to Sv48 on the Sv57 machine by default to align with x86
> and aarch64.
Although that is the behavior on other architectures, I am hesitant to
follow it because it is a somewhat arbitrary restriction. With a generic
flag that can force mmap to provide exactly the number of bits that an
application needs, there is no need for this restriction on riscv. It
may cause problems for applications running on sv57 hardware, however:
1. sv57 hardware does not exist yet
2. A hint address would still be required if following the same behavior
as other architectures.
a. It would aid in the porting of an application to sv57
hardware, but I am not sure that forcing this restriction is
worth having this one piece of parity. Applications using the
proposed generic flag would work as expected on all
architectures as well.
>
> > - Charlie
> >
> >>
> >>> Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
> >>>
> >>>>> The goal of this patch is to help
> >>>>> developers have more consistent mmap behavior, but maybe it is necessary
> >>>>> to hide this behavior behind an mmap flag.
> >>>>
> >>>> Thank you for helping to shape a more consistent mmap behavior.
> >>>> I think this should be fixed ASAP either by allowing the hint address to
> >>>> be ignored
> >>>> (as suggested by the Linux man page), or hide this behavior behind an
> >>>> mmap flag as you said.
> >>>>
> >>>>> - Charlie
> >>>>>
> >>>>>> See alsohttps://github.com/riscv-forks/electron/issues/4
> >>>>>>
> >>>>>>>> - Charlie
> >>>>>> Sincerely,
> >>>>>> Levi
> >>>>>>
> >>>>
> >>>> I accidentally introduced some HTML into this reply so this reply is
> >>>> resent as plain text.
> >>>>
> >>>> Sincerely,
> >>>> Levi
>
>
^ permalink raw reply
* Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available
From: Yangyu Chen @ 2024-08-23 5:28 UTC (permalink / raw)
To: Charlie Jenkins
Cc: Palmer Dabbelt, rsworktech, Alexandre Ghiti, Paul Walmsley,
Albert Ou, Shuah Khan, Jonathan Corbet, linux-mm, linux-riscv,
Linux Kernel Mailing List, linux-kselftest, linux-doc, linux-api
In-Reply-To: <ZsgSgm0zEE2t/9tK@ghost>
> On Aug 23, 2024, at 12:39, Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Thu, Aug 22, 2024 at 10:51:54AM +0800, Yangyu Chen wrote:
>>
>>
>>> On Aug 22, 2024, at 06:17, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>
>>> On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworktech@outlook.com wrote:
>>>> On 2024-08-20 01:00, Charlie Jenkins wrote:
>>>>> On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
>>>>>> On 2024-03-22 22:06, Palmer Dabbelt wrote:
>>>>>>> On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
>>>>>>>> On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
>>>>>>>>> On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
>>>>>>>>>> On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
>>>>>>>>>>> On riscv it is guaranteed that the address returned by mmap is less
>>>>>>>>>>> than
>>>>>>>>>>> the hint address. Allow mmap to return an address all the way up to
>>>>>>>>>>> addr, if provided, rather than just up to the lower address space.
>>>>>>>>>>>>> This provides a performance benefit as well, allowing
>>>>>>>>> mmap to exit
>>>>>>>>>>> after
>>>>>>>>>>> checking that the address is in range rather than searching for a
>>>>>>>>>>> valid
>>>>>>>>>>> address.
>>>>>>>>>>>>> It is possible to provide an address that uses at most the same
>>>>>>>>>>> number
>>>>>>>>>>> of bits, however it is significantly more computationally expensive
>>>>>>>>>>> to
>>>>>>>>>>> provide that number rather than setting the max to be the hint
>>>>>>>>>>> address.
>>>>>>>>>>> There is the instruction clz/clzw in Zbb that returns the highest
>>>>>>>>>>> set
>>>>>>>>>>> bit
>>>>>>>>>>> which could be used to performantly implement this, but it would
>>>>>>>>>>> still
>>>>>>>>>>> be slower than the current implementation. At worst case, half of
>>>>>>>>>>> the
>>>>>>>>>>> address would not be able to be allocated when a hint address is
>>>>>>>>>>> provided.
>>>>>>>>>>>>> Signed-off-by: Charlie Jenkins<charlie@rivosinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>> arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
>>>>>>>>>>> -
>>>>>>>>>>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>>>>>>>>>>>> diff --git a/arch/riscv/include/asm/processor.h
>>>>>>>>>>> b/arch/riscv/include/asm/processor.h
>>>>>>>>>>> index f19f861cda54..8ece7a8f0e18 100644
>>>>>>>>>>> --- a/arch/riscv/include/asm/processor.h
>>>>>>>>>>> +++ b/arch/riscv/include/asm/processor.h
>>>>>>>>>>> @@ -14,22 +14,16 @@
>>>>>>>>>>>
>>>>>>>>>>> #include <asm/ptrace.h>
>>>>>>>>>>>
>>>>>>>>>>> -#ifdef CONFIG_64BIT
>>>>>>>>>>> -#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
>>>>>>>>>>> -#define STACK_TOP_MAX TASK_SIZE_64
>>>>>>>>>>> -
>>>>>>>>>>> #define arch_get_mmap_end(addr, len, flags) \
>>>>>>>>>>> ({ \
>>>>>>>>>>> unsigned long
>>>>>>>>>>> mmap_end; \
>>>>>>>>>>> typeof(addr) _addr = (addr); \
>>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
>>>>>>>>>>> is_compat_task())) \
>>>>>>>>>>> + if ((_addr) == 0 || \
>>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
>>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
>>>>>>>>>>> 1))) \
>>>>>>>>>>> mmap_end = STACK_TOP_MAX; \
>>>>>>>>>>> - else if ((_addr) >= VA_USER_SV57) \
>>>>>>>>>>> - mmap_end = STACK_TOP_MAX; \
>>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
>>>>>>>>>>> VA_BITS_SV48)) \
>>>>>>>>>>> - mmap_end = VA_USER_SV48; \
>>>>>>>>>>> else \
>>>>>>>>>>> - mmap_end = VA_USER_SV39; \
>>>>>>>>>>> + mmap_end = (_addr + len); \
>>>>>>>>>>> mmap_end; \
>>>>>>>>>>> })
>>>>>>>>>>>
>>>>>>>>>>> @@ -39,17 +33,18 @@
>>>>>>>>>>> typeof(addr) _addr = (addr); \
>>>>>>>>>>> typeof(base) _base = (base); \
>>>>>>>>>>> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
>>>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
>>>>>>>>>>> is_compat_task())) \
>>>>>>>>>>> + if ((_addr) == 0 || \
>>>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
>>>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
>>>>>>>>>>> 1))) \
>>>>>>>>>>> mmap_base = (_base); \
>>>>>>>>>>> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
>>>>>>>>>>> VA_BITS_SV57)) \
>>>>>>>>>>> - mmap_base = VA_USER_SV57 - rnd_gap; \
>>>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
>>>>>>>>>>> VA_BITS_SV48)) \
>>>>>>>>>>> - mmap_base = VA_USER_SV48 - rnd_gap; \
>>>>>>>>>>> else \
>>>>>>>>>>> - mmap_base = VA_USER_SV39 - rnd_gap; \
>>>>>>>>>>> + mmap_base = (_addr + len) - rnd_gap; \
>>>>>>>>>>> mmap_base; \
>>>>>>>>>>> })
>>>>>>>>>>>
>>>>>>>>>>> +#ifdef CONFIG_64BIT
>>>>>>>>>>> +#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
>>>>>>>>>>> +#define STACK_TOP_MAX TASK_SIZE_64
>>>>>>>>>>> #else
>>>>>>>>>>> #define DEFAULT_MAP_WINDOW TASK_SIZE
>>>>>>>>>>> #define STACK_TOP_MAX TASK_SIZE
>>>>>>>>>>>>> I have carefully tested your patch on qemu with sv57. A
>>>>>>>>> bug that
>>>>>>>>>> needs
>>>>>>>>>> to be solved is that mmap with the same hint address without
>>>>>>>>>> MAP_FIXED
>>>>>>>>>> set will fail the second time.
>>>>>>>>>>> Userspace code to reproduce the bug:
>>>>>>>>>>> #include <sys/mman.h>
>>>>>>>>>> #include <stdio.h>
>>>>>>>>>> #include <stdint.h>
>>>>>>>>>>> void test(char *addr) {
>>>>>>>>>> char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
>>>>>>>>>> MAP_ANONYMOUS
>>>>>>>>>>> MAP_PRIVATE, -1, 0);
>>>>>>>>>> printf("hint %p got %p.\n", addr, res);
>>>>>>>>>> }
>>>>>>>>>>> int main (void) {
>>>>>>>>>> test(1<<30);
>>>>>>>>>> test(1<<30);
>>>>>>>>>> test(1<<30);
>>>>>>>>>> return 0;
>>>>>>>>>> }
>>>>>>>>>>> output:
>>>>>>>>>>> hint 0x40000000 got 0x40000000.
>>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
>>>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
>>>>>>>>>>> output on x86:
>>>>>>>>>>> hint 0x40000000 got 0x40000000.
>>>>>>>>>> hint 0x40000000 got 0x7f9171363000.
>>>>>>>>>> hint 0x40000000 got 0x7f9171362000.
>>>>>>>>>>> It may need to implement a special arch_get_unmapped_area and
>>>>>>>>>> arch_get_unmapped_area_topdown function.
>>>>>>>>>>
>>>>>>>>> This is because hint address < rnd_gap. I have tried to let mmap_base =
>>>>>>>>> min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
>>>>>>>>> does not work for bottom-up while ulimit -s is unlimited. You said this
>>>>>>>>> behavior is expected from patch v2 review. However it brings a new
>>>>>>>>> regression even on sv39 systems.
>>>>>>>>>
>>>>>>>>> I still don't know the reason why use addr+len as the upper-bound. I
>>>>>>>>> think solution like x86/arm64/powerpc provide two address space switch
>>>>>>>>> based on whether hint address above the default map window is enough.
>>>>>>>>>
>>>>>>>> Yep this is expected. It is up to the maintainers to decide.
>>>>>>> Sorry I forgot to reply to this, I had a buffer sitting around somewhere
>>>>>>> but I must have lost it.
>>>>>>>
>>>>>>> I think Charlie's approach is the right way to go. Putting my userspace
>>>>>>> hat on, I'd much rather have my allocations fail rather than silently
>>>>>>> ignore the hint when there's memory pressure.
>>>>>>>
>>>>>>> If there's some real use case that needs these low hints to be silently
>>>>>>> ignored under VA pressure then we can try and figure something out that
>>>>>>> makes those applications work.
>>>>>> I could confirm that this patch has broken chromium's partition allocator on
>>>>>> riscv64. The minimal reproduction I use is chromium-mmap.c:
>>>>>>
>>>>>> #include <stdio.h>
>>>>>> #include <sys/mman.h>
>>>>>>
>>>>>> int main() {
>>>>>> void* expected = (void*)0x400000000;
>>>>>> void* addr = mmap(expected, 17179869184, PROT_NONE,
>>>>>> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
>>>>>> if (addr != expected) {
>>>>> It is not valid to assume that the address returned by mmap will be the
>>>>> hint address. If the hint address is not available, mmap will return a
>>>>> different address.
>>>>
>>>> Oh, sorry I didn't make it clear what is the expected behavior.
>>>> The printf here is solely for debugging purpose and I don't mean that
>>>> chromium expect it will get the hint address. The expected behavior is
>>>> that both the two mmap calls will succeed.
>>>>
>>>>>> printf("Not expected address: %p != %p\n", addr, expected);
>>>>>> }
>>>>>> expected = (void*)0x3fffff000;
>>>>>> addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
>>>>>> -1, 0);
>>>>>> if (addr != expected) {
>>>>>> printf("Not expected address: %p != %p\n", addr, expected);
>>>>>> }
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> The second mmap fails with ENOMEM. Manually reverting this commit fixes the
>>>>>> issue for me. So I think it's clearly a regression and breaks userspace.
>>>>>>
>>>>> The issue here is that overlapping memory is being requested. This
>>>>> second mmap will never be able to provide an address at 0x3fffff000 with
>>>>> a size of 0x400001000 since mmap just provided an address at 0x400000000
>>>>> with a size of 0x400000000.
>>>>>
>>>>> Before this patch, this request causes mmap to return a completely
>>>>> arbitrary value. There is no reason to use a hint address in this manner
>>>>> because the hint can never be respected. Since an arbitrary address is
>>>>> desired, a hint of zero should be used.
>>>>>
>>>>> This patch causes the behavior to be more deterministic. Instead of
>>>>> providing an arbitrary address, it causes the address to be less than or
>>>>> equal to the hint address. This allows for applications to make
>>>>> assumptions about the returned address.
>>>>
>>>> About the overlap, of course the partition allocator's request for
>>>> overlapped vma seems unreasonable.
>>>>
>>>> But I still don't quite understand why mmap cannot use an address higher
>>>> than the hint address.
>>>> The hint address, after all, is a hint, not a requirement.
>>>>
>>>> Quoting the man page:
>>>>
>>>>> If another mapping already exists there, the kernel picks
>>>>> a new address that may or may not depend on the hint. The
>>>>> address of the new mapping is returned as the result of the call.
>>>>
>>>> So for casual programmers that only reads man page but not architecture
>>>> specific kernel documentation, the current behavior of mmap on riscv64
>>>> failing on overlapped address ranges are quite surprising IMO.
>>>>
>>>> And quoting the man page again about the errno:
>>>>
>>>>> ENOMEM No memory is available.
>>>>>
>>>>> ENOMEM The process's maximum number of mappings would have been
>>>>> exceeded. This error can also occur for munmap(), when
>>>>> unmapping a region in the middle of an existing mapping,
>>>>> since this results in two smaller mappings on either side
>>>>> of the region being unmapped.
>>>>>
>>>>> ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
>>>>> described in getrlimit(2), would have been exceeded.
>>>>>
>>>>> ENOMEM We don't like addr, because it exceeds the virtual address
>>>>> space of the CPU.
>>>>>
>>>>
>>>> There's no matching description for the ENOMEM returned here.
>>>> I would suggest removing "because it exceeds the virtual address
>>>> space of the CPU." from the last item if the ENOMEM behavior here
>>>> is expected.
>>>>
>>>>> This code is unfortunately relying on the previously mostly undefined
>>>>> behavior of the hint address in mmap.
>>>>
>>>> Although I haven't read the code of chromium's partition allocator to
>>>> judge whether it should
>>>> be improved or fixed for riscv64, I do know that the kernel "don't break
>>>> userspace" and "never EVER blame the user programs".
>>>
>>> Ya, sorry for breaking stuff.
>>>
>>> The goal here was to move to the mmap flag behavor similar to what arm64 and x86 have, as that was done in a way that didn't appear to break userspace -- or at least any real userspace programs. IIRC that first test was pretty broken (it actually depended on the hint address), but sounds like that's not the case.
>>>
>>> I think maybe this is just luck: we didn't chunk the address space up, we're just hinting on every bit, so we're just more likely to hit the exhaustion. Doesn't really matter, though, as if it's breaking stuff so we've got to deal with it.
>>>
>>> Charlie and I are just talking, and best we can come up with is to move to the behavior where we fall back to larger allocation regions when there's no space in the smaller allocation region.
>>
>>
>> For this solution, the only difference from the mmap behavior of
>> x86 and aarch64 is that we will first try to allocate some memory
>> from an address less or equal to the request address + size. But
>> for most cases, I think there is no need to do that, especially for
>> those addresses < BIT(47), as most program works fine on x86-64,
>> which has 47bit available userspace address space to use. And for
>> that program that wants an address < BIT(32), we already have
>> MAP_32BIT now.
>>
>> I think we can just fix like that patch:
>> https://lore.kernel.org/lkml/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
>
> This patch does not satisfy the requirement of having the ability to guarantee
> that mmap returns an address that is less than the hint address.
Indeed. My intuition is to remove it and align it with x86 and aarch64.
> This
> patch only allows an address to be less than the DEFAULT_MAP_WINDOW
> which is 32 bits on sv32, 39 bits on sv39, and 48 bits on sv48 or sv57.
>
> This patch also again falls into the trap of using the hint address to
> forcefully restrict the address space.
Indeed. However, x86 and aarch64 also use this behavior to restrict
va >= BIT(47) by default unless we have the hint address larger
than BIT(47).
> I agree with Levi that it is not
> very good behavior to have a "hint" cause mmap to fail if conforming to
> the hint isn't possible. Instead, I believe it to be more logical to try
> to allocate at the hint address, otherwise give a random address.
>
I also agree with this.
> The current behavior can then be maintained through the flag
> MAP_BELOW_HINT. This way the user explicitly selects that they want mmap
> to fail if an address could not be found within the hint address
> constraints.
>
I think restricting the addresses with the MAP_BELOW_HINT flag
would be the best choice. However, it remains a problem: What should
the behavior be when there is no MAP_BELOW_HINT? I think we can
fallback to Sv48 on the Sv57 machine by default to align with x86
and aarch64.
> - Charlie
>
>>
>>> Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
>>>
>>>>> The goal of this patch is to help
>>>>> developers have more consistent mmap behavior, but maybe it is necessary
>>>>> to hide this behavior behind an mmap flag.
>>>>
>>>> Thank you for helping to shape a more consistent mmap behavior.
>>>> I think this should be fixed ASAP either by allowing the hint address to
>>>> be ignored
>>>> (as suggested by the Linux man page), or hide this behavior behind an
>>>> mmap flag as you said.
>>>>
>>>>> - Charlie
>>>>>
>>>>>> See alsohttps://github.com/riscv-forks/electron/issues/4
>>>>>>
>>>>>>>> - Charlie
>>>>>> Sincerely,
>>>>>> Levi
>>>>>>
>>>>
>>>> I accidentally introduced some HTML into this reply so this reply is
>>>> resent as plain text.
>>>>
>>>> Sincerely,
>>>> Levi
^ permalink raw reply
* Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available
From: Charlie Jenkins @ 2024-08-23 4:39 UTC (permalink / raw)
To: Yangyu Chen
Cc: Palmer Dabbelt, rsworktech, Alexandre Ghiti, Paul Walmsley,
Albert Ou, Shuah Khan, Jonathan Corbet, linux-mm, linux-riscv,
Linux Kernel Mailing List, linux-kselftest, linux-doc, linux-api
In-Reply-To: <tencent_86551D71707162B243861AC9F8EC0573B409@qq.com>
On Thu, Aug 22, 2024 at 10:51:54AM +0800, Yangyu Chen wrote:
>
>
> > On Aug 22, 2024, at 06:17, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> > On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworktech@outlook.com wrote:
> >> On 2024-08-20 01:00, Charlie Jenkins wrote:
> >>> On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
> >>>> On 2024-03-22 22:06, Palmer Dabbelt wrote:
> >>>>> On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
> >>>>>> On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
> >>>>>>> On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
> >>>>>>>> On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
> >>>>>>>>> On riscv it is guaranteed that the address returned by mmap is less
> >>>>>>>>> than
> >>>>>>>>> the hint address. Allow mmap to return an address all the way up to
> >>>>>>>>> addr, if provided, rather than just up to the lower address space.
> >>>>>>>>>>> This provides a performance benefit as well, allowing
> >>>>>>> mmap to exit
> >>>>>>>>> after
> >>>>>>>>> checking that the address is in range rather than searching for a
> >>>>>>>>> valid
> >>>>>>>>> address.
> >>>>>>>>>>> It is possible to provide an address that uses at most the same
> >>>>>>>>> number
> >>>>>>>>> of bits, however it is significantly more computationally expensive
> >>>>>>>>> to
> >>>>>>>>> provide that number rather than setting the max to be the hint
> >>>>>>>>> address.
> >>>>>>>>> There is the instruction clz/clzw in Zbb that returns the highest
> >>>>>>>>> set
> >>>>>>>>> bit
> >>>>>>>>> which could be used to performantly implement this, but it would
> >>>>>>>>> still
> >>>>>>>>> be slower than the current implementation. At worst case, half of
> >>>>>>>>> the
> >>>>>>>>> address would not be able to be allocated when a hint address is
> >>>>>>>>> provided.
> >>>>>>>>>>> Signed-off-by: Charlie Jenkins<charlie@rivosinc.com>
> >>>>>>>>> ---
> >>>>>>>>> arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
> >>>>>>>>> -
> >>>>>>>>> 1 file changed, 11 insertions(+), 16 deletions(-)
> >>>>>>>>>>> diff --git a/arch/riscv/include/asm/processor.h
> >>>>>>>>> b/arch/riscv/include/asm/processor.h
> >>>>>>>>> index f19f861cda54..8ece7a8f0e18 100644
> >>>>>>>>> --- a/arch/riscv/include/asm/processor.h
> >>>>>>>>> +++ b/arch/riscv/include/asm/processor.h
> >>>>>>>>> @@ -14,22 +14,16 @@
> >>>>>>>>>
> >>>>>>>>> #include <asm/ptrace.h>
> >>>>>>>>>
> >>>>>>>>> -#ifdef CONFIG_64BIT
> >>>>>>>>> -#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
> >>>>>>>>> -#define STACK_TOP_MAX TASK_SIZE_64
> >>>>>>>>> -
> >>>>>>>>> #define arch_get_mmap_end(addr, len, flags) \
> >>>>>>>>> ({ \
> >>>>>>>>> unsigned long
> >>>>>>>>> mmap_end; \
> >>>>>>>>> typeof(addr) _addr = (addr); \
> >>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> >>>>>>>>> is_compat_task())) \
> >>>>>>>>> + if ((_addr) == 0 || \
> >>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> >>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
> >>>>>>>>> 1))) \
> >>>>>>>>> mmap_end = STACK_TOP_MAX; \
> >>>>>>>>> - else if ((_addr) >= VA_USER_SV57) \
> >>>>>>>>> - mmap_end = STACK_TOP_MAX; \
> >>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> >>>>>>>>> VA_BITS_SV48)) \
> >>>>>>>>> - mmap_end = VA_USER_SV48; \
> >>>>>>>>> else \
> >>>>>>>>> - mmap_end = VA_USER_SV39; \
> >>>>>>>>> + mmap_end = (_addr + len); \
> >>>>>>>>> mmap_end; \
> >>>>>>>>> })
> >>>>>>>>>
> >>>>>>>>> @@ -39,17 +33,18 @@
> >>>>>>>>> typeof(addr) _addr = (addr); \
> >>>>>>>>> typeof(base) _base = (base); \
> >>>>>>>>> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
> >>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> >>>>>>>>> is_compat_task())) \
> >>>>>>>>> + if ((_addr) == 0 || \
> >>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
> >>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
> >>>>>>>>> 1))) \
> >>>>>>>>> mmap_base = (_base); \
> >>>>>>>>> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
> >>>>>>>>> VA_BITS_SV57)) \
> >>>>>>>>> - mmap_base = VA_USER_SV57 - rnd_gap; \
> >>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> >>>>>>>>> VA_BITS_SV48)) \
> >>>>>>>>> - mmap_base = VA_USER_SV48 - rnd_gap; \
> >>>>>>>>> else \
> >>>>>>>>> - mmap_base = VA_USER_SV39 - rnd_gap; \
> >>>>>>>>> + mmap_base = (_addr + len) - rnd_gap; \
> >>>>>>>>> mmap_base; \
> >>>>>>>>> })
> >>>>>>>>>
> >>>>>>>>> +#ifdef CONFIG_64BIT
> >>>>>>>>> +#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
> >>>>>>>>> +#define STACK_TOP_MAX TASK_SIZE_64
> >>>>>>>>> #else
> >>>>>>>>> #define DEFAULT_MAP_WINDOW TASK_SIZE
> >>>>>>>>> #define STACK_TOP_MAX TASK_SIZE
> >>>>>>>>>>> I have carefully tested your patch on qemu with sv57. A
> >>>>>>> bug that
> >>>>>>>> needs
> >>>>>>>> to be solved is that mmap with the same hint address without
> >>>>>>>> MAP_FIXED
> >>>>>>>> set will fail the second time.
> >>>>>>>>> Userspace code to reproduce the bug:
> >>>>>>>>> #include <sys/mman.h>
> >>>>>>>> #include <stdio.h>
> >>>>>>>> #include <stdint.h>
> >>>>>>>>> void test(char *addr) {
> >>>>>>>> char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
> >>>>>>>> MAP_ANONYMOUS
> >>>>>>>>> MAP_PRIVATE, -1, 0);
> >>>>>>>> printf("hint %p got %p.\n", addr, res);
> >>>>>>>> }
> >>>>>>>>> int main (void) {
> >>>>>>>> test(1<<30);
> >>>>>>>> test(1<<30);
> >>>>>>>> test(1<<30);
> >>>>>>>> return 0;
> >>>>>>>> }
> >>>>>>>>> output:
> >>>>>>>>> hint 0x40000000 got 0x40000000.
> >>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
> >>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
> >>>>>>>>> output on x86:
> >>>>>>>>> hint 0x40000000 got 0x40000000.
> >>>>>>>> hint 0x40000000 got 0x7f9171363000.
> >>>>>>>> hint 0x40000000 got 0x7f9171362000.
> >>>>>>>>> It may need to implement a special arch_get_unmapped_area and
> >>>>>>>> arch_get_unmapped_area_topdown function.
> >>>>>>>>
> >>>>>>> This is because hint address < rnd_gap. I have tried to let mmap_base =
> >>>>>>> min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
> >>>>>>> does not work for bottom-up while ulimit -s is unlimited. You said this
> >>>>>>> behavior is expected from patch v2 review. However it brings a new
> >>>>>>> regression even on sv39 systems.
> >>>>>>>
> >>>>>>> I still don't know the reason why use addr+len as the upper-bound. I
> >>>>>>> think solution like x86/arm64/powerpc provide two address space switch
> >>>>>>> based on whether hint address above the default map window is enough.
> >>>>>>>
> >>>>>> Yep this is expected. It is up to the maintainers to decide.
> >>>>> Sorry I forgot to reply to this, I had a buffer sitting around somewhere
> >>>>> but I must have lost it.
> >>>>>
> >>>>> I think Charlie's approach is the right way to go. Putting my userspace
> >>>>> hat on, I'd much rather have my allocations fail rather than silently
> >>>>> ignore the hint when there's memory pressure.
> >>>>>
> >>>>> If there's some real use case that needs these low hints to be silently
> >>>>> ignored under VA pressure then we can try and figure something out that
> >>>>> makes those applications work.
> >>>> I could confirm that this patch has broken chromium's partition allocator on
> >>>> riscv64. The minimal reproduction I use is chromium-mmap.c:
> >>>>
> >>>> #include <stdio.h>
> >>>> #include <sys/mman.h>
> >>>>
> >>>> int main() {
> >>>> void* expected = (void*)0x400000000;
> >>>> void* addr = mmap(expected, 17179869184, PROT_NONE,
> >>>> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> >>>> if (addr != expected) {
> >>> It is not valid to assume that the address returned by mmap will be the
> >>> hint address. If the hint address is not available, mmap will return a
> >>> different address.
> >>
> >> Oh, sorry I didn't make it clear what is the expected behavior.
> >> The printf here is solely for debugging purpose and I don't mean that
> >> chromium expect it will get the hint address. The expected behavior is
> >> that both the two mmap calls will succeed.
> >>
> >>>> printf("Not expected address: %p != %p\n", addr, expected);
> >>>> }
> >>>> expected = (void*)0x3fffff000;
> >>>> addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
> >>>> -1, 0);
> >>>> if (addr != expected) {
> >>>> printf("Not expected address: %p != %p\n", addr, expected);
> >>>> }
> >>>> return 0;
> >>>> }
> >>>>
> >>>> The second mmap fails with ENOMEM. Manually reverting this commit fixes the
> >>>> issue for me. So I think it's clearly a regression and breaks userspace.
> >>>>
> >>> The issue here is that overlapping memory is being requested. This
> >>> second mmap will never be able to provide an address at 0x3fffff000 with
> >>> a size of 0x400001000 since mmap just provided an address at 0x400000000
> >>> with a size of 0x400000000.
> >>>
> >>> Before this patch, this request causes mmap to return a completely
> >>> arbitrary value. There is no reason to use a hint address in this manner
> >>> because the hint can never be respected. Since an arbitrary address is
> >>> desired, a hint of zero should be used.
> >>>
> >>> This patch causes the behavior to be more deterministic. Instead of
> >>> providing an arbitrary address, it causes the address to be less than or
> >>> equal to the hint address. This allows for applications to make
> >>> assumptions about the returned address.
> >>
> >> About the overlap, of course the partition allocator's request for
> >> overlapped vma seems unreasonable.
> >>
> >> But I still don't quite understand why mmap cannot use an address higher
> >> than the hint address.
> >> The hint address, after all, is a hint, not a requirement.
> >>
> >> Quoting the man page:
> >>
> >>> If another mapping already exists there, the kernel picks
> >>> a new address that may or may not depend on the hint. The
> >>> address of the new mapping is returned as the result of the call.
> >>
> >> So for casual programmers that only reads man page but not architecture
> >> specific kernel documentation, the current behavior of mmap on riscv64
> >> failing on overlapped address ranges are quite surprising IMO.
> >>
> >> And quoting the man page again about the errno:
> >>
> >>> ENOMEM No memory is available.
> >>>
> >>> ENOMEM The process's maximum number of mappings would have been
> >>> exceeded. This error can also occur for munmap(), when
> >>> unmapping a region in the middle of an existing mapping,
> >>> since this results in two smaller mappings on either side
> >>> of the region being unmapped.
> >>>
> >>> ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
> >>> described in getrlimit(2), would have been exceeded.
> >>>
> >>> ENOMEM We don't like addr, because it exceeds the virtual address
> >>> space of the CPU.
> >>>
> >>
> >> There's no matching description for the ENOMEM returned here.
> >> I would suggest removing "because it exceeds the virtual address
> >> space of the CPU." from the last item if the ENOMEM behavior here
> >> is expected.
> >>
> >>> This code is unfortunately relying on the previously mostly undefined
> >>> behavior of the hint address in mmap.
> >>
> >> Although I haven't read the code of chromium's partition allocator to
> >> judge whether it should
> >> be improved or fixed for riscv64, I do know that the kernel "don't break
> >> userspace" and "never EVER blame the user programs".
> >
> > Ya, sorry for breaking stuff.
> >
> > The goal here was to move to the mmap flag behavor similar to what arm64 and x86 have, as that was done in a way that didn't appear to break userspace -- or at least any real userspace programs. IIRC that first test was pretty broken (it actually depended on the hint address), but sounds like that's not the case.
> >
> > I think maybe this is just luck: we didn't chunk the address space up, we're just hinting on every bit, so we're just more likely to hit the exhaustion. Doesn't really matter, though, as if it's breaking stuff so we've got to deal with it.
> >
> > Charlie and I are just talking, and best we can come up with is to move to the behavior where we fall back to larger allocation regions when there's no space in the smaller allocation region.
>
>
> For this solution, the only difference from the mmap behavior of
> x86 and aarch64 is that we will first try to allocate some memory
> from an address less or equal to the request address + size. But
> for most cases, I think there is no need to do that, especially for
> those addresses < BIT(47), as most program works fine on x86-64,
> which has 47bit available userspace address space to use. And for
> that program that wants an address < BIT(32), we already have
> MAP_32BIT now.
>
> I think we can just fix like that patch:
> https://lore.kernel.org/lkml/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
This patch does not satisfy the requirement of having the ability to guarantee
that mmap returns an address that is less than the hint address. This
patch only allows an address to be less than the DEFAULT_MAP_WINDOW
which is 32 bits on sv32, 39 bits on sv39, and 48 bits on sv48 or sv57.
This patch also again falls into the trap of using the hint address to
forcefully restrict the address space. I agree with Levi that it is not
very good behavior to have a "hint" cause mmap to fail if conforming to
the hint isn't possible. Instead, I believe it to be more logical to try
to allocate at the hint address, otherwise give a random address.
The current behavior can then be maintained through the flag
MAP_BELOW_HINT. This way the user explicitly selects that they want mmap
to fail if an address could not be found within the hint address
constraints.
- Charlie
>
> > Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
> >
> >>> The goal of this patch is to help
> >>> developers have more consistent mmap behavior, but maybe it is necessary
> >>> to hide this behavior behind an mmap flag.
> >>
> >> Thank you for helping to shape a more consistent mmap behavior.
> >> I think this should be fixed ASAP either by allowing the hint address to
> >> be ignored
> >> (as suggested by the Linux man page), or hide this behavior behind an
> >> mmap flag as you said.
> >>
> >>> - Charlie
> >>>
> >>>> See alsohttps://github.com/riscv-forks/electron/issues/4
> >>>>
> >>>>>> - Charlie
> >>>> Sincerely,
> >>>> Levi
> >>>>
> >>
> >> I accidentally introduced some HTML into this reply so this reply is
> >> resent as plain text.
> >>
> >> Sincerely,
> >> Levi
>
>
^ permalink raw reply
* Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available
From: Yangyu Chen @ 2024-08-22 2:51 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: rsworktech, Charlie Jenkins, Alexandre Ghiti, Paul Walmsley,
Albert Ou, Shuah Khan, Jonathan Corbet, linux-mm, linux-riscv,
Linux Kernel Mailing List, linux-kselftest, linux-doc, linux-api
In-Reply-To: <mhng-7d9e2b27-a53d-4579-b78e-0aec038290fb@palmer-ri-x1c9>
> On Aug 22, 2024, at 06:17, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworktech@outlook.com wrote:
>> On 2024-08-20 01:00, Charlie Jenkins wrote:
>>> On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
>>>> On 2024-03-22 22:06, Palmer Dabbelt wrote:
>>>>> On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
>>>>>> On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
>>>>>>> On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
>>>>>>>> On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
>>>>>>>>> On riscv it is guaranteed that the address returned by mmap is less
>>>>>>>>> than
>>>>>>>>> the hint address. Allow mmap to return an address all the way up to
>>>>>>>>> addr, if provided, rather than just up to the lower address space.
>>>>>>>>>>> This provides a performance benefit as well, allowing
>>>>>>> mmap to exit
>>>>>>>>> after
>>>>>>>>> checking that the address is in range rather than searching for a
>>>>>>>>> valid
>>>>>>>>> address.
>>>>>>>>>>> It is possible to provide an address that uses at most the same
>>>>>>>>> number
>>>>>>>>> of bits, however it is significantly more computationally expensive
>>>>>>>>> to
>>>>>>>>> provide that number rather than setting the max to be the hint
>>>>>>>>> address.
>>>>>>>>> There is the instruction clz/clzw in Zbb that returns the highest
>>>>>>>>> set
>>>>>>>>> bit
>>>>>>>>> which could be used to performantly implement this, but it would
>>>>>>>>> still
>>>>>>>>> be slower than the current implementation. At worst case, half of
>>>>>>>>> the
>>>>>>>>> address would not be able to be allocated when a hint address is
>>>>>>>>> provided.
>>>>>>>>>>> Signed-off-by: Charlie Jenkins<charlie@rivosinc.com>
>>>>>>>>> ---
>>>>>>>>> arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
>>>>>>>>> -
>>>>>>>>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>>>>>>>>>> diff --git a/arch/riscv/include/asm/processor.h
>>>>>>>>> b/arch/riscv/include/asm/processor.h
>>>>>>>>> index f19f861cda54..8ece7a8f0e18 100644
>>>>>>>>> --- a/arch/riscv/include/asm/processor.h
>>>>>>>>> +++ b/arch/riscv/include/asm/processor.h
>>>>>>>>> @@ -14,22 +14,16 @@
>>>>>>>>>
>>>>>>>>> #include <asm/ptrace.h>
>>>>>>>>>
>>>>>>>>> -#ifdef CONFIG_64BIT
>>>>>>>>> -#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
>>>>>>>>> -#define STACK_TOP_MAX TASK_SIZE_64
>>>>>>>>> -
>>>>>>>>> #define arch_get_mmap_end(addr, len, flags) \
>>>>>>>>> ({ \
>>>>>>>>> unsigned long
>>>>>>>>> mmap_end; \
>>>>>>>>> typeof(addr) _addr = (addr); \
>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
>>>>>>>>> is_compat_task())) \
>>>>>>>>> + if ((_addr) == 0 || \
>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
>>>>>>>>> 1))) \
>>>>>>>>> mmap_end = STACK_TOP_MAX; \
>>>>>>>>> - else if ((_addr) >= VA_USER_SV57) \
>>>>>>>>> - mmap_end = STACK_TOP_MAX; \
>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
>>>>>>>>> VA_BITS_SV48)) \
>>>>>>>>> - mmap_end = VA_USER_SV48; \
>>>>>>>>> else \
>>>>>>>>> - mmap_end = VA_USER_SV39; \
>>>>>>>>> + mmap_end = (_addr + len); \
>>>>>>>>> mmap_end; \
>>>>>>>>> })
>>>>>>>>>
>>>>>>>>> @@ -39,17 +33,18 @@
>>>>>>>>> typeof(addr) _addr = (addr); \
>>>>>>>>> typeof(base) _base = (base); \
>>>>>>>>> unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base); \
>>>>>>>>> - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
>>>>>>>>> is_compat_task())) \
>>>>>>>>> + if ((_addr) == 0 || \
>>>>>>>>> + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
>>>>>>>>> + ((_addr + len) > BIT(VA_BITS -
>>>>>>>>> 1))) \
>>>>>>>>> mmap_base = (_base); \
>>>>>>>>> - else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
>>>>>>>>> VA_BITS_SV57)) \
>>>>>>>>> - mmap_base = VA_USER_SV57 - rnd_gap; \
>>>>>>>>> - else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
>>>>>>>>> VA_BITS_SV48)) \
>>>>>>>>> - mmap_base = VA_USER_SV48 - rnd_gap; \
>>>>>>>>> else \
>>>>>>>>> - mmap_base = VA_USER_SV39 - rnd_gap; \
>>>>>>>>> + mmap_base = (_addr + len) - rnd_gap; \
>>>>>>>>> mmap_base; \
>>>>>>>>> })
>>>>>>>>>
>>>>>>>>> +#ifdef CONFIG_64BIT
>>>>>>>>> +#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
>>>>>>>>> +#define STACK_TOP_MAX TASK_SIZE_64
>>>>>>>>> #else
>>>>>>>>> #define DEFAULT_MAP_WINDOW TASK_SIZE
>>>>>>>>> #define STACK_TOP_MAX TASK_SIZE
>>>>>>>>>>> I have carefully tested your patch on qemu with sv57. A
>>>>>>> bug that
>>>>>>>> needs
>>>>>>>> to be solved is that mmap with the same hint address without
>>>>>>>> MAP_FIXED
>>>>>>>> set will fail the second time.
>>>>>>>>> Userspace code to reproduce the bug:
>>>>>>>>> #include <sys/mman.h>
>>>>>>>> #include <stdio.h>
>>>>>>>> #include <stdint.h>
>>>>>>>>> void test(char *addr) {
>>>>>>>> char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
>>>>>>>> MAP_ANONYMOUS
>>>>>>>>> MAP_PRIVATE, -1, 0);
>>>>>>>> printf("hint %p got %p.\n", addr, res);
>>>>>>>> }
>>>>>>>>> int main (void) {
>>>>>>>> test(1<<30);
>>>>>>>> test(1<<30);
>>>>>>>> test(1<<30);
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>> output:
>>>>>>>>> hint 0x40000000 got 0x40000000.
>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
>>>>>>>> hint 0x40000000 got 0xffffffffffffffff.
>>>>>>>>> output on x86:
>>>>>>>>> hint 0x40000000 got 0x40000000.
>>>>>>>> hint 0x40000000 got 0x7f9171363000.
>>>>>>>> hint 0x40000000 got 0x7f9171362000.
>>>>>>>>> It may need to implement a special arch_get_unmapped_area and
>>>>>>>> arch_get_unmapped_area_topdown function.
>>>>>>>>
>>>>>>> This is because hint address < rnd_gap. I have tried to let mmap_base =
>>>>>>> min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
>>>>>>> does not work for bottom-up while ulimit -s is unlimited. You said this
>>>>>>> behavior is expected from patch v2 review. However it brings a new
>>>>>>> regression even on sv39 systems.
>>>>>>>
>>>>>>> I still don't know the reason why use addr+len as the upper-bound. I
>>>>>>> think solution like x86/arm64/powerpc provide two address space switch
>>>>>>> based on whether hint address above the default map window is enough.
>>>>>>>
>>>>>> Yep this is expected. It is up to the maintainers to decide.
>>>>> Sorry I forgot to reply to this, I had a buffer sitting around somewhere
>>>>> but I must have lost it.
>>>>>
>>>>> I think Charlie's approach is the right way to go. Putting my userspace
>>>>> hat on, I'd much rather have my allocations fail rather than silently
>>>>> ignore the hint when there's memory pressure.
>>>>>
>>>>> If there's some real use case that needs these low hints to be silently
>>>>> ignored under VA pressure then we can try and figure something out that
>>>>> makes those applications work.
>>>> I could confirm that this patch has broken chromium's partition allocator on
>>>> riscv64. The minimal reproduction I use is chromium-mmap.c:
>>>>
>>>> #include <stdio.h>
>>>> #include <sys/mman.h>
>>>>
>>>> int main() {
>>>> void* expected = (void*)0x400000000;
>>>> void* addr = mmap(expected, 17179869184, PROT_NONE,
>>>> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
>>>> if (addr != expected) {
>>> It is not valid to assume that the address returned by mmap will be the
>>> hint address. If the hint address is not available, mmap will return a
>>> different address.
>>
>> Oh, sorry I didn't make it clear what is the expected behavior.
>> The printf here is solely for debugging purpose and I don't mean that
>> chromium expect it will get the hint address. The expected behavior is
>> that both the two mmap calls will succeed.
>>
>>>> printf("Not expected address: %p != %p\n", addr, expected);
>>>> }
>>>> expected = (void*)0x3fffff000;
>>>> addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
>>>> -1, 0);
>>>> if (addr != expected) {
>>>> printf("Not expected address: %p != %p\n", addr, expected);
>>>> }
>>>> return 0;
>>>> }
>>>>
>>>> The second mmap fails with ENOMEM. Manually reverting this commit fixes the
>>>> issue for me. So I think it's clearly a regression and breaks userspace.
>>>>
>>> The issue here is that overlapping memory is being requested. This
>>> second mmap will never be able to provide an address at 0x3fffff000 with
>>> a size of 0x400001000 since mmap just provided an address at 0x400000000
>>> with a size of 0x400000000.
>>>
>>> Before this patch, this request causes mmap to return a completely
>>> arbitrary value. There is no reason to use a hint address in this manner
>>> because the hint can never be respected. Since an arbitrary address is
>>> desired, a hint of zero should be used.
>>>
>>> This patch causes the behavior to be more deterministic. Instead of
>>> providing an arbitrary address, it causes the address to be less than or
>>> equal to the hint address. This allows for applications to make
>>> assumptions about the returned address.
>>
>> About the overlap, of course the partition allocator's request for
>> overlapped vma seems unreasonable.
>>
>> But I still don't quite understand why mmap cannot use an address higher
>> than the hint address.
>> The hint address, after all, is a hint, not a requirement.
>>
>> Quoting the man page:
>>
>>> If another mapping already exists there, the kernel picks
>>> a new address that may or may not depend on the hint. The
>>> address of the new mapping is returned as the result of the call.
>>
>> So for casual programmers that only reads man page but not architecture
>> specific kernel documentation, the current behavior of mmap on riscv64
>> failing on overlapped address ranges are quite surprising IMO.
>>
>> And quoting the man page again about the errno:
>>
>>> ENOMEM No memory is available.
>>>
>>> ENOMEM The process's maximum number of mappings would have been
>>> exceeded. This error can also occur for munmap(), when
>>> unmapping a region in the middle of an existing mapping,
>>> since this results in two smaller mappings on either side
>>> of the region being unmapped.
>>>
>>> ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
>>> described in getrlimit(2), would have been exceeded.
>>>
>>> ENOMEM We don't like addr, because it exceeds the virtual address
>>> space of the CPU.
>>>
>>
>> There's no matching description for the ENOMEM returned here.
>> I would suggest removing "because it exceeds the virtual address
>> space of the CPU." from the last item if the ENOMEM behavior here
>> is expected.
>>
>>> This code is unfortunately relying on the previously mostly undefined
>>> behavior of the hint address in mmap.
>>
>> Although I haven't read the code of chromium's partition allocator to
>> judge whether it should
>> be improved or fixed for riscv64, I do know that the kernel "don't break
>> userspace" and "never EVER blame the user programs".
>
> Ya, sorry for breaking stuff.
>
> The goal here was to move to the mmap flag behavor similar to what arm64 and x86 have, as that was done in a way that didn't appear to break userspace -- or at least any real userspace programs. IIRC that first test was pretty broken (it actually depended on the hint address), but sounds like that's not the case.
>
> I think maybe this is just luck: we didn't chunk the address space up, we're just hinting on every bit, so we're just more likely to hit the exhaustion. Doesn't really matter, though, as if it's breaking stuff so we've got to deal with it.
>
> Charlie and I are just talking, and best we can come up with is to move to the behavior where we fall back to larger allocation regions when there's no space in the smaller allocation region.
For this solution, the only difference from the mmap behavior of
x86 and aarch64 is that we will first try to allocate some memory
from an address less or equal to the request address + size. But
for most cases, I think there is no need to do that, especially for
those addresses < BIT(47), as most program works fine on x86-64,
which has 47bit available userspace address space to use. And for
that program that wants an address < BIT(32), we already have
MAP_32BIT now.
I think we can just fix like that patch:
https://lore.kernel.org/lkml/tencent_B2D0435BC011135736262764B511994F4805@qq.com/
> Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
>
>>> The goal of this patch is to help
>>> developers have more consistent mmap behavior, but maybe it is necessary
>>> to hide this behavior behind an mmap flag.
>>
>> Thank you for helping to shape a more consistent mmap behavior.
>> I think this should be fixed ASAP either by allowing the hint address to
>> be ignored
>> (as suggested by the Linux man page), or hide this behavior behind an
>> mmap flag as you said.
>>
>>> - Charlie
>>>
>>>> See alsohttps://github.com/riscv-forks/electron/issues/4
>>>>
>>>>>> - Charlie
>>>> Sincerely,
>>>> Levi
>>>>
>>
>> I accidentally introduced some HTML into this reply so this reply is
>> resent as plain text.
>>
>> Sincerely,
>> Levi
^ 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