Re: [PATCH RFC 0/6] pidfs: implement file handle support

Amir Goldstein
2024-11-30T12:22:18Z

On Fri, Nov 29, 2024 at 2:39 PM Christian Brauner <brauner@kernel.org> wrote:
>
> Hey,
>
> Now that we have the preliminaries to lookup struct pid based on its
> inode number alone we can implement file handle support.
>
> This is based on custom export operation methods which allows pidfs to
> implement permission checking and opening of pidfs file handles cleanly
> without hacking around in the core file handle code too much.
>
> This is lightly tested.

With my comments addressed as you pushed to vfs-6.14.pidfs branch
in your tree, you may add to the patches posted:

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

HOWEVER,
IMO there is still one thing that has to be addressed before merge -
We must make sure that nfsd cannot export pidfs.

In principal, SB_NOUSER filesystems should not be accessible to
userspace paths, so exportfs should not be able to configure nfsd
export of pidfs, but maybe this limitation can be worked around by
using magic link paths?

I think it may be worth explicitly disallowing nfsd export of SB_NOUSER
filesystems and we could also consider blocking SB_KERNMOUNT,
but may there are users exporting ramfs?

Jeff has mentioned that he thinks we are blocking export of cgroupfs
by nfsd, but I really don't see where that is being enforced.
The requirement for FS_REQUIRES_DEV in check_export() is weak
because user can overrule it with manual fsid argument to exportfs.

So maybe we disallow nfsd export of kernfs and backport to stable kernels
to be on the safe side?

On top of that, we may also want to reject nfsd export of any fs
with custom ->open() or ->permission() export ops, on the grounds
that nfsd does not call these ops?

Regarding the two other kernel users of exportfs, namely,
overlayfs and fanotify -

For overlayfs, I think that in ovl_can_decode_fh() we can safely
opt-out of SB_NOUSER and SB_KERNMOUNT filesystems,
to not allow nfs exporting of overlayfs over those lower fs.

For fanotify, there is already a check in fanotify_events_supported()
to disallow sb/mount marks on SB_NOUSER and a comment that
questions the value of allowing them for SB_KERNMOUNT.
So for pidfs there is no risk wrt fanotify and it does not look like pidfs
is going to generate any fanotify events anyway.

Thanks,
Amir.