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: <f3dc1a01fae6759a350adabf944892417a63d529.camel@redhat.com>
Subject: Re: [PATCH RFC] NFSD: Bump the ref count on nf_inode
From:   Jeff Layton <jlayton@redhat.com>
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-nfs@vger.kernel.org
Date:   Thu, 07 Jul 2022 12:55:00 -0400
In-Reply-To: <165720933938.1180.14325183467695610136.stgit@klimt.1015granger.net>
References: <165720933938.1180.14325183467695610136.stgit@klimt.1015granger.net>
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 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 ha=
shed
>  * 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 shoul=
d
>  * never be dereferenced, only used for comparison.
>  */
>=20
> However, nfsd_file_mark_find_or_create() does dereference the pointer sto=
red
> 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

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.


>=20
> 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 =3D get_current_cred();
>  		nf->nf_net =3D net;
>  		nf->nf_flags =3D 0;
> +		ihold(inode);
>  		nf->nf_inode =3D inode;
>  		nf->nf_hashval =3D hashval;
>  		refcount_set(&nf->nf_ref, 1);
> @@ -210,6 +211,7 @@ nfsd_file_free(struct nfsd_file *nf)
>  		fput(nf->nf_file);
>  		flush =3D 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 s=
vc_fh *fhp,
>  	if (nf =3D=3D NULL)
>  		goto open_file;
>  	spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock);
> +	iput(new->nf_inode);
>  	nfsd_file_slab_free(&new->nf_rcu);
> =20
>  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 {
> =20
>  /*
>   * A representation of a file that has been opened by knfsd. These are h=
ashed
> - * 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 shou=
ld
> - * never be dereferenced, only used for comparison.
> + * in the hashtable by inode pointer value.
>   */
>  struct nfsd_file {
>  	struct hlist_node	nf_node;
>=20
>=20

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