Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

Chuck Lever III
2024-10-11T18:40:28Z

> On Oct 11, 2024, at 2:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>>
>>
>>
>>> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>
>>> Christian,
>>>
>>> These patches bring the NFS connectable file handles feature to
>>> userspace servers.
>>>
>>> They rely on your and Aleksa's changes recently merged to v6.12.
>>>
>>> This v4 incorporates the review comments on Jeff and Jan (thanks!)
>>> and there does not seem to be any objection for this new API, so
>>> I think it is ready for staging.
>>>
>>> The API I chose for encoding conenctable file handles is pretty
>>> conventional (AT_HANDLE_CONNECTABLE).
>>>
>>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find
>>> it more useful API that encoding a connectable file handle can mandate
>>> the resolving of a connected fd, without having to opt-in for a
>>> connected fd independently.
>>>
>>> I chose to implemnent this by using upper bits in the handle type field
>>> It may be that out-of-tree filesystems return a handle type with upper
>>> bits set, but AFAIK, no in-tree filesystem does that.
>>> I added some warnings just in case we encouter that.
>>>
>>> I have written an fstest [4] and a man page draft [5] for the feature.
>>>
>>> Thanks,
>>> Amir.
>>>
>>> Changes since v3 [3]:
>>> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
>>> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
>>> - Add explicit check for negative type values (Jan)
>>> - Added fstest and man-page draft
>>>
>>> Changes since v2 [2]:
>>> - Use bit arithmetics instead of bitfileds (Jeff)
>>> - Add assertions about use of high type bits
>>>
>>> Changes since v1 [1]:
>>> - Assert on encode for disconnected path (Jeff)
>>> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
>>> - Drop the O_PATH mount_fd API hack (Jeff)
>>> - Encode an explicit "connectable" flag in handle type
>>>
>>> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
>>> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
>>> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
>>> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
>>> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/
>>>
>>> Amir Goldstein (3):
>>> fs: prepare for "explicit connectable" file handles
>>> fs: name_to_handle_at() support for "explicit connectable" file
>>> handles
>>> fs: open_by_handle_at() support for decoding "explicit connectable"
>>> file handles
>>>
>>> fs/exportfs/expfs.c | 17 ++++++++-
>>> fs/fhandle.c | 75 +++++++++++++++++++++++++++++++++++---
>>> include/linux/exportfs.h | 13 +++++++
>>> include/uapi/linux/fcntl.h | 1 +
>>> 4 files changed, 98 insertions(+), 8 deletions(-)
>>>
>>> --
>>> 2.34.1
>>>
>>
>> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>>
>> Assuming this is going directly to Christian's tree.
>>
>> I'm a little concerned about how this new facility might be
>> abused to get access to parts of the file system that a user
>> is not authorized to access.
>
> That's exactly the sort of thing I would like to be reviewed,
> but what makes you feel concerned?
>
> Are you concerned about handcrafted file handles?

Yes; a user could construct a file handle that could bypass
the usual authorization checks when it gets connected. It's
a little hare-brained and hand-wavy because this is a new
area for me.

> Correct me if I am wrong, but I think that any parts of the filesystem
> that could be accessed (by user with CAP_DAC_READ_SEARCH)
> using a handcrafted connectable file handle, could have also been
> accessed by the parent fid part before, so I do not see how connectable
> file handles create new ways to get access?
>
>> But follow-up comments from Amir
>> suggest that (with the current code) it is difficult or
>> impossible to do.
>>
>> Are there self-tests or unit-tests for exportfs?
>
> There are fstests, particularly, the "exportfs" test group
> and I added this one for connectable file handles:
>
> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
>
> Did you mean another form of unit tests?

No, actually; fstests wfm. That's something I could set
up via kdevops and run on a regular basis.

--
Chuck Lever