> 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. How about f->nf_file->f_inode ? -- Chuck Lever
