Re: [PATCH RFC] NFSD: Bump the ref count on nf_inode

On Thu, 2022-07-07 at 17:25 +0000, Chuck Lever III wrote:

quoted

On Jul 7, 2022, at 12:55 PM, Jeff Layton [off-list ref] wrote:

On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:

quoted

The documenting comment for struct nf_file states:

/*
* A representation of a file that has been opened by knfsd. These are hashed
* in the hashtable by inode pointer value. Note that this object doesn't
* hold a reference to the inode by itself, so the nf_inode pointer should
* never be dereferenced, only used for comparison.
*/

However, nfsd_file_mark_find_or_create() does dereference the pointer stored
in this field.

Signed-off-by: Chuck Lever <redacted>
---
fs/nfsd/filecache.c | 3 +++
fs/nfsd/filecache.h | 4 +---
2 files changed, 4 insertions(+), 3 deletions(-)

Hi Jeff-

I'm still testing this one, but I'm wondering what you think of it.
I did hit a KASAN splat that might be related, but it's not 100%
reproducible.

My first thought is "what the hell was I thinking, tracking an inode
field without holding a reference to it?"

But now that I look, the nf_inode value only gets dereferenced in one
place -- nfs4_show_superblock, and I think that's a bug. The comments
over struct nfsd_file say:

"Note that this object doesn't hold a reference to the inode by itself,
so the nf_inode pointer should never be dereferenced, only used for
comparison."

We should probably annotate nf_inode better. __attribute__((noderef))
maybe? It would also be good to make nfs4_show_superblock use a
different way to get the sb.

How about f->nf_file->f_inode ?

I'd probably prefer:

    file_inode(f->nf_file)

...and I don't think there is a potential crash here either.
nfs4_show_superblock is called while holding the cl_lock. I don't think
the inode can disappear out from under you with that.

-- 
Jeff Layton [off-list ref]