On Fri, Oct 4, 2024 at 1:45 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> overlayfs uses struct fd for the things where it does not quite
> fit. There we want not "file reference or nothing" - it's "file reference
> or an error". For pointers we can do that by use of ERR_PTR(); strictly
> speaking, that's an abuse of C pointers, but the kernel does make sure
> that the uppermost page worth of virtual addresses never gets mapped
> and no architecture we care about has those as trap representations.
> It might be possible to use the same trick for struct fd; however, for
> most of the regular struct fd users that would be a pointless headache -
> it would pessimize the code generation for no real benefit. I considered
> a possibility of using represenation of ERR_PTR(-EBADF) for empty struct
> fd, but that's far from being consistent among the struct fd users and
> that ends up with bad code generation even for the users that want to
> treat empty as "fail with -EBADF".
>
> It's better to introduce a separate type, say, struct fderr.
> Representable values:
> * borrowed file reference (address of struct file instance)
> * cloned file reference (address of struct file instance)
> * error (a small negative integer).
>
> With sane constructors we get rid of the field-by-field mess in
> ovl_real_fdget{,_meta}() and have them serve as initializers, always
> returning a valid struct fderr value.
>
> That results in mostly sane code; however, there are several
> places where we run into an unpleasant problem - the intended scope
> is nested inside inode_lock()/inode_unlock(), with problematic gotos.
>
> Possible solutions:
> 1) turn inode_lock() into guard-based thing. No, with the side of
> "fuck, no". guard() has its uses, but
> * the area covered by it should be _very_ easy to see
> * it should not be mixed with gotos, now *OR* later, so
> any subsequent changes in the area should remember not to use those.
> * it should never fall into hands of Markus-level entities.
> inode_lock fails all those; guard-based variant _will_ be abstracted
> away by well-meaning folks, and it will start spreading. And existing
> users are often long, have non-trivial amounts of goto-based cleanups
> in them and lifetimes of the objects involved are not particularly
> easy to convert to __cleanup-based variants (to put it very mildly).
>
> We might eventually need to reconsider that, but for now the only sane
> policy is "no guard-based variants of VFS locks".
>
> 2) turn the scopes into explicit compound statements.
>
> 3) take them into helper functions.
4) Get rid of the cloned real file references
I was going to explain what I mean, but it was easier to write patches:
https://lore.kernel.org/linux-fsdevel/20241004102342.179434-1-amir73il@xxxxxxxxx/
(also at https://github.com/amir73il/linux/commits/ovl_real_file/)
After my patches, there is no longer any use for CLONED_FDERR()
in overlayfs code and there is no need for guarded real fd in
overlayfs file methods, so the value of this work for overlayfs is
reduced.
I have no objection to the fderr abstraction, but I don't think
that it will improve overlayfs code after my change. WDYT?
Thanks,
Amir.
