> On Jul 7, 2022, at 12:55 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
>> 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@xxxxxxxxxx>
>> ---
>> 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().
>> 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 <jlayton@xxxxxxxxxx>
--
Chuck Lever
