linux-kernel - Re: [PATCH RFC v3 0/2] fhandle: expose u64 mount id to name_to_handle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240802.010502-peachy.struggle.moldy.shape-YcRNLL7bq7EE@cyphar.com>
Date: Fri, 2 Aug 2024 11:43:50 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: Josef Bacik <josef@...icpanda.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Chuck Lever <chuck.lever@...cle.com>, 
	Jeff Layton <jlayton@...nel.org>, Amir Goldstein <amir73il@...il.com>, 
	Alexander Aring <alex.aring@...il.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, 
	Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>, 
	Adrian Hunter <adrian.hunter@...el.com>, linux-fsdevel@...r.kernel.org, linux-nfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-api@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH RFC v3 0/2] fhandle: expose u64 mount id to
 name_to_handle_at(2)

On 2024-08-01, Josef Bacik <josef@...icpanda.com> wrote:
> On Thu, Aug 01, 2024 at 01:52:39PM +1000, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx(2), we
> > can now provide a race-free way for name_to_handle_at(2) to provide a
> > file handle and corresponding mount without needing to worry about
> > racing with /proc/mountinfo parsing or having to open a file just to do
> > statx(2).
> > 
> > While this is not necessary if you are using AT_EMPTY_PATH and don't
> > care about an extra statx(2) call, users that pass full paths into
> > name_to_handle_at(2) need to know which mount the file handle comes from
> > (to make sure they don't try to open_by_handle_at a file handle from a
> > different filesystem) and switching to AT_EMPTY_PATH would require
> > allocating a file for every name_to_handle_at(2) call, turning
> > 
> >   err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
> >                           AT_HANDLE_MNT_ID_UNIQUE);
> > 
> > into
> > 
> >   int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
> >   err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
> >   err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
> >   mntid = statxbuf.stx_mnt_id;
> >   close(fd);
> > 
> > Also, this series adds a patch to clarify how AT_* flag allocation
> > should work going forwards.
> > 
> > Signed-off-by: Aleksa Sarai <cyphar@...har.com>
> > ---
> > Changes in v3:
> > - Added a patch describing how AT_* flags should be allocated in the
> >   future, based on Amir's suggestions.
> > - Included AT_* aliases for RENAME_* flags to further indicate that
> >   renameat2(2) is an *at(2) syscall and to indicate that those flags
> >   have been allocated already in the per-syscall range.
> > - Switched AT_HANDLE_MNT_ID_UNIQUE to use 0x01 (to reuse
> >   (AT_)RENAME_NOREPLACE).
> > - v2: <https://lore.kernel.org/r/20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
> > Changes in v2:
> > - Fixed a few minor compiler warnings and a buggy copy_to_user() check.
> > - Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
> > - Switched to using an AT_* bit from 0xFF and defining that range as
> >   being "per-syscall" for future usage.
> > - Sync tools/ copy of <linux/fcntl.h> to include changes.
> > - v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>
> > 
> > ---
> > Aleksa Sarai (2):
> >       uapi: explain how per-syscall AT_* flags should be allocated
> >       fhandle: expose u64 mount id to name_to_handle_at(2)
> > 
> 
> Wasn't the conclusion from this discussion last time that we needed to revisit
> this API completely?  Christoph had some pretty adamant objections.

There was a discussion about reworking the API and I agree with most of
the issues raised about file handles (I personally don't really like
this interface and it's a bit of a shame that it seems this is going to
be the interface that replaces inode numbers) so I'm not at all opposed
to reworking it.

However, I agree with Christian[1] that we can fix this existing issue
in the existing API fairly easily and then work on a new API separately.
The existing usage of name_to_handle_at() is fundamentally unsafe (as
outlined in the man page) and we can fix that fairly easily.

[1]: https://lore.kernel.org/all/20240527-hagel-thunfisch-75781b0cf75d@brauner/

> That being said the uapi comments patch looks good to me, you can add
> 
> Reviewed-by: Josef Bacik <josef@...icpanda.com>
> 
> to that one.  The other one I'm going to let others who have stronger opinions
> than me argue about.  Thanks,
> 
> Josef

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)