The purpose of this series is to replace the svc_put* macros in the
Linux kernel server's RPC reply header construction code with
xdr_stream helpers. I've measured no change in CPU utilization after
the overhaul.
Memory safety: Buffer bounds checking after encoding each XDR item
is more memory-safe than the current mechanism. Subsequent memory
safety improvements to the common xdr_stream helpers will benefit
all who use them.
Audit friendliness: The new code has additional comments and other
clean-up to help align it with the relevant RPC protocol
specifications. The use of common helpers also makes the encoders
easier to audit and maintain.
I've split the full series in half to make it easier to review. The
patches posted here are the second half, handling RPC reply header
encoding.
Note that another benefit of this work is that we are taking one or
two more strides closer to greater commonality between the client
and server implementations of RPCSEC GSS.
---
Chuck Lever (27):
SUNRPC: Clean up svcauth_gss_release()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
SUNRPC: Check rq_auth_stat when preparing to wrap a response
SUNRPC: Remove the rpc_stat variable in svc_process_common()
SUNRPC: Add XDR encoding helper for opaque_auth
SUNRPC: Push svcxdr_init_encode() into svc_process_common()
SUNRPC: Move svcxdr_init_encode() into ->accept methods
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
SUNRPC: Convert unwrap data paths to use xdr_stream for replies
SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
SUNRPC: Use xdr_stream for encoding GSS reply verifiers
SUNRPC: Hoist init_encode out of svc_authenticate()
SUNRPC: Convert RPC Reply header encoding to use xdr_stream
SUNRPC: Final clean-up of svc_process_common()
SUNRPC: Remove no-longer-used helper functions
SUNRPC: Refactor RPC server dispatch method
SUNRPC: Set rq_accept_statp inside ->accept methods
SUNRPC: Go back to using gsd->body_start
fs/lockd/svc.c | 5 +-
fs/nfs/callback_xdr.c | 6 +-
fs/nfsd/nfscache.c | 4 +-
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 10 +-
include/linux/sunrpc/svc.h | 116 +++----
include/linux/sunrpc/xdr.h | 23 ++
include/trace/events/rpcgss.h | 22 ++
net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
net/sunrpc/svc.c | 91 +++---
net/sunrpc/svcauth_unix.c | 40 ++-
net/sunrpc/xdr.c | 29 ++
12 files changed, 451 insertions(+), 402 deletions(-)
--
Chuck Lever
From: Chuck Lever <[email protected]>
Now that svcauth_gss_prepare_to_wrap() no longer computes the
location of RPC header fields in the response buffer,
svcauth_gss_accept() can save the location of the databody
rather than the location of the verifier.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 78 +++++++++++++++++--------------------
1 file changed, 36 insertions(+), 42 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 333873abb7d9..419d5ad6311c 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -71,9 +71,7 @@
struct gss_svc_data {
/* decoded gss client cred: */
struct rpc_gss_wire_cred clcred;
- /* save a pointer to the beginning of the encoded verifier,
- * for use in encryption/checksumming in svcauth_gss_release: */
- __be32 *verf_start;
+ u32 gsd_databody_offset;
struct rsc *rsci;
/* for temporary results */
@@ -1595,7 +1593,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
if (!svcdata)
goto auth_err;
rqstp->rq_auth_data = svcdata;
- svcdata->verf_start = NULL;
+ svcdata->gsd_databody_offset = 0;
svcdata->rsci = NULL;
gc = &svcdata->clcred;
@@ -1647,11 +1645,11 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
goto complete;
case RPC_GSS_PROC_DATA:
rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
- svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
goto auth_err;
if (!svcxdr_set_accept_stat(rqstp))
goto auth_err;
+ svcdata->gsd_databody_offset = xdr_stream_pos(&rqstp->rq_res_stream);
rqstp->rq_cred = rsci->cred;
get_group_info(rsci->cred.cr_group_info);
rqstp->rq_auth_stat = rpc_autherr_badcred;
@@ -1705,30 +1703,24 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
return ret;
}
-static __be32 *
+static u32
svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)
{
- __be32 *p;
- u32 verf_len;
+ u32 offset;
- p = gsd->verf_start;
- gsd->verf_start = NULL;
+ /* Release can be called twice, but we only wrap once. */
+ offset = gsd->gsd_databody_offset;
+ gsd->gsd_databody_offset = 0;
/* AUTH_ERROR replies are not wrapped. */
if (rqstp->rq_auth_stat != rpc_auth_ok)
- return NULL;
-
- /* Skip the verifier: */
- p += 1;
- verf_len = ntohl(*p++);
- p += XDR_QUADLEN(verf_len);
+ return 0;
/* Also don't wrap if the accept_stat is nonzero: */
if (*rqstp->rq_accept_statp != rpc_success)
- return NULL;
+ return 0;
- p++;
- return p;
+ return offset;
}
/*
@@ -1756,21 +1748,21 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
struct xdr_buf *buf = xdr->buf;
struct xdr_buf databody_integ;
struct xdr_netobj checksum;
- u32 offset, len, maj_stat;
- __be32 *p;
+ u32 offset, maj_stat;
- p = svcauth_gss_prepare_to_wrap(rqstp, gsd);
- if (p == NULL)
+ offset = svcauth_gss_prepare_to_wrap(rqstp, gsd);
+ if (!offset)
goto out;
- offset = (u8 *)(p + 1) - (u8 *)buf->head[0].iov_base;
- len = buf->len - offset;
- if (xdr_buf_subsegment(buf, &databody_integ, offset, len))
+ if (xdr_buf_subsegment(buf, &databody_integ, offset + XDR_UNIT,
+ buf->len - offset - XDR_UNIT))
goto wrap_failed;
/* Buffer space for these has already been reserved in
* svcauth_gss_accept(). */
- *p++ = cpu_to_be32(len);
- *p = cpu_to_be32(gc->gc_seq);
+ if (xdr_encode_word(buf, offset, databody_integ.len))
+ goto wrap_failed;
+ if (xdr_encode_word(buf, offset + XDR_UNIT, gc->gc_seq))
+ goto wrap_failed;
checksum.data = gsd->gsd_scratch;
maj_stat = gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum);
@@ -1817,17 +1809,19 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
struct kvec *head = buf->head;
struct kvec *tail = buf->tail;
u32 offset, pad, maj_stat;
- __be32 *p, *lenp;
+ __be32 *p;
- p = svcauth_gss_prepare_to_wrap(rqstp, gsd);
- if (p == NULL)
+ offset = svcauth_gss_prepare_to_wrap(rqstp, gsd);
+ if (!offset)
return 0;
- lenp = p++;
- offset = (u8 *)p - (u8 *)head->iov_base;
- /* Buffer space for this field has already been reserved
- * in svcauth_gss_accept(). */
- *p = cpu_to_be32(gc->gc_seq);
+ /*
+ * Buffer space for this field has already been reserved
+ * in svcauth_gss_accept(). Note that the GSS sequence
+ * number is encrypted along with the RPC reply payload.
+ */
+ if (xdr_encode_word(buf, offset + XDR_UNIT, gc->gc_seq))
+ goto wrap_failed;
/*
* If there is currently tail data, make sure there is
@@ -1863,12 +1857,15 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)
tail->iov_len = 0;
}
- maj_stat = gss_wrap(gsd->rsci->mechctx, offset, buf, buf->pages);
+ maj_stat = gss_wrap(gsd->rsci->mechctx, offset + XDR_UNIT, buf,
+ buf->pages);
if (maj_stat != GSS_S_COMPLETE)
goto bad_wrap;
- *lenp = cpu_to_be32(buf->len - offset);
- pad = xdr_pad_size(buf->len - offset);
+ /* Wrapping can change the size of databody_priv. */
+ if (xdr_encode_word(buf, offset, buf->len - offset - XDR_UNIT))
+ goto wrap_failed;
+ pad = xdr_pad_size(buf->len - offset - XDR_UNIT);
p = (__be32 *)(tail->iov_base + tail->iov_len);
memset(p, 0, pad);
tail->iov_len += pad;
@@ -1908,9 +1905,6 @@ svcauth_gss_release(struct svc_rqst *rqstp)
gc = &gsd->clcred;
if (gc->gc_proc != RPC_GSS_PROC_DATA)
goto out;
- /* Release can be called twice, but we only wrap once. */
- if (gsd->verf_start == NULL)
- goto out;
switch (gc->gc_svc) {
case RPC_GSS_SVC_NONE:
From: Chuck Lever <[email protected]>
Done as part of hardening the server-side RPC header encoding path.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 81 ++++---------------------------------
1 file changed, 8 insertions(+), 73 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 9f3633c42ebd..89333669af26 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -693,28 +693,6 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
goto out;
}
-static inline u32 round_up_to_quad(u32 i)
-{
- return (i + 3 ) & ~3;
-}
-
-static inline int
-svc_safe_putnetobj(struct kvec *resv, struct xdr_netobj *o)
-{
- u8 *p;
-
- if (resv->iov_len + 4 > PAGE_SIZE)
- return -1;
- svc_putnl(resv, o->len);
- p = resv->iov_base + resv->iov_len;
- resv->iov_len += round_up_to_quad(o->len);
- if (resv->iov_len > PAGE_SIZE)
- return -1;
- memcpy(p, o->data, o->len);
- memset(p + o->len, 0, round_up_to_quad(o->len) - o->len);
- return 0;
-}
-
/*
* Decode and verify a Call's verifier field. For RPC_AUTH_GSS Calls,
* the body of this field contains a variable length checksum.
@@ -772,42 +750,6 @@ svcauth_gss_verify_header(struct svc_rqst *rqstp, struct rsc *rsci,
return SVC_OK;
}
-static int
-gss_write_verf(struct svc_rqst *rqstp, struct gss_ctx *ctx_id, u32 seq)
-{
- __be32 *xdr_seq;
- u32 maj_stat;
- struct xdr_buf verf_data;
- struct xdr_netobj mic;
- __be32 *p;
- struct kvec iov;
- int err = -1;
-
- svc_putnl(rqstp->rq_res.head, RPC_AUTH_GSS);
- xdr_seq = kmalloc(4, GFP_KERNEL);
- if (!xdr_seq)
- return -ENOMEM;
- *xdr_seq = htonl(seq);
-
- iov.iov_base = xdr_seq;
- iov.iov_len = 4;
- xdr_buf_from_iov(&iov, &verf_data);
- p = rqstp->rq_res.head->iov_base + rqstp->rq_res.head->iov_len;
- mic.data = (u8 *)(p + 1);
- maj_stat = gss_get_mic(ctx_id, &verf_data, &mic);
- if (maj_stat != GSS_S_COMPLETE)
- goto out;
- *p++ = htonl(mic.len);
- memset((u8 *)p + mic.len, 0, round_up_to_quad(mic.len) - mic.len);
- p += XDR_QUADLEN(mic.len);
- if (!xdr_ressize_check(rqstp, p))
- goto out;
- err = 0;
-out:
- kfree(xdr_seq);
- return err;
-}
-
/*
* Construct and encode a Reply's verifier field. The verifier's body
* field contains a variable-length checksum of the GSS sequence
@@ -1454,8 +1396,6 @@ svcauth_gss_proc_init(struct svc_rqst *rqstp, struct rpc_gss_wire_cred *gc)
u32 flavor, len;
void *body;
- svcxdr_init_encode(rqstp);
-
/* Call's verf field: */
if (xdr_stream_decode_opaque_auth(xdr, &flavor, &body, &len) < 0)
return SVC_GARBAGE;
@@ -1642,15 +1582,15 @@ svcauth_gss_decode_credbody(struct xdr_stream *xdr,
static int
svcauth_gss_accept(struct svc_rqst *rqstp)
{
- struct kvec *resv = &rqstp->rq_res.head[0];
struct gss_svc_data *svcdata = rqstp->rq_auth_data;
__be32 *rpcstart;
struct rpc_gss_wire_cred *gc;
struct rsc *rsci = NULL;
- __be32 *reject_stat = resv->iov_base + resv->iov_len;
int ret;
struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
+ svcxdr_init_encode(rqstp);
+
rqstp->rq_auth_stat = rpc_autherr_badcred;
if (!svcdata)
svcdata = kmalloc(sizeof(*svcdata), GFP_KERNEL);
@@ -1700,28 +1640,25 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
/* now act upon the command: */
switch (gc->gc_proc) {
case RPC_GSS_PROC_DESTROY:
- if (gss_write_verf(rqstp, rsci->mechctx, gc->gc_seq))
+ if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
goto auth_err;
/* Delete the entry from the cache_list and call cache_put */
sunrpc_cache_unhash(sn->rsc_cache, &rsci->h);
- if (resv->iov_len + 4 > PAGE_SIZE)
- goto drop;
- svc_putnl(resv, RPC_SUCCESS);
+ if (xdr_stream_encode_u32(&rqstp->rq_res_stream, RPC_SUCCESS) < 0)
+ goto auth_err;
goto complete;
case RPC_GSS_PROC_DATA:
rqstp->rq_auth_stat = rpcsec_gsserr_ctxproblem;
- svcdata->verf_start = resv->iov_base + resv->iov_len;
- if (gss_write_verf(rqstp, rsci->mechctx, gc->gc_seq))
+ svcdata->verf_start = xdr_reserve_space(&rqstp->rq_res_stream, 0);
+ if (!svcauth_gss_encode_verf(rqstp, rsci->mechctx, gc->gc_seq))
goto auth_err;
rqstp->rq_cred = rsci->cred;
get_group_info(rsci->cred.cr_group_info);
rqstp->rq_auth_stat = rpc_autherr_badcred;
switch (gc->gc_svc) {
case RPC_GSS_SVC_NONE:
- svcxdr_init_encode(rqstp);
break;
case RPC_GSS_SVC_INTEGRITY:
- svcxdr_init_encode(rqstp);
/* placeholders for body length and seq. number: */
xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2);
if (svcauth_gss_unwrap_integ(rqstp, gc->gc_seq,
@@ -1730,7 +1667,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
svcxdr_set_auth_slack(rqstp, RPC_MAX_AUTH_SIZE);
break;
case RPC_GSS_SVC_PRIVACY:
- svcxdr_init_encode(rqstp);
/* placeholders for body length and seq. number: */
xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2);
if (svcauth_gss_unwrap_priv(rqstp, gc->gc_seq,
@@ -1755,8 +1691,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
ret = SVC_GARBAGE;
goto out;
auth_err:
- /* Restore write pointer to its original value: */
- xdr_ressize_check(rqstp, reject_stat);
+ xdr_truncate_encode(&rqstp->rq_res_stream, XDR_UNIT * 2);
ret = SVC_DENIED;
goto out;
complete:
From: Chuck Lever <[email protected]>
Currently, svcauth_gss_accept() pre-reserves response buffer space
for the RPC payload length and GSS sequence number before returning
to the dispatcher, which then adds the header's accept_stat field.
The problem is the accept_stat field is supposed to go before the
length and seq_num fields. So svcauth_gss_release() has to relocate
the accept_stat value (see svcauth_gss_prepare_to_wrap()).
To enable these fields to be added to the response buffer in the
correct (final) order, the pointer to the accept_stat has to be made
available to svcauth_gss_accept() so that it can set it before
reserving space for the length and seq_num fields.
As a first step, move the pointer to the location of the accept_stat
field into struct svc_rqst.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/lockd/svc.c | 4 ++--
fs/nfs/callback_xdr.c | 4 ++--
fs/nfsd/nfscache.c | 2 +-
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 4 ++--
include/linux/sunrpc/svc.h | 5 +++--
net/sunrpc/svc.c | 10 +++++-----
7 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 642e394e7a2d..0b28a6cf9303 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -685,15 +685,15 @@ module_exit(exit_nlm);
/**
* nlmsvc_dispatch - Process an NLM Request
* @rqstp: incoming request
- * @statp: pointer to location of accept_stat field in RPC Reply buffer
*
* Return values:
* %0: Processing complete; do not send a Reply
* %1: Processing complete; send Reply in rqstp->rq_res
*/
-static int nlmsvc_dispatch(struct svc_rqst *rqstp, __be32 *statp)
+static int nlmsvc_dispatch(struct svc_rqst *rqstp)
{
const struct svc_procedure *procp = rqstp->rq_procinfo;
+ __be32 *statp = rqstp->rq_accept_statp;
if (!procp->pc_decode(rqstp, &rqstp->rq_arg_stream))
goto out_decode_err;
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index b4c3b7182198..13b2af55497d 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -980,11 +980,11 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp)
}
static int
-nfs_callback_dispatch(struct svc_rqst *rqstp, __be32 *statp)
+nfs_callback_dispatch(struct svc_rqst *rqstp)
{
const struct svc_procedure *procp = rqstp->rq_procinfo;
- *statp = procp->pc_func(rqstp);
+ *rqstp->rq_accept_statp = procp->pc_func(rqstp);
return 1;
}
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index ef5ee548053b..041faa13b852 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -509,7 +509,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp)
* nfsd_cache_update - Update an entry in the duplicate reply cache.
* @rqstp: svc_rqst with a finished Reply
* @cachetype: which cache to update
- * @statp: Reply's status code
+ * @statp: pointer to Reply's NFS status code, or NULL
*
* This is called from nfsd_dispatch when the procedure has been
* executed and the complete reply is in rqstp->rq_res.
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 93b42ef9ed91..a7de2ffe943b 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -86,7 +86,7 @@ bool nfssvc_encode_voidres(struct svc_rqst *rqstp,
* Function prototypes.
*/
int nfsd_svc(int nrservs, struct net *net, const struct cred *cred);
-int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp);
+int nfsd_dispatch(struct svc_rqst *rqstp);
int nfsd_nrthreads(struct net *);
int nfsd_nrpools(struct net *);
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index dfa8ee6c04d5..ff10c46b62d3 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1022,7 +1022,6 @@ nfsd(void *vrqstp)
/**
* nfsd_dispatch - Process an NFS or NFSACL Request
* @rqstp: incoming request
- * @statp: pointer to location of accept_stat field in RPC Reply buffer
*
* This RPC dispatcher integrates the NFS server's duplicate reply cache.
*
@@ -1030,9 +1029,10 @@ nfsd(void *vrqstp)
* %0: Processing complete; do not send a Reply
* %1: Processing complete; send Reply in rqstp->rq_res
*/
-int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
+int nfsd_dispatch(struct svc_rqst *rqstp)
{
const struct svc_procedure *proc = rqstp->rq_procinfo;
+ __be32 *statp = rqstp->rq_accept_statp;
/*
* Give the xdr decoder a chance to change this if it wants
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 32eb98e621c3..f40a90ca5de6 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -251,6 +251,7 @@ struct svc_rqst {
void * rq_argp; /* decoded arguments */
void * rq_resp; /* xdr'd results */
+ __be32 *rq_accept_statp;
void * rq_auth_data; /* flavor-specific data */
__be32 rq_auth_stat; /* authentication status */
int rq_auth_slack; /* extra space xdr code
@@ -337,7 +338,7 @@ struct svc_deferred_req {
struct svc_process_info {
union {
- int (*dispatch)(struct svc_rqst *, __be32 *);
+ int (*dispatch)(struct svc_rqst *rqstp);
struct {
unsigned int lovers;
unsigned int hivers;
@@ -389,7 +390,7 @@ struct svc_version {
bool vs_need_cong_ctrl;
/* Dispatch function */
- int (*vs_dispatch)(struct svc_rqst *, __be32 *);
+ int (*vs_dispatch)(struct svc_rqst *rqstp);
};
/*
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index bb58915622ca..3c194e6f8f5e 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1232,9 +1232,9 @@ svc_process_common(struct svc_rqst *rqstp)
const struct svc_procedure *procp = NULL;
struct svc_serv *serv = rqstp->rq_server;
struct svc_process_info process;
- __be32 *p, *statp;
int auth_res, rc;
unsigned int aoffset;
+ __be32 *p;
/* Will be turned off by GSS integrity and privacy services */
__set_bit(RQ_SPLICE_OK, &rqstp->rq_flags);
@@ -1314,8 +1314,8 @@ svc_process_common(struct svc_rqst *rqstp)
trace_svc_process(rqstp, progp->pg_name);
aoffset = xdr_stream_pos(xdr);
- statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
- *statp = rpc_success;
+ rqstp->rq_accept_statp = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT);
+ *rqstp->rq_accept_statp = rpc_success;
/* un-reserve some of the out-queue now that we have a
* better idea of reply size
@@ -1324,7 +1324,7 @@ svc_process_common(struct svc_rqst *rqstp)
svc_reserve_auth(rqstp, procp->pc_xdrressize<<2);
/* Call the function that processes the request. */
- rc = process.dispatch(rqstp, statp);
+ rc = process.dispatch(rqstp);
if (procp->pc_release)
procp->pc_release(rqstp);
if (!rc)
@@ -1332,7 +1332,7 @@ svc_process_common(struct svc_rqst *rqstp)
if (rqstp->rq_auth_stat != rpc_auth_ok)
goto err_bad_auth;
- if (*statp != rpc_success)
+ if (*rqstp->rq_accept_statp != rpc_success)
xdr_truncate_encode(xdr, aoffset);
if (procp->pc_encode == NULL)
From: Chuck Lever <[email protected]>
The @resv parameter is no longer used.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/svc.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index bcca1553c75a..bb58915622ca 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1225,7 +1225,7 @@ EXPORT_SYMBOL_GPL(svc_generic_init_request);
* Common routine for processing the RPC request.
*/
static int
-svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
+svc_process_common(struct svc_rqst *rqstp)
{
struct xdr_stream *xdr = &rqstp->rq_res_stream;
struct svc_program *progp;
@@ -1455,7 +1455,7 @@ svc_process(struct svc_rqst *rqstp)
if (unlikely(*p != rpc_call))
goto out_baddir;
- if (!svc_process_common(rqstp, resv))
+ if (!svc_process_common(rqstp))
goto out_drop;
return svc_send(rqstp);
@@ -1478,7 +1478,6 @@ int
bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
struct svc_rqst *rqstp)
{
- struct kvec *resv = &rqstp->rq_res.head[0];
struct rpc_task *task;
int proc_error;
int error;
@@ -1509,22 +1508,21 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
rqstp->rq_arg.len = rqstp->rq_arg.head[0].iov_len +
rqstp->rq_arg.page_len;
- /* reset result send buffer "put" position */
- resv->iov_len = 0;
-
- svcxdr_init_decode(rqstp);
+ /* Reset the response buffer */
+ rqstp->rq_res.head[0].iov_len = 0;
/*
* Skip the XID and calldir fields because they've already
* been processed by the caller.
*/
+ svcxdr_init_decode(rqstp);
if (!xdr_inline_decode(&rqstp->rq_arg_stream, XDR_UNIT * 2)) {
error = -EINVAL;
goto out;
}
/* Parse and execute the bc call */
- proc_error = svc_process_common(rqstp, resv);
+ proc_error = svc_process_common(rqstp);
atomic_dec(&req->rq_xprt->bc_slot_count);
if (!proc_error) {
From: Chuck Lever <[email protected]>
Now that each ->accept method has been converted, the
svcxdr_init_encode() calls can be hoisted back up into the generic
RPC server code.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 2 --
net/sunrpc/svc.c | 2 ++
net/sunrpc/svcauth_unix.c | 3 ---
3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 89333669af26..560fb8a2803d 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1589,8 +1589,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)
int ret;
struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
- svcxdr_init_encode(rqstp);
-
rqstp->rq_auth_stat = rpc_autherr_badcred;
if (!svcdata)
svcdata = kmalloc(sizeof(*svcdata), GFP_KERNEL);
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 9951311790bf..393eebd1f6fe 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1262,6 +1262,8 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)
if (rqstp->rq_prog == progp->pg_prog)
break;
+ svcxdr_init_encode(rqstp);
+
/*
* Decode auth data, and add verifier to reply buffer.
* We do this before anything else in order to get a decent
diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
index 632150a6b947..b101700d155c 100644
--- a/net/sunrpc/svcauth_unix.c
+++ b/net/sunrpc/svcauth_unix.c
@@ -772,7 +772,6 @@ svcauth_null_accept(struct svc_rqst *rqstp)
if (cred->cr_group_info == NULL)
return SVC_CLOSE; /* kmalloc failure - client must retry */
- svcxdr_init_encode(rqstp);
if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
RPC_AUTH_NULL, NULL, 0) < 0)
return SVC_CLOSE;
@@ -855,7 +854,6 @@ svcauth_tls_accept(struct svc_rqst *rqstp)
if (cred->cr_group_info == NULL)
return SVC_CLOSE;
- svcxdr_init_encode(rqstp);
if (rqstp->rq_xprt->xpt_ops->xpo_start_tls) {
p = xdr_reserve_space(&rqstp->rq_res_stream, XDR_UNIT * 2 + 8);
if (!p)
@@ -959,7 +957,6 @@ svcauth_unix_accept(struct svc_rqst *rqstp)
return SVC_DENIED;
}
- svcxdr_init_encode(rqstp);
if (xdr_stream_encode_opaque_auth(&rqstp->rq_res_stream,
RPC_AUTH_NULL, NULL, 0) < 0)
return SVC_CLOSE;
From: Chuck Lever <[email protected]>
Clean up: To help orient readers, name the stack variables to match
the XDR field names.
Additionally, the explicit type cast on @gsd is unnecessary; and
@resbuf is renamed to match the variable naming in the unwrap
functions.
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 70 +++++++++++++++++++++++--------------
1 file changed, 43 insertions(+), 27 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 4a576ed7aa32..fe0bd0ad8ace 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1758,49 +1758,65 @@ svcauth_gss_prepare_to_wrap(struct xdr_buf *resbuf, struct gss_svc_data *gsd)
return p;
}
-static inline int
-svcauth_gss_wrap_resp_integ(struct svc_rqst *rqstp)
+/*
+ * RFC 2203, Section 5.3.2.2
+ *
+ * struct rpc_gss_integ_data {
+ * opaque databody_integ<>;
+ * opaque checksum<>;
+ * };
+ *
+ * struct rpc_gss_data_t {
+ * unsigned int seq_num;
+ * proc_req_arg_t arg;
+ * };
+ *
+ * The RPC Reply message has already been XDR-encoded. rq_res_stream
+ * is now positioned so that the checksum can be written just past
+ * the RPC Reply message.
+ */
+static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)
{
- struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
+ struct gss_svc_data *gsd = rqstp->rq_auth_data;
struct rpc_gss_wire_cred *gc = &gsd->clcred;
- struct xdr_buf *resbuf = &rqstp->rq_res;
- struct xdr_buf integ_buf;
- struct xdr_netobj mic;
+ struct xdr_buf *buf = &rqstp->rq_res;
+ struct xdr_buf databody_integ;
+ struct xdr_netobj checksum;
struct kvec *resv;
+ u32 offset, len;
__be32 *p;
- int integ_offset, integ_len;
int stat = -EINVAL;
- p = svcauth_gss_prepare_to_wrap(resbuf, gsd);
+ p = svcauth_gss_prepare_to_wrap(buf, gsd);
if (p == NULL)
goto out;
- integ_offset = (u8 *)(p + 1) - (u8 *)resbuf->head[0].iov_base;
- integ_len = resbuf->len - integ_offset;
- if (integ_len & 3)
+ offset = (u8 *)(p + 1) - (u8 *)buf->head[0].iov_base;
+ len = buf->len - offset;
+ if (len & 3)
goto out;
- *p++ = htonl(integ_len);
+ *p++ = htonl(len);
*p++ = htonl(gc->gc_seq);
- if (xdr_buf_subsegment(resbuf, &integ_buf, integ_offset, integ_len)) {
+ if (xdr_buf_subsegment(buf, &databody_integ, offset, len)) {
WARN_ON_ONCE(1);
goto out_err;
}
- if (resbuf->tail[0].iov_base == NULL) {
- if (resbuf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
+ if (!buf->tail[0].iov_base) {
+ if (buf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
goto out_err;
- resbuf->tail[0].iov_base = resbuf->head[0].iov_base
- + resbuf->head[0].iov_len;
- resbuf->tail[0].iov_len = 0;
+ buf->tail[0].iov_base = buf->head[0].iov_base
+ + buf->head[0].iov_len;
+ buf->tail[0].iov_len = 0;
}
- resv = &resbuf->tail[0];
- mic.data = (u8 *)resv->iov_base + resv->iov_len + 4;
- if (gss_get_mic(gsd->rsci->mechctx, &integ_buf, &mic))
+ resv = &buf->tail[0];
+ checksum.data = (u8 *)resv->iov_base + resv->iov_len + 4;
+ if (gss_get_mic(gsd->rsci->mechctx, &databody_integ, &checksum))
goto out_err;
- svc_putnl(resv, mic.len);
- memset(mic.data + mic.len, 0,
- round_up_to_quad(mic.len) - mic.len);
- resv->iov_len += XDR_QUADLEN(mic.len) << 2;
+ svc_putnl(resv, checksum.len);
+ memset(checksum.data + checksum.len, 0,
+ round_up_to_quad(checksum.len) - checksum.len);
+ resv->iov_len += XDR_QUADLEN(checksum.len) << 2;
/* not strictly required: */
- resbuf->len += XDR_QUADLEN(mic.len) << 2;
+ buf->len += XDR_QUADLEN(checksum.len) << 2;
if (resv->iov_len > PAGE_SIZE)
goto out_err;
out:
@@ -1909,7 +1925,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
case RPC_GSS_SVC_NONE:
break;
case RPC_GSS_SVC_INTEGRITY:
- stat = svcauth_gss_wrap_resp_integ(rqstp);
+ stat = svcauth_gss_wrap_integ(rqstp);
if (stat)
goto out_err;
break;
From: Chuck Lever <[email protected]>
The svc_get/put helpers are no longer used.
Signed-off-by: Chuck Lever <[email protected]>
---
include/linux/sunrpc/svc.h | 66 +-------------------------------------------
1 file changed, 1 insertion(+), 65 deletions(-)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index dd9f68acd9f1..32eb98e621c3 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -193,40 +193,6 @@ extern u32 svc_max_payload(const struct svc_rqst *rqstp);
#define RPCSVC_MAXPAGES ((RPCSVC_MAXPAYLOAD+PAGE_SIZE-1)/PAGE_SIZE \
+ 2 + 1)
-static inline u32 svc_getnl(struct kvec *iov)
-{
- __be32 val, *vp;
- vp = iov->iov_base;
- val = *vp++;
- iov->iov_base = (void*)vp;
- iov->iov_len -= sizeof(__be32);
- return ntohl(val);
-}
-
-static inline void svc_putnl(struct kvec *iov, u32 val)
-{
- __be32 *vp = iov->iov_base + iov->iov_len;
- *vp = htonl(val);
- iov->iov_len += sizeof(__be32);
-}
-
-static inline __be32 svc_getu32(struct kvec *iov)
-{
- __be32 val, *vp;
- vp = iov->iov_base;
- val = *vp++;
- iov->iov_base = (void*)vp;
- iov->iov_len -= sizeof(__be32);
- return val;
-}
-
-static inline void svc_putu32(struct kvec *iov, __be32 val)
-{
- __be32 *vp = iov->iov_base + iov->iov_len;
- *vp = val;
- iov->iov_len += sizeof(__be32);
-}
-
/*
* The context of a single thread, including the request currently being
* processed.
@@ -345,29 +311,6 @@ static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
return (struct sockaddr *) &rqst->rq_daddr;
}
-/*
- * Check buffer bounds after decoding arguments
- */
-static inline int
-xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
-{
- char *cp = (char *)p;
- struct kvec *vec = &rqstp->rq_arg.head[0];
- return cp >= (char*)vec->iov_base
- && cp <= (char*)vec->iov_base + vec->iov_len;
-}
-
-static inline int
-xdr_ressize_check(struct svc_rqst *rqstp, __be32 *p)
-{
- struct kvec *vec = &rqstp->rq_res.head[0];
- char *cp = (char*)p;
-
- vec->iov_len = cp - (char*)vec->iov_base;
-
- return vec->iov_len <= PAGE_SIZE;
-}
-
static inline void svc_free_res_pages(struct svc_rqst *rqstp)
{
while (rqstp->rq_next_page != rqstp->rq_respages) {
@@ -540,9 +483,6 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space)
* svcxdr_init_decode - Prepare an xdr_stream for Call decoding
* @rqstp: controlling server RPC transaction context
*
- * This function currently assumes the RPC header in rq_arg has
- * already been decoded. Upon return, xdr->p points to the
- * location of the upper layer header.
*/
static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
{
@@ -550,11 +490,7 @@ static inline void svcxdr_init_decode(struct svc_rqst *rqstp)
struct xdr_buf *buf = &rqstp->rq_arg;
struct kvec *argv = buf->head;
- /*
- * svc_getnl() and friends do not keep the xdr_buf's ::len
- * field up to date. Refresh that field before initializing
- * the argument decoding stream.
- */
+ WARN_ON(buf->len != buf->head->iov_len + buf->page_len + buf->tail->iov_len);
buf->len = buf->head->iov_len + buf->page_len + buf->tail->iov_len;
xdr_init_decode(xdr, buf, argv->iov_base, NULL);
From: Chuck Lever <[email protected]>
Now that upper layers use an xdr_stream to track the construction
of each RPC Reply message, resbuf->len is kept up-to-date
automatically. There's no need to recompute it in svc_gss_release().
Signed-off-by: Chuck Lever <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 2e603358fae1..4a576ed7aa32 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -969,12 +969,6 @@ svcauth_gss_unwrap_integ(struct svc_rqst *rqstp, u32 seq, struct gss_ctx *ctx)
return -EINVAL;
}
-static inline int
-total_buf_len(struct xdr_buf *buf)
-{
- return buf->head[0].iov_len + buf->page_len + buf->tail[0].iov_len;
-}
-
/*
* RFC 2203, Section 5.3.2.3
*
@@ -1882,14 +1876,25 @@ svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
return 0;
}
+/**
+ * svcauth_gss_release - Wrap payload and release resources
+ * @rqstp: RPC transaction context
+ *
+ * Return values:
+ * %0: the Reply is ready to be sent
+ * %-ENOMEM: failed to allocate memory
+ * %-EINVAL: encoding error
+ *
+ * XXX: These return values do not match the return values documented
+ * for the auth_ops ->release method in linux/sunrpc/svcauth.h.
+ */
static int
svcauth_gss_release(struct svc_rqst *rqstp)
{
- struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
- struct rpc_gss_wire_cred *gc;
- struct xdr_buf *resbuf = &rqstp->rq_res;
- int stat = -EINVAL;
struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
+ struct gss_svc_data *gsd = rqstp->rq_auth_data;
+ struct rpc_gss_wire_cred *gc;
+ int stat;
if (!gsd)
goto out;
@@ -1899,10 +1904,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
/* Release can be called twice, but we only wrap once. */
if (gsd->verf_start == NULL)
goto out;
- /* normally not set till svc_send, but we need it here: */
- /* XXX: what for? Do we mess it up the moment we call svc_putu32
- * or whatever? */
- resbuf->len = total_buf_len(resbuf);
+
switch (gc->gc_svc) {
case RPC_GSS_SVC_NONE:
break;
On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
> From: Chuck Lever <[email protected]>
>
> Now that upper layers use an xdr_stream to track the construction
> of each RPC Reply message, resbuf->len is kept up-to-date
> automatically. There's no need to recompute it in svc_gss_release().
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 2e603358fae1..4a576ed7aa32 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -969,12 +969,6 @@ svcauth_gss_unwrap_integ(struct svc_rqst *rqstp, u32 seq, struct gss_ctx *ctx)
> return -EINVAL;
> }
>
> -static inline int
> -total_buf_len(struct xdr_buf *buf)
> -{
> - return buf->head[0].iov_len + buf->page_len + buf->tail[0].iov_len;
> -}
> -
> /*
> * RFC 2203, Section 5.3.2.3
> *
> @@ -1882,14 +1876,25 @@ svcauth_gss_wrap_resp_priv(struct svc_rqst *rqstp)
> return 0;
> }
>
> +/**
> + * svcauth_gss_release - Wrap payload and release resources
> + * @rqstp: RPC transaction context
> + *
> + * Return values:
> + * %0: the Reply is ready to be sent
> + * %-ENOMEM: failed to allocate memory
> + * %-EINVAL: encoding error
> + *
> + * XXX: These return values do not match the return values documented
> + * for the auth_ops ->release method in linux/sunrpc/svcauth.h.
As an aside...
It looks like the only place ->release is called is in svc_authorise,
and its callers either ignore the return, or just test whether it
succeeded (returned 0). If it fails then it looks like the xprt is
closed.
The actual return code doesn't matter at all. We could make ->release a
bool return to make that clear.
That's not directly related to this set though.
> static int
> svcauth_gss_release(struct svc_rqst *rqstp)
> {
> - struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;
> - struct rpc_gss_wire_cred *gc;
> - struct xdr_buf *resbuf = &rqstp->rq_res;
> - int stat = -EINVAL;
> struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
> + struct gss_svc_data *gsd = rqstp->rq_auth_data;
> + struct rpc_gss_wire_cred *gc;
> + int stat;
>
> if (!gsd)
> goto out;
> @@ -1899,10 +1904,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
> /* Release can be called twice, but we only wrap once. */
> if (gsd->verf_start == NULL)
> goto out;
> - /* normally not set till svc_send, but we need it here: */
> - /* XXX: what for? Do we mess it up the moment we call svc_putu32
> - * or whatever? */
> - resbuf->len = total_buf_len(resbuf);
> +
> switch (gc->gc_svc) {
> case RPC_GSS_SVC_NONE:
> break;
>
>
The patch itself looks fine.
Reviewed-by: Jeff Layton <[email protected]>
On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
> The purpose of this series is to replace the svc_put* macros in the
> Linux kernel server's RPC reply header construction code with
> xdr_stream helpers. I've measured no change in CPU utilization after
> the overhaul.
>
> Memory safety: Buffer bounds checking after encoding each XDR item
> is more memory-safe than the current mechanism. Subsequent memory
> safety improvements to the common xdr_stream helpers will benefit
> all who use them.
>
> Audit friendliness: The new code has additional comments and other
> clean-up to help align it with the relevant RPC protocol
> specifications. The use of common helpers also makes the encoders
> easier to audit and maintain.
>
> I've split the full series in half to make it easier to review. The
> patches posted here are the second half, handling RPC reply header
> encoding.
>
> Note that another benefit of this work is that we are taking one or
> two more strides closer to greater commonality between the client
> and server implementations of RPCSEC GSS.
>
> ---
>
> Chuck Lever (27):
> SUNRPC: Clean up svcauth_gss_release()
> SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
> SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
> SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
> SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
> SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
> SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
> SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
> SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
> SUNRPC: Check rq_auth_stat when preparing to wrap a response
> SUNRPC: Remove the rpc_stat variable in svc_process_common()
> SUNRPC: Add XDR encoding helper for opaque_auth
> SUNRPC: Push svcxdr_init_encode() into svc_process_common()
> SUNRPC: Move svcxdr_init_encode() into ->accept methods
> SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
> SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
> SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
> SUNRPC: Convert unwrap data paths to use xdr_stream for replies
> SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
> SUNRPC: Use xdr_stream for encoding GSS reply verifiers
> SUNRPC: Hoist init_encode out of svc_authenticate()
> SUNRPC: Convert RPC Reply header encoding to use xdr_stream
> SUNRPC: Final clean-up of svc_process_common()
> SUNRPC: Remove no-longer-used helper functions
> SUNRPC: Refactor RPC server dispatch method
> SUNRPC: Set rq_accept_statp inside ->accept methods
> SUNRPC: Go back to using gsd->body_start
>
>
> fs/lockd/svc.c | 5 +-
> fs/nfs/callback_xdr.c | 6 +-
> fs/nfsd/nfscache.c | 4 +-
> fs/nfsd/nfsd.h | 2 +-
> fs/nfsd/nfssvc.c | 10 +-
> include/linux/sunrpc/svc.h | 116 +++----
> include/linux/sunrpc/xdr.h | 23 ++
> include/trace/events/rpcgss.h | 22 ++
> net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
> net/sunrpc/svc.c | 91 +++---
> net/sunrpc/svcauth_unix.c | 40 ++-
> net/sunrpc/xdr.c | 29 ++
> 12 files changed, 451 insertions(+), 402 deletions(-)
>
> --
> Chuck Lever
>
I went through the whole set and this all looks like good stuff to me.
The result is a lot more readable, and there is a lot less manual
fiddling with buffer lengths and such.
Do you have a public branch with the current state of this set?
You can add:
Reviewed-by: Jeff Layton <[email protected]>
> On Jan 10, 2023, at 9:53 AM, Jeff Layton <[email protected]> wrote:
>
> On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
>> The purpose of this series is to replace the svc_put* macros in the
>> Linux kernel server's RPC reply header construction code with
>> xdr_stream helpers. I've measured no change in CPU utilization after
>> the overhaul.
>>
>> Memory safety: Buffer bounds checking after encoding each XDR item
>> is more memory-safe than the current mechanism. Subsequent memory
>> safety improvements to the common xdr_stream helpers will benefit
>> all who use them.
>>
>> Audit friendliness: The new code has additional comments and other
>> clean-up to help align it with the relevant RPC protocol
>> specifications. The use of common helpers also makes the encoders
>> easier to audit and maintain.
>>
>> I've split the full series in half to make it easier to review. The
>> patches posted here are the second half, handling RPC reply header
>> encoding.
>>
>> Note that another benefit of this work is that we are taking one or
>> two more strides closer to greater commonality between the client
>> and server implementations of RPCSEC GSS.
>>
>> ---
>>
>> Chuck Lever (27):
>> SUNRPC: Clean up svcauth_gss_release()
>> SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
>> SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
>> SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
>> SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
>> SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
>> SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
>> SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
>> SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
>> SUNRPC: Check rq_auth_stat when preparing to wrap a response
>> SUNRPC: Remove the rpc_stat variable in svc_process_common()
>> SUNRPC: Add XDR encoding helper for opaque_auth
>> SUNRPC: Push svcxdr_init_encode() into svc_process_common()
>> SUNRPC: Move svcxdr_init_encode() into ->accept methods
>> SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
>> SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
>> SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
>> SUNRPC: Convert unwrap data paths to use xdr_stream for replies
>> SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
>> SUNRPC: Use xdr_stream for encoding GSS reply verifiers
>> SUNRPC: Hoist init_encode out of svc_authenticate()
>> SUNRPC: Convert RPC Reply header encoding to use xdr_stream
>> SUNRPC: Final clean-up of svc_process_common()
>> SUNRPC: Remove no-longer-used helper functions
>> SUNRPC: Refactor RPC server dispatch method
>> SUNRPC: Set rq_accept_statp inside ->accept methods
>> SUNRPC: Go back to using gsd->body_start
>>
>>
>> fs/lockd/svc.c | 5 +-
>> fs/nfs/callback_xdr.c | 6 +-
>> fs/nfsd/nfscache.c | 4 +-
>> fs/nfsd/nfsd.h | 2 +-
>> fs/nfsd/nfssvc.c | 10 +-
>> include/linux/sunrpc/svc.h | 116 +++----
>> include/linux/sunrpc/xdr.h | 23 ++
>> include/trace/events/rpcgss.h | 22 ++
>> net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
>> net/sunrpc/svc.c | 91 +++---
>> net/sunrpc/svcauth_unix.c | 40 ++-
>> net/sunrpc/xdr.c | 29 ++
>> 12 files changed, 451 insertions(+), 402 deletions(-)
>>
>> --
>> Chuck Lever
>>
>
> I went through the whole set and this all looks like good stuff to me.
> The result is a lot more readable, and there is a lot less manual
> fiddling with buffer lengths and such.
>
> Do you have a public branch with the current state of this set?
These are in the topic-rpcsec-gss-krb5-enhancements branch in
my repo at kernel.org, although I'm about to push them to nfsd's
for-next.
> You can add:
>
> Reviewed-by: Jeff Layton <[email protected]>
I very much appreciate your time!
--
Chuck Lever