On Thu, 2022-07-07 at 16:58 +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 <chuck.lever@oracle.com>
---
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.

In any case, this is unlikely to fix anything unless the crash happened
in nfs4_show_superblock.

Thanks for the look. I will treat this as a clean-up, then, and see
what can be done about nfsd_file_mark_find_or_create() and
nfs4_show_superblock().

I don't see a real need to hold a separate inode reference in the
nfsd_file as you should have one already by virtue of the open file
itself. It probably won't hurt anything to hold one though if you decide
that's safer.

quoted

quoted

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 9cb2d590c036..7b43bb427a53 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -180,6 +180,7 @@ nfsd_file_alloc(struct inode *inode, unsigned int may, unsigned int hashval,

		nf->nf_cred = get_current_cred();
		nf->nf_net = net;
		nf->nf_flags = 0;
+		ihold(inode);
		nf->nf_inode = inode;
		nf->nf_hashval = hashval;
		refcount_set(&nf->nf_ref, 1);

@@ -210,6 +211,7 @@ nfsd_file_free(struct nfsd_file *nf)

		fput(nf->nf_file);
		flush = true;
	}
+	iput(nf->nf_inode);
	call_rcu(&nf->nf_rcu, nfsd_file_slab_free);
	return flush;
}

@@ -940,6 +942,7 @@ nfsd_do_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,

	if (nf == NULL)
		goto open_file;
	spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock);
+	iput(new->nf_inode);
	nfsd_file_slab_free(&new->nf_rcu);

wait_for_construction:

diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
index 1da0c79a5580..01fbf6e88cce 100644
--- a/fs/nfsd/filecache.h
+++ b/fs/nfsd/filecache.h

@@ -24,9 +24,7 @@ struct nfsd_file_mark {

/*
* 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.
+ * in the hashtable by inode pointer value.
*/
struct nfsd_file {
	struct hlist_node	nf_node;

-- 
Jeff Layton [off-list ref]

--
Chuck Lever

-- 
Jeff Layton [off-list ref]