Re: [PATCH v2 3/3] pidfs: implement file handle support

Erin Shepherd
2024-11-14T13:13:10Z

On 14/11/2024 13:52, Christian Brauner wrote:

> I think you need at least something like the following completely
> untested draft on top:
>
> - the pidfs_finish_open_by_handle_at() is somewhat of a clutch to handle
> thread vs thread-group pidfds but it works.
>
> - In contrast to pidfd_open() that uses dentry_open() to create a pidfd
> open_by_handle_at() uses file_open_root(). That's overall fine but
> makes pidfds subject to security hooks which they aren't via
> pidfd_open(). It also necessitats pidfs_finish_open_by_handle_at().
> There's probably other solutions I'm not currently seeing.

These two concerns combined with the special flag make me wonder if pidfs
is so much of a special snowflake we should just special case it up front
and skip all of the shared handle decode logic?

> - The exportfs_decode_fh_raw() call that's used to decode the pidfd is
> passed vfs_dentry_acceptable() as acceptability callback. For pidfds
> we don't need any of that functionality and we don't need any of the
> disconnected dentry handling logic. So the easiest way to fix that is
> to rely on EXPORT_OP_UNRESTRICTED_OPEN to skip everything. That in
> turns means the only acceptability we have is the nop->fh_to_dentry()
> callback for pidfs.

With the current logic we go exportfs_decode_fh_raw(...) ->
find_acceptable_alias(result) -> vfs_dentry_acceptable(context, result).

vfs_dentry_acceptable immediately returns 1 if ctx->flags is 0, which will
always be the case if EXPORT_OP_UNRESTRICTED_OPEN was set, so we immediately
fall back out of the call tree and return result.

So I'm not 100% sure we actually need this special case but I'm not opposed.

> - This all really needs rigorous selftests before we can even think of
> merging any of this.