Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7bc372783500c40d39c6a6a2eac1b7690f1ab3ad.camel@redhat.com>
Subject: Re: [PATCH RFC] NFSD: Bump the ref count on nf_inode
From:   Jeff Layton <jlayton@redhat.com>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Thu, 07 Jul 2022 13:30:35 -0400
In-Reply-To: <5B84D6D2-E9BC-4518-B52C-ABAF240DE2A1@oracle.com>
References: <165720933938.1180.14325183467695610136.stgit@klimt.1015granger.net>
         <f3dc1a01fae6759a350adabf944892417a63d529.camel@redhat.com>
         <5B84D6D2-E9BC-4518-B52C-ABAF240DE2A1@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2022-07-07 at 17:25 +0000, Chuck Lever III wrote:
>=20
> > On Jul 7, 2022, at 12:55 PM, Jeff Layton <jlayton@redhat.com> wrote:
> >=20
> > On Thu, 2022-07-07 at 11:58 -0400, Chuck Lever wrote:
> > > The documenting comment for struct nf_file states:
> > >=20
> > > /*
> > > * 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 does=
n't
> > > * hold a reference to the inode by itself, so the nf_inode pointer sh=
ould
> > > * never be dereferenced, only used for comparison.
> > > */
> > >=20
> > > However, nfsd_file_mark_find_or_create() does dereference the pointer=
 stored
> > > in this field.
> > >=20
> > > 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(-)
> > >=20
> > > Hi Jeff-
> > >=20
> > > 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.
> > >=20
> >=20
> > My first thought is "what the hell was I thinking, tracking an inode
> > field without holding a reference to it?"
> >=20
> > 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:
> >=20
> > "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."
> >=20
> > 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.
>=20
> How about f->nf_file->f_inode ?
>=20
>=20

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.

--=20
Jeff Layton <jlayton@redhat.com>