On Fri, Nov 29, 2024 at 2:39 PM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxx> 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.
