Thread (3 messages) 3 messages, 3 authors, 2023-10-13

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help