* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-09-03 7:54 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: <20240902164554.928371-2-cyphar@cyphar.com>
On Mon, Sep 2, 2024 at 6:46 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/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
>
> src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 99 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index d9c802ca9bd1..0ad591da632e 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -86,10 +86,16 @@ Examples:
> #include <errno.h>
> #include <linux/limits.h>
> #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.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 {
> @@ -120,6 +126,94 @@ void usage(void)
> exit(EXIT_FAILURE);
> }
>
> +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> +{
> + int ret;
> + int mntid_short;
> +
> + static bool skip_mntid_unique;
> +
> + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> + struct statx statxbuf;
> +
> + /* Get both the short and unique mount id. */
> + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
This fails build on top of latest for-next branch with commit
873e36c9 - statx.h: update to latest kernel UAPI
It can be fixed by changing to use the private xfstests_statx()
implementation, same as in stat_test.c.
I am not sure how elegant this is, but that's the easy fix.
> + 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 (!skip_mntid_unique) {
> + 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;
> + }
> + /*
> + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> + * kernel doesn't give us a unique mount ID just skip it.
> + */
> + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
This verbose print breaks all existing "exportfs" tests which do not
expect it in the golden output.
I understand that silently ignoring the failure is not good, but I also
would like to add this test coverage to all the existing tests.
One solution is to resurrect the command line option -M from v1,
but instead of meaning "test unique mount id" let it mean
"do not allow to skip unique mount id" test.
Then you can add a new test that runs open_by_handle -M, but also
implement a helper _require_unique_mntid similar to _require_btime
which is needed for the new test to run only on new kernels.
I'm sorry for this complication, but fstest is a testsuite that runs on
disto and stable kernels as well and we need to allow test coverage
of new features along with stability of the test env.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-09-03 9:08 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: <20240903.044647-some.sprint.silent.snacks-jdKnAVp7XuBZ@cyphar.com>
On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Mon, Sep 2, 2024 at 6:46 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/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > ---
> > > v2:
> > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> >
> > Looks good.
> >
> > You may add:
> >
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> > It'd be nice to get a verification that this is indeed tested on the latest
> > upstream and does not regress the tests that run the open_by_handle program.
>
> I've tested that the fallback works on mainline and correctly does the
> test on patched kernels (by running open_by_handle directly) but I
> haven't run the suite yet (still getting my mkosi testing setup working
> to run fstests...).
I am afraid this has to be tested.
I started testing myself and found that it breaks existing tests.
Even if you make the test completely opt-in as in v1 it need to be
tested and _notrun on old kernels.
If you have a new version, I can test it until you get your fstests setup
ready, because anyway I would want to check that your test also
works with overlayfs which has some specialized exportfs tests.
Test by running ./check -overlay -g exportfs, but I can also do that for you.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-03 9:11 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: <CAOQ4uxi291jBJ5ycZgiicVebjkcRQjhXJRgOgvSPBV4-TOcQvA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]
On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Sep 2, 2024 at 6:46 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/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > v2:
> > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> >
> > src/open_by_handle.c | 128 +++++++++++++++++++++++++++++++++----------
> > 1 file changed, 99 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index d9c802ca9bd1..0ad591da632e 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -86,10 +86,16 @@ Examples:
> > #include <errno.h>
> > #include <linux/limits.h>
> > #include <libgen.h>
> > +#include <stdint.h>
> > +#include <stdbool.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 {
> > @@ -120,6 +126,94 @@ void usage(void)
> > exit(EXIT_FAILURE);
> > }
> >
> > +int do_name_to_handle_at(const char *fname, struct file_handle *fh, int bufsz)
> > +{
> > + int ret;
> > + int mntid_short;
> > +
> > + static bool skip_mntid_unique;
> > +
> > + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> > + struct statx statxbuf;
> > +
> > + /* Get both the short and unique mount id. */
> > + if (statx(AT_FDCWD, fname, 0, STATX_MNT_ID, &statxbuf) < 0) {
>
> This fails build on top of latest for-next branch with commit
> 873e36c9 - statx.h: update to latest kernel UAPI
>
> It can be fixed by changing to use the private xfstests_statx()
> implementation, same as in stat_test.c.
>
> I am not sure how elegant this is, but that's the easy fix.
Ah, I was using master as the base. Sorry about that...
> > + 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 (!skip_mntid_unique) {
> > + 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;
> > + }
> > + /*
> > + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> > + * kernel doesn't give us a unique mount ID just skip it.
> > + */
> > + if ((skip_mntid_unique |= !(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)))
> > + printf("statx(STATX_MNT_ID_UNIQUE) not supported by running kernel -- skipping unique mount ID test\n");
>
> This verbose print breaks all existing "exportfs" tests which do not
> expect it in the golden output.
>
> I understand that silently ignoring the failure is not good, but I also
> would like to add this test coverage to all the existing tests.
>
> One solution is to resurrect the command line option -M from v1,
> but instead of meaning "test unique mount id" let it mean
> "do not allow to skip unique mount id" test.
>
> Then you can add a new test that runs open_by_handle -M, but also
> implement a helper _require_unique_mntid similar to _require_btime
> which is needed for the new test to run only on new kernels.
>
> I'm sorry for this complication, but fstest is a testsuite that runs on
> disto and stable kernels as well and we need to allow test coverage
> of new features along with stability of the test env.
No worries, I'll write it up. I'm not familiar with the exact
requirements of xfstests, sorry for the noise! (^_^")
>
> 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: [PATCH] Add provision to busyloop for events in ep_poll.
From: Joe Damato @ 2024-09-04 15:58 UTC (permalink / raw)
To: Naman Gulati
Cc: Alexander Viro, Christian Brauner, Jan Kara, David S. Miller,
Eric Dumazet, netdev, Stanislav Fomichev, linux-kernel, skhawaja,
Willem de Bruijn, mkarsten, linux-api
In-Reply-To: <CAMP57yUuvvE-n-Xx--GRUsHLC2n4LgaNF=uViDhggvbG=5r9zQ@mail.gmail.com>
On Tue, Sep 03, 2024 at 07:18:14PM -0700, Naman Gulati wrote:
> Thanks all for the comments and apologies for the delay in replying.
> Stanislav and Joe I’ve addressed some of the common concerns below.
>
> On Thu, Aug 29, 2024 at 3:40 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Wed, Aug 28, 2024 at 06:10:11PM +0000, Naman Gulati wrote:
> > > NAPI busypolling in ep_busy_loop loops on napi_poll and checks for new
> > > epoll events after every napi poll. Checking just for epoll events in a
> > > tight loop in the kernel context delivers latency gains to applications
> > > that are not interested in napi busypolling with epoll.
> > >
> > > This patch adds an option to loop just for new events inside
> > > ep_busy_loop, guarded by the EPIOCSPARAMS ioctl that controls epoll napi
> > > busypolling.
> >
> > This makes an API change, so I think that linux-api@vger.kernel.org
> > needs to be CC'd ?
> >
> > > A comparison with neper tcp_rr shows that busylooping for events in
> > > epoll_wait boosted throughput by ~3-7% and reduced median latency by
> > > ~10%.
> > >
> > > To demonstrate the latency and throughput improvements, a comparison was
> > > made of neper tcp_rr running with:
> > > 1. (baseline) No busylooping
> >
> > Is there NAPI-based steering to threads via SO_INCOMING_NAPI_ID in
> > this case? More details, please, on locality. If there is no
> > NAPI-based flow steering in this case, perhaps the improvements you
> > are seeing are a result of both syscall overhead avoidance and data
> > locality?
>
> The benchmarks were run with no NAPI steering.
>
> Regarding syscall overhead, I reproduced the above experiment with
> mitigations=off
> and found similar results as above. Pointing to the fact that the above
> gains are
> materialized from more than just avoiding syscall overhead.
I think the cover letter needs more benchmark cases, data points,
and documentation about precisely how the benchmark is being run and
configured. The data feels quite sparse, the explanation of the
experiment leaves too much out, and - with respect - I am not
convinced by the results posted.
> By locality are you referring to Numa locality?
CPU cache locality.
> >
> > > 2. (epoll busylooping) enabling the epoll busy looping on all epoll
> > > fd's
> >
> > This is the case you've added, event_poll_only ? It seems like in
> > this case you aren't busy looping exactly, you are essentially
> > allowing IRQ/softIRQ to drive processing and checking on wakeup that
> > events are available.
> >
> > IMHO, I'm not sure if "epoll busylooping" is an appropriate
> > description.
>
> I see your point, perhaps "spinning" or just “looping” could be a closer
> word?
I don't know how to describe the patch, to be totally honest. I
think the basis of the mechanism needs to be more thoroughly
understood. See below.
> >
> > > 3. (userspace busylooping) looping on epoll_wait in userspace
> > > with timeout=0
> >
> > Same question as Stanislav; timeout=0 should get ep_loop to transfer
> > events immediately (if there are any) and return without actually
> > invoking busy poll. So, it would seem that your ioctl change
> > shouldn't be necessary since the equivalent behavior is already
> > possible with timeout=0.
> >
> > I'd probably investigate both syscall overhead and data locality
> > before approving this patch because it seems a bit suspicious to me.
> >
> > >
> > > Stats for two machines with 100Gbps NICs running tcp_rr with 5 threads
> > > and varying flows:
> > >
> > > Type Flows Throughput Latency (μs)
> > > (B/s) P50 P90 P99 P99.9
> P99.99
> > > baseline 15 272145 57.2 71.9 91.4 100.6
> 111.6
> > > baseline 30 464952 66.8 78.8 98.1 113.4
> 122.4
> > > baseline 60 695920 80.9 118.5 143.4 161.8
> 174.6
> > > epoll busyloop 15 301751 44.7 70.6 84.3 95.4
> 106.5
> > > epoll busyloop 30 508392 58.9 76.9 96.2 109.3
> 118.5
> > > epoll busyloop 60 745731 77.4 106.2 127.5 143.1
> 155.9
> > > userspace busyloop 15 279202 55.4 73.1 85.2 98.3
> 109.6
> > > userspace busyloop 30 472440 63.7 78.2 96.5 112.2
> 120.1
> > > userspace busyloop 60 720779 77.9 113.5 134.9 152.6
> 165.7
> > >
> > > Per the above data epoll busyloop outperforms baseline and userspace
> > > busylooping in both throughput and latency. As the density of flows per
> > > thread increased, the median latency of all three epoll mechanisms
> > > converges. However epoll busylooping is better at capturing the tail
> > > latencies at high flow counts.
> > >
> > > Signed-off-by: Naman Gulati <namangulati@google.com>
> > > ---
> > > fs/eventpoll.c | 53 ++++++++++++++++++++++++++--------
> > > include/uapi/linux/eventpoll.h | 3 +-
> > > 2 files changed, 43 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > > index f53ca4f7fcedd..6cba79261817a 100644
> > > --- a/fs/eventpoll.c
> > > +++ b/fs/eventpoll.c
> > > @@ -232,7 +232,10 @@ struct eventpoll {
> > > u32 busy_poll_usecs;
> > > /* busy poll packet budget */
> > > u16 busy_poll_budget;
> > > - bool prefer_busy_poll;
> > > + /* prefer to busypoll in napi poll */
> > > + bool napi_prefer_busy_poll;
> >
> > Adding napi seems slightly redundant to me but I could be convinced either
> > way, I suppose.
>
> With the two different polling booleans in the struct, I felt it's better
> to be explicit
> about the scope of each.
>
> >
> > > + /* avoid napi poll when busy looping and poll only for events */
> > > + bool event_poll_only;
> >
> > I'm not sure about this overall; this isn't exactly what I think of
> > when I think about the word "polling" but maybe I'm being too
> > nit-picky.
>
> I'm not sure how else to categorize the operation, is there some other
> phrasing
> you'd recommend?
>
> >
> > > #endif
> > >
> > > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > @@ -430,6 +433,24 @@ static bool ep_busy_loop_end(void *p, unsigned
> long start_time)
> > > return ep_events_available(ep) ||
> busy_loop_ep_timeout(start_time, ep);
> > > }
> > >
> > > +/**
> > > + * ep_event_busy_loop - loop until events available or busy poll
> > > + * times out.
> > > + *
> > > + * @ep: Pointer to the eventpoll context.
> > > + *
> > > + * Return: true if events available, false otherwise.
> > > + */
> > > +static bool ep_event_busy_loop(struct eventpoll *ep)
> > > +{
> > > + unsigned long start_time = busy_loop_current_time();
> > > +
> > > + while (!ep_busy_loop_end(ep, start_time))
> > > + cond_resched();
> > > +
> > > + return ep_events_available(ep);
> > > +}
> > > +
> > > /*
> > > * Busy poll if globally on and supporting sockets found && no events,
> > > * busy loop will return if need_resched or ep_events_available.
> > > @@ -440,23 +461,29 @@ static bool ep_busy_loop(struct eventpoll *ep,
> int nonblock)
> > > {
> > > unsigned int napi_id = READ_ONCE(ep->napi_id);
> > > u16 budget = READ_ONCE(ep->busy_poll_budget);
> > > - bool prefer_busy_poll = READ_ONCE(ep->prefer_busy_poll);
> > > + bool event_poll_only = READ_ONCE(ep->event_poll_only);
> > >
> > > if (!budget)
> > > budget = BUSY_POLL_BUDGET;
> > >
> > > - if (napi_id >= MIN_NAPI_ID && ep_busy_loop_on(ep)) {
> > > + if (!ep_busy_loop_on(ep))
> > > + return false;
> > > +
> > > + if (event_poll_only) {
> > > + return ep_event_busy_loop(ep);
> > > + } else if (napi_id >= MIN_NAPI_ID) {
> > > + bool napi_prefer_busy_poll =
> READ_ONCE(ep->napi_prefer_busy_poll);
> > > +
> > > napi_busy_loop(napi_id, nonblock ? NULL :
> ep_busy_loop_end,
> > > - ep, prefer_busy_poll, budget);
> > > + ep, napi_prefer_busy_poll, budget);
> > > if (ep_events_available(ep))
> > > return true;
> > > /*
> > > - * Busy poll timed out. Drop NAPI ID for now, we can add
> > > - * it back in when we have moved a socket with a valid
> NAPI
> > > - * ID onto the ready list.
> > > - */
> > > + * Busy poll timed out. Drop NAPI ID for now, we can add
> > > + * it back in when we have moved a socket with a valid NAPI
> > > + * ID onto the ready list.
> > > + */
> > > ep->napi_id = 0;
> > > - return false;
> > > }
> > > return false;
> > > }
> > > @@ -523,13 +550,15 @@ static long ep_eventpoll_bp_ioctl(struct file
> *file, unsigned int cmd,
> > >
> > > WRITE_ONCE(ep->busy_poll_usecs,
> epoll_params.busy_poll_usecs);
> > > WRITE_ONCE(ep->busy_poll_budget,
> epoll_params.busy_poll_budget);
> > > - WRITE_ONCE(ep->prefer_busy_poll,
> epoll_params.prefer_busy_poll);
> > > + WRITE_ONCE(ep->napi_prefer_busy_poll,
> epoll_params.prefer_busy_poll);
> > > + WRITE_ONCE(ep->event_poll_only,
> epoll_params.event_poll_only);
> > > return 0;
> > > case EPIOCGPARAMS:
> > > memset(&epoll_params, 0, sizeof(epoll_params));
> > > epoll_params.busy_poll_usecs =
> READ_ONCE(ep->busy_poll_usecs);
> > > epoll_params.busy_poll_budget =
> READ_ONCE(ep->busy_poll_budget);
> > > - epoll_params.prefer_busy_poll =
> READ_ONCE(ep->prefer_busy_poll);
> > > + epoll_params.prefer_busy_poll =
> READ_ONCE(ep->napi_prefer_busy_poll);
> > > + epoll_params.event_poll_only =
> READ_ONCE(ep->event_poll_only);
> > > if (copy_to_user(uarg, &epoll_params,
> sizeof(epoll_params)))
> > > return -EFAULT;
> > > return 0;
> > > @@ -2203,7 +2232,7 @@ static int do_epoll_create(int flags)
> > > #ifdef CONFIG_NET_RX_BUSY_POLL
> > > ep->busy_poll_usecs = 0;
> > > ep->busy_poll_budget = 0;
> > > - ep->prefer_busy_poll = false;
> > > + ep->napi_prefer_busy_poll = false;
> > > #endif
> >
> > Just FYI: This is going to conflict with a patch I've sent to VFS
> > that hasn't quite made its way back to net-next just yet.
> >
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.misc&id=4eb76c5d9a8851735fd3ec5833ecf412e8921655
> >
> Acknowledged.
>
> > > ep->file = file;
> > > fd_install(fd, file);
> > > diff --git a/include/uapi/linux/eventpoll.h
> b/include/uapi/linux/eventpoll.h
> > > index 4f4b948ef3811..3bc0f4eed976c 100644
> > > --- a/include/uapi/linux/eventpoll.h
> > > +++ b/include/uapi/linux/eventpoll.h
> > > @@ -89,9 +89,10 @@ struct epoll_params {
> > > __u32 busy_poll_usecs;
> > > __u16 busy_poll_budget;
> > > __u8 prefer_busy_poll;
> > > + __u8 event_poll_only:1;
> > >
> > > /* pad the struct to a multiple of 64bits */
> > > - __u8 __pad;
> > > + __u8 __pad:7;
> > > };
> >
> > If the above is accepted then a similar change should make its way
> > into glibc, uclibc-ng, and musl. It might be easier to add an
> > entirely new ioctl.
>
> Adding a new ioctl seems preferable, I can look into reworking the code
> accordingly.
My advice is prior to reworking the code: figure out why the results
show an improvement because I can't really see a clear explanation.
If I understand the patch correctly (and perhaps I've gotten it
wrong) it is proposing to add a new mechanism for packet processing
that is quite strange:
Let softIRQ do the work (so there's no 'polling'), but avoid
returning to userland and call cond_resched() to defer execution
to other things until there's events to be returned.
Couldn't this be approximated with a smaller batch size (maxevents)
and a timeout of 0?
Perhaps consider comparing performance with a more real world
example (i.e. not tcp_rr) and use NAPI-steering in the base case to
eliminate cache locality as a variable? Maybe even compare against
the suspend mechanism Martin and I proposed?
- Joe
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-04 16:30 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: <CAOQ4uxhXa-1Xjd58p8oGd9Q4hgfDtGnae1YrmDWwQp3t5uGHeg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3218 bytes --]
On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Mon, Sep 2, 2024 at 6:46 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/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > ---
> > > > v2:
> > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > >
> > > Looks good.
> > >
> > > You may add:
> > >
> > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > It'd be nice to get a verification that this is indeed tested on the latest
> > > upstream and does not regress the tests that run the open_by_handle program.
> >
> > I've tested that the fallback works on mainline and correctly does the
> > test on patched kernels (by running open_by_handle directly) but I
> > haven't run the suite yet (still getting my mkosi testing setup working
> > to run fstests...).
>
> I am afraid this has to be tested.
> I started testing myself and found that it breaks existing tests.
> Even if you make the test completely opt-in as in v1 it need to be
> tested and _notrun on old kernels.
>
> If you have a new version, I can test it until you get your fstests setup
> ready, because anyway I would want to check that your test also
> works with overlayfs which has some specialized exportfs tests.
> Test by running ./check -overlay -g exportfs, but I can also do that for you.
I managed to get fstests running, sorry about that...
For the v3 I have ready (which includes a new test using -M), the
following runs work in my VM:
- ./check -g exportfs
- ./check -overlay -g exportfs
Should I check anything else before sending it?
Also, when running the tests I think I may have found a bug? Using
overlayfs+xfs leads to the following error when doing ./check -overlay
if the scratch device is XFS:
./common/rc: line 299: _xfs_has_feature: command not found
not run: upper fs needs to support d_type
The fix I applied was simply:
diff --git a/common/rc b/common/rc
index 0beaf2ff1126..e6af1b16918f 100644
--- a/common/rc
+++ b/common/rc
@@ -296,6 +296,7 @@ _supports_filetype()
local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
case "$fstyp" in
xfs)
+ . common/xfs
_xfs_has_feature $dir ftype
;;
ext2|ext3|ext4)
Should I include this patch as well, or did I make a mistake somewhere?
(I could add the import to the top instead if you'd prefer that.)
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 related
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Amir Goldstein @ 2024-09-04 16:44 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: <20240904.162424-novel.fangs.vital.nursery-BQvjXGlIi7vb@cyphar.com>
On Wed, Sep 4, 2024 at 6:31 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > On Mon, Sep 2, 2024 at 6:46 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/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > > ---
> > > > > v2:
> > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > > >
> > > > Looks good.
> > > >
> > > > You may add:
> > > >
> > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > It'd be nice to get a verification that this is indeed tested on the latest
> > > > upstream and does not regress the tests that run the open_by_handle program.
> > >
> > > I've tested that the fallback works on mainline and correctly does the
> > > test on patched kernels (by running open_by_handle directly) but I
> > > haven't run the suite yet (still getting my mkosi testing setup working
> > > to run fstests...).
> >
> > I am afraid this has to be tested.
> > I started testing myself and found that it breaks existing tests.
> > Even if you make the test completely opt-in as in v1 it need to be
> > tested and _notrun on old kernels.
> >
> > If you have a new version, I can test it until you get your fstests setup
> > ready, because anyway I would want to check that your test also
> > works with overlayfs which has some specialized exportfs tests.
> > Test by running ./check -overlay -g exportfs, but I can also do that for you.
>
> I managed to get fstests running, sorry about that...
>
> For the v3 I have ready (which includes a new test using -M), the
> following runs work in my VM:
>
> - ./check -g exportfs
> - ./check -overlay -g exportfs
>
> Should I check anything else before sending it?
>
That should be enough.
So you have one new test that does not run on upstream kernel
and runs and passes on patched kernel?
> Also, when running the tests I think I may have found a bug? Using
> overlayfs+xfs leads to the following error when doing ./check -overlay
> if the scratch device is XFS:
>
> ./common/rc: line 299: _xfs_has_feature: command not found
> not run: upper fs needs to support d_type
>
> The fix I applied was simply:
>
> diff --git a/common/rc b/common/rc
> index 0beaf2ff1126..e6af1b16918f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -296,6 +296,7 @@ _supports_filetype()
> local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
> case "$fstyp" in
> xfs)
> + . common/xfs
> _xfs_has_feature $dir ftype
> ;;
> ext2|ext3|ext4)
>
> Should I include this patch as well, or did I make a mistake somewhere?
> (I could add the import to the top instead if you'd prefer that.)
This should already be handled by
if [ -n "$OVL_BASE_FSTYP" ];then
_source_specific_fs $OVL_BASE_FSTYP
fi
in common/overlay
I think what you are missing is to
export FSTYP=xfs
as README.overlay suggests.
It's true that ./check does not *require* defining FSTYP
and can auto detect the test filesystem, but for running -overlay
is it a requirement to define the base FSTYP.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH xfstests v2 2/2] open_by_handle: add tests for u64 mount ID
From: Aleksa Sarai @ 2024-09-04 17:53 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: <CAOQ4uxg7EgOH5s_RZz27XpVSwgWu9bROT9JRzTycxi8D2_3d3A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]
On 2024-09-04, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Sep 4, 2024 at 6:31 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > On 2024-09-03, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Tue, Sep 3, 2024 at 8:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >
> > > > On 2024-09-02, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > On Mon, Sep 2, 2024 at 6:46 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/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> > > > > > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > > > ---
> > > > > > v2:
> > > > > > - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> > > > > > - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> > > > > > or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> > > > > > - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> > > > >
> > > > > Looks good.
> > > > >
> > > > > You may add:
> > > > >
> > > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > It'd be nice to get a verification that this is indeed tested on the latest
> > > > > upstream and does not regress the tests that run the open_by_handle program.
> > > >
> > > > I've tested that the fallback works on mainline and correctly does the
> > > > test on patched kernels (by running open_by_handle directly) but I
> > > > haven't run the suite yet (still getting my mkosi testing setup working
> > > > to run fstests...).
> > >
> > > I am afraid this has to be tested.
> > > I started testing myself and found that it breaks existing tests.
> > > Even if you make the test completely opt-in as in v1 it need to be
> > > tested and _notrun on old kernels.
> > >
> > > If you have a new version, I can test it until you get your fstests setup
> > > ready, because anyway I would want to check that your test also
> > > works with overlayfs which has some specialized exportfs tests.
> > > Test by running ./check -overlay -g exportfs, but I can also do that for you.
> >
> > I managed to get fstests running, sorry about that...
> >
> > For the v3 I have ready (which includes a new test using -M), the
> > following runs work in my VM:
> >
> > - ./check -g exportfs
> > - ./check -overlay -g exportfs
> >
> > Should I check anything else before sending it?
> >
>
> That should be enough.
> So you have one new test that does not run on upstream kernel
> and runs and passes on patched kernel?
Yes. Both of those suite runs succeed without issues on v6.6.49,
v6.11-rc6 and with the AT_HANDLE_MNT_ID_UNIQUE patches.
I also added skipping logic such that it _should_ work on pre-5.8
kernels (pre-STATX_MNT_ID) as well, but I can't test that at the moment
(mkosi-kernel fails to boot kernels that old it seems...).
I'll send it now.
> > Also, when running the tests I think I may have found a bug? Using
> > overlayfs+xfs leads to the following error when doing ./check -overlay
> > if the scratch device is XFS:
> >
> > ./common/rc: line 299: _xfs_has_feature: command not found
> > not run: upper fs needs to support d_type
> >
> > The fix I applied was simply:
> >
> > diff --git a/common/rc b/common/rc
> > index 0beaf2ff1126..e6af1b16918f 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -296,6 +296,7 @@ _supports_filetype()
> > local fstyp=`$DF_PROG $dir | tail -1 | $AWK_PROG '{print $2}'`
> > case "$fstyp" in
> > xfs)
> > + . common/xfs
> > _xfs_has_feature $dir ftype
> > ;;
> > ext2|ext3|ext4)
> >
> > Should I include this patch as well, or did I make a mistake somewhere?
> > (I could add the import to the top instead if you'd prefer that.)
>
> This should already be handled by
> if [ -n "$OVL_BASE_FSTYP" ];then
> _source_specific_fs $OVL_BASE_FSTYP
> fi
>
> in common/overlay
>
> I think what you are missing is to
> export FSTYP=xfs
> as README.overlay suggests.
>
> It's true that ./check does not *require* defining FSTYP
> and can auto detect the test filesystem, but for running -overlay
> is it a requirement to define the base FSTYP.
Ah okay, I missed that. That fixed the issue, 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
* [PATCH xfstests v3 1/2] open_by_handle: verify u32 and u64 mount IDs
From: Aleksa Sarai @ 2024-09-04 17:56 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>
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. Also, add automatic tests for the old u32 mount IDs as well.
By default, we do mount ID checks but silently skip the tests if the
syscalls are not supported by the running kernel (to ensure the tests
continue to work for old kernels). We will add some tests explicitly
checking the new features (with no silent skipping) in a future patch.
The u32 mount ID tests require STATX_MNT_ID (Linux 5.8), while the u64
mount ID tests require STATX_MNT_ID_UNIQUE (Linux 6.9) and
AT_HANDLE_MNT_ID_UNIQUE (linux-next).
Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changed in v3:
- Make skipping completely silent in regular open_by_handle mode. [Amir Goldstein]
- Re-add -M to turn skipping into errors and add a new test that uses
-M, but is skipped on older kernels. [Amir Goldstein]
- v2: <https://lore.kernel.org/all/20240902164554.928371-1-cyphar@cyphar.com/>
Changed in v2:
- Remove -M argument and always do the mount ID tests. [Amir Goldstein]
- Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
- v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
---
src/open_by_handle.c | 132 +++++++++++++++++++++++++++++++++----------
1 file changed, 103 insertions(+), 29 deletions(-)
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..920ec7d9170b 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -87,6 +87,15 @@ Examples:
#include <errno.h>
#include <linux/limits.h>
#include <libgen.h>
+#include <stdint.h>
+#include <stdbool.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
@@ -108,6 +117,7 @@ void usage(void)
fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
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 -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\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] - 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");
@@ -118,6 +128,94 @@ void usage(void)
exit(EXIT_FAILURE);
}
+static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
+ int bufsz)
+{
+ int ret;
+ int mntid_short;
+
+ static bool skip_mntid, skip_mntid_unique;
+
+ uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+ struct statx statxbuf;
+
+ /* Get both the short and unique mount id. */
+ if (!skip_mntid) {
+ if (xfstests_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))
+ skip_mntid = true;
+ else
+ statx_mntid_short = statxbuf.stx_mnt_id;
+ }
+
+ if (!skip_mntid_unique) {
+ if (xfstests_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;
+ }
+ /*
+ * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
+ * kernel doesn't give us a unique mount ID just skip it.
+ */
+ if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+ skip_mntid_unique = true;
+ else
+ 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 (!skip_mntid) {
+ 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;
+ }
+ }
+
+ if (!skip_mntid_unique) {
+ struct handle dummy_fh;
+ uint64_t mntid_unique = 0;
+
+ /*
+ * 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;
+ ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
+ if (ret < 0) {
+ if (errno != EINVAL) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+ skip_mntid_unique = true;
+ } else {
+ 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;
@@ -130,7 +228,7 @@ 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;
@@ -305,21 +403,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);
+ if (ret)
return EXIT_FAILURE;
- }
}
if (keepopen) {
/* Open without close to keep unlinked files around */
@@ -347,21 +433,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);
+ if (ret)
return EXIT_FAILURE;
- }
}
if (out_fd) {
ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
--
2.46.0
^ permalink raw reply related
* [PATCH xfstests v3 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly
From: Aleksa Sarai @ 2024-09-04 17:56 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: <20240904175639.2269694-1-cyphar@cyphar.com>
In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
add a test (based on generic/426) which runs the open_by_handle in a
mode where it will error out if there is a problem with getting mount
IDs. The test is skipped if the kernel doesn't support the necessary
features.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
common/rc | 24 ++++++++++++++++
src/open_by_handle.c | 63 ++++++++++++++++++++++++++++++++++-------
tests/generic/756 | 65 +++++++++++++++++++++++++++++++++++++++++++
tests/generic/756.out | 5 ++++
4 files changed, 147 insertions(+), 10 deletions(-)
create mode 100755 tests/generic/756
create mode 100644 tests/generic/756.out
diff --git a/common/rc b/common/rc
index 9da9fe188297..0beaf2ff1126 100644
--- a/common/rc
+++ b/common/rc
@@ -5178,6 +5178,30 @@ _require_fibmap()
rm -f $file
}
+_require_statx_unique_mountid()
+{
+ # statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
+ # statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
+ # We only need to check the latter.
+
+ export STATX_MNT_ID_UNIQUE=0x4000
+ local statx_mask=$(
+ ${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
+ sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
+ )
+
+ [[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
+ _notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
+}
+
+_require_open_by_handle_unique_mountid()
+{
+ _require_test_program "open_by_handle"
+
+ $here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
+ || _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
+}
+
_try_wipe_scratch_devs()
{
test -x "$WIPEFS_PROG" || return 0
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 920ec7d9170b..b5c1a30abbbc 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -106,9 +106,11 @@ 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, " open_by_handle -C <feature>\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 -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");
fprintf(stderr, "open_by_handle -n <test_dir> [N] - get file handles of test files and try to open by handle without drop caches\n");
fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
@@ -117,19 +119,23 @@ void usage(void)
fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
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 -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\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] - rename test files, drop caches and try to open by handle\n");
+ fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\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");
fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
+ fprintf(stderr, "\n");
+ fprintf(stderr, "open_by_handle -C <feature> - check if <feature> is supported by the kernel.\n");
+ fprintf(stderr, " <feature> can be any of the following values:\n");
+ fprintf(stderr, " - AT_HANDLE_MNT_ID_UNIQUE\n");
exit(EXIT_FAILURE);
}
static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
- int bufsz)
+ int bufsz, bool force_check_mountid)
{
int ret;
int mntid_short;
@@ -145,10 +151,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
return EXIT_FAILURE;
}
- if (!(statxbuf.stx_mask & STATX_MNT_ID))
+ if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+ if (force_check_mountid) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
+ return EXIT_FAILURE;
+ }
skip_mntid = true;
- else
+ } else {
statx_mntid_short = statxbuf.stx_mnt_id;
+ }
}
if (!skip_mntid_unique) {
@@ -160,10 +171,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
* STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
* kernel doesn't give us a unique mount ID just skip it.
*/
- if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+ if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
+ if (force_check_mountid) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+ return EXIT_FAILURE;
+ }
skip_mntid_unique = true;
- else
+ } else {
statx_mntid_unique = statxbuf.stx_mnt_id;
+ }
}
fh->handle_bytes = bufsz;
@@ -204,6 +220,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
return EXIT_FAILURE;
}
/* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+ if (force_check_mountid) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+ return EXIT_FAILURE;
+ }
skip_mntid_unique = true;
} else {
if (mntid_unique != statx_mntid_unique) {
@@ -216,6 +236,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
return 0;
}
+static int check_feature(const char *feature)
+{
+ if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
+ int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
+ /* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
+ if (ret < 0 && errno == EINVAL) {
+ fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
+ return EXIT_FAILURE;
+ }
+ return 0;
+ }
+
+ fprintf(stderr, "unknown feature name '%s'\n", feature);
+ return EXIT_FAILURE;
+}
+
int main(int argc, char **argv)
{
int i, c;
@@ -235,16 +271,20 @@ int main(int argc, char **argv)
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 force_check_mountid = 0;
int bufsz = MAX_HANDLE_SZ;
if (argc < 2)
usage();
- while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
+ while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
switch (c) {
case 'c':
create = 1;
break;
+ case 'C':
+ /* Check kernel feature support. */
+ return check_feature(optarg);
case 'w':
/* Write data before open_by_handle_at() */
wr = 1;
@@ -271,6 +311,9 @@ int main(int argc, char **argv)
case 'm':
move = 1;
break;
+ case 'M':
+ force_check_mountid = 1;
+ break;
case 'p':
parent = 1;
break;
@@ -403,7 +446,7 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+ ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
if (ret)
return EXIT_FAILURE;
}
@@ -433,7 +476,7 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+ ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
if (ret)
return EXIT_FAILURE;
}
diff --git a/tests/generic/756 b/tests/generic/756
new file mode 100755
index 000000000000..c7a82cfd25f4
--- /dev/null
+++ b/tests/generic/756
@@ -0,0 +1,65 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
+#
+# FS QA Test No. 756
+#
+# Check stale handles pointing to unlinked files and non-stale handles pointing
+# to linked files while verifying that u64 mount IDs are correctly returned.
+#
+. ./common/preamble
+_begin_fstest auto quick exportfs
+
+# Import common functions.
+. ./common/filter
+
+
+# Modify as appropriate.
+_require_test
+# _require_exportfs and already requires open_by_handle, but let's not count on it
+_require_test_program "open_by_handle"
+_require_exportfs
+# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
+_require_statx_unique_mountid
+_require_open_by_handle_unique_mountid
+
+NUMFILES=1024
+testdir=$TEST_DIR/$seq-dir
+mkdir -p $testdir
+
+# Create empty test files in test dir
+create_test_files()
+{
+ local dir=$1
+
+ mkdir -p $dir
+ rm -f $dir/*
+ $here/src/open_by_handle -c $dir $NUMFILES
+}
+
+# Test encode/decode file handles
+test_file_handles()
+{
+ local dir=$1
+ local opt=$2
+
+ echo test_file_handles $* | _filter_test_dir
+ $here/src/open_by_handle $opt $dir $NUMFILES
+}
+
+# Check stale handles to deleted files
+create_test_files $testdir
+test_file_handles $testdir -Md
+
+# Check non-stale handles to linked files
+create_test_files $testdir
+test_file_handles $testdir -M
+
+# Check non-stale handles to files that were hardlinked and original deleted
+create_test_files $testdir
+test_file_handles $testdir -Ml
+test_file_handles $testdir -Mu
+
+status=0
+exit
diff --git a/tests/generic/756.out b/tests/generic/756.out
new file mode 100644
index 000000000000..48aed88d87b9
--- /dev/null
+++ b/tests/generic/756.out
@@ -0,0 +1,5 @@
+QA output created by 756
+test_file_handles TEST_DIR/756-dir -Md
+test_file_handles TEST_DIR/756-dir -M
+test_file_handles TEST_DIR/756-dir -Ml
+test_file_handles TEST_DIR/756-dir -Mu
--
2.46.0
^ permalink raw reply related
* Re: [PATCH xfstests v3 1/2] open_by_handle: verify u32 and u64 mount IDs
From: Amir Goldstein @ 2024-09-04 18:29 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: <20240904175639.2269694-1-cyphar@cyphar.com>
On Wed, Sep 4, 2024 at 7:57 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. Also, add automatic tests for the old u32 mount IDs as well.
>
> By default, we do mount ID checks but silently skip the tests if the
> syscalls are not supported by the running kernel (to ensure the tests
> continue to work for old kernels). We will add some tests explicitly
> checking the new features (with no silent skipping) in a future patch.
>
> The u32 mount ID tests require STATX_MNT_ID (Linux 5.8), while the u64
> mount ID tests require STATX_MNT_ID_UNIQUE (Linux 6.9) and
> AT_HANDLE_MNT_ID_UNIQUE (linux-next).
>
> Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> Changed in v3:
> - Make skipping completely silent in regular open_by_handle mode. [Amir Goldstein]
> - Re-add -M to turn skipping into errors and add a new test that uses
> -M, but is skipped on older kernels. [Amir Goldstein]
> - v2: <https://lore.kernel.org/all/20240902164554.928371-1-cyphar@cyphar.com/>
> Changed in v2:
> - Remove -M argument and always do the mount ID tests. [Amir Goldstein]
> - Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
> or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
> - v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
> ---
> src/open_by_handle.c | 132 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 103 insertions(+), 29 deletions(-)
>
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 0f74ed08b1f0..920ec7d9170b 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -87,6 +87,15 @@ Examples:
> #include <errno.h>
> #include <linux/limits.h>
> #include <libgen.h>
> +#include <stdint.h>
> +#include <stdbool.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
>
> @@ -108,6 +117,7 @@ void usage(void)
> fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
> 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 -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\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] - 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");
> @@ -118,6 +128,94 @@ void usage(void)
> exit(EXIT_FAILURE);
> }
>
> +static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> + int bufsz)
> +{
> + int ret;
> + int mntid_short;
> +
> + static bool skip_mntid, skip_mntid_unique;
> +
> + uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
> + struct statx statxbuf;
> +
> + /* Get both the short and unique mount id. */
> + if (!skip_mntid) {
> + if (xfstests_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))
> + skip_mntid = true;
> + else
> + statx_mntid_short = statxbuf.stx_mnt_id;
> + }
> +
> + if (!skip_mntid_unique) {
> + if (xfstests_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;
> + }
> + /*
> + * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> + * kernel doesn't give us a unique mount ID just skip it.
> + */
> + if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
> + skip_mntid_unique = true;
> + else
> + 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 (!skip_mntid) {
> + 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;
> + }
> + }
> +
> + if (!skip_mntid_unique) {
> + struct handle dummy_fh;
> + uint64_t mntid_unique = 0;
> +
> + /*
> + * 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;
> + ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
> + if (ret < 0) {
> + if (errno != EINVAL) {
> + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
> + return EXIT_FAILURE;
> + }
> + /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
> + skip_mntid_unique = true;
> + } else {
> + 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;
> @@ -130,7 +228,7 @@ 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;
> @@ -305,21 +403,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);
> + if (ret)
> return EXIT_FAILURE;
> - }
> }
> if (keepopen) {
> /* Open without close to keep unlinked files around */
> @@ -347,21 +433,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);
> + if (ret)
> return EXIT_FAILURE;
> - }
> }
> if (out_fd) {
> ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
> --
> 2.46.0
>
^ permalink raw reply
* Re: [PATCH xfstests v3 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly
From: Amir Goldstein @ 2024-09-04 18:49 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: <20240904175639.2269694-2-cyphar@cyphar.com>
On Wed, Sep 4, 2024 at 7:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
> add a test (based on generic/426) which runs the open_by_handle in a
> mode where it will error out if there is a problem with getting mount
> IDs. The test is skipped if the kernel doesn't support the necessary
> features.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Apart from one minor nits below, you may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> common/rc | 24 ++++++++++++++++
> src/open_by_handle.c | 63 ++++++++++++++++++++++++++++++++++-------
> tests/generic/756 | 65 +++++++++++++++++++++++++++++++++++++++++++
> tests/generic/756.out | 5 ++++
> 4 files changed, 147 insertions(+), 10 deletions(-)
> create mode 100755 tests/generic/756
> create mode 100644 tests/generic/756.out
>
> diff --git a/common/rc b/common/rc
> index 9da9fe188297..0beaf2ff1126 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5178,6 +5178,30 @@ _require_fibmap()
> rm -f $file
> }
>
> +_require_statx_unique_mountid()
> +{
> + # statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
> + # statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
> + # We only need to check the latter.
> +
> + export STATX_MNT_ID_UNIQUE=0x4000
> + local statx_mask=$(
> + ${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
> + sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
> + )
> +
> + [[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
> + _notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
> +}
> +
> +_require_open_by_handle_unique_mountid()
> +{
> + _require_test_program "open_by_handle"
> +
> + $here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
> + || _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
> +}
> +
> _try_wipe_scratch_devs()
> {
> test -x "$WIPEFS_PROG" || return 0
> diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> index 920ec7d9170b..b5c1a30abbbc 100644
> --- a/src/open_by_handle.c
> +++ b/src/open_by_handle.c
> @@ -106,9 +106,11 @@ 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, " open_by_handle -C <feature>\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 -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");
> fprintf(stderr, "open_by_handle -n <test_dir> [N] - get file handles of test files and try to open by handle without drop caches\n");
> fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
> @@ -117,19 +119,23 @@ void usage(void)
> fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
> 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 -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");
I guess this was not supposed to be in the first patch
> 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] - rename test files, drop caches and try to open by handle\n");
> + fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\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");
> fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
> fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
> fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
> + fprintf(stderr, "\n");
> + fprintf(stderr, "open_by_handle -C <feature> - check if <feature> is supported by the kernel.\n");
> + fprintf(stderr, " <feature> can be any of the following values:\n");
> + fprintf(stderr, " - AT_HANDLE_MNT_ID_UNIQUE\n");
> exit(EXIT_FAILURE);
> }
>
> static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> - int bufsz)
> + int bufsz, bool force_check_mountid)
> {
> int ret;
> int mntid_short;
> @@ -145,10 +151,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> return EXIT_FAILURE;
> }
> - if (!(statxbuf.stx_mask & STATX_MNT_ID))
> + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> + if (force_check_mountid) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
> + return EXIT_FAILURE;
> + }
> skip_mntid = true;
> - else
> + } else {
> statx_mntid_short = statxbuf.stx_mnt_id;
> + }
> }
>
> if (!skip_mntid_unique) {
> @@ -160,10 +171,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> * kernel doesn't give us a unique mount ID just skip it.
> */
> - if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
> + if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> + if (force_check_mountid) {
> + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> + return EXIT_FAILURE;
> + }
> skip_mntid_unique = true;
> - else
> + } else {
> statx_mntid_unique = statxbuf.stx_mnt_id;
> + }
> }
>
> fh->handle_bytes = bufsz;
> @@ -204,6 +220,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> return EXIT_FAILURE;
> }
> /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
> + if (force_check_mountid) {
> + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> + return EXIT_FAILURE;
> + }
> skip_mntid_unique = true;
> } else {
> if (mntid_unique != statx_mntid_unique) {
> @@ -216,6 +236,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> return 0;
> }
>
> +static int check_feature(const char *feature)
> +{
> + if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
> + int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
> + /* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
> + if (ret < 0 && errno == EINVAL) {
> + fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
> + return EXIT_FAILURE;
> + }
> + return 0;
> + }
> +
> + fprintf(stderr, "unknown feature name '%s'\n", feature);
> + return EXIT_FAILURE;
> +}
> +
> int main(int argc, char **argv)
> {
> int i, c;
> @@ -235,16 +271,20 @@ int main(int argc, char **argv)
> 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 force_check_mountid = 0;
> int bufsz = MAX_HANDLE_SZ;
>
> if (argc < 2)
> usage();
>
> - while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> + while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
> switch (c) {
> case 'c':
> create = 1;
> break;
> + case 'C':
> + /* Check kernel feature support. */
> + return check_feature(optarg);
> case 'w':
> /* Write data before open_by_handle_at() */
> wr = 1;
> @@ -271,6 +311,9 @@ int main(int argc, char **argv)
> case 'm':
> move = 1;
> break;
> + case 'M':
> + force_check_mountid = 1;
> + break;
> case 'p':
> parent = 1;
> break;
> @@ -403,7 +446,7 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
> } else {
> - ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
> if (ret)
> return EXIT_FAILURE;
> }
> @@ -433,7 +476,7 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
> } else {
> - ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
> if (ret)
> return EXIT_FAILURE;
> }
> diff --git a/tests/generic/756 b/tests/generic/756
> new file mode 100755
> index 000000000000..c7a82cfd25f4
> --- /dev/null
> +++ b/tests/generic/756
> @@ -0,0 +1,65 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> +# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
> +#
> +# FS QA Test No. 756
> +#
> +# Check stale handles pointing to unlinked files and non-stale handles pointing
> +# to linked files while verifying that u64 mount IDs are correctly returned.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick exportfs
> +
> +# Import common functions.
> +. ./common/filter
> +
> +
> +# Modify as appropriate.
> +_require_test
> +# _require_exportfs and already requires open_by_handle, but let's not count on it
> +_require_test_program "open_by_handle"
> +_require_exportfs
> +# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
> +_require_statx_unique_mountid
> +_require_open_by_handle_unique_mountid
> +
> +NUMFILES=1024
> +testdir=$TEST_DIR/$seq-dir
> +mkdir -p $testdir
> +
> +# Create empty test files in test dir
> +create_test_files()
> +{
> + local dir=$1
> +
> + mkdir -p $dir
> + rm -f $dir/*
> + $here/src/open_by_handle -c $dir $NUMFILES
> +}
> +
> +# Test encode/decode file handles
> +test_file_handles()
> +{
> + local dir=$1
> + local opt=$2
> +
> + echo test_file_handles $* | _filter_test_dir
> + $here/src/open_by_handle $opt $dir $NUMFILES
> +}
> +
> +# Check stale handles to deleted files
> +create_test_files $testdir
> +test_file_handles $testdir -Md
> +
> +# Check non-stale handles to linked files
> +create_test_files $testdir
> +test_file_handles $testdir -M
> +
> +# Check non-stale handles to files that were hardlinked and original deleted
> +create_test_files $testdir
> +test_file_handles $testdir -Ml
> +test_file_handles $testdir -Mu
> +
> +status=0
> +exit
> diff --git a/tests/generic/756.out b/tests/generic/756.out
> new file mode 100644
> index 000000000000..48aed88d87b9
> --- /dev/null
> +++ b/tests/generic/756.out
> @@ -0,0 +1,5 @@
> +QA output created by 756
> +test_file_handles TEST_DIR/756-dir -Md
> +test_file_handles TEST_DIR/756-dir -M
> +test_file_handles TEST_DIR/756-dir -Ml
> +test_file_handles TEST_DIR/756-dir -Mu
> --
> 2.46.0
>
^ permalink raw reply
* Re: [PATCH xfstests v3 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly
From: Aleksa Sarai @ 2024-09-04 19:00 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: <CAOQ4uxj0X2GJLvB=HyR7_kr=SvqQ0dGapVnf9Ft1hWcaXXCJVw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 14683 bytes --]
On 2024-09-04, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Sep 4, 2024 at 7:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
> > add a test (based on generic/426) which runs the open_by_handle in a
> > mode where it will error out if there is a problem with getting mount
> > IDs. The test is skipped if the kernel doesn't support the necessary
> > features.
> >
> > Suggested-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
>
> Apart from one minor nits below, you may add:
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
>
> > ---
> > common/rc | 24 ++++++++++++++++
> > src/open_by_handle.c | 63 ++++++++++++++++++++++++++++++++++-------
> > tests/generic/756 | 65 +++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/756.out | 5 ++++
> > 4 files changed, 147 insertions(+), 10 deletions(-)
> > create mode 100755 tests/generic/756
> > create mode 100644 tests/generic/756.out
> >
> > diff --git a/common/rc b/common/rc
> > index 9da9fe188297..0beaf2ff1126 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -5178,6 +5178,30 @@ _require_fibmap()
> > rm -f $file
> > }
> >
> > +_require_statx_unique_mountid()
> > +{
> > + # statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
> > + # statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
> > + # We only need to check the latter.
> > +
> > + export STATX_MNT_ID_UNIQUE=0x4000
> > + local statx_mask=$(
> > + ${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
> > + sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
> > + )
> > +
> > + [[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
> > + _notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
> > +}
> > +
> > +_require_open_by_handle_unique_mountid()
> > +{
> > + _require_test_program "open_by_handle"
> > +
> > + $here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
> > + || _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
> > +}
> > +
> > _try_wipe_scratch_devs()
> > {
> > test -x "$WIPEFS_PROG" || return 0
> > diff --git a/src/open_by_handle.c b/src/open_by_handle.c
> > index 920ec7d9170b..b5c1a30abbbc 100644
> > --- a/src/open_by_handle.c
> > +++ b/src/open_by_handle.c
> > @@ -106,9 +106,11 @@ 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, " open_by_handle -C <feature>\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 -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");
> > fprintf(stderr, "open_by_handle -n <test_dir> [N] - get file handles of test files and try to open by handle without drop caches\n");
> > fprintf(stderr, "open_by_handle -k <test_dir> [N] - get file handles of files that are kept open, drop caches and try to open by handle\n");
> > @@ -117,19 +119,23 @@ void usage(void)
> > fprintf(stderr, "open_by_handle -a <test_dir> [N] - write data to test files after open by handle\n");
> > 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 -U <test_dir> [N] - verify the mount ID returned with AT_HANDLE_MNT_ID_UNIQUE is correct\n");
>
> I guess this was not supposed to be in the first patch
Yeah, it got mixed up when splitting the patch. I'll fix it up.
> > 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] - rename test files, drop caches and try to open by handle\n");
> > + fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\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");
> > fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
> > fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
> > fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
> > + fprintf(stderr, "\n");
> > + fprintf(stderr, "open_by_handle -C <feature> - check if <feature> is supported by the kernel.\n");
> > + fprintf(stderr, " <feature> can be any of the following values:\n");
> > + fprintf(stderr, " - AT_HANDLE_MNT_ID_UNIQUE\n");
> > exit(EXIT_FAILURE);
> > }
> >
> > static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> > - int bufsz)
> > + int bufsz, bool force_check_mountid)
> > {
> > int ret;
> > int mntid_short;
> > @@ -145,10 +151,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> > fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
> > return EXIT_FAILURE;
> > }
> > - if (!(statxbuf.stx_mask & STATX_MNT_ID))
> > + if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
> > + if (force_check_mountid) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > skip_mntid = true;
> > - else
> > + } else {
> > statx_mntid_short = statxbuf.stx_mnt_id;
> > + }
> > }
> >
> > if (!skip_mntid_unique) {
> > @@ -160,10 +171,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> > * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
> > * kernel doesn't give us a unique mount ID just skip it.
> > */
> > - if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
> > + if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
> > + if (force_check_mountid) {
> > + fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > skip_mntid_unique = true;
> > - else
> > + } else {
> > statx_mntid_unique = statxbuf.stx_mnt_id;
> > + }
> > }
> >
> > fh->handle_bytes = bufsz;
> > @@ -204,6 +220,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> > return EXIT_FAILURE;
> > }
> > /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
> > + if (force_check_mountid) {
> > + fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
> > + return EXIT_FAILURE;
> > + }
> > skip_mntid_unique = true;
> > } else {
> > if (mntid_unique != statx_mntid_unique) {
> > @@ -216,6 +236,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
> > return 0;
> > }
> >
> > +static int check_feature(const char *feature)
> > +{
> > + if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
> > + int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
> > + /* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
> > + if (ret < 0 && errno == EINVAL) {
> > + fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
> > + return EXIT_FAILURE;
> > + }
> > + return 0;
> > + }
> > +
> > + fprintf(stderr, "unknown feature name '%s'\n", feature);
> > + return EXIT_FAILURE;
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > int i, c;
> > @@ -235,16 +271,20 @@ int main(int argc, char **argv)
> > 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 force_check_mountid = 0;
> > int bufsz = MAX_HANDLE_SZ;
> >
> > if (argc < 2)
> > usage();
> >
> > - while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
> > + while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
> > switch (c) {
> > case 'c':
> > create = 1;
> > break;
> > + case 'C':
> > + /* Check kernel feature support. */
> > + return check_feature(optarg);
> > case 'w':
> > /* Write data before open_by_handle_at() */
> > wr = 1;
> > @@ -271,6 +311,9 @@ int main(int argc, char **argv)
> > case 'm':
> > move = 1;
> > break;
> > + case 'M':
> > + force_check_mountid = 1;
> > + break;
> > case 'p':
> > parent = 1;
> > break;
> > @@ -403,7 +446,7 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> > }
> > } else {
> > - ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
> > + ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
> > if (ret)
> > return EXIT_FAILURE;
> > }
> > @@ -433,7 +476,7 @@ int main(int argc, char **argv)
> > return EXIT_FAILURE;
> > }
> > } else {
> > - ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
> > + ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
> > if (ret)
> > return EXIT_FAILURE;
> > }
> > diff --git a/tests/generic/756 b/tests/generic/756
> > new file mode 100755
> > index 000000000000..c7a82cfd25f4
> > --- /dev/null
> > +++ b/tests/generic/756
> > @@ -0,0 +1,65 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
> > +# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
> > +#
> > +# FS QA Test No. 756
> > +#
> > +# Check stale handles pointing to unlinked files and non-stale handles pointing
> > +# to linked files while verifying that u64 mount IDs are correctly returned.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick exportfs
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +
> > +# Modify as appropriate.
> > +_require_test
> > +# _require_exportfs and already requires open_by_handle, but let's not count on it
> > +_require_test_program "open_by_handle"
> > +_require_exportfs
> > +# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
> > +_require_statx_unique_mountid
> > +_require_open_by_handle_unique_mountid
> > +
> > +NUMFILES=1024
> > +testdir=$TEST_DIR/$seq-dir
> > +mkdir -p $testdir
> > +
> > +# Create empty test files in test dir
> > +create_test_files()
> > +{
> > + local dir=$1
> > +
> > + mkdir -p $dir
> > + rm -f $dir/*
> > + $here/src/open_by_handle -c $dir $NUMFILES
> > +}
> > +
> > +# Test encode/decode file handles
> > +test_file_handles()
> > +{
> > + local dir=$1
> > + local opt=$2
> > +
> > + echo test_file_handles $* | _filter_test_dir
> > + $here/src/open_by_handle $opt $dir $NUMFILES
> > +}
> > +
> > +# Check stale handles to deleted files
> > +create_test_files $testdir
> > +test_file_handles $testdir -Md
> > +
> > +# Check non-stale handles to linked files
> > +create_test_files $testdir
> > +test_file_handles $testdir -M
> > +
> > +# Check non-stale handles to files that were hardlinked and original deleted
> > +create_test_files $testdir
> > +test_file_handles $testdir -Ml
> > +test_file_handles $testdir -Mu
> > +
> > +status=0
> > +exit
> > diff --git a/tests/generic/756.out b/tests/generic/756.out
> > new file mode 100644
> > index 000000000000..48aed88d87b9
> > --- /dev/null
> > +++ b/tests/generic/756.out
> > @@ -0,0 +1,5 @@
> > +QA output created by 756
> > +test_file_handles TEST_DIR/756-dir -Md
> > +test_file_handles TEST_DIR/756-dir -M
> > +test_file_handles TEST_DIR/756-dir -Ml
> > +test_file_handles TEST_DIR/756-dir -Mu
> > --
> > 2.46.0
> >
--
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
* [PATCH xfstests v4 1/2] open_by_handle: verify u32 and u64 mount IDs
From: Aleksa Sarai @ 2024-09-04 19:48 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>
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. Also, add automatic tests for the old u32 mount IDs as well.
By default, we do mount ID checks but silently skip the tests if the
syscalls are not supported by the running kernel (to ensure the tests
continue to work for old kernels). We will add some tests explicitly
checking the new features (with no silent skipping) in a future patch.
The u32 mount ID tests require STATX_MNT_ID (Linux 5.8), while the u64
mount ID tests require STATX_MNT_ID_UNIQUE (Linux 6.9) and
AT_HANDLE_MNT_ID_UNIQUE (linux-next).
Link: https://lore.kernel.org/all/20240828-exportfs-u64-mount-id-v3-0-10c2c4c16708@cyphar.com/
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changed in v4:
- Fix minor flub in patch split. [Amir Goldstein]
- v3: <https://lore.kernel.org/all/20240904175639.2269694-1-cyphar@cyphar.com/>
Changed in v3:
- Make skipping completely silent in regular open_by_handle mode. [Amir Goldstein]
- Re-add -M to turn skipping into errors and add a new test that uses
-M, but is skipped on older kernels. [Amir Goldstein]
- v2: <https://lore.kernel.org/all/20240902164554.928371-1-cyphar@cyphar.com/>
Changed in v2:
- Remove -M argument and always do the mount ID tests. [Amir Goldstein]
- Do not error out if the kernel doesn't support STATX_MNT_ID_UNIQUE
or AT_HANDLE_MNT_ID_UNIQUE. [Amir Goldstein]
- v1: <https://lore.kernel.org/all/20240828103706.2393267-1-cyphar@cyphar.com/>
---
src/open_by_handle.c | 131 +++++++++++++++++++++++++++++++++----------
1 file changed, 102 insertions(+), 29 deletions(-)
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index 0f74ed08b1f0..dcbcd35561fb 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -87,6 +87,15 @@ Examples:
#include <errno.h>
#include <linux/limits.h>
#include <libgen.h>
+#include <stdint.h>
+#include <stdbool.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
@@ -118,6 +127,94 @@ void usage(void)
exit(EXIT_FAILURE);
}
+static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
+ int bufsz)
+{
+ int ret;
+ int mntid_short;
+
+ static bool skip_mntid, skip_mntid_unique;
+
+ uint64_t statx_mntid_short = 0, statx_mntid_unique = 0;
+ struct statx statxbuf;
+
+ /* Get both the short and unique mount id. */
+ if (!skip_mntid) {
+ if (xfstests_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))
+ skip_mntid = true;
+ else
+ statx_mntid_short = statxbuf.stx_mnt_id;
+ }
+
+ if (!skip_mntid_unique) {
+ if (xfstests_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;
+ }
+ /*
+ * STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
+ * kernel doesn't give us a unique mount ID just skip it.
+ */
+ if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+ skip_mntid_unique = true;
+ else
+ 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 (!skip_mntid) {
+ 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;
+ }
+ }
+
+ if (!skip_mntid_unique) {
+ struct handle dummy_fh;
+ uint64_t mntid_unique = 0;
+
+ /*
+ * 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;
+ ret = name_to_handle_at(AT_FDCWD, fname, &dummy_fh.fh, (int *) &mntid_unique, AT_HANDLE_MNT_ID_UNIQUE);
+ if (ret < 0) {
+ if (errno != EINVAL) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE): %m\n", fname);
+ return EXIT_FAILURE;
+ }
+ /* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+ skip_mntid_unique = true;
+ } else {
+ 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;
@@ -130,7 +227,7 @@ 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;
@@ -305,21 +402,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);
+ if (ret)
return EXIT_FAILURE;
- }
}
if (keepopen) {
/* Open without close to keep unlinked files around */
@@ -347,21 +432,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);
+ if (ret)
return EXIT_FAILURE;
- }
}
if (out_fd) {
ret = write(out_fd, (char *)&dir_handle, sizeof(*handle));
--
2.46.0
^ permalink raw reply related
* [PATCH xfstests v4 2/2] generic/756: test name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) explicitly
From: Aleksa Sarai @ 2024-09-04 19:48 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: <20240904194823.2456471-1-cyphar@cyphar.com>
In order to make sure we are actually testing AT_HANDLE_MNT_ID_UNIQUE,
add a test (based on generic/426) which runs the open_by_handle in a
mode where it will error out if there is a problem with getting mount
IDs. The test is skipped if the kernel doesn't support the necessary
features.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
common/rc | 24 ++++++++++++++++
src/open_by_handle.c | 61 ++++++++++++++++++++++++++++++++++------
tests/generic/756 | 65 +++++++++++++++++++++++++++++++++++++++++++
tests/generic/756.out | 5 ++++
4 files changed, 146 insertions(+), 9 deletions(-)
create mode 100755 tests/generic/756
create mode 100644 tests/generic/756.out
diff --git a/common/rc b/common/rc
index 9da9fe188297..0beaf2ff1126 100644
--- a/common/rc
+++ b/common/rc
@@ -5178,6 +5178,30 @@ _require_fibmap()
rm -f $file
}
+_require_statx_unique_mountid()
+{
+ # statx(STATX_MNT_ID=0x1000) was added in Linux 5.8.
+ # statx(STATX_MNT_ID_UNIQUE=0x4000) was added in Linux 6.9.
+ # We only need to check the latter.
+
+ export STATX_MNT_ID_UNIQUE=0x4000
+ local statx_mask=$(
+ ${XFS_IO_PROG} -c "statx -m $STATX_MNT_ID_UNIQUE -r" "$TEST_DIR" |
+ sed -En 's/stat\.mask = (0x[0-9a-f]+)/\1/p'
+ )
+
+ [[ $(( statx_mask & STATX_MNT_ID_UNIQUE )) == $((STATX_MNT_ID_UNIQUE)) ]] ||
+ _notrun "statx does not support STATX_MNT_ID_UNIQUE on this kernel"
+}
+
+_require_open_by_handle_unique_mountid()
+{
+ _require_test_program "open_by_handle"
+
+ $here/src/open_by_handle -C AT_HANDLE_MNT_ID_UNIQUE 2>&1 \
+ || _notrun "name_to_handle_at does not support AT_HANDLE_MNT_ID_UNIQUE"
+}
+
_try_wipe_scratch_devs()
{
test -x "$WIPEFS_PROG" || return 0
diff --git a/src/open_by_handle.c b/src/open_by_handle.c
index dcbcd35561fb..a99cce4b3558 100644
--- a/src/open_by_handle.c
+++ b/src/open_by_handle.c
@@ -106,7 +106,8 @@ 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, " open_by_handle -C <feature>\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");
@@ -119,16 +120,21 @@ void usage(void)
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] - rename test files, drop caches and try to open by handle\n");
+ fprintf(stderr, "open_by_handle -M <test_dir> [N] - do not silently skip the mount ID verifications\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");
fprintf(stderr, "open_by_handle -o <handles_file> <test_dir> [N] - get file handles of test files and write handles to file\n");
fprintf(stderr, "open_by_handle -s <test_dir> [N] - wait in sleep loop after opening files by handle to keep them open\n");
fprintf(stderr, "open_by_handle -z <test_dir> [N] - query filesystem required buffer size\n");
+ fprintf(stderr, "\n");
+ fprintf(stderr, "open_by_handle -C <feature> - check if <feature> is supported by the kernel.\n");
+ fprintf(stderr, " <feature> can be any of the following values:\n");
+ fprintf(stderr, " - AT_HANDLE_MNT_ID_UNIQUE\n");
exit(EXIT_FAILURE);
}
static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
- int bufsz)
+ int bufsz, bool force_check_mountid)
{
int ret;
int mntid_short;
@@ -144,10 +150,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
fprintf(stderr, "%s: statx(STATX_MNT_ID): %m\n", fname);
return EXIT_FAILURE;
}
- if (!(statxbuf.stx_mask & STATX_MNT_ID))
+ if (!(statxbuf.stx_mask & STATX_MNT_ID)) {
+ if (force_check_mountid) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID) not supported by running kernel\n", fname);
+ return EXIT_FAILURE;
+ }
skip_mntid = true;
- else
+ } else {
statx_mntid_short = statxbuf.stx_mnt_id;
+ }
}
if (!skip_mntid_unique) {
@@ -159,10 +170,15 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
* STATX_MNT_ID_UNIQUE was added fairly recently in Linux 6.8, so if the
* kernel doesn't give us a unique mount ID just skip it.
*/
- if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE))
+ if (!(statxbuf.stx_mask & STATX_MNT_ID_UNIQUE)) {
+ if (force_check_mountid) {
+ fprintf(stderr, "%s: statx(STATX_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+ return EXIT_FAILURE;
+ }
skip_mntid_unique = true;
- else
+ } else {
statx_mntid_unique = statxbuf.stx_mnt_id;
+ }
}
fh->handle_bytes = bufsz;
@@ -203,6 +219,10 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
return EXIT_FAILURE;
}
/* EINVAL means AT_HANDLE_MNT_ID_UNIQUE is not supported */
+ if (force_check_mountid) {
+ fprintf(stderr, "%s: name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n", fname);
+ return EXIT_FAILURE;
+ }
skip_mntid_unique = true;
} else {
if (mntid_unique != statx_mntid_unique) {
@@ -215,6 +235,22 @@ static int do_name_to_handle_at(const char *fname, struct file_handle *fh,
return 0;
}
+static int check_feature(const char *feature)
+{
+ if (!strcmp(feature, "AT_HANDLE_MNT_ID_UNIQUE")) {
+ int ret = name_to_handle_at(AT_FDCWD, ".", NULL, NULL, AT_HANDLE_MNT_ID_UNIQUE);
+ /* If AT_HANDLE_MNT_ID_UNIQUE is supported, we get EFAULT. */
+ if (ret < 0 && errno == EINVAL) {
+ fprintf(stderr, "name_to_handle_at(AT_HANDLE_MNT_ID_UNIQUE) not supported by running kernel\n");
+ return EXIT_FAILURE;
+ }
+ return 0;
+ }
+
+ fprintf(stderr, "unknown feature name '%s'\n", feature);
+ return EXIT_FAILURE;
+}
+
int main(int argc, char **argv)
{
int i, c;
@@ -234,16 +270,20 @@ int main(int argc, char **argv)
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 force_check_mountid = 0;
int bufsz = MAX_HANDLE_SZ;
if (argc < 2)
usage();
- while ((c = getopt(argc, argv, "cludmrwapknhi:o:sz")) != -1) {
+ while ((c = getopt(argc, argv, "cC:ludmMrwapknhi:o:sz")) != -1) {
switch (c) {
case 'c':
create = 1;
break;
+ case 'C':
+ /* Check kernel feature support. */
+ return check_feature(optarg);
case 'w':
/* Write data before open_by_handle_at() */
wr = 1;
@@ -270,6 +310,9 @@ int main(int argc, char **argv)
case 'm':
move = 1;
break;
+ case 'M':
+ force_check_mountid = 1;
+ break;
case 'p':
parent = 1;
break;
@@ -402,7 +445,7 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz);
+ ret = do_name_to_handle_at(fname, &handle[i].fh, bufsz, force_check_mountid);
if (ret)
return EXIT_FAILURE;
}
@@ -432,7 +475,7 @@ int main(int argc, char **argv)
return EXIT_FAILURE;
}
} else {
- ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz);
+ ret = do_name_to_handle_at(test_dir, &dir_handle.fh, bufsz, force_check_mountid);
if (ret)
return EXIT_FAILURE;
}
diff --git a/tests/generic/756 b/tests/generic/756
new file mode 100755
index 000000000000..c7a82cfd25f4
--- /dev/null
+++ b/tests/generic/756
@@ -0,0 +1,65 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2017 CTERA Networks. All Rights Reserved.
+# Copyright (C) 2024 Aleksa Sarai <cyphar@cyphar.com>
+#
+# FS QA Test No. 756
+#
+# Check stale handles pointing to unlinked files and non-stale handles pointing
+# to linked files while verifying that u64 mount IDs are correctly returned.
+#
+. ./common/preamble
+_begin_fstest auto quick exportfs
+
+# Import common functions.
+. ./common/filter
+
+
+# Modify as appropriate.
+_require_test
+# _require_exportfs and already requires open_by_handle, but let's not count on it
+_require_test_program "open_by_handle"
+_require_exportfs
+# We need both STATX_MNT_ID_UNIQUE and AT_HANDLE_MNT_ID_UNIQUE.
+_require_statx_unique_mountid
+_require_open_by_handle_unique_mountid
+
+NUMFILES=1024
+testdir=$TEST_DIR/$seq-dir
+mkdir -p $testdir
+
+# Create empty test files in test dir
+create_test_files()
+{
+ local dir=$1
+
+ mkdir -p $dir
+ rm -f $dir/*
+ $here/src/open_by_handle -c $dir $NUMFILES
+}
+
+# Test encode/decode file handles
+test_file_handles()
+{
+ local dir=$1
+ local opt=$2
+
+ echo test_file_handles $* | _filter_test_dir
+ $here/src/open_by_handle $opt $dir $NUMFILES
+}
+
+# Check stale handles to deleted files
+create_test_files $testdir
+test_file_handles $testdir -Md
+
+# Check non-stale handles to linked files
+create_test_files $testdir
+test_file_handles $testdir -M
+
+# Check non-stale handles to files that were hardlinked and original deleted
+create_test_files $testdir
+test_file_handles $testdir -Ml
+test_file_handles $testdir -Mu
+
+status=0
+exit
diff --git a/tests/generic/756.out b/tests/generic/756.out
new file mode 100644
index 000000000000..48aed88d87b9
--- /dev/null
+++ b/tests/generic/756.out
@@ -0,0 +1,5 @@
+QA output created by 756
+test_file_handles TEST_DIR/756-dir -Md
+test_file_handles TEST_DIR/756-dir -M
+test_file_handles TEST_DIR/756-dir -Ml
+test_file_handles TEST_DIR/756-dir -Mu
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 00/10] extensible syscalls: CHECK_FIELDS to allow for easier feature detection
From: Aleksa Sarai @ 2024-09-05 14:56 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;
}
}
This series adds CHECK_FIELDS support for the following extensible
struct syscalls, as they are quite likely to grow flags in the near
future:
* openat2
* clone3
* mount_setattr
[1]: https://lwn.net/Articles/830666/
[2]: https://youtu.be/ggD-eb3yPVs
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changes in v2:
- Add CHECK_FIELDS support to mount_setattr(2).
- Fix build failure on architectures with custom errno values.
- Rework selftests to use the tools/ uAPI headers rather than custom
defining EEXTSYS_NOOP.
- Make sure we return -EINVAL and -E2BIG for invalid sizes even if
CHECK_FIELDS is set, and add some tests for that.
- v1: <https://lore.kernel.org/r/20240902-extensible-structs-check_fields-v1-0-545e93ede2f2@cyphar.com>
---
Aleksa Sarai (10):
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
selftests: openat2: add 0xFF poisoned data after misaligned struct
selftests: openat2: add CHECK_FIELDS selftests
clone3: add CHECK_FIELDS flag to usize argument
selftests: clone3: add CHECK_FIELDS selftests
mount_setattr: add CHECK_FIELDS flag to usize argument
selftests: mount_setattr: add CHECK_FIELDS selftest
arch/alpha/include/uapi/asm/errno.h | 3 +
arch/mips/include/uapi/asm/errno.h | 3 +
arch/parisc/include/uapi/asm/errno.h | 3 +
arch/sparc/include/uapi/asm/errno.h | 3 +
fs/namespace.c | 17 ++
fs/open.c | 18 ++
include/linux/uaccess.h | 98 ++++++++
include/uapi/asm-generic/errno.h | 3 +
include/uapi/linux/openat2.h | 2 +
kernel/fork.c | 30 ++-
kernel/sched/syscalls.c | 42 +---
tools/arch/alpha/include/uapi/asm/errno.h | 3 +
tools/arch/mips/include/uapi/asm/errno.h | 3 +
tools/arch/parisc/include/uapi/asm/errno.h | 3 +
tools/arch/sparc/include/uapi/asm/errno.h | 3 +
tools/include/uapi/asm-generic/errno.h | 3 +
tools/include/uapi/asm-generic/posix_types.h | 101 ++++++++
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 4 +-
.../testing/selftests/clone3/clone3_check_fields.c | 264 +++++++++++++++++++++
tools/testing/selftests/mount_setattr/Makefile | 2 +-
.../selftests/mount_setattr/mount_setattr_test.c | 53 ++++-
tools/testing/selftests/openat2/Makefile | 2 +
tools/testing/selftests/openat2/openat2_test.c | 165 ++++++++++++-
24 files changed, 778 insertions(+), 51 deletions(-)
---
base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
change-id: 20240803-extensible-structs-check_fields-a47e94cef691
Best regards,
--
Aleksa Sarai <cyphar@cyphar.com>
^ permalink raw reply
* [PATCH RFC v2 01/10] uaccess: add copy_struct_to_user helper
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
This is based on copy_struct_from_user(), but there is one additional
case to consider when creating a syscall that returns an
extensible-struct to userspace -- how should data in the struct that
cannot fit into the userspace struct be handled (ksize > usize)?
There are three possibilies:
1. The interface is like sched_getattr(2), where new information will
be silently not provided to userspace. This is probably what most
interfaces will want to do, as it provides the most possible
backwards-compatibility.
2. The interface is like lsm_list_modules(2), where you want to return
an error like -EMSGSIZE if not providing information could result in
the userspace program making a serious mistake (such as one that
could lead to a security problem) or if you want to provide some
flag to userspace so they know that they are missing some
information.
3. The interface is like statx(2), where there some kind of a request
mask that indicates what data userspace would like. One could
imagine that statx2(2) (using extensible structs) would want to
return -EMSGSIZE if the user explicitly requested a field that their
structure is too small to fit, but not return an error if the field
was not explicitly requested. This is kind of a mix between (1) and
(2) based on the requested mask.
The copy_struct_to_user() helper includes a an extra argument that is
used to return a boolean flag indicating whether there was a non-zero
byte in the trailing bytes that were not copied to userspace. This can
be used in the following ways to handle all three cases, respectively:
1. Just pass NULL, as you don't care about this case.
2. Return an error (say -EMSGSIZE) if the argument was set to true by
copy_struct_to_user().
3. If the argument was set to true by copy_struct_to_user(), check if
there is a flag that implies a field larger than usize.
This is the only case where callers of copy_struct_to_user() should
check usize themselves. This will probably require scanning an array
that specifies what flags were added for each version of the flags
struct and returning an error if the request mask matches any of the
flags that were added in versions of the struct that are larger than
usize.
At the moment we don't have any users of (3), so this patch doesn't
include any helpers to make the necessary scanning easier, but it should
be fairly easy to add some if necessary.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
include/linux/uaccess.h | 98 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d8e4105a2f21..5d0a590ef65d 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -387,6 +387,104 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src,
return 0;
}
+/**
+ * copy_struct_to_user: copy a struct to userspace
+ * @dst: Destination address, in userspace. This buffer must be @ksize
+ * bytes long.
+ * @usize: (Alleged) size of @dst struct.
+ * @src: Source address, in kernel space.
+ * @ksize: Size of @src struct.
+ * @ignored_trailing: Set to %true if there was a non-zero byte in @src that
+ * userspace cannot see because they are using an smaller struct.
+ *
+ * Copies a struct from kernel space to userspace, in a way that guarantees
+ * backwards-compatibility for struct syscall arguments (as long as future
+ * struct extensions are made such that all new fields are *appended* to the
+ * old struct, and zeroed-out new fields have the same meaning as the old
+ * struct).
+ *
+ * Some syscalls may wish to make sure that userspace knows about everything in
+ * the struct, and if there is a non-zero value that userspce doesn't know
+ * about, they want to return an error (such as -EMSGSIZE) or have some other
+ * fallback (such as adding a "you're missing some information" flag). If
+ * @ignored_trailing is non-%NULL, it will be set to %true if there was a
+ * non-zero byte that could not be copied to userspace (ie. was past @usize).
+ *
+ * While unconditionally returning an error in this case is the simplest
+ * solution, for maximum backward compatibility you should try to only return
+ * -EMSGSIZE if the user explicitly requested the data that couldn't be copied.
+ * Note that structure sizes can change due to header changes and simple
+ * recompilations without code changes(!), so if you care about
+ * @ignored_trailing you probably want to make sure that any new field data is
+ * associated with a flag. Otherwise you might assume that a program knows
+ * about data it does not.
+ *
+ * @ksize is just sizeof(*src), and @usize should've been passed by userspace.
+ * The recommended usage is something like the following:
+ *
+ * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
+ * {
+ * int err;
+ * bool ignored_trailing;
+ * struct foo karg = {};
+ *
+ * if (usize > PAGE_SIZE)
+ * return -E2BIG;
+ * if (usize < FOO_SIZE_VER0)
+ * return -EINVAL;
+ *
+ * // ... modify karg somehow ...
+ *
+ * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg),
+ * &ignored_trailing);
+ * if (err)
+ * return err;
+ * if (ignored_trailing)
+ * return -EMSGSIZE:
+ *
+ * // ...
+ * }
+ *
+ * There are three cases to consider:
+ * * If @usize == @ksize, then it's copied verbatim.
+ * * If @usize < @ksize, then the kernel is trying to pass userspace a newer
+ * struct than it supports. Thus we only copy the interoperable portions
+ * (@usize) and ignore the rest (but @ignored_trailing is set to %true if
+ * any of the trailing (@ksize - @usize) bytes are non-zero).
+ * * If @usize > @ksize, then the kernel is trying to pass userspace an older
+ * struct than userspace supports. In order to make sure the
+ * unknown-to-the-kernel fields don't contain garbage values, we zero the
+ * trailing (@usize - @ksize) bytes.
+ *
+ * Returns (in all cases, some data may have been copied):
+ * * -EFAULT: access to userspace failed.
+ */
+static __always_inline __must_check int
+copy_struct_to_user(void __user *dst, size_t usize, const void *src,
+ size_t ksize, bool *ignored_trailing)
+{
+ size_t size = min(ksize, usize);
+ size_t rest = max(ksize, usize) - size;
+
+ /* Double check if ksize is larger than a known object size. */
+ if (WARN_ON_ONCE(ksize > __builtin_object_size(src, 1)))
+ return -E2BIG;
+
+ /* Deal with trailing bytes. */
+ if (usize > ksize) {
+ int ret = clear_user(dst + size, rest);
+ if (ret)
+ return ret;
+ }
+ if (ignored_trailing)
+ *ignored_trailing = ksize < usize &&
+ memchr_inv(src + size, 0, rest) != NULL;
+ /* Copy the interoperable parts of the struct. */
+ if (copy_to_user(dst, src, size))
+ return -EFAULT;
+ return 0;
+}
+
bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size);
long copy_from_kernel_nofault(void *dst, const void *src, size_t size);
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 02/10] sched_getattr: port to copy_struct_to_user
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
sched_getattr(2) doesn't care about trailing non-zero bytes in the
(ksize > usize) case, so just use copy_struct_to_user() without checking
ignored_trailing.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/sched/syscalls.c | 42 ++----------------------------------------
1 file changed, 2 insertions(+), 40 deletions(-)
diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index ae1b42775ef9..4ccc058bae16 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -1147,45 +1147,6 @@ SYSCALL_DEFINE2(sched_getparam, pid_t, pid, struct sched_param __user *, param)
return copy_to_user(param, &lp, sizeof(*param)) ? -EFAULT : 0;
}
-/*
- * Copy the kernel size attribute structure (which might be larger
- * than what user-space knows about) to user-space.
- *
- * Note that all cases are valid: user-space buffer can be larger or
- * smaller than the kernel-space buffer. The usual case is that both
- * have the same size.
- */
-static int
-sched_attr_copy_to_user(struct sched_attr __user *uattr,
- struct sched_attr *kattr,
- unsigned int usize)
-{
- unsigned int ksize = sizeof(*kattr);
-
- if (!access_ok(uattr, usize))
- return -EFAULT;
-
- /*
- * sched_getattr() ABI forwards and backwards compatibility:
- *
- * If usize == ksize then we just copy everything to user-space and all is good.
- *
- * If usize < ksize then we only copy as much as user-space has space for,
- * this keeps ABI compatibility as well. We skip the rest.
- *
- * If usize > ksize then user-space is using a newer version of the ABI,
- * which part the kernel doesn't know about. Just ignore it - tooling can
- * detect the kernel's knowledge of attributes from the attr->size value
- * which is set to ksize in this case.
- */
- kattr->size = min(usize, ksize);
-
- if (copy_to_user(uattr, kattr, kattr->size))
- return -EFAULT;
-
- return 0;
-}
-
/**
* sys_sched_getattr - similar to sched_getparam, but with sched_attr
* @pid: the pid in question.
@@ -1230,7 +1191,8 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
#endif
}
- return sched_attr_copy_to_user(uattr, &kattr, usize);
+ kattr.size = min(usize, sizeof(kattr));
+ return copy_struct_to_user(uattr, usize, &kattr, sizeof(kattr), NULL);
}
#ifdef CONFIG_SMP
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 03/10] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
While we do currently return -EFAULT in this case, it seems prudent to
follow the behaviour of other syscalls like clone3. It seems quite
unlikely that anyone depends on this error code being EFAULT, but we can
always revert this if it turns out to be an issue.
Cc: <stable@vger.kernel.org> # v5.6+
Fixes: fddb5d430ad9 ("open: introduce openat2(2) syscall")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/open.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/open.c b/fs/open.c
index 22adbef7ecc2..30bfcddd505d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1458,6 +1458,8 @@ SYSCALL_DEFINE4(openat2, int, dfd, const char __user *, filename,
if (unlikely(usize < OPEN_HOW_SIZE_VER0))
return -EINVAL;
+ if (unlikely(usize > PAGE_SIZE))
+ return -E2BIG;
err = copy_struct_from_user(&tmp, sizeof(tmp), how, usize);
if (err)
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 04/10] openat2: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
In order for userspace to be able to know what flags and fields the
kernel supports, it is currently necessary for them to do a bunch of
fairly subtle self-checks where you need to get a syscall to return a
non-EINVAL error or no-op for each flag you wish to use. If you get
-EINVAL you know the flag is unsupported, otherwise you know it is
supported.
This doesn't scale well for programs that need to check many flags, and
not all syscalls can be easily checked (how would you check for new
flags for umount2 or clone3 without side-effects?). To solve this
problem, we can take advantage of the extensible struct API used by
copy_struct_from_user() by providing a special CHECK_FIELDS flag to
extensible struct syscalls (like openat2 and clone3) which will:
1. Cause the syscall to fill the structure with every valid bit the
kernel understands. For flag arguments, this is the set of all valid
flag bits. For pointer and file descriptor arguments, this would be
all 0xFF bits (to indicate that any bits are valid). Userspace can
then easily check whether the flag they wanted is supported (by
doing a simple bitwise AND) or if a field itself is supported (by
checking if it is non-zero / all 0xFF).
2. Return a specific no-op error (-EEXTSYS_NOOP) that is not used as an
error by any other kernel code, so that userspace can be absolutely
sure that the kernel supports CHECK_FIELDS.
Rather than passing CHECK_FIELDS using the standard flags arguments for
the syscall, CHECK_FIELDS is instead the highest bit in the provided
struct size. The high bits of the size are never going to be non-zero
(we currently only allow size to be up to PAGE_SIZE, and it seems very
unlikely we will ever allow several exabyte structure arguments).
By passing the flag in the structure size, we can be sure that old
kernels will return a consistent error code (-EFAULT in openat2's case)
and that seccomp can properly filter this syscall mode (which is
guaranteed to be a no-op on all kernels -- it could even force
-EEXTSYS_NOOP to make the userspace program think the kernel doesn't
support any syscall features).
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;
}
}
Link: https://youtu.be/ggD-eb3yPVs
Link: https://lwn.net/Articles/830666/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
arch/alpha/include/uapi/asm/errno.h | 3 +++
arch/mips/include/uapi/asm/errno.h | 3 +++
arch/parisc/include/uapi/asm/errno.h | 3 +++
arch/sparc/include/uapi/asm/errno.h | 3 +++
fs/open.c | 16 ++++++++++++++++
include/uapi/asm-generic/errno.h | 3 +++
include/uapi/linux/openat2.h | 2 ++
tools/arch/alpha/include/uapi/asm/errno.h | 3 +++
tools/arch/mips/include/uapi/asm/errno.h | 3 +++
tools/arch/parisc/include/uapi/asm/errno.h | 3 +++
tools/arch/sparc/include/uapi/asm/errno.h | 3 +++
tools/include/uapi/asm-generic/errno.h | 3 +++
12 files changed, 48 insertions(+)
diff --git a/arch/alpha/include/uapi/asm/errno.h b/arch/alpha/include/uapi/asm/errno.h
index 3d265f6babaf..41157139fdad 100644
--- a/arch/alpha/include/uapi/asm/errno.h
+++ b/arch/alpha/include/uapi/asm/errno.h
@@ -125,4 +125,7 @@
#define EHWPOISON 139 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 140 /* Extensible syscall performed no operation */
+
#endif
diff --git a/arch/mips/include/uapi/asm/errno.h b/arch/mips/include/uapi/asm/errno.h
index 2fb714e2d6d8..dd1e0ba61105 100644
--- a/arch/mips/include/uapi/asm/errno.h
+++ b/arch/mips/include/uapi/asm/errno.h
@@ -124,6 +124,9 @@
#define EHWPOISON 168 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 169 /* Extensible syscall performed no operation */
+
#define EDQUOT 1133 /* Quota exceeded */
diff --git a/arch/parisc/include/uapi/asm/errno.h b/arch/parisc/include/uapi/asm/errno.h
index 8d94739d75c6..e2f6998ad61c 100644
--- a/arch/parisc/include/uapi/asm/errno.h
+++ b/arch/parisc/include/uapi/asm/errno.h
@@ -122,4 +122,7 @@
#define EHWPOISON 257 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 258 /* Extensible syscall performed no operation */
+
#endif
diff --git a/arch/sparc/include/uapi/asm/errno.h b/arch/sparc/include/uapi/asm/errno.h
index 81a732b902ee..1b11e4651093 100644
--- a/arch/sparc/include/uapi/asm/errno.h
+++ b/arch/sparc/include/uapi/asm/errno.h
@@ -115,4 +115,7 @@
#define EHWPOISON 135 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 136 /* Extensible syscall performed no operation */
+
#endif
diff --git a/fs/open.c b/fs/open.c
index 30bfcddd505d..3fbb0c82fece 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1451,16 +1451,32 @@ SYSCALL_DEFINE4(openat2, int, dfd, const char __user *, filename,
struct open_how __user *, how, size_t, usize)
{
int err;
+ bool check_fields;
struct open_how tmp;
BUILD_BUG_ON(sizeof(struct open_how) < OPEN_HOW_SIZE_VER0);
BUILD_BUG_ON(sizeof(struct open_how) != OPEN_HOW_SIZE_LATEST);
+ check_fields = usize & CHECK_FIELDS;
+ usize &= ~CHECK_FIELDS;
+
if (unlikely(usize < OPEN_HOW_SIZE_VER0))
return -EINVAL;
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
+ if (unlikely(check_fields)) {
+ memset(&tmp, 0, sizeof(tmp));
+ tmp = (struct open_how) {
+ .flags = VALID_OPEN_FLAGS,
+ .mode = S_IALLUGO,
+ .resolve = VALID_RESOLVE_FLAGS,
+ };
+
+ err = copy_struct_to_user(how, usize, &tmp, sizeof(tmp), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
err = copy_struct_from_user(&tmp, sizeof(tmp), how, usize);
if (err)
return err;
diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..f5bfe081e73a 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -120,4 +120,7 @@
#define EHWPOISON 133 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 134 /* Extensible syscall performed no operation */
+
#endif
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index a5feb7604948..6052a504cfa4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -4,6 +4,8 @@
#include <linux/types.h>
+#define CHECK_FIELDS (1ULL << 63)
+
/*
* Arguments for how openat2(2) should open the target path. If only @flags and
* @mode are non-zero, then openat2(2) operates very similarly to openat(2).
diff --git a/tools/arch/alpha/include/uapi/asm/errno.h b/tools/arch/alpha/include/uapi/asm/errno.h
index 3d265f6babaf..41157139fdad 100644
--- a/tools/arch/alpha/include/uapi/asm/errno.h
+++ b/tools/arch/alpha/include/uapi/asm/errno.h
@@ -125,4 +125,7 @@
#define EHWPOISON 139 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 140 /* Extensible syscall performed no operation */
+
#endif
diff --git a/tools/arch/mips/include/uapi/asm/errno.h b/tools/arch/mips/include/uapi/asm/errno.h
index 2fb714e2d6d8..dd1e0ba61105 100644
--- a/tools/arch/mips/include/uapi/asm/errno.h
+++ b/tools/arch/mips/include/uapi/asm/errno.h
@@ -124,6 +124,9 @@
#define EHWPOISON 168 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 169 /* Extensible syscall performed no operation */
+
#define EDQUOT 1133 /* Quota exceeded */
diff --git a/tools/arch/parisc/include/uapi/asm/errno.h b/tools/arch/parisc/include/uapi/asm/errno.h
index 8d94739d75c6..e2f6998ad61c 100644
--- a/tools/arch/parisc/include/uapi/asm/errno.h
+++ b/tools/arch/parisc/include/uapi/asm/errno.h
@@ -122,4 +122,7 @@
#define EHWPOISON 257 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 258 /* Extensible syscall performed no operation */
+
#endif
diff --git a/tools/arch/sparc/include/uapi/asm/errno.h b/tools/arch/sparc/include/uapi/asm/errno.h
index 81a732b902ee..1b11e4651093 100644
--- a/tools/arch/sparc/include/uapi/asm/errno.h
+++ b/tools/arch/sparc/include/uapi/asm/errno.h
@@ -115,4 +115,7 @@
#define EHWPOISON 135 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 136 /* Extensible syscall performed no operation */
+
#endif
diff --git a/tools/include/uapi/asm-generic/errno.h b/tools/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..f5bfe081e73a 100644
--- a/tools/include/uapi/asm-generic/errno.h
+++ b/tools/include/uapi/asm-generic/errno.h
@@ -120,4 +120,7 @@
#define EHWPOISON 133 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 134 /* Extensible syscall performed no operation */
+
#endif
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 05/10] selftests: openat2: add 0xFF poisoned data after misaligned struct
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
We should also verify that poisoned data after a misaligned struct is
also handled correctly by is_zeroed_user(). This test passes with no
kernel changes needed, so is_zeroed_user() was correct already.
Fixes: b28a10aedcd4 ("selftests: add openat2(2) selftests")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/openat2/openat2_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 5790ab446527..4ca175a16ad6 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -112,9 +112,9 @@ void test_openat2_struct(void)
*
* This is effectively to check that is_zeroed_user() works.
*/
- copy = malloc(misalign + sizeof(how_ext));
+ copy = malloc(misalign*2 + sizeof(how_ext));
how_copy = copy + misalign;
- memset(copy, 0xff, misalign);
+ memset(copy, 0xff, misalign*2 + sizeof(how_ext));
memcpy(how_copy, &how_ext, sizeof(how_ext));
}
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 06/10] selftests: openat2: add CHECK_FIELDS selftests
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/openat2/Makefile | 2 +
tools/testing/selftests/openat2/openat2_test.c | 161 ++++++++++++++++++++++++-
2 files changed, 161 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
index 185dc76ebb5f..239a7fd0cb35 100644
--- a/tools/testing/selftests/openat2/Makefile
+++ b/tools/testing/selftests/openat2/Makefile
@@ -12,6 +12,8 @@ ifeq ($(LLVM),)
endif
LOCAL_HDRS += helpers.h
+# Needed for EEXTSYS_NOOP definition.
+CFLAGS += $(TOOLS_INCLUDES)
include ../lib.mk
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 4ca175a16ad6..9d3382bdfce9 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -1,11 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Author: Aleksa Sarai <cyphar@cyphar.com>
- * Copyright (C) 2018-2019 SUSE LLC.
+ * Copyright (C) 2018-2024 SUSE LLC.
*/
#define _GNU_SOURCE
#define __SANE_USERSPACE_TYPES__ // Use ll64
+#include <errno.h>
#include <fcntl.h>
#include <sched.h>
#include <sys/stat.h>
@@ -29,6 +30,10 @@
#define O_LARGEFILE 0x8000
#endif
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
struct open_how_ext {
struct open_how inner;
uint32_t extra1;
@@ -45,6 +50,152 @@ struct struct_test {
int err;
};
+static bool check(bool *failed, bool pred)
+{
+ *failed |= pred;
+ return pred;
+}
+
+#define NUM_OPENAT2_CHECK_FIELDS_BAD_TESTS 2
+
+static void test_openat2_check_fields_bad(const char *name, size_t struct_size)
+{
+ int fd, expected_err;
+ bool failed = false;
+ struct open_how how = {};
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ if (struct_size < sizeof(struct open_how))
+ expected_err = -EINVAL;
+ if (struct_size > 4096)
+ expected_err = -E2BIG;
+
+ if (!openat2_supported) {
+ ksft_print_msg("openat2(2) unsupported\n");
+ resultfn = ksft_test_result_skip;
+ goto skip;
+ }
+
+ fd = raw_openat2(AT_FDCWD, "", &how, CHECK_FIELDS | struct_size);
+ if (check(&failed, fd != expected_err))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned wrong error code: %d (%s) != %d (%s)\n",
+ fd, strerror(-fd),
+ expected_err, strerror(-expected_err));
+
+ if (fd >= 0)
+ close(fd);
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+skip:
+ resultfn("openat2(CHECK_FIELDS) with %s returns %d (%s)\n", name,
+ expected_err, strerror(-expected_err));
+ fflush(stdout);
+}
+
+#define NUM_OPENAT2_CHECK_FIELDS_TESTS 1
+#define NUM_OPENAT2_CHECK_FIELDS_VARIATIONS 13
+
+static void test_openat2_check_fields(void)
+{
+ int misalignments[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 11, 17, 87 };
+
+ for (int i = 0; i < ARRAY_LEN(misalignments); i++) {
+ int fd, misalign = misalignments[i];
+ bool failed = false;
+ char *fdpath = NULL;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ struct open_how_ext how_ext = {}, *how_copy = &how_ext;
+ void *copy = NULL;
+
+ if (!openat2_supported) {
+ ksft_print_msg("openat2(2) unsupported\n");
+ resultfn = ksft_test_result_skip;
+ goto skip;
+ }
+
+ if (misalign) {
+ /*
+ * Explicitly misalign the structure copying it with
+ * the given (mis)alignment offset. The other data is
+ * set to zero and we verify this afterwards to make
+ * sure CHECK_FIELDS doesn't write outside the buffer.
+ */
+ copy = malloc(misalign*2 + sizeof(how_ext));
+ how_copy = copy + misalign;
+ memset(copy, 0x00, misalign*2 + sizeof(how_ext));
+ memcpy(how_copy, &how_ext, sizeof(how_ext));
+ }
+
+ fd = raw_openat2(AT_FDCWD, ".", how_copy, CHECK_FIELDS | sizeof(*how_copy));
+ if (check(&failed, (fd != -EEXTSYS_NOOP)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned wrong error code: %d (%s) != %d\n",
+ fd, strerror(-fd), -EEXTSYS_NOOP);
+ if (fd >= 0) {
+ fdpath = fdreadlink(fd);
+ close(fd);
+ }
+
+ if (failed) {
+ ksft_print_msg("openat2(CHECK_FIELDS) unexpectedly returned ");
+ if (fdpath)
+ ksft_print_msg("%d['%s']\n", fd, fdpath);
+ else
+ ksft_print_msg("%d (%s)\n", fd, strerror(-fd));
+ }
+
+ if (check(&failed, !(how_copy->inner.flags & O_PATH)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned flags is missing O_PATH (0x%.16x): 0x%.16llx\n",
+ O_PATH, how_copy->inner.flags);
+
+ if (check(&failed, (how_copy->inner.mode != 07777)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned mode is invalid (0o%o): 0o%.4llo\n",
+ 07777, how_copy->inner.mode);
+
+ if (check(&failed, !(how_copy->inner.resolve & RESOLVE_IN_ROOT)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned resolve flags is missing RESOLVE_IN_ROOT (0x%.16x): 0x%.16llx\n",
+ RESOLVE_IN_ROOT, how_copy->inner.resolve);
+
+ /* Verify that the buffer space outside the struct wasn't written to. */
+ if (check(&failed, how_copy->extra1 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra1): 0x%x\n",
+ how_copy->extra1);
+ if (check(&failed, how_copy->extra2 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra2): 0x%x\n",
+ how_copy->extra2);
+ if (check(&failed, how_copy->extra3 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra3): 0x%x\n",
+ how_copy->extra3);
+
+ if (misalign) {
+ for (size_t i = 0; i < misalign; i++) {
+ char *p = copy + i;
+ if (check(&failed, *p != '\x00'))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside the size: buffer[%ld] = 0x%.2x\n",
+ p - (char *) copy, *p);
+ }
+ for (size_t i = 0; i < misalign; i++) {
+ char *p = copy + misalign + sizeof(how_ext) + i;
+ if (check(&failed, *p != '\x00'))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside the size: buffer[%ld] = 0x%.2x\n",
+ p - (char *) copy, *p);
+ }
+ }
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+skip:
+ resultfn("openat2(CHECK_FIELDS) [misalign=%d]\n", misalign);
+
+ free(copy);
+ free(fdpath);
+ fflush(stdout);
+ }
+}
+
#define NUM_OPENAT2_STRUCT_TESTS 7
#define NUM_OPENAT2_STRUCT_VARIATIONS 13
@@ -320,7 +471,9 @@ void test_openat2_flags(void)
}
}
-#define NUM_TESTS (NUM_OPENAT2_STRUCT_VARIATIONS * NUM_OPENAT2_STRUCT_TESTS + \
+#define NUM_TESTS (NUM_OPENAT2_CHECK_FIELDS_TESTS * NUM_OPENAT2_CHECK_FIELDS_VARIATIONS + \
+ NUM_OPENAT2_CHECK_FIELDS_BAD_TESTS + \
+ NUM_OPENAT2_STRUCT_VARIATIONS * NUM_OPENAT2_STRUCT_TESTS + \
NUM_OPENAT2_FLAG_TESTS)
int main(int argc, char **argv)
@@ -328,6 +481,10 @@ int main(int argc, char **argv)
ksft_print_header();
ksft_set_plan(NUM_TESTS);
+ test_openat2_check_fields();
+ test_openat2_check_fields_bad("small struct (< v0)", 0);
+ test_openat2_check_fields_bad("large struct (> PAGE_SIZE)", 0xF000);
+
test_openat2_struct();
test_openat2_flags();
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 07/10] clone3: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
As with openat2(2), this allows userspace to easily figure out what
flags and fields are supported by clone3(2). For fields which are not
flag-based, we simply set every bit in the field so that a naive
bitwise-and would show that any value of the field is valid.
For args->exit_signal, since we have an explicit bitmask for the field
defined already (CSIGNAL) we can indicate that only those bits are
supported by current kernels. If we add some extra bits to exit_signal
in the future, being able to detect them as new features would be quite
useful.
The intended way of using this interface to get feature information
looks something like the following:
static bool clone3_clear_sighand_supported;
static bool clone3_cgroup_supported;
int check_clone3_support(void)
{
int err;
struct clone_args args = {};
err = clone3(&args, CHECK_FIELDS | sizeof(args));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
clone3_clear_sighand_supported = (how.flags & CLONE_CLEAR_SIGHAND);
clone3_cgroup_supported = (how.flags & CLONE_INTO_CGROUP) &&
(how.cgroup != 0);
break;
}
}
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/fork.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cc760491f201..bded93f4f5f4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2925,11 +2925,15 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
}
#endif
+
+#define CLONE3_VALID_FLAGS (CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP)
+
noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
struct clone_args __user *uargs,
size_t usize)
{
int err;
+ bool check_fields;
struct clone_args args;
pid_t *kset_tid = kargs->set_tid;
@@ -2941,11 +2945,34 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
CLONE_ARGS_SIZE_VER2);
BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
+ check_fields = usize & CHECK_FIELDS;
+ usize &= ~CHECK_FIELDS;
+
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
if (unlikely(usize < CLONE_ARGS_SIZE_VER0))
return -EINVAL;
+ if (unlikely(check_fields)) {
+ memset(&args, 0, sizeof(args));
+ args = (struct clone_args) {
+ .flags = CLONE3_VALID_FLAGS,
+ .pidfd = 0xFFFFFFFFFFFFFFFF,
+ .child_tid = 0xFFFFFFFFFFFFFFFF,
+ .parent_tid = 0xFFFFFFFFFFFFFFFF,
+ .exit_signal = (u64) CSIGNAL,
+ .stack = 0xFFFFFFFFFFFFFFFF,
+ .stack_size = 0xFFFFFFFFFFFFFFFF,
+ .tls = 0xFFFFFFFFFFFFFFFF,
+ .set_tid = 0xFFFFFFFFFFFFFFFF,
+ .set_tid_size = 0xFFFFFFFFFFFFFFFF,
+ .cgroup = 0xFFFFFFFFFFFFFFFF,
+ };
+
+ err = copy_struct_to_user(uargs, usize, &args, sizeof(args), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
if (err)
return err;
@@ -3025,8 +3052,7 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
- if (kargs->flags &
- ~(CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP))
+ if (kargs->flags & ~CLONE3_VALID_FLAGS)
return false;
/*
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 08/10] selftests: clone3: add CHECK_FIELDS selftests
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 4 +-
.../testing/selftests/clone3/clone3_check_fields.c | 264 +++++++++++++++++++++
3 files changed, 267 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore
index 83c0f6246055..4ec3e1ecd273 100644
--- a/tools/testing/selftests/clone3/.gitignore
+++ b/tools/testing/selftests/clone3/.gitignore
@@ -3,3 +3,4 @@ clone3
clone3_clear_sighand
clone3_set_tid
clone3_cap_checkpoint_restore
+clone3_check_fields
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
index 84832c369a2e..37141ca13f7c 100644
--- a/tools/testing/selftests/clone3/Makefile
+++ b/tools/testing/selftests/clone3/Makefile
@@ -1,8 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES)
+CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
LDLIBS += -lcap
TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \
- clone3_cap_checkpoint_restore
+ clone3_cap_checkpoint_restore clone3_check_fields
include ../lib.mk
diff --git a/tools/testing/selftests/clone3/clone3_check_fields.c b/tools/testing/selftests/clone3/clone3_check_fields.c
new file mode 100644
index 000000000000..477604f9a3fb
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3_check_fields.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Author: Aleksa Sarai <cyphar@cyphar.com>
+ * Copyright (C) 2024 SUSE LLC
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <inttypes.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sched.h>
+
+#include "../kselftest.h"
+#include "clone3_selftests.h"
+
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
+struct __clone_args_v0 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+};
+
+struct __clone_args_v1 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+ __aligned_u64 set_tid;
+ __aligned_u64 set_tid_size;
+};
+
+struct __clone_args_v2 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+ __aligned_u64 set_tid;
+ __aligned_u64 set_tid_size;
+ __aligned_u64 cgroup;
+};
+
+static int call_clone3(void *clone_args, size_t size)
+{
+ int status;
+ pid_t pid;
+
+ pid = sys_clone3(clone_args, size);
+ if (pid < 0)
+ return -errno;
+
+ if (pid == 0) {
+ ksft_print_msg("I am the child, my PID is %d\n", getpid());
+ _exit(EXIT_SUCCESS);
+ }
+
+ ksft_print_msg("I am the parent (%d). My child's pid is %d\n",
+ getpid(), pid);
+
+ if (waitpid(-1, &status, __WALL) < 0) {
+ ksft_print_msg("waitpid() returned %s\n", strerror(errno));
+ return -errno;
+ }
+ if (!WIFEXITED(status)) {
+ ksft_print_msg("Child did not exit normally, status 0x%x\n",
+ status);
+ return EXIT_FAILURE;
+ }
+ if (WEXITSTATUS(status))
+ return WEXITSTATUS(status);
+
+ return 0;
+}
+
+static bool check(bool *failed, bool pred)
+{
+ *failed |= pred;
+ return pred;
+}
+
+static void test_clone3_check_fields(const char *test_name, size_t struct_size)
+{
+ size_t bufsize;
+ void *buffer;
+ pid_t pid;
+ bool failed = false;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ /* Allocate some bytes after clone_args to verify they are cleared. */
+ bufsize = struct_size + 16;
+ buffer = malloc(bufsize);
+ /* Set the structure to dummy values. */
+ memset(buffer, 0, bufsize);
+ memset(buffer, 0xAB, struct_size);
+
+ pid = call_clone3(buffer, CHECK_FIELDS | struct_size);
+ if (check(&failed, (pid != -EEXTSYS_NOOP)))
+ ksft_print_msg("clone3(CHECK_FIELDS) returned the wrong error code: %d (%s) != %d\n",
+ pid, strerror(-pid), -EEXTSYS_NOOP);
+
+ switch (struct_size) {
+ case sizeof(struct __clone_args_v2): {
+ struct __clone_args_v2 *args = buffer;
+
+ if (check(&failed, (args->cgroup != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong cgroup field: 0x%.16llx != 0x%.16llx\n",
+ args->cgroup, 0xFFFFFFFFFFFFFFFF);
+
+ /* fallthrough; */
+ }
+ case sizeof(struct __clone_args_v1): {
+ struct __clone_args_v1 *args = buffer;
+
+ if (check(&failed, (args->set_tid != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong set_tid field: 0x%.16llx != 0x%.16llx\n",
+ args->set_tid, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->set_tid_size != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong set_tid_size field: 0x%.16llx != 0x%.16llx\n",
+ args->set_tid_size, 0xFFFFFFFFFFFFFFFF);
+
+ /* fallthrough; */
+ }
+ case sizeof(struct __clone_args_v0): {
+ struct __clone_args_v0 *args = buffer;
+
+ if (check(&failed, !(args->flags & CLONE_NEWUSER)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_NEWUSER in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_NEWUSER);
+ if (check(&failed, !(args->flags & CLONE_THREAD)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_THREAD in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_THREAD);
+ /*
+ * CLONE_INTO_CGROUP was added in v2, but it will be set even
+ * with smaller structure sizes.
+ */
+ if (check(&failed, !(args->flags & CLONE_INTO_CGROUP)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_INTO_CGROUP in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_INTO_CGROUP);
+
+ if (check(&failed, (args->exit_signal != 0xFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong exit_signal field: 0x%.16llx != 0x%.16llx\n",
+ args->exit_signal, 0xFF);
+
+ if (check(&failed, (args->stack != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong stack field: 0x%.16llx != 0x%.16llx\n",
+ args->stack, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->stack_size != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong stack_size field: 0x%.16llx != 0x%.16llx\n",
+ args->stack_size, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->tls != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong tls field: 0x%.16llx != 0x%.16llx\n",
+ args->tls, 0xFFFFFFFFFFFFFFFF);
+
+ break;
+ }
+ default:
+ fprintf(stderr, "INVALID STRUCTURE SIZE: %d\n", struct_size);
+ abort();
+ }
+
+ /* Verify that the trailing parts of the buffer are still 0. */
+ for (size_t i = struct_size; i < bufsize; i++) {
+ char ch = ((char *)buffer)[i];
+ if (check(&failed, (ch != '\x00')))
+ ksft_print_msg("clone3(CHECK_FIELDS) touched a byte outside the size: buffer[%d] = 0x%.2x\n",
+ i, ch);
+ }
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+ resultfn("clone3(CHECK_FIELDS) with %s\n", test_name);
+ free(buffer);
+}
+
+struct check_fields_test {
+ const char *name;
+ size_t struct_size;
+};
+
+static struct check_fields_test check_fields_tests[] = {
+ {"struct v0", sizeof(struct __clone_args_v0)},
+ {"struct v1", sizeof(struct __clone_args_v1)},
+ {"struct v2", sizeof(struct __clone_args_v2)},
+};
+
+static int test_clone3_check_fields_badsize(const char *test_name,
+ size_t struct_size)
+{
+ void *buffer;
+ pid_t pid;
+ bool failed = false;
+ int expected_err;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ buffer = malloc(struct_size);
+ memset(buffer, 0xAB, struct_size);
+
+ if (struct_size < sizeof(struct __clone_args_v0))
+ expected_err = -EINVAL;
+ if (struct_size > 4096)
+ expected_err = -E2BIG;
+
+ pid = call_clone3(buffer, CHECK_FIELDS | struct_size);
+ if (check(&failed, (pid != expected_err)))
+ ksft_print_msg("clone3(CHECK_FIELDS) returned the wrong error code: %d (%s) != %d (%s)\n",
+ pid, strerror(-pid),
+ expected_err, strerror(-expected_err));
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+ resultfn("clone3(CHECK_FIELDS) with %s returns %d (%s)\n", test_name,
+ expected_err, strerror(-expected_err));
+ free(buffer);
+}
+
+static struct check_fields_test bad_size_tests[] = {
+ {"short struct (< v0)", 1},
+ {"long struct (> PAGE_SIZE)", 0xF000},
+};
+
+int main(void)
+{
+ ksft_print_header();
+ ksft_set_plan(ARRAY_SIZE(check_fields_tests) + ARRAY_SIZE(bad_size_tests));
+ test_clone3_supported();
+
+ for (int i = 0; i < ARRAY_SIZE(check_fields_tests); i++) {
+ struct check_fields_test *test = &check_fields_tests[i];
+ test_clone3_check_fields(test->name, test->struct_size);
+ }
+ for (int i = 0; i < ARRAY_SIZE(bad_size_tests); i++) {
+ struct check_fields_test *test = &bad_size_tests[i];
+ test_clone3_check_fields_badsize(test->name, test->struct_size);
+ }
+
+ ksft_finished();
+}
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 09/10] mount_setattr: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
As with openat2(2), this allows userspace to easily figure out what
flags and fields are supported by mount_setattr(2). As with clone3(2),
for fields which are not flag-based, we simply set every bit in the
field so that a naive bitwise-and would show that any value of the field
is valid.
The intended way of using this interface to get feature information
looks something like the following:
static bool mountattr_nosymfollow_supported;
static bool mountattr_idmap_supported;
int check_clone3_support(void)
{
int err;
struct mount_attr attr = {};
err = mount_attr(-EBADF, "", 0, &args, CHECK_FIELDS | sizeof(args));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
mountattr_nosymfollow_supported =
((attr.attr_clr | attr.attr_set) & MOUNT_ATTR_NOSYMFOLLOW);
mountattr_idmap_supported =
((attr.attr_clr | attr.attr_set) & MOUNT_ATTR_IDMAP) &&
(attr.userns_fd != 0);
break;
}
}
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namespace.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/fs/namespace.c b/fs/namespace.c
index 328087a4df8a..c7ae8d96b7b7 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4771,6 +4771,7 @@ SYSCALL_DEFINE5(mount_setattr, int, dfd, const char __user *, path,
size_t, usize)
{
int err;
+ bool check_fields;
struct path target;
struct mount_attr attr;
struct mount_kattr kattr;
@@ -4783,11 +4784,27 @@ SYSCALL_DEFINE5(mount_setattr, int, dfd, const char __user *, path,
AT_NO_AUTOMOUNT))
return -EINVAL;
+ check_fields = usize & CHECK_FIELDS;
+ usize &= ~CHECK_FIELDS;
+
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
if (unlikely(usize < MOUNT_ATTR_SIZE_VER0))
return -EINVAL;
+ if (unlikely(check_fields)) {
+ memset(&attr, 0, sizeof(attr));
+ attr = (struct mount_attr) {
+ .attr_set = MOUNT_SETATTR_VALID_FLAGS,
+ .attr_clr = MOUNT_SETATTR_VALID_FLAGS,
+ .propagation = MOUNT_SETATTR_PROPAGATION_FLAGS,
+ .userns_fd = 0xFFFFFFFFFFFFFFFF,
+ };
+
+ err = copy_struct_to_user(uattr, usize, &attr, sizeof(attr), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
if (!may_mount())
return -EPERM;
--
2.46.0
^ permalink raw reply related
* [PATCH RFC v2 10/10] selftests: mount_setattr: add CHECK_FIELDS selftest
From: Aleksa Sarai @ 2024-09-05 14:56 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
In-Reply-To: <20240906-extensible-structs-check_fields-v2-0-0f46d2de9bad@cyphar.com>
While we're at it -- to make testing for error codes with ASSERT_*
easier, make the sys_* wrappers return -errno in case of an error.
For some reason, the <sys/sysinfo.h> include doesn't correct import
<asm/posix_types.h>, leading to compilation errors -- so just import
<asm/posix_types.h> manually.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/include/uapi/asm-generic/posix_types.h | 101 +++++++++++++++++++++
tools/testing/selftests/mount_setattr/Makefile | 2 +-
.../selftests/mount_setattr/mount_setattr_test.c | 53 ++++++++++-
3 files changed, 153 insertions(+), 3 deletions(-)
diff --git a/tools/include/uapi/asm-generic/posix_types.h b/tools/include/uapi/asm-generic/posix_types.h
new file mode 100644
index 000000000000..b5f7594eee7a
--- /dev/null
+++ b/tools/include/uapi/asm-generic/posix_types.h
@@ -0,0 +1,101 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __ASM_GENERIC_POSIX_TYPES_H
+#define __ASM_GENERIC_POSIX_TYPES_H
+
+#include <asm/bitsperlong.h>
+/*
+ * This file is generally used by user-level software, so you need to
+ * be a little careful about namespace pollution etc.
+ *
+ * First the types that are often defined in different ways across
+ * architectures, so that you can override them.
+ */
+
+#ifndef __kernel_long_t
+typedef long __kernel_long_t;
+typedef unsigned long __kernel_ulong_t;
+#endif
+
+#ifndef __kernel_ino_t
+typedef __kernel_ulong_t __kernel_ino_t;
+#endif
+
+#ifndef __kernel_mode_t
+typedef unsigned int __kernel_mode_t;
+#endif
+
+#ifndef __kernel_pid_t
+typedef int __kernel_pid_t;
+#endif
+
+#ifndef __kernel_ipc_pid_t
+typedef int __kernel_ipc_pid_t;
+#endif
+
+#ifndef __kernel_uid_t
+typedef unsigned int __kernel_uid_t;
+typedef unsigned int __kernel_gid_t;
+#endif
+
+#ifndef __kernel_suseconds_t
+typedef __kernel_long_t __kernel_suseconds_t;
+#endif
+
+#ifndef __kernel_daddr_t
+typedef int __kernel_daddr_t;
+#endif
+
+#ifndef __kernel_uid32_t
+typedef unsigned int __kernel_uid32_t;
+typedef unsigned int __kernel_gid32_t;
+#endif
+
+#ifndef __kernel_old_uid_t
+typedef __kernel_uid_t __kernel_old_uid_t;
+typedef __kernel_gid_t __kernel_old_gid_t;
+#endif
+
+#ifndef __kernel_old_dev_t
+typedef unsigned int __kernel_old_dev_t;
+#endif
+
+/*
+ * Most 32 bit architectures use "unsigned int" size_t,
+ * and all 64 bit architectures use "unsigned long" size_t.
+ */
+#ifndef __kernel_size_t
+#if __BITS_PER_LONG != 64
+typedef unsigned int __kernel_size_t;
+typedef int __kernel_ssize_t;
+typedef int __kernel_ptrdiff_t;
+#else
+typedef __kernel_ulong_t __kernel_size_t;
+typedef __kernel_long_t __kernel_ssize_t;
+typedef __kernel_long_t __kernel_ptrdiff_t;
+#endif
+#endif
+
+#ifndef __kernel_fsid_t
+typedef struct {
+ int val[2];
+} __kernel_fsid_t;
+#endif
+
+/*
+ * anything below here should be completely generic
+ */
+typedef __kernel_long_t __kernel_off_t;
+typedef long long __kernel_loff_t;
+typedef __kernel_long_t __kernel_old_time_t;
+#ifndef __KERNEL__
+typedef __kernel_long_t __kernel_time_t;
+#endif
+typedef long long __kernel_time64_t;
+typedef __kernel_long_t __kernel_clock_t;
+typedef int __kernel_timer_t;
+typedef int __kernel_clockid_t;
+typedef char * __kernel_caddr_t;
+typedef unsigned short __kernel_uid16_t;
+typedef unsigned short __kernel_gid16_t;
+
+#endif /* __ASM_GENERIC_POSIX_TYPES_H */
diff --git a/tools/testing/selftests/mount_setattr/Makefile b/tools/testing/selftests/mount_setattr/Makefile
index 0c0d7b1234c1..3f260506fe60 100644
--- a/tools/testing/selftests/mount_setattr/Makefile
+++ b/tools/testing/selftests/mount_setattr/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for mount selftests.
-CFLAGS = -g $(KHDR_INCLUDES) -Wall -O2 -pthread
+CFLAGS = -g $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -Wall -O2 -pthread
TEST_GEN_PROGS := mount_setattr_test
diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index c6a8c732b802..36b8286e5f43 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -11,6 +11,7 @@
#include <sys/wait.h>
#include <sys/vfs.h>
#include <sys/statvfs.h>
+#include <asm/posix_types.h> /* get __kernel_* typedefs */
#include <sys/sysinfo.h>
#include <stdlib.h>
#include <unistd.h>
@@ -137,7 +138,8 @@
static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flags,
struct mount_attr *attr, size_t size)
{
- return syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+ int ret = syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+ return ret >= 0 ? ret : -errno;
}
#ifndef OPEN_TREE_CLONE
@@ -152,9 +154,24 @@ static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flag
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
#endif
+#define FSMOUNT_VALID_FLAGS \
+ (MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID | MOUNT_ATTR_NODEV | \
+ MOUNT_ATTR_NOEXEC | MOUNT_ATTR__ATIME | MOUNT_ATTR_NODIRATIME | \
+ MOUNT_ATTR_NOSYMFOLLOW)
+
+#define MOUNT_SETATTR_VALID_FLAGS (FSMOUNT_VALID_FLAGS | MOUNT_ATTR_IDMAP)
+
+#define MOUNT_SETATTR_PROPAGATION_FLAGS \
+ (MS_UNBINDABLE | MS_PRIVATE | MS_SLAVE | MS_SHARED)
+
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
static inline int sys_open_tree(int dfd, const char *filename, unsigned int flags)
{
- return syscall(__NR_open_tree, dfd, filename, flags);
+ int ret = syscall(__NR_open_tree, dfd, filename, flags);
+ return ret >= 0 ? ret : -errno;
}
static ssize_t write_nointr(int fd, const void *buf, size_t count)
@@ -1497,4 +1514,36 @@ TEST_F(mount_setattr, mount_attr_nosymfollow)
ASSERT_EQ(close(fd), 0);
}
+TEST_F(mount_setattr, check_fields)
+{
+ struct mount_attr_ext {
+ struct mount_attr inner;
+ uint64_t extra1;
+ uint64_t extra2;
+ uint64_t extra3;
+ } attr;
+
+ /* Set the structure to dummy values. */
+ memset(&attr, 0xAB, sizeof(attr));
+
+ if (!mount_setattr_supported())
+ SKIP(return, "mount_setattr syscall not supported");
+
+ /* Make sure that invalid sizes are detected even with CHECK_FIELDS. */
+ EXPECT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS), -EINVAL);
+ EXPECT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS | 0xF000), -E2BIG);
+
+ /* Actually get the CHECK_FIELDS results. */
+ ASSERT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS | sizeof(attr)), -EEXTSYS_NOOP);
+
+ EXPECT_EQ(attr.inner.attr_set, MOUNT_SETATTR_VALID_FLAGS);
+ EXPECT_EQ(attr.inner.attr_clr, MOUNT_SETATTR_VALID_FLAGS);
+ EXPECT_EQ(attr.inner.propagation, MOUNT_SETATTR_PROPAGATION_FLAGS);
+ EXPECT_EQ(attr.inner.userns_fd, 0xFFFFFFFFFFFFFFFF);
+ /* Trailing fields outside ksize must be zeroed. */
+ EXPECT_EQ(attr.extra1, 0);
+ EXPECT_EQ(attr.extra2, 0);
+ EXPECT_EQ(attr.extra3, 0);
+}
+
TEST_HARNESS_MAIN
--
2.46.0
^ permalink raw reply related
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