Re: [PATCH] NFSD: Remove a layering violation when encoding lock_denied
From: Benjamin Coddington <hidden>
Date: 2023-10-13 13:21:32
On 13 Oct 2023, at 8:56, Chuck Lever wrote:
quoted hunk ↗ jump to hunk
From: Chuck Lever <redacted> 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 <redacted> --- 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[] = { }, [OP_LOCK] = { .op_func = nfsd4_lock, + .op_release = nfsd4_lock_release, .op_flags = OP_MODIFIES_SOMETHING | OP_NONTRIVIAL_ERROR_ENCODE, .op_name = "OP_LOCK",@@ -3218,6 +3219,7 @@ static const struct nfsd4_operation nfsd4_ops[] = { }, [OP_LOCKT] = { .op_func = nfsd4_lockt, + .op_release = nfsd4_lockt_release, .op_flags = OP_NONTRIVIAL_ERROR_ENCODE, .op_name = "OP_LOCKT", .op_rsize_bop = 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 = &u->lock; + struct nfsd4_lock_denied *deny = &lock->lk_denied; + + kfree(deny->ld_owner.data); +} + /* * The NFSv4 spec allows a client to do a LOCKT without holding an OPEN, * 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 = &u->lockt; + struct nfsd4_lock_denied *deny = &lockt->lt_denied; + + kfree(deny->ld_owner.data); +} + __be32 nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, 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 = &ld->ld_owner; __be32 *p; -again: p = 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 = 0; - conf->data = 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 [off-list ref]
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 ourselves
to return without the conflicting owner instead of outright failing the
operation.
Signed-off-by: J. Bruce Fields [off-list ref]
Ben