linux-kernel - Re: [PATCH RFC 1/2] sunrpc: delay pc_release callback until after sending a reply

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <175158561386.565058.1936125782874530200@noble.neil.brown.name>
Date: Fri, 04 Jul 2025 09:33:33 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Trond Myklebust" <trondmy@...nel.org>, "Anna Schumaker" <anna@...nel.org>,
 "Chuck Lever" <chuck.lever@...cle.com>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, "Mike Snitzer" <snitzer@...nel.org>,
 linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 "Jeff Layton" <jlayton@...nel.org>
Subject: Re: [PATCH RFC 1/2] sunrpc: delay pc_release callback until after
 sending a reply

On Fri, 04 Jul 2025, Jeff Layton wrote:
> The server-side sunrpc code currently calls pc_release before sending
> the reply. A later nfsd patch will change some pc_release callbacks to
> do extra work to clean the pagecache. There is no need to delay sending
> the reply for this, however.
> 
> Change svc_process and svc_process_bc to call pc_release after sending
> the reply instead of before.
> 
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
>  net/sunrpc/svc.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index b1fab3a6954437cf751e4725fa52cfc83eddf2ab..103bb6ba8e140fdccd6cab124e715caeb41bb445 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -1426,8 +1426,6 @@ svc_process_common(struct svc_rqst *rqstp)
>  
>  	/* Call the function that processes the request. */
>  	rc = process.dispatch(rqstp);
> -	if (procp->pc_release)
> -		procp->pc_release(rqstp);
>  	xdr_finish_decode(xdr);
>  
>  	if (!rc)
> @@ -1526,6 +1524,14 @@ static void svc_drop(struct svc_rqst *rqstp)
>  	trace_svc_drop(rqstp);
>  }
>  
> +static void svc_release_rqst(struct svc_rqst *rqstp)
> +{
> +	const struct svc_procedure *procp = rqstp->rq_procinfo;
> +
> +	if (procp && procp->pc_release)
> +		procp->pc_release(rqstp);
> +}
> +
>  /**
>   * svc_process - Execute one RPC transaction
>   * @rqstp: RPC transaction context
> @@ -1533,7 +1539,7 @@ static void svc_drop(struct svc_rqst *rqstp)
>   */
>  void svc_process(struct svc_rqst *rqstp)
>  {
> -	struct kvec		*resv = &rqstp->rq_res.head[0];
> +	struct kvec			*resv = &rqstp->rq_res.head[0];

Commas and Tabs - you can never really have enough of them, can you?

>  	__be32 *p;
>  
>  #if IS_ENABLED(CONFIG_FAIL_SUNRPC)
> @@ -1565,9 +1571,12 @@ void svc_process(struct svc_rqst *rqstp)
>  	if (unlikely(*p != rpc_call))
>  		goto out_baddir;
>  
> -	if (!svc_process_common(rqstp))
> +	if (!svc_process_common(rqstp)) {
> +		svc_release_rqst(rqstp);
>  		goto out_drop;
> +	}
>  	svc_send(rqstp);
> +	svc_release_rqst(rqstp);
>  	return;

Should we, as a general rule, avoid calling any cleanup function more
than once?  When tempted, we DEFINE_FREE() a cleanup function and
declare the variable appropriately.

Though in this case it might be easier to:

  if (svc_process_common(rqstp))
       svc_send(rqstp);
  else
       svc_drop(rqstp);
  svc_rlease_rqst(rqstp);
  return;

svc_process_bc() is a little more awkward.

But in general, delaying the release function until after the send seems
sound, and this patches appears to do it corretly.

Reviewed-by: NeilBrown <neil@...wn.name>

NeilBrown