From: Chuck Lever <[email protected]>
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 <[email protected]>
---
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;
- }
+
p = xdr_encode_hyper(p, ld->ld_start);
p = xdr_encode_hyper(p, ld->ld_length);
*p++ = cpu_to_be32(ld->ld_type);
if (conf->len) {
p = xdr_encode_opaque_fixed(p, &ld->ld_clientid, 8);
p = xdr_encode_opaque(p, conf->data, conf->len);
- kfree(conf->data);
} else { /* non - nfsv4 lock in conflict, no clientid nor owner */
p = xdr_encode_hyper(p, (u64)0); /* clientid */
*p++ = cpu_to_be32(0); /* length of owner name */
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index aba07d5378fc..e6c9daae196e 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -292,12 +292,8 @@ struct nfsd4_lock {
} v;
/* response */
- union {
- struct {
- stateid_t stateid;
- } ok;
- struct nfsd4_lock_denied denied;
- } u;
+ stateid_t lk_resp_stateid;
+ struct nfsd4_lock_denied lk_denied;
};
#define lk_new_open_seqid v.new.open_seqid
#define lk_new_open_stateid v.new.open_stateid
@@ -307,20 +303,15 @@ struct nfsd4_lock {
#define lk_old_lock_stateid v.old.lock_stateid
#define lk_old_lock_seqid v.old.lock_seqid
-#define lk_resp_stateid u.ok.stateid
-#define lk_denied u.denied
-
-
struct nfsd4_lockt {
u32 lt_type;
clientid_t lt_clientid;
struct xdr_netobj lt_owner;
u64 lt_offset;
u64 lt_length;
- struct nfsd4_lock_denied lt_denied;
+ struct nfsd4_lock_denied lt_denied;
};
-
struct nfsd4_locku {
u32 lu_type;
u32 lu_seqid;
@@ -942,8 +933,10 @@ extern __be32 nfsd4_open_downgrade(struct svc_rqst *rqstp,
struct nfsd4_compound_state *, union nfsd4_op_u *u);
extern __be32 nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *,
union nfsd4_op_u *u);
+extern void nfsd4_lock_release(union nfsd4_op_u *u);
extern __be32 nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *,
union nfsd4_op_u *u);
+extern void nfsd4_lockt_release(union nfsd4_op_u *u);
extern __be32 nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *,
union nfsd4_op_u *u);
extern __be32
On Fri, 2023-10-13 at 08:56 -0400, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
> - }
> +
> p = xdr_encode_hyper(p, ld->ld_start);
> p = xdr_encode_hyper(p, ld->ld_length);
> *p++ = cpu_to_be32(ld->ld_type);
> if (conf->len) {
> p = xdr_encode_opaque_fixed(p, &ld->ld_clientid, 8);
> p = xdr_encode_opaque(p, conf->data, conf->len);
> - kfree(conf->data);
> } else { /* non - nfsv4 lock in conflict, no clientid nor owner */
> p = xdr_encode_hyper(p, (u64)0); /* clientid */
> *p++ = cpu_to_be32(0); /* length of owner name */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index aba07d5378fc..e6c9daae196e 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -292,12 +292,8 @@ struct nfsd4_lock {
> } v;
>
> /* response */
> - union {
> - struct {
> - stateid_t stateid;
> - } ok;
> - struct nfsd4_lock_denied denied;
> - } u;
> + stateid_t lk_resp_stateid;
> + struct nfsd4_lock_denied lk_denied;
> };
> #define lk_new_open_seqid v.new.open_seqid
> #define lk_new_open_stateid v.new.open_stateid
> @@ -307,20 +303,15 @@ struct nfsd4_lock {
> #define lk_old_lock_stateid v.old.lock_stateid
> #define lk_old_lock_seqid v.old.lock_seqid
>
> -#define lk_resp_stateid u.ok.stateid
> -#define lk_denied u.denied
> -
> -
> struct nfsd4_lockt {
> u32 lt_type;
> clientid_t lt_clientid;
> struct xdr_netobj lt_owner;
> u64 lt_offset;
> u64 lt_length;
> - struct nfsd4_lock_denied lt_denied;
> + struct nfsd4_lock_denied lt_denied;
> };
>
> -
> struct nfsd4_locku {
> u32 lu_type;
> u32 lu_seqid;
> @@ -942,8 +933,10 @@ extern __be32 nfsd4_open_downgrade(struct svc_rqst *rqstp,
> struct nfsd4_compound_state *, union nfsd4_op_u *u);
> extern __be32 nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *,
> union nfsd4_op_u *u);
> +extern void nfsd4_lock_release(union nfsd4_op_u *u);
> extern __be32 nfsd4_lockt(struct svc_rqst *rqstp, struct nfsd4_compound_state *,
> union nfsd4_op_u *u);
> +extern void nfsd4_lockt_release(union nfsd4_op_u *u);
> extern __be32 nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *,
> union nfsd4_op_u *u);
> extern __be32
>
>
Reviewed-by: Jeff Layton <[email protected]>
On 13 Oct 2023, at 8:56, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Ben