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: <f38b7fe087b1dd296bf7a5b693e685936ebecada.camel@kernel.org>
Subject: Re: [PATCH RFC] NFSD: Hold rcu_read_lock while getting refs
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Date:   Mon, 03 Oct 2022 07:34:07 -0400
In-Reply-To: <166463917715.10124.3789034969503323129.stgit@bazille.1015granger.net>
References: <166463917715.10124.3789034969503323129.stgit@bazille.1015granger.net>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Sat, 2022-10-01 at 11:48 -0400, Chuck Lever wrote:
> nfsd_file is RCU-freed, so it's possible that one could be found
> that's in the process of being freed and the memory recycled. Ensure
> we hold the rcu_read_lock while attempting to get a reference on the
> object.
>=20
> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/filecache.c |   34 +++++++++++-----------------------
>  fs/nfsd/trace.h     |   27 ---------------------------
>  2 files changed, 11 insertions(+), 50 deletions(-)
>=20
> This is what I was thinking... Compile-tested only.
>=20
>=20

Looks reasonable. I had something pretty similar that I'll send along in
a bit.

> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index be152e3e3a80..6e17f74fb29f 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1056,10 +1056,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, stru=
ct svc_fh *fhp,
> =20
>  retry:
>  	/* Avoid allocation if the item is already in cache */
> -	nf =3D rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> -				    nfsd_file_rhash_params);
> +	rcu_read_lock();
> +	nf =3D rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> +			       nfsd_file_rhash_params);
>  	if (nf)
>  		nf =3D nfsd_file_get(nf);
> +	rcu_read_unlock();
>  	if (nf)
>  		goto wait_for_construction;
> =20
> @@ -1069,21 +1071,14 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, stru=
ct svc_fh *fhp,
>  		goto out_status;
>  	}
> =20
> -	nf =3D rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> -					      &key, &new->nf_rhash,
> -					      nfsd_file_rhash_params);
> -	if (!nf) {
> -		nf =3D new;
> -		goto open_file;
> -	}
> -	if (IS_ERR(nf))
> -		goto insert_err;
> -	nf =3D nfsd_file_get(nf);
> -	if (nf =3D=3D NULL) {
> -		nf =3D new;
> -		goto open_file;
> +	if (rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> +					 &key, &new->nf_rhash,
> +					 nfsd_file_rhash_params)) {
> +		nfsd_file_slab_free(&new->nf_rcu);
> +		goto retry;

This can return other errors besides -EEXIST. I'm not sure we want to
goto retry on those others.

>  	}
> -	nfsd_file_slab_free(&new->nf_rcu);
> +	nf =3D new;
> +	goto open_file;
> =20
>  wait_for_construction:
>  	wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> @@ -1143,13 +1138,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struc=
t svc_fh *fhp,
>  	smp_mb__after_atomic();
>  	wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
>  	goto out;
> -
> -insert_err:
> -	nfsd_file_slab_free(&new->nf_rcu);
> -	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, PTR_ERR(nf));
> -	nf =3D NULL;
> -	status =3D nfserr_jukebox;
> -	goto out_status;
>  }
> =20
>  /**
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index 06a96e955bd0..c15467b2e8d9 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -954,33 +954,6 @@ TRACE_EVENT(nfsd_file_create,
>  	)
>  );
> =20
> -TRACE_EVENT(nfsd_file_insert_err,
> -	TP_PROTO(
> -		const struct svc_rqst *rqstp,
> -		const struct inode *inode,
> -		unsigned int may_flags,
> -		long error
> -	),
> -	TP_ARGS(rqstp, inode, may_flags, error),
> -	TP_STRUCT__entry(
> -		__field(u32, xid)
> -		__field(const void *, inode)
> -		__field(unsigned long, may_flags)
> -		__field(long, error)
> -	),
> -	TP_fast_assign(
> -		__entry->xid =3D be32_to_cpu(rqstp->rq_xid);
> -		__entry->inode =3D inode;
> -		__entry->may_flags =3D may_flags;
> -		__entry->error =3D error;
> -	),
> -	TP_printk("xid=3D0x%x inode=3D%p may_flags=3D%s error=3D%ld",
> -		__entry->xid, __entry->inode,
> -		show_nfsd_may_flags(__entry->may_flags),
> -		__entry->error
> -	)
> -);
> -
>  TRACE_EVENT(nfsd_file_cons_err,
>  	TP_PROTO(
>  		const struct svc_rqst *rqstp,
>=20
>=20

--=20
Jeff Layton <jlayton@kernel.org>