On Sun, 14 Sep 2025, Al Viro wrote:
> On Mon, Sep 08, 2025 at 10:05:57AM +0100, Al Viro wrote:
>
> > > Fudging some type-state with C may well be useful but I suspect it is at
> > > most part of a solution. Simplification, documentation, run-time checks
> > > might also be important parts. As the type-state flag-day is a big
> > > thing, maybe it shouldn't be first.
> >
> > All of that requires being able to answer questions about what's there in
> > the existing filesystems. Which is pretty much the same problem as
> > those audits, obviously. And static annotations are way easier to
> > reason about.
>
> Speaking of annoyances, d_exact_alias() is gone, and good riddance, but there's
> another fun issue in the same area - environment for d_splice_alias() call *and*
> for one of those d_drop()-just-in-case.
>
> The call tree is still the same:
> _nfs4_open_and_get_state()
> <- _nfs4_do_open()
> <- nfs4_do_open()
> <- nfs4_atomic_open()
> == nfs_rpc_ops:open_context
> <- nfs_atomic_open()
> == ->atomic_open
> <- nfs4_file_open()
> == ->open
> <- nfs4_proc_create()
> == nfs_rpc_ops:create
> <- nfs_do_create()
> <- nfs_create()
> == ->create
> <- nfs_atomic_open_v23(), with O_CREAT
> == ->atomic_open
> # won't reach nfs4 stuff?
>
> ->create() and ->atomic_open() have the parent held at least shared;
> ->open() does not, but the chunk in question is hit only if dentry
> is negative, which won't happen in case of ->open().
>
> Additional complication comes from the possibility for _nfs4_open_and_get_state()
> to fail after that d_splice_alias(). In that case we have _nfs4_do_open()
> return an error; its caller is inside a do-while loop in nfs4_do_open() and
> I think we can't end up going around the loop after such late failure (the
> only error possible after that is -EACCES/-NFS4ERR_ACCESS and that's not one
> of those that can lead to more iterations.
>
> However, looking at that late failure, that's the only call of
> nfs4_opendata_access(), and that function seems to expect the possibility
> of state->inode being a directory; can that really happen?
No that cannot happen. In the NFSv4 protocol, OPEN is only used for
files.
Directories use the same model as v3 where you pass a filehandle to
READDIR without establishing and "open" state.
Why do you think nfs4_opendata_access() expects the possibilty of a
directory? The purpose of the function is to handle to Posix weirdness
around being able to exec() a file that you cannot read(). Over NFS the
client needs to be able to read a file in order to execute it, so NFS
blurs the two permissions together. This function is check which
permission was asked for and then checking access permissions to make
sure it is allowed. i.e. the server might let the client read the file
storing the application, but the client might not let the application be
run.
>
> Because if it can, we have a problem:
> alias = d_splice_alias(igrab(state->inode), dentry);
> /* d_splice_alias() can't fail here - it's a non-directory */
> if (alias) {
> dput(ctx->dentry);
> ctx->dentry = dentry = alias;
> }
> very much *can* fail if it's reached with state->inode being a directory -
> we can get ERR_PTR() out of that d_splice_alias() and that will oops at
>
> if (d_inode(dentry) == state->inode)
> nfs_inode_attach_open_context(ctx);
> shortly afterwards (incidentally, what is that check about? It can only
> fail in case of nfs4_file_open(); should we have open(2) succeed in such
> situation?)
I don't know what is going on here.
Based on the commit that introduced this code
Commit: c45ffdd26961 ("NFSv4: Close another NFSv4 recovery race")
there is presumably some race that can cause the test to fail.
Maybe Trond (cc:ed) could help explain.
NeilBrown
>
> Sigh...
>
