Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
From:   Benjamin Coddington <bcodding@redhat.com>
To:     Chuck Lever <cel@kernel.org>
Cc:     linux-nfs@vger.kernel.org, Chuck Lever <chuck.lever@oracle.com>
Subject: Re: [PATCH] NFSD: Remove a layering violation when encoding
 lock_denied
Date:   Fri, 13 Oct 2023 09:20:29 -0400
Message-ID: <EF49AC8F-D046-4C9A-91CA-1D2AC4B1A070@redhat.com>
In-Reply-To: <169720169786.5129.10628552146876100129.stgit@oracle-102.nfsv4bat.org>
References: <169720169786.5129.10628552146876100129.stgit@oracle-102.nfsv4bat.org>
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 13 Oct 2023, at 8:56, Chuck Lever wrote:

> From: Chuck Lever <chuck.lever@oracle.com>
>
> An XDR encoder is responsible for marshaling results, not releasing
> memory that was allocated by the upper layer. We have .op_release
> for that purpose.
>
> Move the release of the ld_owner.data string to op_release functions
> for LOCK and LOCKT.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c  |    2 ++
>  fs/nfsd/nfs4state.c |   16 ++++++++++++++++
>  fs/nfsd/nfs4xdr.c   |   16 ++--------------
>  fs/nfsd/xdr4.h      |   17 +++++------------
>  4 files changed, 25 insertions(+), 26 deletions(-)
>
> Fix for kmemleak found during Bake-a-thon.
>
> I'm planning to insert this fix into nfsd-next just before the
> patches that clean up the lock_denied encoder.
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 60262fd27b15..f288039545e3 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -3210,6 +3210,7 @@ static const struct nfsd4_operation nfsd4_ops[] =3D=
 {
>  	},
>  	[OP_LOCK] =3D {
>  		.op_func =3D nfsd4_lock,
> +		.op_release =3D nfsd4_lock_release,
>  		.op_flags =3D OP_MODIFIES_SOMETHING |
>  				OP_NONTRIVIAL_ERROR_ENCODE,
>  		.op_name =3D "OP_LOCK",
> @@ -3218,6 +3219,7 @@ static const struct nfsd4_operation nfsd4_ops[] =3D=
 {
>  	},
>  	[OP_LOCKT] =3D {
>  		.op_func =3D nfsd4_lockt,
> +		.op_release =3D nfsd4_lockt_release,
>  		.op_flags =3D OP_NONTRIVIAL_ERROR_ENCODE,
>  		.op_name =3D "OP_LOCKT",
>  		.op_rsize_bop =3D nfsd4_lock_rsize,
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 07840ee721ef..305c353a416c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7773,6 +7773,14 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_=
compound_state *cstate,
>  	return status;
>  }
>
> +void nfsd4_lock_release(union nfsd4_op_u *u)
> +{
> +	struct nfsd4_lock *lock =3D &u->lock;
> +	struct nfsd4_lock_denied *deny =3D &lock->lk_denied;
> +
> +	kfree(deny->ld_owner.data);
> +}
> +
>  /*
>   * The NFSv4 spec allows a client to do a LOCKT without holding an OPE=
N,
>   * so we do a temporary open here just to get an open file to pass to
> @@ -7878,6 +7886,14 @@ nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4=
_compound_state *cstate,
>  	return status;
>  }
>
> +void nfsd4_lockt_release(union nfsd4_op_u *u)
> +{
> +	struct nfsd4_lockt *lockt =3D &u->lockt;
> +	struct nfsd4_lock_denied *deny =3D &lockt->lt_denied;
> +
> +	kfree(deny->ld_owner.data);
> +}
> +
>  __be32
>  nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstat=
e,
>  	    union nfsd4_op_u *u)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index dafb3a91235e..26e7bb6d32ab 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3990,28 +3990,16 @@ nfsd4_encode_lock_denied(struct xdr_stream *xdr=
, struct nfsd4_lock_denied *ld)
>  	struct xdr_netobj *conf =3D &ld->ld_owner;
>  	__be32 *p;
>
> -again:
>  	p =3D xdr_reserve_space(xdr, 32 + XDR_LEN(conf->len));
> -	if (!p) {
> -		/*
> -		 * Don't fail to return the result just because we can't
> -		 * return the conflicting open:
> -		 */
> -		if (conf->len) {
> -			kfree(conf->data);
> -			conf->len =3D 0;
> -			conf->data =3D NULL;
> -			goto again;
> -		}
> +	if (!p)
>  		return nfserr_resource;

Should we worry about removing this corner-case fix?  I'm not sure I
understand it.. just wanted to bring it up.  Here's what Bruce originally=

said:

commit 8c7424cff6bd33459945646cfcbf6dc6c899ab24
Author: J. Bruce Fields <bfields@fieldses.org>
Date:   Mon Mar 10 12:19:10 2014 -0400

    nfsd4: don't try to encode conflicting owner if low on space

    I ran into this corner case in testing: in theory clients can provide=

    state owners up to 1024 bytes long.  In the sessions case there might=
 be
    a risk of this pushing us over the DRC slot size.

    The conflicting owner isn't really that important, so let's humor a
    client that provides a small maxresponsize_cached by allowing ourselv=
es
    to return without the conflicting owner instead of outright failing t=
he
    operation.

    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Ben