Received-SPF: pass (google.com: domain of linux-nfs+bounces-3466-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
 <ZlMADupKkN0ITgG5@infradead.org> <30137c868039a3ae17f4ae74d07383099bfa4db8.camel@hammerspace.com>
 <ZlRzNquWNalhYtux@infradead.org> <86065f6a4f3d2f3d78f39e7a276a2d6e25bfbc9d.camel@hammerspace.com>
 <ZlS0_DWzGk24GYZA@infradead.org> <20240528101152.kyvtx623djnxwonm@quack3>
 <ZlW4a6Zdt9SPTt80@infradead.org> <ZlZn/fcphsx8u/Ph@dread.disaster.area> <ZlbKEr15IXO2jxXd@infradead.org>
In-Reply-To: <ZlbKEr15IXO2jxXd@infradead.org>
From: Amir Goldstein <amir73il@gmail.com>
Date: Wed, 29 May 2024 10:23:20 +0300
Message-ID: <CAOQ4uxiXVX-dJTN0UOjP7Zcfnks_kJoHBE+XFBecnUzzK-e5_w@mail.gmail.com>
Subject: Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
To: "hch@infradead.org" <hch@infradead.org>
Cc: Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.cz>, 
	Trond Myklebust <trondmy@hammerspace.com>, "chuck.lever@oracle.com" <chuck.lever@oracle.com>, 
	"linux-api@vger.kernel.org" <linux-api@vger.kernel.org>, 
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, "brauner@kernel.org" <brauner@kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"alex.aring@gmail.com" <alex.aring@gmail.com>, "cyphar@cyphar.com" <cyphar@cyphar.com>, 
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>, "jlayton@kernel.org" <jlayton@kernel.org>, 
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 29, 2024 at 9:24=E2=80=AFAM hch@infradead.org <hch@infradead.or=
g> wrote:
>
> On Wed, May 29, 2024 at 09:25:49AM +1000, Dave Chinner wrote:
> > But no-one has bothered to reply or acknowledge my comments so I'll
> > point them out again and repeat: Filehandles generated by
> > the kernel for unprivileged use *must* be self describing and self
> > validating

Very nice requirement for a very strong feature,
which is far out of scope for the proposed patch IMO.

What is "generated by the kernel for unprivileged use"?
name_to_handle_at() is unprivileged and for example, fanotify unprivileged
users can use it to compare a marked (i.e. watched) object with an
object that is the subject of an event.
This does not break any security model.

> > as the kernel must be able to detect and prevent
> > unprivelged users from generating custom filehandles that can be
> > used to access files outside the restricted scope of their
> > container.

Emphasis on "that can be used to access files".
The patch in this thread to get a unique 64bit mntid does not make any
difference wrt to the concern above, so I am having a hard time
understand what the fuss is about.

>
> We must not generate file handle for unprivileged use at all, as they
> bypass all the path based access controls.
>

Again, how is "generate file handle for unprivileged use" related to
the patch in question.

The simple solution to the race that Aleksa is trying to prevent,
as was mentioned several times in this thread, is to allow
name_to_handle_at() on an empty path, e.g.:
fd =3D openat(.., O_PATH); fstatat(fd,..); name_to_handle_at(fd,..);

Aleksa prefers the unique mntid solution to save 2 syscalls.
Would any of the objections here been called for letting
name_to_handle_at() take an empty path?

I would like to offer a different solution (also may have been
mentioned before).

I always disliked the fact that mount_id and mount_fd arguments of
name_to_handle_at()/open_by_handle_at() are not symmetric, when
at first glance they appear to be symmetric as the handle argument.

What if we can make them symmetric and save the openat() syscall?

int path_fd;
int ret =3D name_to_handle_at(dirfd, path, handle, &path_fd,
                                              AT_HADNLE_PATH_FD);

and then kernel returns an O_PATH fd to the path object
in addition to the file handle (requires same privilege as
encoding the fh).

Userspace can use path_fd to query unique mntid, fsid, uuid
or whatever it needs to know in order to make sure that a
later call in the future to open_by_handle_at() will use
a mount_fd from the same filesystem/mount whatever,
whether in the same boot or after reboot.

If we are doing this, it also makes sense to allow mount_fd argument
to open_by_handle_at() to accept O_PATH fd, but that makes
sense anyway, because mount_fd is essentially a reference to
a path.

Thanks,
Amir.