On Mon, 24 Jul 2023 at 09:46, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> So thinking a little about it I think that doesn't work.
> /proc/<pid>/fd/<xyz> does a reopen and for good reasons. The original
> open will have gone through the module's/subsytem's ->open() method
> which might stash additional refcounted data in e.g., file->private_data
> if we simply copy that file or sm then we risk UAFs.
Oh, absolutely, we';d absolutely need to do all the re-open things.
That said, we could limit it to FMODE_ATOMIC_POS - so just regular
files and directories. The only thing that sets that bit is
do_dentry_open().
And honestly, the only thing that *really* cares is directories,
because they generally have special rules for pos changing.
The regular files have the "POSIX rules" reason, but hey, if you use
pidfd_getfd() and mess with the pos behind the back of the process,
it's no different from using a debugger to change it, so the POSIX
rules issue just isn't relevant.
I really hate making the traditional unix single-threaded file
descriptor case take that lock.
Maybe it doesn't matter. Obviously it can't have contention, and your
patch in that sense is pretty benign.
But locking is just fundamentally expensive in the first place, and it
annoys me that I never realized that pidfd_getfd() did that thing that
I knew was broken for /proc.
Linus
