* [PATCH RFC 00/10] SUNRPC GSS overhaul
@ 2019-02-01 19:57 Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions Chuck Lever
` (9 more replies)
0 siblings, 10 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Hi-
Request For Comments on this series, which converts the rpc_auth
paths (including GSS) to use xdr_stream. It applies on top of the
series I sent last Friday.
I'd eventually like to see this work in v5.1.
---
Chuck Lever (10):
SUNRPC: Remove some dprintk() call sites from auth functions
SUNRPC: Remove rpc_xprt::tsh_size
SUNRPC: Add build option to disable support for insecure enctypes
SUNRPC: Add common byte-swapped RPC header constants
SUNRPC: Use struct xdr_stream when constructing RPC Call header
SUNRPC: Clean up rpc_verify_header()
SUNRPC: Use struct xdr_stream when decoding RPC Reply header
SUNRPC: Introduce trace points in rpc_auth_gss.ko
SUNRPC: Remove xdr_buf_trim()
SUNRPC: Add SPDX IDs to some net/sunrpc/auth_gss/ files
include/linux/sunrpc/auth.h | 30 +-
include/linux/sunrpc/auth_gss.h | 5
include/linux/sunrpc/gss_krb5_enctypes.h | 42 ++
include/linux/sunrpc/xdr.h | 67 ++-
include/linux/sunrpc/xprt.h | 7
include/trace/events/rpcgss.h | 361 +++++++++++++++++++
include/trace/events/rpcrdma.h | 12 +
include/trace/events/sunrpc.h | 142 +++++++
net/sunrpc/Kconfig | 16 +
net/sunrpc/auth.c | 136 ++++---
net/sunrpc/auth_gss/Makefile | 2
net/sunrpc/auth_gss/auth_gss.c | 542 ++++++++++++++--------------
net/sunrpc/auth_gss/gss_krb5_mech.c | 29 -
net/sunrpc/auth_gss/gss_krb5_wrap.c | 8
net/sunrpc/auth_gss/gss_mech_switch.c | 27 -
net/sunrpc/auth_gss/gss_rpc_upcall.c | 15 -
net/sunrpc/auth_gss/gss_rpc_upcall.h | 16 -
net/sunrpc/auth_gss/gss_rpc_xdr.c | 15 -
net/sunrpc/auth_gss/gss_rpc_xdr.h | 17 -
net/sunrpc/auth_gss/svcauth_gss.c | 3
net/sunrpc/auth_gss/trace.c | 11 +
net/sunrpc/auth_null.c | 54 +--
net/sunrpc/auth_unix.c | 112 +++---
net/sunrpc/clnt.c | 360 +++++++++----------
net/sunrpc/svc.c | 19 -
net/sunrpc/xdr.c | 41 --
net/sunrpc/xprt.c | 10 -
net/sunrpc/xprtrdma/svc_rdma_backchannel.c | 1
net/sunrpc/xprtrdma/transport.c | 1
net/sunrpc/xprtsock.c | 91 +++--
30 files changed, 1326 insertions(+), 866 deletions(-)
create mode 100644 include/trace/events/rpcgss.h
create mode 100644 net/sunrpc/auth_gss/trace.c
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
@ 2019-02-01 19:57 ` Chuck Lever
2019-02-04 19:04 ` J. Bruce Fields
2019-02-01 19:57 ` [PATCH RFC 02/10] SUNRPC: Remove rpc_xprt::tsh_size Chuck Lever
` (8 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Clean up: Reduce dprintk noise by removing dprintk() call sites
from hot path that do not report exceptions. These are usually
replaceable with function graph tracing.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
net/sunrpc/auth.c | 29 -----------------------------
net/sunrpc/auth_unix.c | 9 +--------
2 files changed, 1 insertion(+), 37 deletions(-)
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 8dfab61..275e84e 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -17,10 +17,6 @@
#include <linux/sunrpc/gss_api.h>
#include <linux/spinlock.h>
-#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
-# define RPCDBG_FACILITY RPCDBG_AUTH
-#endif
-
#define RPC_CREDCACHE_DEFAULT_HASHBITS (4)
struct rpc_cred_cache {
struct hlist_head *hashtable;
@@ -267,8 +263,6 @@ static int param_get_hashtbl_sz(char *buffer, const struct kernel_param *kp)
}
}
rcu_read_unlock();
-
- dprintk("RPC: %s returns %d\n", __func__, result);
return result;
}
EXPORT_SYMBOL_GPL(rpcauth_list_flavors);
@@ -636,9 +630,6 @@ struct rpc_cred *
struct rpc_cred *ret;
const struct cred *cred = current_cred();
- dprintk("RPC: looking up %s cred\n",
- auth->au_ops->au_name);
-
memset(&acred, 0, sizeof(acred));
acred.cred = cred;
ret = auth->au_ops->lookup_cred(auth, &acred, flags);
@@ -670,8 +661,6 @@ struct rpc_cred *
};
struct rpc_cred *ret;
- dprintk("RPC: %5u looking up %s cred\n",
- task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
ret = auth->au_ops->lookup_cred(auth, &acred, lookupflags);
put_cred(acred.cred);
return ret;
@@ -688,8 +677,6 @@ struct rpc_cred *
if (!acred.principal)
return NULL;
- dprintk("RPC: %5u looking up %s machine cred\n",
- task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
return auth->au_ops->lookup_cred(auth, &acred, lookupflags);
}
@@ -698,8 +685,6 @@ struct rpc_cred *
{
struct rpc_auth *auth = task->tk_client->cl_auth;
- dprintk("RPC: %5u looking up %s cred\n",
- task->tk_pid, auth->au_ops->au_name);
return rpcauth_lookupcred(auth, lookupflags);
}
@@ -776,9 +761,6 @@ struct rpc_cred *
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
- dprintk("RPC: %5u marshaling %s cred %p\n",
- task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
-
return cred->cr_ops->crmarshal(task, p);
}
@@ -787,9 +769,6 @@ struct rpc_cred *
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
- dprintk("RPC: %5u validating %s cred %p\n",
- task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
-
return cred->cr_ops->crvalidate(task, p);
}
@@ -808,8 +787,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
- dprintk("RPC: %5u using %s cred %p to wrap rpc data\n",
- task->tk_pid, cred->cr_ops->cr_name, cred);
if (cred->cr_ops->crwrap_req)
return cred->cr_ops->crwrap_req(task, encode, rqstp, data, obj);
/* By default, we encode the arguments normally. */
@@ -833,8 +810,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
- dprintk("RPC: %5u using %s cred %p to unwrap rpc data\n",
- task->tk_pid, cred->cr_ops->cr_name, cred);
if (cred->cr_ops->crunwrap_resp)
return cred->cr_ops->crunwrap_resp(task, decode, rqstp,
data, obj);
@@ -865,8 +840,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
goto out;
cred = task->tk_rqstp->rq_cred;
}
- dprintk("RPC: %5u refreshing %s cred %p\n",
- task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
err = cred->cr_ops->crrefresh(task);
out:
@@ -880,8 +853,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
- dprintk("RPC: %5u invalidating %s cred %p\n",
- task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
if (cred)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
}
diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
index 387f6b3..fc8a591 100644
--- a/net/sunrpc/auth_unix.c
+++ b/net/sunrpc/auth_unix.c
@@ -28,8 +28,6 @@
static struct rpc_auth *
unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
{
- dprintk("RPC: creating UNIX authenticator for client %p\n",
- clnt);
refcount_inc(&unix_auth.au_count);
return &unix_auth;
}
@@ -37,7 +35,6 @@
static void
unx_destroy(struct rpc_auth *auth)
{
- dprintk("RPC: destroying UNIX authenticator %p\n", auth);
}
/*
@@ -48,10 +45,6 @@
{
struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
- dprintk("RPC: allocating UNIX cred for uid %d gid %d\n",
- from_kuid(&init_user_ns, acred->cred->fsuid),
- from_kgid(&init_user_ns, acred->cred->fsgid));
-
rpcauth_init_cred(ret, acred, auth, &unix_credops);
ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
return ret;
@@ -61,7 +54,7 @@
unx_free_cred_callback(struct rcu_head *head)
{
struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
- dprintk("RPC: unx_free_cred %p\n", rpc_cred);
+
put_cred(rpc_cred->cr_cred);
mempool_free(rpc_cred, unix_pool);
}
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 02/10] SUNRPC: Remove rpc_xprt::tsh_size
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions Chuck Lever
@ 2019-02-01 19:57 ` Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 03/10] SUNRPC: Add build option to disable support for insecure enctypes Chuck Lever
` (7 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
tsh_size was added to accommodate transports that send a pre-amble
before each RPC message. However, this assumes the pre-amble is
fixed in size, which isn't true for some transports. That makes
tsh_size not very generic.
Also I'd like to make the estimation of RPC send and receive
buffer sizes more precise. tsh_size doesn't currently appear to be
accounted for at all by call_allocate.
Therefore let's just remove the tsh_size concept, and make the only
transports that have a non-zero tsh_size employ a direct approach.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/xprt.h | 7 --
net/sunrpc/auth_gss/auth_gss.c | 3 -
net/sunrpc/clnt.c | 1
net/sunrpc/svc.c | 19 +-----
net/sunrpc/xprtrdma/svc_rdma_backchannel.c | 1
net/sunrpc/xprtrdma/transport.c | 1
net/sunrpc/xprtsock.c | 91 ++++++++++++++++++----------
7 files changed, 65 insertions(+), 58 deletions(-)
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index ad7e910..3a39154 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -196,8 +196,6 @@ struct rpc_xprt {
size_t max_payload; /* largest RPC payload size,
in bytes */
- unsigned int tsh_size; /* size of transport specific
- header */
struct rpc_wait_queue binding; /* requests waiting on rpcbind */
struct rpc_wait_queue sending; /* requests waiting to send */
@@ -362,11 +360,6 @@ struct rpc_xprt * xprt_alloc(struct net *net, size_t size,
unsigned int max_req);
void xprt_free(struct rpc_xprt *);
-static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *p)
-{
- return p + xprt->tsh_size;
-}
-
static inline int
xprt_enable_swap(struct rpc_xprt *xprt)
{
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index a42672e..4b52e2b 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1563,8 +1563,7 @@ static void gss_pipe_free(struct gss_pipe *p)
/* We compute the checksum for the verifier over the xdr-encoded bytes
* starting with the xid and ending at the end of the credential: */
- iov.iov_base = xprt_skip_transport_header(req->rq_xprt,
- req->rq_snd_buf.head[0].iov_base);
+ iov.iov_base = req->rq_snd_buf.head[0].iov_base;
iov.iov_len = (u8 *)p - (u8 *)iov.iov_base;
xdr_buf_from_iov(&iov, &verf_buf);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d7ec613..c4203f6 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2331,7 +2331,6 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
/* FIXME: check buffer size? */
- p = xprt_skip_transport_header(req->rq_xprt, p);
*p++ = req->rq_xid; /* XID */
*p++ = htonl(RPC_CALL); /* CALL */
*p++ = htonl(RPC_VERSION); /* RPC version */
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index e87ddb9..dbd1969 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1145,17 +1145,6 @@ static __printf(2,3) void svc_printk(struct svc_rqst *rqstp, const char *fmt, ..
#endif
/*
- * Setup response header for TCP, it has a 4B record length field.
- */
-static void svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
-{
- struct kvec *resv = &rqstp->rq_res.head[0];
-
- /* tcp needs a space for the record length... */
- svc_putnl(resv, 0);
-}
-
-/*
* Common routine for processing the RPC request.
*/
static int
@@ -1182,10 +1171,6 @@ static void svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
set_bit(RQ_USEDEFERRAL, &rqstp->rq_flags);
clear_bit(RQ_DROPME, &rqstp->rq_flags);
- /* Setup reply header */
- if (rqstp->rq_prot == IPPROTO_TCP)
- svc_tcp_prep_reply_hdr(rqstp);
-
svc_putu32(resv, rqstp->rq_xid);
vers = svc_getnl(argv);
@@ -1443,6 +1428,10 @@ static void svc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
goto out_drop;
}
+ /* Reserve space for the record marker */
+ if (rqstp->rq_prot == IPPROTO_TCP)
+ svc_putnl(resv, 0);
+
/* Returns 1 for send, 0 for drop */
if (likely(svc_process_common(rqstp, argv, resv)))
return svc_send(rqstp);
diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index b908f2c..907464c 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -304,7 +304,6 @@ static int svc_rdma_bc_sendto(struct svcxprt_rdma *rdma,
xprt->idle_timeout = RPCRDMA_IDLE_DISC_TO;
xprt->prot = XPRT_TRANSPORT_BC_RDMA;
- xprt->tsh_size = 0;
xprt->ops = &xprt_rdma_bc_procs;
memcpy(&xprt->addr, args->dstaddr, args->addrlen);
diff --git a/net/sunrpc/xprtrdma/transport.c b/net/sunrpc/xprtrdma/transport.c
index fbc171e..e7274dc 100644
--- a/net/sunrpc/xprtrdma/transport.c
+++ b/net/sunrpc/xprtrdma/transport.c
@@ -332,7 +332,6 @@
xprt->idle_timeout = RPCRDMA_IDLE_DISC_TO;
xprt->resvport = 0; /* privileged port not needed */
- xprt->tsh_size = 0; /* RPC-RDMA handles framing */
xprt->ops = &xprt_rdma_procs;
/*
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 7754aa3..ae09d85 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -696,6 +696,40 @@ static void xs_stream_data_receive_workfn(struct work_struct *work)
#define XS_SENDMSG_FLAGS (MSG_DONTWAIT | MSG_NOSIGNAL)
+/* Common case:
+ * - stream transport
+ * - sending from byte 0 of the message
+ * - the message is wholly contained in @xdr's head iovec
+ */
+static int xs_send_rm_and_kvec(struct socket *sock, struct xdr_buf *xdr,
+ unsigned int remainder)
+{
+ struct msghdr msg = {
+ .msg_flags = XS_SENDMSG_FLAGS | (remainder ? MSG_MORE : 0)
+ };
+ rpc_fraghdr marker = cpu_to_be32(RPC_LAST_STREAM_FRAGMENT |
+ (u32)xdr->len);
+ struct kvec iov[2] = {
+ {
+ .iov_base = &marker,
+ .iov_len = sizeof(marker)
+ },
+ {
+ .iov_base = xdr->head[0].iov_base,
+ .iov_len = xdr->head[0].iov_len
+ },
+ };
+ int ret;
+
+ ret = kernel_sendmsg(sock, &msg, iov, 2,
+ iov[0].iov_len + iov[1].iov_len);
+ if (ret < 0)
+ return ret;
+ if (ret < iov[0].iov_len)
+ return -EPIPE;
+ return ret - iov[0].iov_len;
+}
+
static int xs_send_kvec(struct socket *sock, struct sockaddr *addr, int addrlen, struct kvec *vec, unsigned int base, int more)
{
struct msghdr msg = {
@@ -779,7 +813,11 @@ static int xs_sendpages(struct socket *sock, struct sockaddr *addr, int addrlen,
if (base < xdr->head[0].iov_len || addr != NULL) {
unsigned int len = xdr->head[0].iov_len - base;
remainder -= len;
- err = xs_send_kvec(sock, addr, addrlen, &xdr->head[0], base, remainder != 0);
+ if (!base && !addr)
+ err = xs_send_rm_and_kvec(sock, xdr, remainder);
+ else
+ err = xs_send_kvec(sock, addr, addrlen, &xdr->head[0],
+ base, remainder != 0);
if (remainder == 0 || err != len)
goto out;
*sent_p += err;
@@ -869,16 +907,6 @@ static int xs_nospace(struct rpc_rqst *req)
return transport->xmit.offset != 0 && req->rq_bytes_sent == 0;
}
-/*
- * Construct a stream transport record marker in @buf.
- */
-static inline void xs_encode_stream_record_marker(struct xdr_buf *buf)
-{
- u32 reclen = buf->len - sizeof(rpc_fraghdr);
- rpc_fraghdr *base = buf->head[0].iov_base;
- *base = cpu_to_be32(RPC_LAST_STREAM_FRAGMENT | reclen);
-}
-
/**
* xs_local_send_request - write an RPC request to an AF_LOCAL socket
* @req: pointer to RPC request
@@ -905,8 +933,6 @@ static int xs_local_send_request(struct rpc_rqst *req)
return -ENOTCONN;
}
- xs_encode_stream_record_marker(&req->rq_snd_buf);
-
xs_pktdump("packet data:",
req->rq_svec->iov_base, req->rq_svec->iov_len);
@@ -1057,8 +1083,6 @@ static int xs_tcp_send_request(struct rpc_rqst *req)
return -ENOTCONN;
}
- xs_encode_stream_record_marker(&req->rq_snd_buf);
-
xs_pktdump("packet data:",
req->rq_svec->iov_base,
req->rq_svec->iov_len);
@@ -2534,26 +2558,35 @@ static int bc_sendto(struct rpc_rqst *req)
{
int len;
struct xdr_buf *xbufp = &req->rq_snd_buf;
- struct rpc_xprt *xprt = req->rq_xprt;
struct sock_xprt *transport =
- container_of(xprt, struct sock_xprt, xprt);
- struct socket *sock = transport->sock;
+ container_of(req->rq_xprt, struct sock_xprt, xprt);
unsigned long headoff;
unsigned long tailoff;
+ struct page *tailpage;
+ struct msghdr msg = {
+ .msg_flags = MSG_MORE
+ };
+ rpc_fraghdr marker = cpu_to_be32(RPC_LAST_STREAM_FRAGMENT |
+ (u32)xbufp->len);
+ struct kvec iov = {
+ .iov_base = &marker,
+ .iov_len = sizeof(marker),
+ };
- xs_encode_stream_record_marker(xbufp);
+ len = kernel_sendmsg(transport->sock, &msg, &iov, 1, iov.iov_len);
+ if (len != iov.iov_len)
+ return -EAGAIN;
+ tailpage = NULL;
+ if (xbufp->tail[0].iov_len)
+ tailpage = virt_to_page(xbufp->tail[0].iov_base);
tailoff = (unsigned long)xbufp->tail[0].iov_base & ~PAGE_MASK;
headoff = (unsigned long)xbufp->head[0].iov_base & ~PAGE_MASK;
- len = svc_send_common(sock, xbufp,
+ len = svc_send_common(transport->sock, xbufp,
virt_to_page(xbufp->head[0].iov_base), headoff,
- xbufp->tail[0].iov_base, tailoff);
-
- if (len != xbufp->len) {
- printk(KERN_NOTICE "Error sending entire callback!\n");
- len = -EAGAIN;
- }
-
+ tailpage, tailoff);
+ if (len != xbufp->len)
+ return -EAGAIN;
return len;
}
@@ -2793,7 +2826,6 @@ static struct rpc_xprt *xs_setup_local(struct xprt_create *args)
transport = container_of(xprt, struct sock_xprt, xprt);
xprt->prot = 0;
- xprt->tsh_size = sizeof(rpc_fraghdr) / sizeof(u32);
xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
xprt->bind_timeout = XS_BIND_TO;
@@ -2862,7 +2894,6 @@ static struct rpc_xprt *xs_setup_udp(struct xprt_create *args)
transport = container_of(xprt, struct sock_xprt, xprt);
xprt->prot = IPPROTO_UDP;
- xprt->tsh_size = 0;
/* XXX: header size can vary due to auth type, IPv6, etc. */
xprt->max_payload = (1U << 16) - (MAX_HEADER << 3);
@@ -2942,7 +2973,6 @@ static struct rpc_xprt *xs_setup_tcp(struct xprt_create *args)
transport = container_of(xprt, struct sock_xprt, xprt);
xprt->prot = IPPROTO_TCP;
- xprt->tsh_size = sizeof(rpc_fraghdr) / sizeof(u32);
xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
xprt->bind_timeout = XS_BIND_TO;
@@ -3015,7 +3045,6 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
transport = container_of(xprt, struct sock_xprt, xprt);
xprt->prot = IPPROTO_TCP;
- xprt->tsh_size = sizeof(rpc_fraghdr) / sizeof(u32);
xprt->max_payload = RPC_MAX_FRAGMENT_SIZE;
xprt->timeout = &xs_tcp_default_timeout;
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 03/10] SUNRPC: Add build option to disable support for insecure enctypes
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 02/10] SUNRPC: Remove rpc_xprt::tsh_size Chuck Lever
@ 2019-02-01 19:57 ` Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants Chuck Lever
` (6 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Enable distributions to enforce the rejection of ancient and
insecure Kerberos enctypes in the kernel's RPCSEC_GSS
implementation. These are the single-DES encryption types that
were deprecated in 2012 by RFC 6649.
Enctypes that were deprecated more recently (by RFC 8429) remain
fully supported for now because they are still likely to be widely
used.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Acked-by: Simo Sorce <simo@redhat.com>
---
include/linux/sunrpc/gss_krb5_enctypes.h | 42 +++++++++++++++++++++++++++++-
net/sunrpc/Kconfig | 16 +++++++++++
net/sunrpc/auth_gss/gss_krb5_mech.c | 2 +
3 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/include/linux/sunrpc/gss_krb5_enctypes.h b/include/linux/sunrpc/gss_krb5_enctypes.h
index ec6234e..981c89c 100644
--- a/include/linux/sunrpc/gss_krb5_enctypes.h
+++ b/include/linux/sunrpc/gss_krb5_enctypes.h
@@ -1,4 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 */
/*
- * Dumb way to share this static piece of information with nfsd
+ * Define the string that exports the set of kernel-supported
+ * Kerberos enctypes. This list is sent via upcall to gssd, and
+ * is also exposed via the nfsd /proc API. The consumers generally
+ * treat this as an ordered list, where the first item in the list
+ * is the most preferred.
+ */
+
+#ifndef _LINUX_SUNRPC_GSS_KRB5_ENCTYPES_H
+#define _LINUX_SUNRPC_GSS_KRB5_ENCTYPES_H
+
+#ifdef CONFIG_SUNRPC_DISABLE_INSECURE_ENCTYPES
+
+/*
+ * NB: This list includes encryption types that were deprecated
+ * by RFC 8429 (DES3_CBC_SHA1 and ARCFOUR_HMAC).
+ *
+ * ENCTYPE_AES256_CTS_HMAC_SHA1_96
+ * ENCTYPE_AES128_CTS_HMAC_SHA1_96
+ * ENCTYPE_DES3_CBC_SHA1
+ * ENCTYPE_ARCFOUR_HMAC
+ */
+#define KRB5_SUPPORTED_ENCTYPES "18,17,16,23"
+
+#else /* CONFIG_SUNRPC_DISABLE_INSECURE_ENCTYPES */
+
+/*
+ * NB: This list includes encryption types that were deprecated
+ * by RFC 8429 and RFC 6649.
+ *
+ * ENCTYPE_AES256_CTS_HMAC_SHA1_96
+ * ENCTYPE_AES128_CTS_HMAC_SHA1_96
+ * ENCTYPE_DES3_CBC_SHA1
+ * ENCTYPE_ARCFOUR_HMAC
+ * ENCTYPE_DES_CBC_MD5
+ * ENCTYPE_DES_CBC_CRC
+ * ENCTYPE_DES_CBC_MD4
*/
#define KRB5_SUPPORTED_ENCTYPES "18,17,16,23,3,1,2"
+
+#endif /* CONFIG_SUNRPC_DISABLE_INSECURE_ENCTYPES */
+
+#endif /* _LINUX_SUNRPC_GSS_KRB5_ENCTYPES_H */
diff --git a/net/sunrpc/Kconfig b/net/sunrpc/Kconfig
index ac09ca8..83f5617 100644
--- a/net/sunrpc/Kconfig
+++ b/net/sunrpc/Kconfig
@@ -34,6 +34,22 @@ config RPCSEC_GSS_KRB5
If unsure, say Y.
+config CONFIG_SUNRPC_DISABLE_INSECURE_ENCTYPES
+ bool "Secure RPC: Disable insecure Kerberos encryption types"
+ depends on RPCSEC_GSS_KRB5
+ default n
+ help
+ Choose Y here to disable the use of deprecated encryption types
+ with the Kerberos version 5 GSS-API mechanism (RFC 1964). The
+ deprecated encryption types include DES-CBC-MD5, DES-CBC-CRC,
+ and DES-CBC-MD4. These types were deprecated by RFC 6649 because
+ they were found to be insecure.
+
+ N is the default because many sites have deployed KDCs and
+ keytabs that contain only these deprecated encryption types.
+ Choosing Y prevents the use of known-insecure encryption types
+ but might result in compatibility problems.
+
config SUNRPC_DEBUG
bool "RPC: Enable dprintk debugging"
depends on SUNRPC && SYSCTL
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
index eab71fc..be31a58 100644
--- a/net/sunrpc/auth_gss/gss_krb5_mech.c
+++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
@@ -53,6 +53,7 @@
static struct gss_api_mech gss_kerberos_mech; /* forward declaration */
static const struct gss_krb5_enctype supported_gss_krb5_enctypes[] = {
+#ifndef CONFIG_SUNRPC_DISABLE_INSECURE_ENCTYPES
/*
* DES (All DES enctypes are mapped to the same gss functionality)
*/
@@ -74,6 +75,7 @@
.cksumlength = 8,
.keyed_cksum = 0,
},
+#endif /* CONFIG_SUNRPC_DISABLE_INSECURE_ENCTYPES */
/*
* RC4-HMAC
*/
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (2 preceding siblings ...)
2019-02-01 19:57 ` [PATCH RFC 03/10] SUNRPC: Add build option to disable support for insecure enctypes Chuck Lever
@ 2019-02-01 19:57 ` Chuck Lever
2019-02-02 2:30 ` Tom Talpey
2019-02-02 17:02 ` Christoph Hellwig
2019-02-01 19:57 ` [PATCH RFC 05/10] SUNRPC: Use struct xdr_stream when constructing RPC Call header Chuck Lever
` (5 subsequent siblings)
9 siblings, 2 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Byte-swapping causes a CPU pipeline bubble on some processors. When
a decoder is comparing an on-the-wire value for equality, byte-
swapping can be avoided by comparing it directly to a pre-byte-
swapped constant value.
The current set of pre-xdr'd constants is missing some common values
used in the RPC header. Fill those out.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/auth_gss.h | 5 ++-
include/linux/sunrpc/xdr.h | 66 ++++++++++++++++++++++++---------------
2 files changed, 45 insertions(+), 26 deletions(-)
diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h
index 30427b7..adc4be2 100644
--- a/include/linux/sunrpc/auth_gss.h
+++ b/include/linux/sunrpc/auth_gss.h
@@ -19,7 +19,10 @@
#include <linux/sunrpc/svc.h>
#include <linux/sunrpc/gss_api.h>
-#define RPC_GSS_VERSION 1
+enum {
+ RPC_GSS_VERSION = 1,
+ rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION)
+};
#define MAXSEQ 0x80000000 /* maximum legal sequence number, from rfc 2203 */
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 787939d..69161cb 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -17,6 +17,7 @@
#include <asm/byteorder.h>
#include <asm/unaligned.h>
#include <linux/scatterlist.h>
+#include <linux/sunrpc/msg_prot.h>
struct bio_vec;
struct rpc_rqst;
@@ -79,31 +80,46 @@ struct xdr_buf {
buf->buflen = len;
}
-/*
- * pre-xdr'ed macros.
- */
-
-#define xdr_zero cpu_to_be32(0)
-#define xdr_one cpu_to_be32(1)
-#define xdr_two cpu_to_be32(2)
-
-#define rpc_success cpu_to_be32(RPC_SUCCESS)
-#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
-#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
-#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL)
-#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS)
-#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ERR)
-#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPLY)
-
-#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK)
-#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED)
-#define rpc_autherr_rejectedcred cpu_to_be32(RPC_AUTH_REJECTEDCRED)
-#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF)
-#define rpc_autherr_rejectedverf cpu_to_be32(RPC_AUTH_REJECTEDVERF)
-#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK)
-#define rpcsec_gsserr_credproblem cpu_to_be32(RPCSEC_GSS_CREDPROBLEM)
-#define rpcsec_gsserr_ctxproblem cpu_to_be32(RPCSEC_GSS_CTXPROBLEM)
-#define rpc_autherr_oldseqnum cpu_to_be32(101)
+enum xdr_be32_equivalents {
+ xdr_zero = cpu_to_be32(0),
+ xdr_one = cpu_to_be32(1),
+ xdr_two = cpu_to_be32(2),
+
+ rpc_version = cpu_to_be32(RPC_VERSION),
+
+ rpc_auth_null = cpu_to_be32(RPC_AUTH_NULL),
+ rpc_auth_unix = cpu_to_be32(RPC_AUTH_UNIX),
+ rpc_auth_short = cpu_to_be32(RPC_AUTH_SHORT),
+ rpc_auth_des = cpu_to_be32(RPC_AUTH_DES),
+ rpc_auth_krb = cpu_to_be32(RPC_AUTH_KRB),
+ rpc_auth_gss = cpu_to_be32(RPC_AUTH_GSS),
+
+ rpc_call = cpu_to_be32(RPC_CALL),
+ rpc_reply = cpu_to_be32(RPC_REPLY),
+
+ rpc_msg_accepted = cpu_to_be32(RPC_MSG_ACCEPTED),
+ rpc_msg_denied = cpu_to_be32(RPC_MSG_DENIED),
+
+ rpc_success = cpu_to_be32(RPC_SUCCESS),
+ rpc_prog_unavail = cpu_to_be32(RPC_PROG_UNAVAIL),
+ rpc_prog_mismatch = cpu_to_be32(RPC_PROG_MISMATCH),
+ rpc_proc_unavail = cpu_to_be32(RPC_PROC_UNAVAIL),
+ rpc_garbage_args = cpu_to_be32(RPC_GARBAGE_ARGS),
+ rpc_system_err = cpu_to_be32(RPC_SYSTEM_ERR),
+ rpc_drop_reply = cpu_to_be32(RPC_DROP_REPLY),
+
+ rpc_mismatch = cpu_to_be32(RPC_MISMATCH),
+ rpc_auth_error = cpu_to_be32(RPC_AUTH_ERROR),
+
+ rpc_auth_ok = cpu_to_be32(RPC_AUTH_OK),
+ rpc_autherr_badcred = cpu_to_be32(RPC_AUTH_BADCRED),
+ rpc_autherr_rejectedcred = cpu_to_be32(RPC_AUTH_REJECTEDCRED),
+ rpc_autherr_badverf = cpu_to_be32(RPC_AUTH_BADVERF),
+ rpc_autherr_rejectedverf = cpu_to_be32(RPC_AUTH_REJECTEDVERF),
+ rpc_autherr_tooweak = cpu_to_be32(RPC_AUTH_TOOWEAK),
+ rpcsec_gsserr_credproblem = cpu_to_be32(RPCSEC_GSS_CREDPROBLEM),
+ rpcsec_gsserr_ctxproblem = cpu_to_be32(RPCSEC_GSS_CTXPROBLEM),
+};
/*
* Miscellaneous XDR helper functions
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 05/10] SUNRPC: Use struct xdr_stream when constructing RPC Call header
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (3 preceding siblings ...)
2019-02-01 19:57 ` [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants Chuck Lever
@ 2019-02-01 19:57 ` Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 06/10] SUNRPC: Clean up rpc_verify_header() Chuck Lever
` (4 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Modernize and harden the code path that constructs each RPC Call
message.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/auth.h | 15 ++-
include/trace/events/sunrpc.h | 29 ++++++
net/sunrpc/auth.c | 56 ++++++++----
net/sunrpc/auth_gss/auth_gss.c | 191 +++++++++++++++++++---------------------
net/sunrpc/auth_null.c | 23 +++--
net/sunrpc/auth_unix.c | 61 ++++++++-----
net/sunrpc/clnt.c | 66 +++++++-------
7 files changed, 260 insertions(+), 181 deletions(-)
diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
index eed3cb1..96e237f 100644
--- a/include/linux/sunrpc/auth.h
+++ b/include/linux/sunrpc/auth.h
@@ -131,11 +131,12 @@ struct rpc_credops {
void (*crdestroy)(struct rpc_cred *);
int (*crmatch)(struct auth_cred *, struct rpc_cred *, int);
- __be32 * (*crmarshal)(struct rpc_task *, __be32 *);
+ int (*crmarshal)(struct rpc_task *task,
+ struct xdr_stream *xdr);
int (*crrefresh)(struct rpc_task *);
__be32 * (*crvalidate)(struct rpc_task *, __be32 *);
- int (*crwrap_req)(struct rpc_task *, kxdreproc_t,
- void *, __be32 *, void *);
+ int (*crwrap_req)(struct rpc_task *task,
+ struct xdr_stream *xdr);
int (*crunwrap_resp)(struct rpc_task *, kxdrdproc_t,
void *, __be32 *, void *);
int (*crkey_timeout)(struct rpc_cred *);
@@ -165,9 +166,13 @@ int rpcauth_get_gssinfo(rpc_authflavor_t,
void rpcauth_init_cred(struct rpc_cred *, const struct auth_cred *, struct rpc_auth *, const struct rpc_credops *);
struct rpc_cred * rpcauth_lookupcred(struct rpc_auth *, int);
void put_rpccred(struct rpc_cred *);
-__be32 * rpcauth_marshcred(struct rpc_task *, __be32 *);
+int rpcauth_marshcred(struct rpc_task *task,
+ struct xdr_stream *xdr);
__be32 * rpcauth_checkverf(struct rpc_task *, __be32 *);
-int rpcauth_wrap_req(struct rpc_task *task, kxdreproc_t encode, void *rqstp, __be32 *data, void *obj);
+int rpcauth_wrap_req_encode(struct rpc_task *task,
+ struct xdr_stream *xdr);
+int rpcauth_wrap_req(struct rpc_task *task,
+ struct xdr_stream *xdr);
int rpcauth_unwrap_resp(struct rpc_task *task, kxdrdproc_t decode, void *rqstp, __be32 *data, void *obj);
bool rpcauth_xmit_need_reencode(struct rpc_task *task);
int rpcauth_refreshcred(struct rpc_task *);
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 6276508..2b3f9d1 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -213,6 +213,35 @@
DEFINE_RPC_QUEUED_EVENT(sleep);
DEFINE_RPC_QUEUED_EVENT(wakeup);
+DECLARE_EVENT_CLASS(rpc_failure,
+
+ TP_PROTO(const struct rpc_task *task),
+
+ TP_ARGS(task),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ ),
+
+ TP_printk("task:%u@%u",
+ __entry->task_id, __entry->client_id)
+);
+
+#define DEFINE_RPC_FAILURE(name) \
+ DEFINE_EVENT(rpc_failure, rpc_bad_##name, \
+ TP_PROTO( \
+ const struct rpc_task *task \
+ ), \
+ TP_ARGS(task))
+
+DEFINE_RPC_FAILURE(callhdr);
+
TRACE_EVENT(rpc_stats_latency,
TP_PROTO(
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 275e84e..add2135 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -756,12 +756,21 @@ struct rpc_cred *
}
EXPORT_SYMBOL_GPL(put_rpccred);
-__be32 *
-rpcauth_marshcred(struct rpc_task *task, __be32 *p)
+/**
+ * rpcauth_marshcred - Append RPC credential to end of @xdr
+ * @task: controlling RPC task
+ * @xdr: xdr_stream containing initial portion of RPC Call header
+ *
+ * On success, an appropriate verifier is added to @xdr, @xdr is
+ * updated to point past the verifier, and zero is returned.
+ * Otherwise, @xdr is in an undefined state and a negative errno
+ * is returned.
+ */
+int rpcauth_marshcred(struct rpc_task *task, struct xdr_stream *xdr)
{
- struct rpc_cred *cred = task->tk_rqstp->rq_cred;
+ const struct rpc_credops *ops = task->tk_rqstp->rq_cred->cr_ops;
- return cred->cr_ops->crmarshal(task, p);
+ return ops->crmarshal(task, xdr);
}
__be32 *
@@ -772,26 +781,37 @@ struct rpc_cred *
return cred->cr_ops->crvalidate(task, p);
}
-static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
- __be32 *data, void *obj)
+/**
+ * rpcauth_wrap_req_encode - XDR encode the RPC procedure
+ * @task: controlling RPC task
+ * @xdr: stream where on-the-wire bytes are to be marshalled
+ *
+ * On success, @xdr contains the encoded and wrapped message.
+ * Otherwise, @xdr is in an undefined state.
+ */
+int rpcauth_wrap_req_encode(struct rpc_task *task, struct xdr_stream *xdr)
{
- struct xdr_stream xdr;
+ kxdreproc_t encode = task->tk_msg.rpc_proc->p_encode;
- xdr_init_encode(&xdr, &rqstp->rq_snd_buf, data, rqstp);
- encode(rqstp, &xdr, obj);
+ encode(task->tk_rqstp, xdr, task->tk_msg.rpc_argp);
+ return 0;
}
+EXPORT_SYMBOL_GPL(rpcauth_wrap_req_encode);
-int
-rpcauth_wrap_req(struct rpc_task *task, kxdreproc_t encode, void *rqstp,
- __be32 *data, void *obj)
+/**
+ * rpcauth_wrap_req - XDR encode and wrap the RPC procedure
+ * @task: controlling RPC task
+ * @xdr: stream where on-the-wire bytes are to be marshalled
+ *
+ * On success, @xdr contains the encoded and wrapped message,
+ * and zero is returned. Otherwise, @xdr is in an undefined
+ * state and a negative errno is returned.
+ */
+int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
{
- struct rpc_cred *cred = task->tk_rqstp->rq_cred;
+ const struct rpc_credops *ops = task->tk_rqstp->rq_cred->cr_ops;
- if (cred->cr_ops->crwrap_req)
- return cred->cr_ops->crwrap_req(task, encode, rqstp, data, obj);
- /* By default, we encode the arguments normally. */
- rpcauth_wrap_req_encode(encode, rqstp, data, obj);
- return 0;
+ return ops->crwrap_req(task, xdr);
}
static int
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 4b52e2b..b333b1b 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1526,18 +1526,20 @@ static void gss_pipe_free(struct gss_pipe *p)
}
/*
-* Marshal credentials.
-* Maybe we should keep a cached credential for performance reasons.
-*/
-static __be32 *
-gss_marshal(struct rpc_task *task, __be32 *p)
+ * Marshal credentials.
+ *
+ * The expensive part is computing the verifier. We can't cache a
+ * pre-computed version of the verifier because the seqno, which
+ * is different every time, is included in the MIC.
+ */
+static int gss_marshal(struct rpc_task *task, struct xdr_stream *xdr)
{
struct rpc_rqst *req = task->tk_rqstp;
struct rpc_cred *cred = req->rq_cred;
struct gss_cred *gss_cred = container_of(cred, struct gss_cred,
gc_base);
struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
- __be32 *cred_len;
+ __be32 *p, *cred_len;
u32 maj_stat = 0;
struct xdr_netobj mic;
struct kvec iov;
@@ -1545,7 +1547,13 @@ static void gss_pipe_free(struct gss_pipe *p)
dprintk("RPC: %5u %s\n", task->tk_pid, __func__);
- *p++ = htonl(RPC_AUTH_GSS);
+ /* Credential */
+
+ p = xdr_reserve_space(xdr, 7 * sizeof(*p) +
+ ctx->gc_wire_ctx.len);
+ if (!p)
+ goto out_put_ctx;
+ *p++ = rpc_auth_gss;
cred_len = p++;
spin_lock(&ctx->gc_seq_lock);
@@ -1554,12 +1562,14 @@ static void gss_pipe_free(struct gss_pipe *p)
if (req->rq_seqno == MAXSEQ)
goto out_expired;
- *p++ = htonl((u32) RPC_GSS_VERSION);
- *p++ = htonl((u32) ctx->gc_proc);
- *p++ = htonl((u32) req->rq_seqno);
- *p++ = htonl((u32) gss_cred->gc_service);
+ *p++ = cpu_to_be32(RPC_GSS_VERSION);
+ *p++ = cpu_to_be32(ctx->gc_proc);
+ *p++ = cpu_to_be32(req->rq_seqno);
+ *p++ = cpu_to_be32(gss_cred->gc_service);
p = xdr_encode_netobj(p, &ctx->gc_wire_ctx);
- *cred_len = htonl((p - (cred_len + 1)) << 2);
+ *cred_len = cpu_to_be32((p - (cred_len + 1)) << 2);
+
+ /* Verifier */
/* We compute the checksum for the verifier over the xdr-encoded bytes
* starting with the xid and ending at the end of the credential: */
@@ -1567,27 +1577,27 @@ static void gss_pipe_free(struct gss_pipe *p)
iov.iov_len = (u8 *)p - (u8 *)iov.iov_base;
xdr_buf_from_iov(&iov, &verf_buf);
- /* set verifier flavor*/
- *p++ = htonl(RPC_AUTH_GSS);
-
+ p = xdr_reserve_space(xdr, sizeof(*p));
+ if (!p)
+ goto out_put_ctx;
+ *p++ = rpc_auth_gss;
mic.data = (u8 *)(p + 1);
maj_stat = gss_get_mic(ctx->gc_gss_ctx, &verf_buf, &mic);
- if (maj_stat == GSS_S_CONTEXT_EXPIRED) {
+ if (maj_stat == GSS_S_CONTEXT_EXPIRED)
goto out_expired;
- } else if (maj_stat != 0) {
- pr_warn("gss_marshal: gss_get_mic FAILED (%d)\n", maj_stat);
- task->tk_status = -EIO;
+ else if (maj_stat != 0)
+ goto out_put_ctx;
+ if (xdr_stream_encode_opaque_inline(xdr, (void **)&p, mic.len) < 0)
goto out_put_ctx;
- }
- p = xdr_encode_opaque(p, NULL, mic.len);
gss_put_ctx(ctx);
- return p;
+ return 0;
out_expired:
+ gss_put_ctx(ctx);
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
- task->tk_status = -EKEYEXPIRED;
+ return -EKEYEXPIRED;
out_put_ctx:
gss_put_ctx(ctx);
- return NULL;
+ return -EMSGSIZE;
}
static int gss_renew_cred(struct rpc_task *task)
@@ -1716,61 +1726,45 @@ static int gss_cred_is_negative_entry(struct rpc_cred *cred)
return ret;
}
-static void gss_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
- __be32 *p, void *obj)
-{
- struct xdr_stream xdr;
-
- xdr_init_encode(&xdr, &rqstp->rq_snd_buf, p, rqstp);
- encode(rqstp, &xdr, obj);
-}
-
-static inline int
-gss_wrap_req_integ(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
- kxdreproc_t encode, struct rpc_rqst *rqstp,
- __be32 *p, void *obj)
+static int gss_wrap_req_integ(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
+ struct rpc_task *task, struct xdr_stream *xdr)
{
- struct xdr_buf *snd_buf = &rqstp->rq_snd_buf;
- struct xdr_buf integ_buf;
- __be32 *integ_len = NULL;
+ struct rpc_rqst *rqstp = task->tk_rqstp;
+ struct xdr_buf integ_buf, *snd_buf = &rqstp->rq_snd_buf;
struct xdr_netobj mic;
- u32 offset;
- __be32 *q;
- struct kvec *iov;
- u32 maj_stat = 0;
- int status = -EIO;
+ __be32 *p, *integ_len;
+ u32 offset, maj_stat;
+ p = xdr_reserve_space(xdr, 2 * sizeof(*p));
+ if (!p)
+ goto wrap_failed;
integ_len = p++;
- offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
- *p++ = htonl(rqstp->rq_seqno);
+ *p = cpu_to_be32(rqstp->rq_seqno);
- gss_wrap_req_encode(encode, rqstp, p, obj);
+ if (rpcauth_wrap_req_encode(task, xdr))
+ goto wrap_failed;
+ offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
if (xdr_buf_subsegment(snd_buf, &integ_buf,
offset, snd_buf->len - offset))
- return status;
- *integ_len = htonl(integ_buf.len);
+ goto wrap_failed;
+ *integ_len = cpu_to_be32(integ_buf.len);
- /* guess whether we're in the head or the tail: */
- if (snd_buf->page_len || snd_buf->tail[0].iov_len)
- iov = snd_buf->tail;
- else
- iov = snd_buf->head;
- p = iov->iov_base + iov->iov_len;
+ p = xdr_reserve_space(xdr, 0);
+ if (!p)
+ goto wrap_failed;
mic.data = (u8 *)(p + 1);
-
maj_stat = gss_get_mic(ctx->gc_gss_ctx, &integ_buf, &mic);
- status = -EIO; /* XXX? */
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
else if (maj_stat)
- return status;
- q = xdr_encode_opaque(p, NULL, mic.len);
-
- offset = (u8 *)q - (u8 *)p;
- iov->iov_len += offset;
- snd_buf->len += offset;
+ goto wrap_failed;
+ /* Check that the trailing MIC fit in the buffer, after the fact */
+ if (xdr_stream_encode_opaque_inline(xdr, (void **)&p, mic.len) < 0)
+ goto wrap_failed;
return 0;
+wrap_failed:
+ return -EMSGSIZE;
}
static void
@@ -1821,61 +1815,63 @@ static void gss_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
return -EAGAIN;
}
-static inline int
-gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
- kxdreproc_t encode, struct rpc_rqst *rqstp,
- __be32 *p, void *obj)
+static int gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
+ struct rpc_task *task, struct xdr_stream *xdr)
{
+ struct rpc_rqst *rqstp = task->tk_rqstp;
struct xdr_buf *snd_buf = &rqstp->rq_snd_buf;
- u32 offset;
- u32 maj_stat;
+ u32 pad, offset, maj_stat;
int status;
- __be32 *opaque_len;
+ __be32 *p, *opaque_len;
struct page **inpages;
int first;
- int pad;
struct kvec *iov;
- char *tmp;
+ status = -EIO;
+ p = xdr_reserve_space(xdr, 2 * sizeof(*p));
+ if (!p)
+ goto wrap_failed;
opaque_len = p++;
- offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
- *p++ = htonl(rqstp->rq_seqno);
+ *p = cpu_to_be32(rqstp->rq_seqno);
- gss_wrap_req_encode(encode, rqstp, p, obj);
+ if (rpcauth_wrap_req_encode(task, xdr))
+ goto wrap_failed;
status = alloc_enc_pages(rqstp);
- if (status)
- return status;
+ if (unlikely(status))
+ goto wrap_failed;
first = snd_buf->page_base >> PAGE_SHIFT;
inpages = snd_buf->pages + first;
snd_buf->pages = rqstp->rq_enc_pages;
snd_buf->page_base -= first << PAGE_SHIFT;
/*
- * Give the tail its own page, in case we need extra space in the
- * head when wrapping:
+ * Move the tail into its own page, in case gss_wrap needs
+ * more space in the head when wrapping.
*
- * call_allocate() allocates twice the slack space required
- * by the authentication flavor to rq_callsize.
- * For GSS, slack is GSS_CRED_SLACK.
+ * Still... Why can't gss_wrap just slide the tail down?
*/
if (snd_buf->page_len || snd_buf->tail[0].iov_len) {
+ char *tmp;
+
tmp = page_address(rqstp->rq_enc_pages[rqstp->rq_enc_pages_num - 1]);
memcpy(tmp, snd_buf->tail[0].iov_base, snd_buf->tail[0].iov_len);
snd_buf->tail[0].iov_base = tmp;
}
+ status = -EIO;
+ offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
maj_stat = gss_wrap(ctx->gc_gss_ctx, offset, snd_buf, inpages);
/* slack space should prevent this ever happening: */
- BUG_ON(snd_buf->len > snd_buf->buflen);
- status = -EIO;
+ if (unlikely(snd_buf->len > snd_buf->buflen))
+ goto wrap_failed;
/* We're assuming that when GSS_S_CONTEXT_EXPIRED, the encryption was
* done anyway, so it's safe to put the request on the wire: */
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
else if (maj_stat)
- return status;
+ goto wrap_failed;
- *opaque_len = htonl(snd_buf->len - offset);
- /* guess whether we're in the head or the tail: */
+ *opaque_len = cpu_to_be32(snd_buf->len - offset);
+ /* guess whether the pad goes into the head or the tail: */
if (snd_buf->page_len || snd_buf->tail[0].iov_len)
iov = snd_buf->tail;
else
@@ -1887,37 +1883,36 @@ static void gss_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
snd_buf->len += pad;
return 0;
+wrap_failed:
+ return status;
}
-static int
-gss_wrap_req(struct rpc_task *task,
- kxdreproc_t encode, void *rqstp, __be32 *p, void *obj)
+static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
struct gss_cred *gss_cred = container_of(cred, struct gss_cred,
gc_base);
struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
- int status = -EIO;
+ int status;
dprintk("RPC: %5u %s\n", task->tk_pid, __func__);
+ status = -EIO;
if (ctx->gc_proc != RPC_GSS_PROC_DATA) {
/* The spec seems a little ambiguous here, but I think that not
* wrapping context destruction requests makes the most sense.
*/
- gss_wrap_req_encode(encode, rqstp, p, obj);
- status = 0;
+ status = rpcauth_wrap_req_encode(task, xdr);
goto out;
}
switch (gss_cred->gc_service) {
case RPC_GSS_SVC_NONE:
- gss_wrap_req_encode(encode, rqstp, p, obj);
- status = 0;
+ status = rpcauth_wrap_req_encode(task, xdr);
break;
case RPC_GSS_SVC_INTEGRITY:
- status = gss_wrap_req_integ(cred, ctx, encode, rqstp, p, obj);
+ status = gss_wrap_req_integ(cred, ctx, task, xdr);
break;
case RPC_GSS_SVC_PRIVACY:
- status = gss_wrap_req_priv(cred, ctx, encode, rqstp, p, obj);
+ status = gss_wrap_req_priv(cred, ctx, task, xdr);
break;
}
out:
diff --git a/net/sunrpc/auth_null.c b/net/sunrpc/auth_null.c
index d0ceac5..797f8472 100644
--- a/net/sunrpc/auth_null.c
+++ b/net/sunrpc/auth_null.c
@@ -59,15 +59,21 @@
/*
* Marshal credential.
*/
-static __be32 *
-nul_marshal(struct rpc_task *task, __be32 *p)
+static int
+nul_marshal(struct rpc_task *task, struct xdr_stream *xdr)
{
- *p++ = htonl(RPC_AUTH_NULL);
- *p++ = 0;
- *p++ = htonl(RPC_AUTH_NULL);
- *p++ = 0;
-
- return p;
+ __be32 *p;
+
+ p = xdr_reserve_space(xdr, 4 * sizeof(*p));
+ if (!p)
+ return -EMSGSIZE;
+ /* Credential */
+ *p++ = rpc_auth_null;
+ *p++ = xdr_zero;
+ /* Verifier */
+ *p++ = rpc_auth_null;
+ *p = xdr_zero;
+ return 0;
}
/*
@@ -125,6 +131,7 @@ struct rpc_auth null_auth = {
.crdestroy = nul_destroy_cred,
.crmatch = nul_match,
.crmarshal = nul_marshal,
+ .crwrap_req = rpcauth_wrap_req_encode,
.crrefresh = nul_refresh,
.crvalidate = nul_validate,
};
diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
index fc8a591..1d5b7ed 100644
--- a/net/sunrpc/auth_unix.c
+++ b/net/sunrpc/auth_unix.c
@@ -99,37 +99,55 @@
* Marshal credentials.
* Maybe we should keep a cached credential for performance reasons.
*/
-static __be32 *
-unx_marshal(struct rpc_task *task, __be32 *p)
+static int
+unx_marshal(struct rpc_task *task, struct xdr_stream *xdr)
{
struct rpc_clnt *clnt = task->tk_client;
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
- __be32 *base, *hold;
+ __be32 *p, *cred_len, *gidarr_len;
int i;
struct group_info *gi = cred->cr_cred->group_info;
- *p++ = htonl(RPC_AUTH_UNIX);
- base = p++;
- *p++ = htonl(jiffies/HZ);
-
- /*
- * Copy the UTS nodename captured when the client was created.
- */
- p = xdr_encode_array(p, clnt->cl_nodename, clnt->cl_nodelen);
-
- *p++ = htonl((u32) from_kuid(&init_user_ns, cred->cr_cred->fsuid));
- *p++ = htonl((u32) from_kgid(&init_user_ns, cred->cr_cred->fsgid));
- hold = p++;
+ /* Credential */
+
+ p = xdr_reserve_space(xdr, 3 * sizeof(*p));
+ if (!p)
+ goto marshal_failed;
+ *p++ = rpc_auth_unix;
+ cred_len = p++;
+ *p++ = xdr_zero; /* stamp */
+ if (xdr_stream_encode_opaque(xdr, clnt->cl_nodename,
+ clnt->cl_nodelen) < 0)
+ goto marshal_failed;
+ p = xdr_reserve_space(xdr, 3 * sizeof(*p));
+ if (!p)
+ goto marshal_failed;
+ *p++ = cpu_to_be32(from_kuid(&init_user_ns, cred->cr_cred->fsuid));
+ *p++ = cpu_to_be32(from_kgid(&init_user_ns, cred->cr_cred->fsgid));
+
+ gidarr_len = p++;
if (gi)
for (i = 0; i < UNX_NGROUPS && i < gi->ngroups; i++)
- *p++ = htonl((u32) from_kgid(&init_user_ns, gi->gid[i]));
- *hold = htonl(p - hold - 1); /* gid array length */
- *base = htonl((p - base - 1) << 2); /* cred length */
+ *p++ = cpu_to_be32(from_kgid(&init_user_ns,
+ gi->gid[i]));
+ *gidarr_len = cpu_to_be32(p - gidarr_len - 1);
+ *cred_len = cpu_to_be32((p - cred_len - 1) << 2);
+ p = xdr_reserve_space(xdr, (p - gidarr_len - 1) << 2);
+ if (!p)
+ goto marshal_failed;
+
+ /* Verifier */
+
+ p = xdr_reserve_space(xdr, 2 * sizeof(*p));
+ if (!p)
+ goto marshal_failed;
+ *p++ = rpc_auth_null;
+ *p = xdr_zero;
- *p++ = htonl(RPC_AUTH_NULL);
- *p++ = htonl(0);
+ return 0;
- return p;
+marshal_failed:
+ return -EMSGSIZE;
}
/*
@@ -202,6 +220,7 @@ struct rpc_auth unix_auth = {
.crdestroy = unx_destroy_cred,
.crmatch = unx_match,
.crmarshal = unx_marshal,
+ .crwrap_req = rpcauth_wrap_req_encode,
.crrefresh = unx_refresh,
.crvalidate = unx_validate,
};
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index c4203f6..83549c4 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -77,7 +77,8 @@
static void call_connect(struct rpc_task *task);
static void call_connect_status(struct rpc_task *task);
-static __be32 *rpc_encode_header(struct rpc_task *task);
+static int rpc_encode_header(struct rpc_task *task,
+ struct xdr_stream *xdr);
static __be32 *rpc_verify_header(struct rpc_task *task);
static int rpc_ping(struct rpc_clnt *clnt);
@@ -1728,10 +1729,7 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
rpc_xdr_encode(struct rpc_task *task)
{
struct rpc_rqst *req = task->tk_rqstp;
- kxdreproc_t encode;
- __be32 *p;
-
- dprint_status(task);
+ struct xdr_stream xdr;
xdr_buf_init(&req->rq_snd_buf,
req->rq_buffer,
@@ -1740,18 +1738,13 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
req->rq_rbuffer,
req->rq_rcvsize);
- p = rpc_encode_header(task);
- if (p == NULL)
+ req->rq_snd_buf.head[0].iov_len = 0;
+ xdr_init_encode(&xdr, &req->rq_snd_buf,
+ req->rq_snd_buf.head[0].iov_base, req);
+ if (rpc_encode_header(task, &xdr))
return;
- encode = task->tk_msg.rpc_proc->p_encode;
- if (encode == NULL)
- return;
-
- task->tk_status = rpcauth_wrap_req(task, encode, req, p,
- task->tk_msg.rpc_argp);
- if (task->tk_status == 0)
- xprt_request_prepare(req);
+ task->tk_status = rpcauth_wrap_req(task, &xdr);
}
/*
@@ -1762,6 +1755,7 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
{
if (!rpc_task_need_encode(task))
goto out;
+ dprint_status(task);
/* Encode here so that rpcsec_gss can use correct sequence number. */
rpc_xdr_encode(task);
/* Did the encode result in an error condition? */
@@ -1779,6 +1773,8 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
rpc_exit(task, task->tk_status);
}
return;
+ } else {
+ xprt_request_prepare(task->tk_rqstp);
}
/* Add task to reply queue before transmission to avoid races */
@@ -2322,25 +2318,33 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
}
}
-static __be32 *
-rpc_encode_header(struct rpc_task *task)
+static int
+rpc_encode_header(struct rpc_task *task, struct xdr_stream *xdr)
{
struct rpc_clnt *clnt = task->tk_client;
struct rpc_rqst *req = task->tk_rqstp;
- __be32 *p = req->rq_svec[0].iov_base;
-
- /* FIXME: check buffer size? */
-
- *p++ = req->rq_xid; /* XID */
- *p++ = htonl(RPC_CALL); /* CALL */
- *p++ = htonl(RPC_VERSION); /* RPC version */
- *p++ = htonl(clnt->cl_prog); /* program number */
- *p++ = htonl(clnt->cl_vers); /* program version */
- *p++ = htonl(task->tk_msg.rpc_proc->p_proc); /* procedure */
- p = rpcauth_marshcred(task, p);
- if (p)
- req->rq_slen = xdr_adjust_iovec(&req->rq_svec[0], p);
- return p;
+ __be32 *p;
+ int error;
+
+ error = -EMSGSIZE;
+ p = xdr_reserve_space(xdr, RPC_CALLHDRSIZE << 2);
+ if (!p)
+ goto out_fail;
+ *p++ = req->rq_xid;
+ *p++ = rpc_call;
+ *p++ = rpc_version;
+ *p++ = cpu_to_be32(clnt->cl_prog);
+ *p++ = cpu_to_be32(clnt->cl_vers);
+ *p = cpu_to_be32(task->tk_msg.rpc_proc->p_proc);
+
+ error = rpcauth_marshcred(task, xdr);
+ if (error < 0)
+ goto out_fail;
+ return 0;
+out_fail:
+ trace_rpc_bad_callhdr(task);
+ rpc_exit(task, error);
+ return error;
}
static __be32 *
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 06/10] SUNRPC: Clean up rpc_verify_header()
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (4 preceding siblings ...)
2019-02-01 19:57 ` [PATCH RFC 05/10] SUNRPC: Use struct xdr_stream when constructing RPC Call header Chuck Lever
@ 2019-02-01 19:57 ` Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 07/10] SUNRPC: Use struct xdr_stream when decoding RPC Reply header Chuck Lever
` (3 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:57 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
- Recover some instruction count because I'm about to introduce a
few xdr_inline_decode call sites
- Replace dprintk() call sites with trace points
- Reduce the hot path so it fits in fewer cachelines
I've also renamed it rpc_decode_header() to match everything else
in the RPC client.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/trace/events/sunrpc.h | 52 ++++++++++
net/sunrpc/clnt.c | 223 ++++++++++++++++++-----------------------
2 files changed, 148 insertions(+), 127 deletions(-)
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 2b3f9d1..0531fc4 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -241,6 +241,58 @@
TP_ARGS(task))
DEFINE_RPC_FAILURE(callhdr);
+DEFINE_RPC_FAILURE(verifier);
+
+DECLARE_EVENT_CLASS(rpc_reply_event,
+
+ TP_PROTO(
+ const struct rpc_task *task
+ ),
+
+ TP_ARGS(task),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, xid)
+ __string(progname, task->tk_client->cl_program->name)
+ __field(u32, version)
+ __string(procname, rpc_proc_name(task))
+ __string(servername, task->tk_xprt->servername)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ __entry->xid = be32_to_cpu(task->tk_rqstp->rq_xid);
+ __assign_str(progname, task->tk_client->cl_program->name)
+ __entry->version = task->tk_client->cl_vers;
+ __assign_str(procname, rpc_proc_name(task))
+ __assign_str(servername, task->tk_xprt->servername)
+ ),
+
+ TP_printk("task:%u@%d server=%s xid=0x%08x %sv%d %s",
+ __entry->task_id, __entry->client_id, __get_str(servername),
+ __entry->xid, __get_str(progname), __entry->version,
+ __get_str(procname))
+)
+
+#define DEFINE_RPC_REPLY_EVENT(name) \
+ DEFINE_EVENT(rpc_reply_event, rpc_##name, \
+ TP_PROTO( \
+ const struct rpc_task *task \
+ ), \
+ TP_ARGS(task))
+
+DEFINE_RPC_REPLY_EVENT(prog_unavail);
+DEFINE_RPC_REPLY_EVENT(prog_mismatch);
+DEFINE_RPC_REPLY_EVENT(proc_unavail);
+DEFINE_RPC_REPLY_EVENT(garbage_args);
+DEFINE_RPC_REPLY_EVENT(unparsable);
+DEFINE_RPC_REPLY_EVENT(mismatch);
+DEFINE_RPC_REPLY_EVENT(stale_creds);
+DEFINE_RPC_REPLY_EVENT(bad_creds);
+DEFINE_RPC_REPLY_EVENT(auth_tooweak);
TRACE_EVENT(rpc_stats_latency,
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 83549c4..2461b18 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -79,7 +79,7 @@
static int rpc_encode_header(struct rpc_task *task,
struct xdr_stream *xdr);
-static __be32 *rpc_verify_header(struct rpc_task *task);
+static __be32 *rpc_decode_header(struct rpc_task *task);
static int rpc_ping(struct rpc_clnt *clnt);
static void rpc_register_client(struct rpc_clnt *clnt)
@@ -2292,7 +2292,7 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
goto out_retry;
}
- p = rpc_verify_header(task);
+ p = rpc_decode_header(task);
if (IS_ERR(p)) {
if (p == ERR_PTR(-EAGAIN))
goto out_retry;
@@ -2308,7 +2308,7 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
return;
out_retry:
task->tk_status = 0;
- /* Note: rpc_verify_header() may have freed the RPC slot */
+ /* Note: rpc_decode_header() may have freed the RPC slot */
if (task->tk_rqstp == req) {
xdr_free_bvec(&req->rq_rcv_buf);
req->rq_reply_bytes_recvd = req->rq_rcv_buf.len = 0;
@@ -2347,164 +2347,133 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
return error;
}
-static __be32 *
-rpc_verify_header(struct rpc_task *task)
+static noinline __be32 *
+rpc_decode_header(struct rpc_task *task)
{
struct rpc_clnt *clnt = task->tk_client;
struct kvec *iov = &task->tk_rqstp->rq_rcv_buf.head[0];
int len = task->tk_rqstp->rq_rcv_buf.len >> 2;
__be32 *p = iov->iov_base;
- u32 n;
int error = -EACCES;
- if ((task->tk_rqstp->rq_rcv_buf.len & 3) != 0) {
- /* RFC-1014 says that the representation of XDR data must be a
- * multiple of four bytes
- * - if it isn't pointer subtraction in the NFS client may give
- * undefined results
- */
- dprintk("RPC: %5u %s: XDR representation not a multiple of"
- " 4 bytes: 0x%x\n", task->tk_pid, __func__,
- task->tk_rqstp->rq_rcv_buf.len);
- error = -EIO;
- goto out_err;
- }
+ /* RFC-1014 says that the representation of XDR data must be a
+ * multiple of four bytes
+ * - if it isn't pointer subtraction in the NFS client may give
+ * undefined results
+ */
+ if (task->tk_rqstp->rq_rcv_buf.len & 3)
+ goto out_badlen;
if ((len -= 3) < 0)
- goto out_overflow;
+ goto out_unparsable;
- p += 1; /* skip XID */
- if ((n = ntohl(*p++)) != RPC_REPLY) {
- dprintk("RPC: %5u %s: not an RPC reply: %x\n",
- task->tk_pid, __func__, n);
- error = -EIO;
- goto out_garbage;
- }
+ p++; /* skip XID */
+ if (*p++ != rpc_reply)
+ goto out_unparsable;
+ if (*p++ != rpc_msg_accepted)
+ goto out_msg_denied;
- if ((n = ntohl(*p++)) != RPC_MSG_ACCEPTED) {
- if (--len < 0)
- goto out_overflow;
- switch ((n = ntohl(*p++))) {
- case RPC_AUTH_ERROR:
- break;
- case RPC_MISMATCH:
- dprintk("RPC: %5u %s: RPC call version mismatch!\n",
- task->tk_pid, __func__);
- error = -EPROTONOSUPPORT;
- goto out_err;
- default:
- dprintk("RPC: %5u %s: RPC call rejected, "
- "unknown error: %x\n",
- task->tk_pid, __func__, n);
- error = -EIO;
- goto out_err;
- }
- if (--len < 0)
- goto out_overflow;
- switch ((n = ntohl(*p++))) {
- case RPC_AUTH_REJECTEDCRED:
- case RPC_AUTH_REJECTEDVERF:
- case RPCSEC_GSS_CREDPROBLEM:
- case RPCSEC_GSS_CTXPROBLEM:
- if (!task->tk_cred_retry)
- break;
- task->tk_cred_retry--;
- dprintk("RPC: %5u %s: retry stale creds\n",
- task->tk_pid, __func__);
- rpcauth_invalcred(task);
- /* Ensure we obtain a new XID! */
- xprt_release(task);
- task->tk_action = call_reserve;
- goto out_retry;
- case RPC_AUTH_BADCRED:
- case RPC_AUTH_BADVERF:
- /* possibly garbled cred/verf? */
- if (!task->tk_garb_retry)
- break;
- task->tk_garb_retry--;
- dprintk("RPC: %5u %s: retry garbled creds\n",
- task->tk_pid, __func__);
- task->tk_action = call_encode;
- goto out_retry;
- case RPC_AUTH_TOOWEAK:
- printk(KERN_NOTICE "RPC: server %s requires stronger "
- "authentication.\n",
- task->tk_xprt->servername);
- break;
- default:
- dprintk("RPC: %5u %s: unknown auth error: %x\n",
- task->tk_pid, __func__, n);
- error = -EIO;
- }
- dprintk("RPC: %5u %s: call rejected %d\n",
- task->tk_pid, __func__, n);
- goto out_err;
- }
p = rpcauth_checkverf(task, p);
- if (IS_ERR(p)) {
- error = PTR_ERR(p);
- dprintk("RPC: %5u %s: auth check failed with %d\n",
- task->tk_pid, __func__, error);
- goto out_garbage; /* bad verifier, retry */
- }
+ if (IS_ERR(p))
+ goto out_verifier;
+
len = p - (__be32 *)iov->iov_base - 1;
if (len < 0)
- goto out_overflow;
- switch ((n = ntohl(*p++))) {
- case RPC_SUCCESS:
+ goto out_unparsable;
+ switch (*p++) {
+ case rpc_success:
return p;
- case RPC_PROG_UNAVAIL:
- dprintk("RPC: %5u %s: program %u is unsupported "
- "by server %s\n", task->tk_pid, __func__,
- (unsigned int)clnt->cl_prog,
- task->tk_xprt->servername);
+ case rpc_prog_unavail:
+ trace_rpc_prog_unavail(task);
error = -EPFNOSUPPORT;
goto out_err;
- case RPC_PROG_MISMATCH:
- dprintk("RPC: %5u %s: program %u, version %u unsupported "
- "by server %s\n", task->tk_pid, __func__,
- (unsigned int)clnt->cl_prog,
- (unsigned int)clnt->cl_vers,
- task->tk_xprt->servername);
+ case rpc_prog_mismatch:
+ trace_rpc_prog_mismatch(task);
error = -EPROTONOSUPPORT;
goto out_err;
- case RPC_PROC_UNAVAIL:
- dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
- "version %u on server %s\n",
- task->tk_pid, __func__,
- rpc_proc_name(task),
- clnt->cl_prog, clnt->cl_vers,
- task->tk_xprt->servername);
+ case rpc_proc_unavail:
+ trace_rpc_proc_unavail(task);
error = -EOPNOTSUPP;
goto out_err;
- case RPC_GARBAGE_ARGS:
- dprintk("RPC: %5u %s: server saw garbage\n",
- task->tk_pid, __func__);
- break; /* retry */
+ case rpc_garbage_args:
+ trace_rpc_garbage_args(task);
+ break;
default:
- dprintk("RPC: %5u %s: server accept status: %x\n",
- task->tk_pid, __func__, n);
- /* Also retry */
+ trace_rpc_unparsable(task);
}
out_garbage:
clnt->cl_stats->rpcgarbage++;
if (task->tk_garb_retry) {
task->tk_garb_retry--;
- dprintk("RPC: %5u %s: retrying\n",
- task->tk_pid, __func__);
task->tk_action = call_encode;
-out_retry:
return ERR_PTR(-EAGAIN);
}
out_err:
rpc_exit(task, error);
- dprintk("RPC: %5u %s: call failed with error %d\n", task->tk_pid,
- __func__, error);
return ERR_PTR(error);
-out_overflow:
- dprintk("RPC: %5u %s: server reply was truncated.\n", task->tk_pid,
- __func__);
+
+out_badlen:
+ trace_rpc_unparsable(task);
+ error = -EIO;
+ goto out_err;
+
+out_unparsable:
+ trace_rpc_unparsable(task);
+ error = -EIO;
goto out_garbage;
+
+out_verifier:
+ trace_rpc_bad_verifier(task);
+ error = PTR_ERR(p);
+ goto out_garbage;
+
+out_msg_denied:
+ switch (*p++) {
+ case rpc_auth_error:
+ break;
+ case rpc_mismatch:
+ trace_rpc_mismatch(task);
+ error = -EPROTONOSUPPORT;
+ goto out_err;
+ default:
+ trace_rpc_unparsable(task);
+ error = -EIO;
+ goto out_err;
+ }
+
+ switch (*p++) {
+ case rpc_autherr_rejectedcred:
+ case rpc_autherr_rejectedverf:
+ case rpcsec_gsserr_credproblem:
+ case rpcsec_gsserr_ctxproblem:
+ if (!task->tk_cred_retry)
+ break;
+ task->tk_cred_retry--;
+ trace_rpc_stale_creds(task);
+ rpcauth_invalcred(task);
+ /* Ensure we obtain a new XID! */
+ xprt_release(task);
+ task->tk_action = call_reserve;
+ return ERR_PTR(-EAGAIN);
+ case rpc_autherr_badcred:
+ case rpc_autherr_badverf:
+ /* possibly garbled cred/verf? */
+ if (!task->tk_garb_retry)
+ break;
+ task->tk_garb_retry--;
+ trace_rpc_bad_creds(task);
+ task->tk_action = call_encode;
+ return ERR_PTR(-EAGAIN);
+ case rpc_autherr_tooweak:
+ trace_rpc_auth_tooweak(task);
+ pr_warn("RPC: server %s requires stronger authentication.\n",
+ task->tk_xprt->servername);
+ break;
+ default:
+ trace_rpc_unparsable(task);
+ error = -EIO;
+ }
+ goto out_err;
}
static void rpcproc_encode_null(struct rpc_rqst *rqstp, struct xdr_stream *xdr,
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 07/10] SUNRPC: Use struct xdr_stream when decoding RPC Reply header
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (5 preceding siblings ...)
2019-02-01 19:57 ` [PATCH RFC 06/10] SUNRPC: Clean up rpc_verify_header() Chuck Lever
@ 2019-02-01 19:58 ` Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 08/10] SUNRPC: Introduce trace points in rpc_auth_gss.ko Chuck Lever
` (2 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:58 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Modernize and harden the code path that parses an RPC Reply
message.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/auth.h | 15 ++-
net/sunrpc/auth.c | 63 ++++++++----
net/sunrpc/auth_gss/auth_gss.c | 204 ++++++++++++++++++++++------------------
net/sunrpc/auth_null.c | 31 +++---
net/sunrpc/auth_unix.c | 42 +++++---
net/sunrpc/clnt.c | 88 +++++++++--------
6 files changed, 242 insertions(+), 201 deletions(-)
diff --git a/include/linux/sunrpc/auth.h b/include/linux/sunrpc/auth.h
index 96e237f..c51e189 100644
--- a/include/linux/sunrpc/auth.h
+++ b/include/linux/sunrpc/auth.h
@@ -134,11 +134,12 @@ struct rpc_credops {
int (*crmarshal)(struct rpc_task *task,
struct xdr_stream *xdr);
int (*crrefresh)(struct rpc_task *);
- __be32 * (*crvalidate)(struct rpc_task *, __be32 *);
+ int (*crvalidate)(struct rpc_task *task,
+ struct xdr_stream *xdr);
int (*crwrap_req)(struct rpc_task *task,
struct xdr_stream *xdr);
- int (*crunwrap_resp)(struct rpc_task *, kxdrdproc_t,
- void *, __be32 *, void *);
+ int (*crunwrap_resp)(struct rpc_task *task,
+ struct xdr_stream *xdr);
int (*crkey_timeout)(struct rpc_cred *);
char * (*crstringify_acceptor)(struct rpc_cred *);
bool (*crneed_reencode)(struct rpc_task *);
@@ -168,12 +169,16 @@ int rpcauth_get_gssinfo(rpc_authflavor_t,
void put_rpccred(struct rpc_cred *);
int rpcauth_marshcred(struct rpc_task *task,
struct xdr_stream *xdr);
-__be32 * rpcauth_checkverf(struct rpc_task *, __be32 *);
+int rpcauth_checkverf(struct rpc_task *task,
+ struct xdr_stream *xdr);
int rpcauth_wrap_req_encode(struct rpc_task *task,
struct xdr_stream *xdr);
int rpcauth_wrap_req(struct rpc_task *task,
struct xdr_stream *xdr);
-int rpcauth_unwrap_resp(struct rpc_task *task, kxdrdproc_t decode, void *rqstp, __be32 *data, void *obj);
+int rpcauth_unwrap_resp_decode(struct rpc_task *task,
+ struct xdr_stream *xdr);
+int rpcauth_unwrap_resp(struct rpc_task *task,
+ struct xdr_stream *xdr);
bool rpcauth_xmit_need_reencode(struct rpc_task *task);
int rpcauth_refreshcred(struct rpc_task *);
void rpcauth_invalcred(struct rpc_task *);
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index add2135..e786102 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -17,6 +17,8 @@
#include <linux/sunrpc/gss_api.h>
#include <linux/spinlock.h>
+#include <trace/events/sunrpc.h>
+
#define RPC_CREDCACHE_DEFAULT_HASHBITS (4)
struct rpc_cred_cache {
struct hlist_head *hashtable;
@@ -773,14 +775,6 @@ int rpcauth_marshcred(struct rpc_task *task, struct xdr_stream *xdr)
return ops->crmarshal(task, xdr);
}
-__be32 *
-rpcauth_checkverf(struct rpc_task *task, __be32 *p)
-{
- struct rpc_cred *cred = task->tk_rqstp->rq_cred;
-
- return cred->cr_ops->crvalidate(task, p);
-}
-
/**
* rpcauth_wrap_req_encode - XDR encode the RPC procedure
* @task: controlling RPC task
@@ -814,27 +808,52 @@ int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
return ops->crwrap_req(task, xdr);
}
-static int
-rpcauth_unwrap_req_decode(kxdrdproc_t decode, struct rpc_rqst *rqstp,
- __be32 *data, void *obj)
+/**
+ * rpcauth_checkverf - Validate verifier in RPC Reply header
+ * @task: controlling RPC task
+ * @xdr: xdr_stream containing RPC Reply header
+ *
+ * On success, @xdr is updated to point past the verifier and
+ * zero is returned. Otherwise, @xdr is in an undefined state
+ * and a negative errno is returned.
+ */
+int
+rpcauth_checkverf(struct rpc_task *task, struct xdr_stream *xdr)
{
- struct xdr_stream xdr;
+ const struct rpc_credops *ops = task->tk_rqstp->rq_cred->cr_ops;
- xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, data, rqstp);
- return decode(rqstp, &xdr, obj);
+ return ops->crvalidate(task, xdr);
}
+/**
+ * rpcauth_unwrap_resp_decode - Invoke XDR decode function
+ * @task: controlling RPC task
+ * @xdr: stream where the Reply message resides
+ *
+ * Returns zero on success; otherwise a negative errno is returned.
+ */
int
-rpcauth_unwrap_resp(struct rpc_task *task, kxdrdproc_t decode, void *rqstp,
- __be32 *data, void *obj)
+rpcauth_unwrap_resp_decode(struct rpc_task *task, struct xdr_stream *xdr)
{
- struct rpc_cred *cred = task->tk_rqstp->rq_cred;
+ kxdrdproc_t decode = task->tk_msg.rpc_proc->p_decode;
+
+ return decode(task->tk_rqstp, xdr, task->tk_msg.rpc_resp);
+}
+EXPORT_SYMBOL_GPL(rpcauth_unwrap_resp_decode);
+
+/**
+ * rpcauth_unwrap_resp - Invoke unwrap and decode function for the cred
+ * @task: controlling RPC task
+ * @xdr: stream where the Reply message resides
+ *
+ * Returns zero on success; otherwise a negative errno is returned.
+ */
+int
+rpcauth_unwrap_resp(struct rpc_task *task, struct xdr_stream *xdr)
+{
+ const struct rpc_credops *ops = task->tk_rqstp->rq_cred->cr_ops;
- if (cred->cr_ops->crunwrap_resp)
- return cred->cr_ops->crunwrap_resp(task, decode, rqstp,
- data, obj);
- /* By default, we decode the arguments normally. */
- return rpcauth_unwrap_req_decode(decode, rqstp, data, obj);
+ return ops->crunwrap_resp(task, xdr);
}
bool
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index b333b1b..206788e 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1671,59 +1671,62 @@ static int gss_cred_is_negative_entry(struct rpc_cred *cred)
return 0;
}
-static __be32 *
-gss_validate(struct rpc_task *task, __be32 *p)
+static int
+gss_validate(struct rpc_task *task, struct xdr_stream *xdr)
{
struct rpc_cred *cred = task->tk_rqstp->rq_cred;
struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
- __be32 *seq = NULL;
+ __be32 *p, *seq = NULL;
struct kvec iov;
struct xdr_buf verf_buf;
struct xdr_netobj mic;
- u32 flav,len;
- u32 maj_stat;
- __be32 *ret = ERR_PTR(-EIO);
+ u32 len, maj_stat;
+ int status;
- dprintk("RPC: %5u %s\n", task->tk_pid, __func__);
+ p = xdr_inline_decode(xdr, 2 * sizeof(*p));
+ if (!p)
+ goto validate_failed;
+ if (*p++ != rpc_auth_gss)
+ goto validate_failed;
+ len = be32_to_cpup(p);
+ if (len > RPC_MAX_AUTH_SIZE)
+ goto validate_failed;
+ p = xdr_inline_decode(xdr, len);
+ if (!p)
+ goto validate_failed;
- flav = ntohl(*p++);
- if ((len = ntohl(*p++)) > RPC_MAX_AUTH_SIZE)
- goto out_bad;
- if (flav != RPC_AUTH_GSS)
- goto out_bad;
seq = kmalloc(4, GFP_NOFS);
if (!seq)
- goto out_bad;
- *seq = htonl(task->tk_rqstp->rq_seqno);
+ goto validate_failed;
+ *seq = cpu_to_be32(task->tk_rqstp->rq_seqno);
iov.iov_base = seq;
iov.iov_len = 4;
xdr_buf_from_iov(&iov, &verf_buf);
mic.data = (u8 *)p;
mic.len = len;
-
- ret = ERR_PTR(-EACCES);
maj_stat = gss_verify_mic(ctx->gc_gss_ctx, &verf_buf, &mic);
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
- if (maj_stat) {
- dprintk("RPC: %5u %s: gss_verify_mic returned error 0x%08x\n",
- task->tk_pid, __func__, maj_stat);
- goto out_bad;
- }
+ if (maj_stat)
+ goto bad_mic;
+
/* We leave it to unwrap to calculate au_rslack. For now we just
* calculate the length of the verifier: */
cred->cr_auth->au_verfsize = XDR_QUADLEN(len) + 2;
+ status = 0;
+out:
gss_put_ctx(ctx);
- dprintk("RPC: %5u %s: gss_verify_mic succeeded.\n",
- task->tk_pid, __func__);
- kfree(seq);
- return p + XDR_QUADLEN(len);
-out_bad:
- gss_put_ctx(ctx);
- dprintk("RPC: %5u %s failed ret %ld.\n", task->tk_pid, __func__,
- PTR_ERR(ret));
kfree(seq);
- return ret;
+ return status;
+
+validate_failed:
+ status = -EIO;
+ goto out;
+bad_mic:
+ dprintk("RPC: %5u %s: gss_verify_mic returned error 0x%08x\n",
+ task->tk_pid, __func__, maj_stat);
+ status = -EACCES;
+ goto out;
}
static int gss_wrap_req_integ(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
@@ -1921,79 +1924,98 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
return status;
}
-static inline int
+static int
+gss_unwrap_resp_auth(struct rpc_cred *cred)
+{
+ cred->cr_auth->au_rslack = cred->cr_auth->au_verfsize;
+ return 0;
+}
+
+static int
gss_unwrap_resp_integ(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
- struct rpc_rqst *rqstp, __be32 **p)
+ struct rpc_rqst *rqstp, struct xdr_stream *xdr)
{
- struct xdr_buf *rcv_buf = &rqstp->rq_rcv_buf;
- struct xdr_buf integ_buf;
+ struct xdr_buf integ_buf, *rcv_buf = &rqstp->rq_rcv_buf;
+ u32 data_offset, mic_offset, integ_len, maj_stat;
struct xdr_netobj mic;
- u32 data_offset, mic_offset;
- u32 integ_len;
- u32 maj_stat;
- int status = -EIO;
+ __be32 *p;
- integ_len = ntohl(*(*p)++);
+ p = xdr_inline_decode(xdr, 2 * sizeof(*p));
+ if (unlikely(!p))
+ goto unwrap_failed;
+ integ_len = be32_to_cpup(p++);
if (integ_len & 3)
- return status;
- data_offset = (u8 *)(*p) - (u8 *)rcv_buf->head[0].iov_base;
+ goto unwrap_failed;
+ data_offset = (u8 *)(p) - (u8 *)rcv_buf->head[0].iov_base;
mic_offset = integ_len + data_offset;
if (mic_offset > rcv_buf->len)
- return status;
- if (ntohl(*(*p)++) != rqstp->rq_seqno)
- return status;
-
- if (xdr_buf_subsegment(rcv_buf, &integ_buf, data_offset,
- mic_offset - data_offset))
- return status;
+ goto unwrap_failed;
+ if (be32_to_cpup(p) != rqstp->rq_seqno)
+ goto unwrap_failed;
+ if (xdr_buf_subsegment(rcv_buf, &integ_buf, data_offset, integ_len))
+ goto unwrap_failed;
if (xdr_buf_read_netobj(rcv_buf, &mic, mic_offset))
- return status;
-
+ goto unwrap_failed;
maj_stat = gss_verify_mic(ctx->gc_gss_ctx, &integ_buf, &mic);
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
if (maj_stat != GSS_S_COMPLETE)
- return status;
+ goto bad_mic;
+
+ cred->cr_auth->au_rslack = cred->cr_auth->au_verfsize + 2 +
+ 1 + XDR_QUADLEN(mic.len);
return 0;
+unwrap_failed:
+ return -EIO;
+bad_mic:
+ dprintk("RPC: %s: gss_verify_mic returned error 0x%08x\n",
+ __func__, maj_stat);
+ return -EIO;
}
-static inline int
+static int
gss_unwrap_resp_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
- struct rpc_rqst *rqstp, __be32 **p)
-{
- struct xdr_buf *rcv_buf = &rqstp->rq_rcv_buf;
- u32 offset;
- u32 opaque_len;
- u32 maj_stat;
- int status = -EIO;
-
- opaque_len = ntohl(*(*p)++);
- offset = (u8 *)(*p) - (u8 *)rcv_buf->head[0].iov_base;
+ struct rpc_rqst *rqstp, struct xdr_stream *xdr)
+{
+ struct xdr_buf *rcv_buf = &rqstp->rq_rcv_buf;
+ struct kvec *head = rqstp->rq_rcv_buf.head;
+ unsigned int savedlen = rcv_buf->len;
+ u32 offset, opaque_len, maj_stat;
+ __be32 *p;
+
+ p = xdr_inline_decode(xdr, 2 * sizeof(*p));
+ if (unlikely(!p))
+ goto unwrap_failed;
+ opaque_len = be32_to_cpup(p++);
+ offset = (u8 *)(p) - (u8 *)head->iov_base;
if (offset + opaque_len > rcv_buf->len)
- return status;
- /* remove padding: */
+ goto unwrap_failed;
rcv_buf->len = offset + opaque_len;
maj_stat = gss_unwrap(ctx->gc_gss_ctx, offset, rcv_buf);
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
if (maj_stat != GSS_S_COMPLETE)
- return status;
- if (ntohl(*(*p)++) != rqstp->rq_seqno)
- return status;
-
- return 0;
-}
+ goto bad_unwrap;
+ /* gss_unwrap decrypted the sequence number */
+ if (be32_to_cpup(p++) != rqstp->rq_seqno)
+ goto unwrap_failed;
-static int
-gss_unwrap_req_decode(kxdrdproc_t decode, struct rpc_rqst *rqstp,
- __be32 *p, void *obj)
-{
- struct xdr_stream xdr;
+ /* gss_unwrap redacts the opaque blob from the head iovec.
+ * rcv_buf has changed, thus the stream needs to be reset.
+ */
+ xdr_init_decode(xdr, rcv_buf, p, rqstp);
- xdr_init_decode(&xdr, &rqstp->rq_rcv_buf, p, rqstp);
- return decode(rqstp, &xdr, obj);
+ cred->cr_auth->au_rslack = cred->cr_auth->au_verfsize + 2 +
+ XDR_QUADLEN(savedlen - rcv_buf->len);
+ return 0;
+unwrap_failed:
+ return -EIO;
+bad_unwrap:
+ dprintk("RPC: %s: gss_unwrap returned error 0x%08x\n",
+ __func__, maj_stat);
+ return -EIO;
}
static bool
@@ -2037,39 +2059,33 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
}
static int
-gss_unwrap_resp(struct rpc_task *task,
- kxdrdproc_t decode, void *rqstp, __be32 *p, void *obj)
+gss_unwrap_resp(struct rpc_task *task, struct xdr_stream *xdr)
{
- struct rpc_cred *cred = task->tk_rqstp->rq_cred;
+ struct rpc_rqst *rqstp = task->tk_rqstp;
+ struct rpc_cred *cred = rqstp->rq_cred;
struct gss_cred *gss_cred = container_of(cred, struct gss_cred,
gc_base);
struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
- __be32 *savedp = p;
- struct kvec *head = ((struct rpc_rqst *)rqstp)->rq_rcv_buf.head;
- int savedlen = head->iov_len;
- int status = -EIO;
+ int status = -EIO;
if (ctx->gc_proc != RPC_GSS_PROC_DATA)
goto out_decode;
switch (gss_cred->gc_service) {
case RPC_GSS_SVC_NONE:
+ status = gss_unwrap_resp_auth(cred);
break;
case RPC_GSS_SVC_INTEGRITY:
- status = gss_unwrap_resp_integ(cred, ctx, rqstp, &p);
- if (status)
- goto out;
+ status = gss_unwrap_resp_integ(cred, ctx, rqstp, xdr);
break;
case RPC_GSS_SVC_PRIVACY:
- status = gss_unwrap_resp_priv(cred, ctx, rqstp, &p);
- if (status)
- goto out;
+ status = gss_unwrap_resp_priv(cred, ctx, rqstp, xdr);
break;
}
- /* take into account extra slack for integrity and privacy cases: */
- cred->cr_auth->au_rslack = cred->cr_auth->au_verfsize + (p - savedp)
- + (savedlen - head->iov_len);
+ if (status)
+ goto out;
+
out_decode:
- status = gss_unwrap_req_decode(decode, rqstp, p, obj);
+ status = rpcauth_unwrap_resp_decode(task, xdr);
out:
gss_put_ctx(ctx);
dprintk("RPC: %5u %s returning %d\n",
diff --git a/net/sunrpc/auth_null.c b/net/sunrpc/auth_null.c
index 797f8472..bf96975 100644
--- a/net/sunrpc/auth_null.c
+++ b/net/sunrpc/auth_null.c
@@ -86,25 +86,19 @@
return 0;
}
-static __be32 *
-nul_validate(struct rpc_task *task, __be32 *p)
+static int
+nul_validate(struct rpc_task *task, struct xdr_stream *xdr)
{
- rpc_authflavor_t flavor;
- u32 size;
-
- flavor = ntohl(*p++);
- if (flavor != RPC_AUTH_NULL) {
- printk("RPC: bad verf flavor: %u\n", flavor);
- return ERR_PTR(-EIO);
- }
-
- size = ntohl(*p++);
- if (size != 0) {
- printk("RPC: bad verf size: %u\n", size);
- return ERR_PTR(-EIO);
- }
-
- return p;
+ __be32 *p;
+
+ p = xdr_inline_decode(xdr, 2 * sizeof(*p));
+ if (!p)
+ return -EIO;
+ if (*p++ != rpc_auth_null)
+ return -EIO;
+ if (*p != xdr_zero)
+ return -EIO;
+ return 0;
}
const struct rpc_authops authnull_ops = {
@@ -134,6 +128,7 @@ struct rpc_auth null_auth = {
.crwrap_req = rpcauth_wrap_req_encode,
.crrefresh = nul_refresh,
.crvalidate = nul_validate,
+ .crunwrap_resp = rpcauth_unwrap_resp_decode,
};
static
diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
index 1d5b7ed..5ea84a9 100644
--- a/net/sunrpc/auth_unix.c
+++ b/net/sunrpc/auth_unix.c
@@ -160,29 +160,32 @@
return 0;
}
-static __be32 *
-unx_validate(struct rpc_task *task, __be32 *p)
+static int
+unx_validate(struct rpc_task *task, struct xdr_stream *xdr)
{
- rpc_authflavor_t flavor;
- u32 size;
-
- flavor = ntohl(*p++);
- if (flavor != RPC_AUTH_NULL &&
- flavor != RPC_AUTH_UNIX &&
- flavor != RPC_AUTH_SHORT) {
- printk("RPC: bad verf flavor: %u\n", flavor);
- return ERR_PTR(-EIO);
- }
+ __be32 *p;
+ u32 size;
- size = ntohl(*p++);
- if (size > RPC_MAX_AUTH_SIZE) {
- printk("RPC: giant verf size: %u\n", size);
- return ERR_PTR(-EIO);
+ p = xdr_inline_decode(xdr, 2 * sizeof(*p));
+ if (!p)
+ return -EIO;
+ switch (*p++) {
+ case rpc_auth_null:
+ case rpc_auth_unix:
+ case rpc_auth_short:
+ break;
+ default:
+ return -EIO;
}
- task->tk_rqstp->rq_cred->cr_auth->au_rslack = (size >> 2) + 2;
- p += (size >> 2);
+ size = be32_to_cpup(p);
+ if (size > RPC_MAX_AUTH_SIZE)
+ return -EIO;
+ p = xdr_inline_decode(xdr, size);
+ if (!p)
+ return -EIO;
- return p;
+ task->tk_rqstp->rq_cred->cr_auth->au_rslack = (size >> 2) + 2;
+ return 0;
}
int __init rpc_init_authunix(void)
@@ -223,4 +226,5 @@ struct rpc_auth unix_auth = {
.crwrap_req = rpcauth_wrap_req_encode,
.crrefresh = unx_refresh,
.crvalidate = unx_validate,
+ .crunwrap_resp = rpcauth_unwrap_resp_decode,
};
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 2461b18..8a58517 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -79,7 +79,8 @@
static int rpc_encode_header(struct rpc_task *task,
struct xdr_stream *xdr);
-static __be32 *rpc_decode_header(struct rpc_task *task);
+static int rpc_decode_header(struct rpc_task *task,
+ struct xdr_stream *xdr);
static int rpc_ping(struct rpc_clnt *clnt);
static void rpc_register_client(struct rpc_clnt *clnt)
@@ -2251,12 +2252,11 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
{
struct rpc_clnt *clnt = task->tk_client;
struct rpc_rqst *req = task->tk_rqstp;
- kxdrdproc_t decode = task->tk_msg.rpc_proc->p_decode;
- __be32 *p;
+ struct xdr_stream xdr;
dprint_status(task);
- if (!decode) {
+ if (!task->tk_msg.rpc_proc->p_decode) {
task->tk_action = rpc_exit_task;
return;
}
@@ -2292,29 +2292,27 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
goto out_retry;
}
- p = rpc_decode_header(task);
- if (IS_ERR(p)) {
- if (p == ERR_PTR(-EAGAIN))
- goto out_retry;
+ xdr_init_decode(&xdr, &req->rq_rcv_buf,
+ req->rq_rcv_buf.head[0].iov_base, req);
+ switch (rpc_decode_header(task, &xdr)) {
+ case 0:
+ task->tk_action = rpc_exit_task;
+ task->tk_status = rpcauth_unwrap_resp(task, &xdr);
+ dprintk("RPC: %5u %s result %d\n",
+ task->tk_pid, __func__, task->tk_status);
return;
- }
- task->tk_action = rpc_exit_task;
-
- task->tk_status = rpcauth_unwrap_resp(task, decode, req, p,
- task->tk_msg.rpc_resp);
-
- dprintk("RPC: %5u call_decode result %d\n", task->tk_pid,
- task->tk_status);
- return;
+ case -EAGAIN:
out_retry:
- task->tk_status = 0;
- /* Note: rpc_decode_header() may have freed the RPC slot */
- if (task->tk_rqstp == req) {
- xdr_free_bvec(&req->rq_rcv_buf);
- req->rq_reply_bytes_recvd = req->rq_rcv_buf.len = 0;
- if (task->tk_client->cl_discrtry)
- xprt_conditional_disconnect(req->rq_xprt,
- req->rq_connect_cookie);
+ task->tk_status = 0;
+ /* Note: rpc_decode_header() may have freed the RPC slot */
+ if (task->tk_rqstp == req) {
+ xdr_free_bvec(&req->rq_rcv_buf);
+ req->rq_reply_bytes_recvd = 0;
+ req->rq_rcv_buf.len = 0;
+ if (task->tk_client->cl_discrtry)
+ xprt_conditional_disconnect(req->rq_xprt,
+ req->rq_connect_cookie);
+ }
}
}
@@ -2347,14 +2345,12 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
return error;
}
-static noinline __be32 *
-rpc_decode_header(struct rpc_task *task)
+static noinline int
+rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
{
struct rpc_clnt *clnt = task->tk_client;
- struct kvec *iov = &task->tk_rqstp->rq_rcv_buf.head[0];
- int len = task->tk_rqstp->rq_rcv_buf.len >> 2;
- __be32 *p = iov->iov_base;
int error = -EACCES;
+ __be32 *p;
/* RFC-1014 says that the representation of XDR data must be a
* multiple of four bytes
@@ -2363,25 +2359,26 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
*/
if (task->tk_rqstp->rq_rcv_buf.len & 3)
goto out_badlen;
- if ((len -= 3) < 0)
- goto out_unparsable;
+ p = xdr_inline_decode(xdr, 3 * sizeof(*p));
+ if (!p)
+ goto out_unparsable;
p++; /* skip XID */
if (*p++ != rpc_reply)
goto out_unparsable;
if (*p++ != rpc_msg_accepted)
goto out_msg_denied;
- p = rpcauth_checkverf(task, p);
- if (IS_ERR(p))
+ error = rpcauth_checkverf(task, xdr);
+ if (error)
goto out_verifier;
- len = p - (__be32 *)iov->iov_base - 1;
- if (len < 0)
+ p = xdr_inline_decode(xdr, sizeof(*p));
+ if (!p)
goto out_unparsable;
- switch (*p++) {
+ switch (*p) {
case rpc_success:
- return p;
+ return 0;
case rpc_prog_unavail:
trace_rpc_prog_unavail(task);
error = -EPFNOSUPPORT;
@@ -2406,11 +2403,11 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
if (task->tk_garb_retry) {
task->tk_garb_retry--;
task->tk_action = call_encode;
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
}
out_err:
rpc_exit(task, error);
- return ERR_PTR(error);
+ return error;
out_badlen:
trace_rpc_unparsable(task);
@@ -2424,10 +2421,12 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
out_verifier:
trace_rpc_bad_verifier(task);
- error = PTR_ERR(p);
goto out_garbage;
out_msg_denied:
+ p = xdr_inline_decode(xdr, sizeof(*p));
+ if (!p)
+ goto out_unparsable;
switch (*p++) {
case rpc_auth_error:
break;
@@ -2441,6 +2440,9 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
goto out_err;
}
+ p = xdr_inline_decode(xdr, sizeof(*p));
+ if (!p)
+ goto out_unparsable;
switch (*p++) {
case rpc_autherr_rejectedcred:
case rpc_autherr_rejectedverf:
@@ -2454,7 +2456,7 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
/* Ensure we obtain a new XID! */
xprt_release(task);
task->tk_action = call_reserve;
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
case rpc_autherr_badcred:
case rpc_autherr_badverf:
/* possibly garbled cred/verf? */
@@ -2463,7 +2465,7 @@ void rpc_force_rebind(struct rpc_clnt *clnt)
task->tk_garb_retry--;
trace_rpc_bad_creds(task);
task->tk_action = call_encode;
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
case rpc_autherr_tooweak:
trace_rpc_auth_tooweak(task);
pr_warn("RPC: server %s requires stronger authentication.\n",
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 08/10] SUNRPC: Introduce trace points in rpc_auth_gss.ko
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (6 preceding siblings ...)
2019-02-01 19:58 ` [PATCH RFC 07/10] SUNRPC: Use struct xdr_stream when decoding RPC Reply header Chuck Lever
@ 2019-02-01 19:58 ` Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim() Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 10/10] SUNRPC: Add SPDX IDs to some net/sunrpc/auth_gss/ files Chuck Lever
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:58 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Add infrastructure for trace points in the RPC_AUTH_GSS kernel
module, and add a few sample trace points. These report exceptional
or unexpected events, and observe the assignment of GSS sequence
numbers.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/trace/events/rpcgss.h | 361 ++++++++++++++++++++++++++++++++++++++++
include/trace/events/rpcrdma.h | 12 +
include/trace/events/sunrpc.h | 61 +++++++
net/sunrpc/auth_gss/Makefile | 2
net/sunrpc/auth_gss/auth_gss.c | 165 +++++++++---------
net/sunrpc/auth_gss/trace.c | 11 +
net/sunrpc/xprt.c | 10 +
7 files changed, 530 insertions(+), 92 deletions(-)
create mode 100644 include/trace/events/rpcgss.h
create mode 100644 net/sunrpc/auth_gss/trace.c
diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
new file mode 100644
index 0000000..d1f7fe1
--- /dev/null
+++ b/include/trace/events/rpcgss.h
@@ -0,0 +1,361 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018 Oracle. All rights reserved.
+ *
+ * Trace point definitions for the "rpcgss" subsystem.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM rpcgss
+
+#if !defined(_TRACE_RPCRDMA_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_RPCGSS_H
+
+#include <linux/tracepoint.h>
+
+/**
+ ** GSS-API related trace events
+ **/
+
+TRACE_DEFINE_ENUM(GSS_S_BAD_MECH);
+TRACE_DEFINE_ENUM(GSS_S_BAD_NAME);
+TRACE_DEFINE_ENUM(GSS_S_BAD_NAMETYPE);
+TRACE_DEFINE_ENUM(GSS_S_BAD_BINDINGS);
+TRACE_DEFINE_ENUM(GSS_S_BAD_STATUS);
+TRACE_DEFINE_ENUM(GSS_S_BAD_SIG);
+TRACE_DEFINE_ENUM(GSS_S_NO_CRED);
+TRACE_DEFINE_ENUM(GSS_S_NO_CONTEXT);
+TRACE_DEFINE_ENUM(GSS_S_DEFECTIVE_TOKEN);
+TRACE_DEFINE_ENUM(GSS_S_DEFECTIVE_CREDENTIAL);
+TRACE_DEFINE_ENUM(GSS_S_CREDENTIALS_EXPIRED);
+TRACE_DEFINE_ENUM(GSS_S_CONTEXT_EXPIRED);
+TRACE_DEFINE_ENUM(GSS_S_FAILURE);
+TRACE_DEFINE_ENUM(GSS_S_BAD_QOP);
+TRACE_DEFINE_ENUM(GSS_S_UNAUTHORIZED);
+TRACE_DEFINE_ENUM(GSS_S_UNAVAILABLE);
+TRACE_DEFINE_ENUM(GSS_S_DUPLICATE_ELEMENT);
+TRACE_DEFINE_ENUM(GSS_S_NAME_NOT_MN);
+TRACE_DEFINE_ENUM(GSS_S_CONTINUE_NEEDED);
+TRACE_DEFINE_ENUM(GSS_S_DUPLICATE_TOKEN);
+TRACE_DEFINE_ENUM(GSS_S_OLD_TOKEN);
+TRACE_DEFINE_ENUM(GSS_S_UNSEQ_TOKEN);
+TRACE_DEFINE_ENUM(GSS_S_GAP_TOKEN);
+
+#define show_gss_status(x) \
+ __print_flags(x, "|", \
+ { GSS_S_BAD_MECH, "GSS_S_BAD_MECH" }, \
+ { GSS_S_BAD_NAME, "GSS_S_BAD_NAME" }, \
+ { GSS_S_BAD_NAMETYPE, "GSS_S_BAD_NAMETYPE" }, \
+ { GSS_S_BAD_BINDINGS, "GSS_S_BAD_BINDINGS" }, \
+ { GSS_S_BAD_STATUS, "GSS_S_BAD_STATUS" }, \
+ { GSS_S_BAD_SIG, "GSS_S_BAD_SIG" }, \
+ { GSS_S_NO_CRED, "GSS_S_NO_CRED" }, \
+ { GSS_S_NO_CONTEXT, "GSS_S_NO_CONTEXT" }, \
+ { GSS_S_DEFECTIVE_TOKEN, "GSS_S_DEFECTIVE_TOKEN" }, \
+ { GSS_S_DEFECTIVE_CREDENTIAL, "GSS_S_DEFECTIVE_CREDENTIAL" }, \
+ { GSS_S_CREDENTIALS_EXPIRED, "GSS_S_CREDENTIALS_EXPIRED" }, \
+ { GSS_S_CONTEXT_EXPIRED, "GSS_S_CONTEXT_EXPIRED" }, \
+ { GSS_S_FAILURE, "GSS_S_FAILURE" }, \
+ { GSS_S_BAD_QOP, "GSS_S_BAD_QOP" }, \
+ { GSS_S_UNAUTHORIZED, "GSS_S_UNAUTHORIZED" }, \
+ { GSS_S_UNAVAILABLE, "GSS_S_UNAVAILABLE" }, \
+ { GSS_S_DUPLICATE_ELEMENT, "GSS_S_DUPLICATE_ELEMENT" }, \
+ { GSS_S_NAME_NOT_MN, "GSS_S_NAME_NOT_MN" }, \
+ { GSS_S_CONTINUE_NEEDED, "GSS_S_CONTINUE_NEEDED" }, \
+ { GSS_S_DUPLICATE_TOKEN, "GSS_S_DUPLICATE_TOKEN" }, \
+ { GSS_S_OLD_TOKEN, "GSS_S_OLD_TOKEN" }, \
+ { GSS_S_UNSEQ_TOKEN, "GSS_S_UNSEQ_TOKEN" }, \
+ { GSS_S_GAP_TOKEN, "GSS_S_GAP_TOKEN" })
+
+
+DECLARE_EVENT_CLASS(rpcgss_gssapi_event,
+ TP_PROTO(
+ const struct rpc_task *task,
+ u32 maj_stat
+ ),
+
+ TP_ARGS(task, maj_stat),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, maj_stat)
+
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ __entry->maj_stat = maj_stat;
+ ),
+
+ TP_printk("task:%u@%u maj_stat=%s",
+ __entry->task_id, __entry->client_id,
+ __entry->maj_stat == 0 ?
+ "GSS_S_COMPLETE" : show_gss_status(__entry->maj_stat))
+);
+
+#define DEFINE_GSSAPI_EVENT(name) \
+ DEFINE_EVENT(rpcgss_gssapi_event, rpcgss_##name, \
+ TP_PROTO( \
+ const struct rpc_task *task, \
+ u32 maj_stat \
+ ), \
+ TP_ARGS(task, maj_stat))
+
+TRACE_EVENT(rpcgss_import_ctx,
+ TP_PROTO(
+ int status
+ ),
+
+ TP_ARGS(status),
+
+ TP_STRUCT__entry(
+ __field(int, status)
+ ),
+
+ TP_fast_assign(
+ __entry->status = status;
+ ),
+
+ TP_printk("status=%d", __entry->status)
+);
+
+DEFINE_GSSAPI_EVENT(get_mic);
+DEFINE_GSSAPI_EVENT(verify_mic);
+DEFINE_GSSAPI_EVENT(wrap);
+DEFINE_GSSAPI_EVENT(unwrap);
+
+
+/**
+ ** GSS auth unwrap failures
+ **/
+
+TRACE_EVENT(rpcgss_unwrap_failed,
+ TP_PROTO(
+ const struct rpc_task *task
+ ),
+
+ TP_ARGS(task),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ ),
+
+ TP_printk("task:%u@%u", __entry->task_id, __entry->client_id)
+);
+
+TRACE_EVENT(rpcgss_bad_seqno,
+ TP_PROTO(
+ const struct rpc_task *task,
+ u32 expected,
+ u32 received
+ ),
+
+ TP_ARGS(task, expected, received),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, expected)
+ __field(u32, received)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ __entry->expected = expected;
+ __entry->received = received;
+ ),
+
+ TP_printk("task:%u@%u expected seqno %u, received seqno %u",
+ __entry->task_id, __entry->client_id,
+ __entry->expected, __entry->received)
+);
+
+TRACE_EVENT(rpcgss_seqno,
+ TP_PROTO(
+ const struct rpc_task *task
+ ),
+
+ TP_ARGS(task),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, xid)
+ __field(u32, seqno)
+ ),
+
+ TP_fast_assign(
+ const struct rpc_rqst *rqst = task->tk_rqstp;
+
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ __entry->xid = be32_to_cpu(rqst->rq_xid);
+ __entry->seqno = rqst->rq_seqno;
+ ),
+
+ TP_printk("task:%u@%u xid=0x%08x seqno=%u",
+ __entry->task_id, __entry->client_id,
+ __entry->xid, __entry->seqno)
+);
+
+TRACE_EVENT(rpcgss_need_reencode,
+ TP_PROTO(
+ const struct rpc_task *task,
+ u32 seq_xmit,
+ bool ret
+ ),
+
+ TP_ARGS(task, seq_xmit, ret),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, xid)
+ __field(u32, seq_xmit)
+ __field(u32, seqno)
+ __field(bool, ret)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ __entry->xid = be32_to_cpu(task->tk_rqstp->rq_xid);
+ __entry->seq_xmit = seq_xmit;
+ __entry->seqno = task->tk_rqstp->rq_seqno;
+ __entry->ret = ret;
+ ),
+
+ TP_printk("task:%u@%u xid=0x%08x rq_seqno=%u seq_xmit=%u reencode %sneeded",
+ __entry->task_id, __entry->client_id,
+ __entry->xid, __entry->seqno, __entry->seq_xmit,
+ __entry->ret ? "" : "un")
+);
+
+/**
+ ** gssd upcall related trace events
+ **/
+
+TRACE_EVENT(rpcgss_upcall_msg,
+ TP_PROTO(
+ const char *buf
+ ),
+
+ TP_ARGS(buf),
+
+ TP_STRUCT__entry(
+ __string(msg, buf)
+ ),
+
+ TP_fast_assign(
+ __assign_str(msg, buf)
+ ),
+
+ TP_printk("msg='%s'", __get_str(msg))
+);
+
+TRACE_EVENT(rpcgss_upcall_result,
+ TP_PROTO(
+ u32 uid,
+ int result
+ ),
+
+ TP_ARGS(uid, result),
+
+ TP_STRUCT__entry(
+ __field(u32, uid)
+ __field(int, result)
+
+ ),
+
+ TP_fast_assign(
+ __entry->uid = uid;
+ __entry->result = result;
+ ),
+
+ TP_printk("for uid %u, result=%d", __entry->uid, __entry->result)
+);
+
+TRACE_EVENT(rpcgss_context,
+ TP_PROTO(
+ unsigned long expiry,
+ unsigned long now,
+ unsigned int timeout,
+ unsigned int len,
+ const u8 *data
+ ),
+
+ TP_ARGS(expiry, now, timeout, len, data),
+
+ TP_STRUCT__entry(
+ __field(unsigned long, expiry)
+ __field(unsigned long, now)
+ __field(unsigned int, timeout)
+ __field(int, len)
+ __string(acceptor, data)
+ ),
+
+ TP_fast_assign(
+ __entry->expiry = expiry;
+ __entry->now = now;
+ __entry->timeout = timeout;
+ __entry->len = len;
+ strncpy(__get_str(acceptor), data, len);
+ ),
+
+ TP_printk("gc_expiry=%lu now=%lu timeout=%u acceptor=%.*s",
+ __entry->expiry, __entry->now, __entry->timeout,
+ __entry->len, __get_str(acceptor))
+);
+
+
+/**
+ ** Miscellaneous events
+ */
+
+TRACE_DEFINE_ENUM(RPC_AUTH_GSS_KRB5);
+TRACE_DEFINE_ENUM(RPC_AUTH_GSS_KRB5I);
+TRACE_DEFINE_ENUM(RPC_AUTH_GSS_KRB5P);
+
+#define show_pseudoflavor(x) \
+ __print_symbolic(x, \
+ { RPC_AUTH_GSS_KRB5, "RPC_AUTH_GSS_KRB5" }, \
+ { RPC_AUTH_GSS_KRB5I, "RPC_AUTH_GSS_KRB5I" }, \
+ { RPC_AUTH_GSS_KRB5P, "RPC_AUTH_GSS_KRB5P" })
+
+
+TRACE_EVENT(rpcgss_createauth,
+ TP_PROTO(
+ unsigned int flavor,
+ int error
+ ),
+
+ TP_ARGS(flavor, error),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, flavor)
+ __field(int, error)
+
+ ),
+
+ TP_fast_assign(
+ __entry->flavor = flavor;
+ __entry->error = error;
+ ),
+
+ TP_printk("flavor=%s error=%d",
+ show_pseudoflavor(__entry->flavor), __entry->error)
+);
+
+
+#endif /* _TRACE_RPCGSS_H */
+
+#include <trace/define_trace.h>
diff --git a/include/trace/events/rpcrdma.h b/include/trace/events/rpcrdma.h
index 399b1ae..962975b 100644
--- a/include/trace/events/rpcrdma.h
+++ b/include/trace/events/rpcrdma.h
@@ -521,12 +521,18 @@
TP_STRUCT__entry(
__field(const void *, req)
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
__field(int, num_sge)
__field(int, signaled)
__field(int, status)
),
TP_fast_assign(
+ const struct rpc_rqst *rqst = &req->rl_slot;
+
+ __entry->task_id = rqst->rq_task->tk_pid;
+ __entry->client_id = rqst->rq_task->tk_client->cl_clid;
__entry->req = req;
__entry->num_sge = req->rl_sendctx->sc_wr.num_sge;
__entry->signaled = req->rl_sendctx->sc_wr.send_flags &
@@ -534,9 +540,11 @@
__entry->status = status;
),
- TP_printk("req=%p, %d SGEs%s, status=%d",
+ TP_printk("task:%u@%u req=%p (%d SGE%s) %sstatus=%d",
+ __entry->task_id, __entry->client_id,
__entry->req, __entry->num_sge,
- (__entry->signaled ? ", signaled" : ""),
+ (__entry->num_sge == 1 ? "" : "s"),
+ (__entry->signaled ? "signaled " : ""),
__entry->status
)
);
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 0531fc4..a267ced 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -655,9 +655,68 @@
DEFINE_RPC_XPRT_EVENT(timer);
DEFINE_RPC_XPRT_EVENT(lookup_rqst);
-DEFINE_RPC_XPRT_EVENT(transmit);
DEFINE_RPC_XPRT_EVENT(complete_rqst);
+TRACE_EVENT(xprt_transmit,
+ TP_PROTO(
+ const struct rpc_rqst *rqst,
+ int status
+ ),
+
+ TP_ARGS(rqst, status),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, xid)
+ __field(u32, seqno)
+ __field(int, status)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = rqst->rq_task->tk_pid;
+ __entry->client_id = rqst->rq_task->tk_client->cl_clid;
+ __entry->xid = be32_to_cpu(rqst->rq_xid);
+ __entry->seqno = rqst->rq_seqno;
+ __entry->status = status;
+ ),
+
+ TP_printk(
+ "task:%u@%u xid=0x%08x seqno=%u status=%d",
+ __entry->task_id, __entry->client_id, __entry->xid,
+ __entry->seqno, __entry->status)
+);
+
+TRACE_EVENT(xprt_enq_xmit,
+ TP_PROTO(
+ const struct rpc_task *task,
+ int stage
+ ),
+
+ TP_ARGS(task, stage),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, task_id)
+ __field(unsigned int, client_id)
+ __field(u32, xid)
+ __field(u32, seqno)
+ __field(int, stage)
+ ),
+
+ TP_fast_assign(
+ __entry->task_id = task->tk_pid;
+ __entry->client_id = task->tk_client->cl_clid;
+ __entry->xid = be32_to_cpu(task->tk_rqstp->rq_xid);
+ __entry->seqno = task->tk_rqstp->rq_seqno;
+ __entry->stage = stage;
+ ),
+
+ TP_printk(
+ "task:%u@%u xid=0x%08x seqno=%u stage=%d",
+ __entry->task_id, __entry->client_id, __entry->xid,
+ __entry->seqno, __entry->stage)
+);
+
TRACE_EVENT(xprt_ping,
TP_PROTO(const struct rpc_xprt *xprt, int status),
diff --git a/net/sunrpc/auth_gss/Makefile b/net/sunrpc/auth_gss/Makefile
index c374268..4a29f4c 100644
--- a/net/sunrpc/auth_gss/Makefile
+++ b/net/sunrpc/auth_gss/Makefile
@@ -7,7 +7,7 @@ obj-$(CONFIG_SUNRPC_GSS) += auth_rpcgss.o
auth_rpcgss-y := auth_gss.o gss_generic_token.o \
gss_mech_switch.o svcauth_gss.o \
- gss_rpc_upcall.o gss_rpc_xdr.o
+ gss_rpc_upcall.o gss_rpc_xdr.o trace.o
obj-$(CONFIG_RPCSEC_GSS_KRB5) += rpcsec_gss_krb5.o
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 206788e..3d1fbd6 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -55,6 +55,8 @@
#include "../netns.h"
+#include <trace/events/rpcgss.h>
+
static const struct rpc_authops authgss_ops;
static const struct rpc_credops gss_credops;
@@ -260,6 +262,7 @@ struct gss_auth {
}
ret = gss_import_sec_context(p, seclen, gm, &ctx->gc_gss_ctx, NULL, GFP_NOFS);
if (ret < 0) {
+ trace_rpcgss_import_ctx(ret);
p = ERR_PTR(ret);
goto err;
}
@@ -275,12 +278,9 @@ struct gss_auth {
if (IS_ERR(p))
goto err;
done:
- dprintk("RPC: %s Success. gc_expiry %lu now %lu timeout %u acceptor %.*s\n",
- __func__, ctx->gc_expiry, now, timeout, ctx->gc_acceptor.len,
- ctx->gc_acceptor.data);
- return p;
+ trace_rpcgss_context(ctx->gc_expiry, now, timeout,
+ ctx->gc_acceptor.len, ctx->gc_acceptor.data);
err:
- dprintk("RPC: %s returns error %ld\n", __func__, -PTR_ERR(p));
return p;
}
@@ -354,10 +354,8 @@ static void put_pipe_version(struct net *net)
if (auth && pos->auth->service != auth->service)
continue;
refcount_inc(&pos->count);
- dprintk("RPC: %s found msg %p\n", __func__, pos);
return pos;
}
- dprintk("RPC: %s found nothing\n", __func__);
return NULL;
}
@@ -456,7 +454,7 @@ static int gss_encode_v1_msg(struct gss_upcall_msg *gss_msg,
size_t buflen = sizeof(gss_msg->databuf);
int len;
- len = scnprintf(p, buflen, "mech=%s uid=%d ", mech->gm_name,
+ len = scnprintf(p, buflen, "mech=%s uid=%d", mech->gm_name,
from_kuid(&init_user_ns, gss_msg->uid));
buflen -= len;
p += len;
@@ -467,7 +465,7 @@ static int gss_encode_v1_msg(struct gss_upcall_msg *gss_msg,
* identity that we are authenticating to.
*/
if (target_name) {
- len = scnprintf(p, buflen, "target=%s ", target_name);
+ len = scnprintf(p, buflen, " target=%s", target_name);
buflen -= len;
p += len;
gss_msg->msg.len += len;
@@ -487,11 +485,11 @@ static int gss_encode_v1_msg(struct gss_upcall_msg *gss_msg,
char *c = strchr(service_name, '@');
if (!c)
- len = scnprintf(p, buflen, "service=%s ",
+ len = scnprintf(p, buflen, " service=%s",
service_name);
else
len = scnprintf(p, buflen,
- "service=%.*s srchost=%s ",
+ " service=%.*s srchost=%s",
(int)(c - service_name),
service_name, c + 1);
buflen -= len;
@@ -500,17 +498,17 @@ static int gss_encode_v1_msg(struct gss_upcall_msg *gss_msg,
}
if (mech->gm_upcall_enctypes) {
- len = scnprintf(p, buflen, "enctypes=%s ",
+ len = scnprintf(p, buflen, " enctypes=%s",
mech->gm_upcall_enctypes);
buflen -= len;
p += len;
gss_msg->msg.len += len;
}
+ trace_rpcgss_upcall_msg(gss_msg->databuf);
len = scnprintf(p, buflen, "\n");
if (len == 0)
goto out_overflow;
gss_msg->msg.len += len;
-
gss_msg->msg.data = gss_msg->databuf;
return 0;
out_overflow:
@@ -603,8 +601,6 @@ static void warn_gssd(void)
struct rpc_pipe *pipe;
int err = 0;
- dprintk("RPC: %5u %s for uid %u\n",
- task->tk_pid, __func__, from_kuid(&init_user_ns, cred->cr_cred->fsuid));
gss_msg = gss_setup_upcall(gss_auth, cred);
if (PTR_ERR(gss_msg) == -EAGAIN) {
/* XXX: warning on the first, under the assumption we
@@ -612,7 +608,8 @@ static void warn_gssd(void)
warn_gssd();
task->tk_timeout = 15*HZ;
rpc_sleep_on(&pipe_version_rpc_waitqueue, task, NULL);
- return -EAGAIN;
+ err = -EAGAIN;
+ goto out;
}
if (IS_ERR(gss_msg)) {
err = PTR_ERR(gss_msg);
@@ -635,9 +632,8 @@ static void warn_gssd(void)
spin_unlock(&pipe->lock);
gss_release_msg(gss_msg);
out:
- dprintk("RPC: %5u %s for uid %u result %d\n",
- task->tk_pid, __func__,
- from_kuid(&init_user_ns, cred->cr_cred->fsuid), err);
+ trace_rpcgss_upcall_result(from_kuid(&init_user_ns,
+ cred->cr_cred->fsuid), err);
return err;
}
@@ -652,14 +648,13 @@ static void warn_gssd(void)
DEFINE_WAIT(wait);
int err;
- dprintk("RPC: %s for uid %u\n",
- __func__, from_kuid(&init_user_ns, cred->cr_cred->fsuid));
retry:
err = 0;
/* if gssd is down, just skip upcalling altogether */
if (!gssd_running(net)) {
warn_gssd();
- return -EACCES;
+ err = -EACCES;
+ goto out;
}
gss_msg = gss_setup_upcall(gss_auth, cred);
if (PTR_ERR(gss_msg) == -EAGAIN) {
@@ -700,8 +695,8 @@ static void warn_gssd(void)
finish_wait(&gss_msg->waitqueue, &wait);
gss_release_msg(gss_msg);
out:
- dprintk("RPC: %s for uid %u result %d\n",
- __func__, from_kuid(&init_user_ns, cred->cr_cred->fsuid), err);
+ trace_rpcgss_upcall_result(from_kuid(&init_user_ns,
+ cred->cr_cred->fsuid), err);
return err;
}
@@ -794,7 +789,6 @@ static void warn_gssd(void)
err:
kfree(buf);
out:
- dprintk("RPC: %s returning %zd\n", __func__, err);
return err;
}
@@ -863,8 +857,6 @@ static int gss_pipe_open_v1(struct inode *inode)
struct gss_upcall_msg *gss_msg = container_of(msg, struct gss_upcall_msg, msg);
if (msg->errno < 0) {
- dprintk("RPC: %s releasing msg %p\n",
- __func__, gss_msg);
refcount_inc(&gss_msg->count);
gss_unhash_msg(gss_msg);
if (msg->errno == -ETIMEDOUT)
@@ -1024,8 +1016,6 @@ static void gss_pipe_free(struct gss_pipe *p)
struct rpc_auth * auth;
int err = -ENOMEM; /* XXX? */
- dprintk("RPC: creating GSS authenticator for client %p\n", clnt);
-
if (!try_module_get(THIS_MODULE))
return ERR_PTR(err);
if (!(gss_auth = kmalloc(sizeof(*gss_auth), GFP_KERNEL)))
@@ -1041,10 +1031,8 @@ static void gss_pipe_free(struct gss_pipe *p)
gss_auth->net = get_net(rpc_net_ns(clnt));
err = -EINVAL;
gss_auth->mech = gss_mech_get_by_pseudoflavor(flavor);
- if (!gss_auth->mech) {
- dprintk("RPC: Pseudoflavor %d not found!\n", flavor);
+ if (!gss_auth->mech)
goto err_put_net;
- }
gss_auth->service = gss_pseudoflavor_to_service(gss_auth->mech, flavor);
if (gss_auth->service == 0)
goto err_put_mech;
@@ -1099,6 +1087,7 @@ static void gss_pipe_free(struct gss_pipe *p)
kfree(gss_auth);
out_dec:
module_put(THIS_MODULE);
+ trace_rpcgss_createauth(flavor, err);
return ERR_PTR(err);
}
@@ -1135,9 +1124,6 @@ static void gss_pipe_free(struct gss_pipe *p)
struct gss_auth *gss_auth = container_of(auth,
struct gss_auth, rpc_auth);
- dprintk("RPC: destroying GSS authenticator %p flavor %d\n",
- auth, auth->au_flavor);
-
if (hash_hashed(&gss_auth->hash)) {
spin_lock(&gss_auth_hash_lock);
hash_del(&gss_auth->hash);
@@ -1300,8 +1286,6 @@ static void gss_pipe_free(struct gss_pipe *p)
static void
gss_do_free_ctx(struct gss_cl_ctx *ctx)
{
- dprintk("RPC: %s\n", __func__);
-
gss_delete_sec_context(&ctx->gc_gss_ctx);
kfree(ctx->gc_wire_ctx.data);
kfree(ctx->gc_acceptor.data);
@@ -1324,7 +1308,6 @@ static void gss_pipe_free(struct gss_pipe *p)
static void
gss_free_cred(struct gss_cred *gss_cred)
{
- dprintk("RPC: %s cred=%p\n", __func__, gss_cred);
kfree(gss_cred);
}
@@ -1381,10 +1364,6 @@ static void gss_pipe_free(struct gss_pipe *p)
struct gss_cred *cred = NULL;
int err = -ENOMEM;
- dprintk("RPC: %s for uid %d, flavor %d\n",
- __func__, from_kuid(&init_user_ns, acred->cred->fsuid),
- auth->au_flavor);
-
if (!(cred = kzalloc(sizeof(*cred), gfp)))
goto out_err;
@@ -1400,7 +1379,6 @@ static void gss_pipe_free(struct gss_pipe *p)
return &cred->gc_base;
out_err:
- dprintk("RPC: %s failed with error %d\n", __func__, err);
return ERR_PTR(err);
}
@@ -1544,15 +1522,14 @@ static int gss_marshal(struct rpc_task *task, struct xdr_stream *xdr)
struct xdr_netobj mic;
struct kvec iov;
struct xdr_buf verf_buf;
-
- dprintk("RPC: %5u %s\n", task->tk_pid, __func__);
+ int status;
/* Credential */
p = xdr_reserve_space(xdr, 7 * sizeof(*p) +
ctx->gc_wire_ctx.len);
if (!p)
- goto out_put_ctx;
+ goto marshal_failed;
*p++ = rpc_auth_gss;
cred_len = p++;
@@ -1560,7 +1537,8 @@ static int gss_marshal(struct rpc_task *task, struct xdr_stream *xdr)
req->rq_seqno = (ctx->gc_seq < MAXSEQ) ? ctx->gc_seq++ : MAXSEQ;
spin_unlock(&ctx->gc_seq_lock);
if (req->rq_seqno == MAXSEQ)
- goto out_expired;
+ goto expired;
+ trace_rpcgss_seqno(task);
*p++ = cpu_to_be32(RPC_GSS_VERSION);
*p++ = cpu_to_be32(ctx->gc_proc);
@@ -1579,25 +1557,31 @@ static int gss_marshal(struct rpc_task *task, struct xdr_stream *xdr)
p = xdr_reserve_space(xdr, sizeof(*p));
if (!p)
- goto out_put_ctx;
+ goto marshal_failed;
*p++ = rpc_auth_gss;
mic.data = (u8 *)(p + 1);
maj_stat = gss_get_mic(ctx->gc_gss_ctx, &verf_buf, &mic);
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
- goto out_expired;
+ goto expired;
else if (maj_stat != 0)
- goto out_put_ctx;
+ goto bad_mic;
if (xdr_stream_encode_opaque_inline(xdr, (void **)&p, mic.len) < 0)
- goto out_put_ctx;
- gss_put_ctx(ctx);
- return 0;
-out_expired:
+ goto marshal_failed;
+ status = 0;
+out:
gss_put_ctx(ctx);
+ return status;
+expired:
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
- return -EKEYEXPIRED;
-out_put_ctx:
- gss_put_ctx(ctx);
- return -EMSGSIZE;
+ status = -EKEYEXPIRED;
+ goto out;
+marshal_failed:
+ status = -EMSGSIZE;
+ goto out;
+bad_mic:
+ trace_rpcgss_get_mic(task, maj_stat);
+ status = -EIO;
+ goto out;
}
static int gss_renew_cred(struct rpc_task *task)
@@ -1723,8 +1707,7 @@ static int gss_cred_is_negative_entry(struct rpc_cred *cred)
status = -EIO;
goto out;
bad_mic:
- dprintk("RPC: %5u %s: gss_verify_mic returned error 0x%08x\n",
- task->tk_pid, __func__, maj_stat);
+ trace_rpcgss_verify_mic(task, maj_stat);
status = -EACCES;
goto out;
}
@@ -1761,13 +1744,16 @@ static int gss_wrap_req_integ(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
else if (maj_stat)
- goto wrap_failed;
+ goto bad_mic;
/* Check that the trailing MIC fit in the buffer, after the fact */
if (xdr_stream_encode_opaque_inline(xdr, (void **)&p, mic.len) < 0)
goto wrap_failed;
return 0;
wrap_failed:
return -EMSGSIZE;
+bad_mic:
+ trace_rpcgss_get_mic(task, maj_stat);
+ return -EIO;
}
static void
@@ -1860,7 +1846,6 @@ static int gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
memcpy(tmp, snd_buf->tail[0].iov_base, snd_buf->tail[0].iov_len);
snd_buf->tail[0].iov_base = tmp;
}
- status = -EIO;
offset = (u8 *)p - (u8 *)snd_buf->head[0].iov_base;
maj_stat = gss_wrap(ctx->gc_gss_ctx, offset, snd_buf, inpages);
/* slack space should prevent this ever happening: */
@@ -1871,7 +1856,7 @@ static int gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
if (maj_stat == GSS_S_CONTEXT_EXPIRED)
clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
else if (maj_stat)
- goto wrap_failed;
+ goto bad_wrap;
*opaque_len = cpu_to_be32(snd_buf->len - offset);
/* guess whether the pad goes into the head or the tail: */
@@ -1888,6 +1873,9 @@ static int gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
return 0;
wrap_failed:
return status;
+bad_wrap:
+ trace_rpcgss_wrap(task, maj_stat);
+ return -EIO;
}
static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
@@ -1898,7 +1886,6 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
int status;
- dprintk("RPC: %5u %s\n", task->tk_pid, __func__);
status = -EIO;
if (ctx->gc_proc != RPC_GSS_PROC_DATA) {
/* The spec seems a little ambiguous here, but I think that not
@@ -1917,10 +1904,11 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
case RPC_GSS_SVC_PRIVACY:
status = gss_wrap_req_priv(cred, ctx, task, xdr);
break;
+ default:
+ status = -EIO;
}
out:
gss_put_ctx(ctx);
- dprintk("RPC: %5u %s returning %d\n", task->tk_pid, __func__, status);
return status;
}
@@ -1932,8 +1920,9 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
}
static int
-gss_unwrap_resp_integ(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
- struct rpc_rqst *rqstp, struct xdr_stream *xdr)
+gss_unwrap_resp_integ(struct rpc_task *task, struct rpc_cred *cred,
+ struct gss_cl_ctx *ctx, struct rpc_rqst *rqstp,
+ struct xdr_stream *xdr)
{
struct xdr_buf integ_buf, *rcv_buf = &rqstp->rq_rcv_buf;
u32 data_offset, mic_offset, integ_len, maj_stat;
@@ -1951,7 +1940,7 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
if (mic_offset > rcv_buf->len)
goto unwrap_failed;
if (be32_to_cpup(p) != rqstp->rq_seqno)
- goto unwrap_failed;
+ goto bad_seqno;
if (xdr_buf_subsegment(rcv_buf, &integ_buf, data_offset, integ_len))
goto unwrap_failed;
@@ -1967,16 +1956,20 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
1 + XDR_QUADLEN(mic.len);
return 0;
unwrap_failed:
+ trace_rpcgss_unwrap_failed(task);
+ return -EIO;
+bad_seqno:
+ trace_rpcgss_bad_seqno(task, rqstp->rq_seqno, be32_to_cpup(p));
return -EIO;
bad_mic:
- dprintk("RPC: %s: gss_verify_mic returned error 0x%08x\n",
- __func__, maj_stat);
+ trace_rpcgss_verify_mic(task, maj_stat);
return -EIO;
}
static int
-gss_unwrap_resp_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx,
- struct rpc_rqst *rqstp, struct xdr_stream *xdr)
+gss_unwrap_resp_priv(struct rpc_task *task, struct rpc_cred *cred,
+ struct gss_cl_ctx *ctx, struct rpc_rqst *rqstp,
+ struct xdr_stream *xdr)
{
struct xdr_buf *rcv_buf = &rqstp->rq_rcv_buf;
struct kvec *head = rqstp->rq_rcv_buf.head;
@@ -2000,7 +1993,7 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
goto bad_unwrap;
/* gss_unwrap decrypted the sequence number */
if (be32_to_cpup(p++) != rqstp->rq_seqno)
- goto unwrap_failed;
+ goto bad_seqno;
/* gss_unwrap redacts the opaque blob from the head iovec.
* rcv_buf has changed, thus the stream needs to be reset.
@@ -2011,10 +2004,13 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
XDR_QUADLEN(savedlen - rcv_buf->len);
return 0;
unwrap_failed:
+ trace_rpcgss_unwrap_failed(task);
+ return -EIO;
+bad_seqno:
+ trace_rpcgss_bad_seqno(task, rqstp->rq_seqno, be32_to_cpup(--p));
return -EIO;
bad_unwrap:
- dprintk("RPC: %s: gss_unwrap returned error 0x%08x\n",
- __func__, maj_stat);
+ trace_rpcgss_unwrap(task, maj_stat);
return -EIO;
}
@@ -2030,14 +2026,14 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
struct rpc_rqst *req = task->tk_rqstp;
struct rpc_cred *cred = req->rq_cred;
struct gss_cl_ctx *ctx = gss_cred_get_ctx(cred);
- u32 win, seq_xmit;
+ u32 win, seq_xmit = 0;
bool ret = true;
if (!ctx)
- return true;
+ goto out;
if (gss_seq_is_newer(req->rq_seqno, READ_ONCE(ctx->gc_seq)))
- goto out;
+ goto out_ctx;
seq_xmit = READ_ONCE(ctx->gc_seq_xmit);
while (gss_seq_is_newer(req->rq_seqno, seq_xmit)) {
@@ -2046,15 +2042,18 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
seq_xmit = cmpxchg(&ctx->gc_seq_xmit, tmp, req->rq_seqno);
if (seq_xmit == tmp) {
ret = false;
- goto out;
+ goto out_ctx;
}
}
win = ctx->gc_win;
if (win > 0)
ret = !gss_seq_is_newer(req->rq_seqno, seq_xmit - win);
-out:
+
+out_ctx:
gss_put_ctx(ctx);
+out:
+ trace_rpcgss_need_reencode(task, seq_xmit, ret);
return ret;
}
@@ -2075,10 +2074,10 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
status = gss_unwrap_resp_auth(cred);
break;
case RPC_GSS_SVC_INTEGRITY:
- status = gss_unwrap_resp_integ(cred, ctx, rqstp, xdr);
+ status = gss_unwrap_resp_integ(task, cred, ctx, rqstp, xdr);
break;
case RPC_GSS_SVC_PRIVACY:
- status = gss_unwrap_resp_priv(cred, ctx, rqstp, xdr);
+ status = gss_unwrap_resp_priv(task, cred, ctx, rqstp, xdr);
break;
}
if (status)
@@ -2088,8 +2087,6 @@ static int gss_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
status = rpcauth_unwrap_resp_decode(task, xdr);
out:
gss_put_ctx(ctx);
- dprintk("RPC: %5u %s returning %d\n",
- task->tk_pid, __func__, status);
return status;
}
diff --git a/net/sunrpc/auth_gss/trace.c b/net/sunrpc/auth_gss/trace.c
new file mode 100644
index 0000000..5576f1e
--- /dev/null
+++ b/net/sunrpc/auth_gss/trace.c
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, 2019 Oracle. All rights reserved.
+ */
+
+#include <linux/sunrpc/clnt.h>
+#include <linux/sunrpc/sched.h>
+#include <linux/sunrpc/gss_err.h>
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/rpcgss.h>
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index f1ec211..bc7489f 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1165,6 +1165,7 @@ void xprt_request_wait_receive(struct rpc_task *task)
/* Note: req is added _before_ pos */
list_add_tail(&req->rq_xmit, &pos->rq_xmit);
INIT_LIST_HEAD(&req->rq_xmit2);
+ trace_xprt_enq_xmit(task, 1);
goto out;
}
} else if (RPC_IS_SWAPPER(task)) {
@@ -1176,6 +1177,7 @@ void xprt_request_wait_receive(struct rpc_task *task)
/* Note: req is added _before_ pos */
list_add_tail(&req->rq_xmit, &pos->rq_xmit);
INIT_LIST_HEAD(&req->rq_xmit2);
+ trace_xprt_enq_xmit(task, 2);
goto out;
}
} else if (!req->rq_seqno) {
@@ -1184,11 +1186,13 @@ void xprt_request_wait_receive(struct rpc_task *task)
continue;
list_add_tail(&req->rq_xmit2, &pos->rq_xmit2);
INIT_LIST_HEAD(&req->rq_xmit);
+ trace_xprt_enq_xmit(task, 3);
goto out;
}
}
list_add_tail(&req->rq_xmit, &xprt->xmit_queue);
INIT_LIST_HEAD(&req->rq_xmit2);
+ trace_xprt_enq_xmit(task, 4);
out:
set_bit(RPC_TASK_NEED_XMIT, &task->tk_runstate);
spin_unlock(&xprt->queue_lock);
@@ -1313,8 +1317,6 @@ void xprt_end_transmit(struct rpc_task *task)
int is_retrans = RPC_WAS_SENT(task);
int status;
- dprintk("RPC: %5u xprt_transmit(%u)\n", task->tk_pid, req->rq_slen);
-
if (!req->rq_bytes_sent) {
if (xprt_request_data_received(task)) {
status = 0;
@@ -1336,9 +1338,9 @@ void xprt_end_transmit(struct rpc_task *task)
connect_cookie = xprt->connect_cookie;
status = xprt->ops->send_request(req);
- trace_xprt_transmit(xprt, req->rq_xid, status);
if (status != 0) {
req->rq_ntrans--;
+ trace_xprt_transmit(req, status);
return status;
}
@@ -1347,7 +1349,6 @@ void xprt_end_transmit(struct rpc_task *task)
xprt_inject_disconnect(xprt);
- dprintk("RPC: %5u xmit complete\n", task->tk_pid);
task->tk_flags |= RPC_TASK_SENT;
spin_lock_bh(&xprt->transport_lock);
@@ -1360,6 +1361,7 @@ void xprt_end_transmit(struct rpc_task *task)
req->rq_connect_cookie = connect_cookie;
out_dequeue:
+ trace_xprt_transmit(req, status);
xprt_request_dequeue_transmit(task);
rpc_wake_up_queued_task_set_status(&xprt->sending, task, status);
return status;
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (7 preceding siblings ...)
2019-02-01 19:58 ` [PATCH RFC 08/10] SUNRPC: Introduce trace points in rpc_auth_gss.ko Chuck Lever
@ 2019-02-01 19:58 ` Chuck Lever
2019-02-04 19:46 ` J. Bruce Fields
2019-02-01 19:58 ` [PATCH RFC 10/10] SUNRPC: Add SPDX IDs to some net/sunrpc/auth_gss/ files Chuck Lever
9 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:58 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
The key action of xdr_buf_trim() is that it shortens buf->len, the
length of the xdr_buf' content. The other actions -- shortening the
head, pages, and tail components -- are actually not necessary. In
some cases, changing the size of those components corrupts the RPC
message contained in the buffer.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
include/linux/sunrpc/xdr.h | 1 -
net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++---
net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
net/sunrpc/xdr.c | 41 -----------------------------------
4 files changed, 6 insertions(+), 46 deletions(-)
diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
index 69161cb..4ae398c 100644
--- a/include/linux/sunrpc/xdr.h
+++ b/include/linux/sunrpc/xdr.h
@@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le
extern void xdr_shift_buf(struct xdr_buf *, size_t);
extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
-extern void xdr_buf_trim(struct xdr_buf *, unsigned int);
extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
index 5cdde6c..14a0aff 100644
--- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
+++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
@@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift)
*/
movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len);
movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
- BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
- buf->head[0].iov_len);
+ if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
+ buf->head[0].iov_len)
+ return GSS_S_FAILURE;
memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
/* Trim off the trailing "extra count" and checksum blob */
- xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
+ buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip;
+
return GSS_S_COMPLETE;
}
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 152790e..f1aabab 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom)
if (svc_getnl(&buf->head[0]) != seq)
goto out;
/* trim off the mic and padding at the end before returning */
- xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
+ buf->len -= 4 + round_up_to_quad(mic.len);
stat = 0;
out:
kfree(mic.data);
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 5f0aa53..4bce619 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len)
}
EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
-/**
- * xdr_buf_trim - lop at most "len" bytes off the end of "buf"
- * @buf: buf to be trimmed
- * @len: number of bytes to reduce "buf" by
- *
- * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note
- * that it's possible that we'll trim less than that amount if the xdr_buf is
- * too small, or if (for instance) it's all in the head and the parser has
- * already read too far into it.
- */
-void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
-{
- size_t cur;
- unsigned int trim = len;
-
- if (buf->tail[0].iov_len) {
- cur = min_t(size_t, buf->tail[0].iov_len, trim);
- buf->tail[0].iov_len -= cur;
- trim -= cur;
- if (!trim)
- goto fix_len;
- }
-
- if (buf->page_len) {
- cur = min_t(unsigned int, buf->page_len, trim);
- buf->page_len -= cur;
- trim -= cur;
- if (!trim)
- goto fix_len;
- }
-
- if (buf->head[0].iov_len) {
- cur = min_t(size_t, buf->head[0].iov_len, trim);
- buf->head[0].iov_len -= cur;
- trim -= cur;
- }
-fix_len:
- buf->len -= (len - trim);
-}
-EXPORT_SYMBOL_GPL(xdr_buf_trim);
-
static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len)
{
unsigned int this_len;
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH RFC 10/10] SUNRPC: Add SPDX IDs to some net/sunrpc/auth_gss/ files
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
` (8 preceding siblings ...)
2019-02-01 19:58 ` [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim() Chuck Lever
@ 2019-02-01 19:58 ` Chuck Lever
9 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-01 19:58 UTC (permalink / raw)
To: linux-nfs; +Cc: simo
Files under net/sunrpc/auth_gss/ do not yet have SPDX ID tags.
This directory is somewhat complicated because most of these files
have license boilerplate that is not strictly GPL 2.0.
In this patch I add ID tags where there is an obvious match. The
less recognizable licenses are still under research.
For reference, SPDX IDs added in this patch correspond to the
following license text:
GPL-2.0 https://spdx.org/licenses/GPL-2.0.html
GPL-2.0+ https://spdx.org/licenses/GPL-2.0+.html
BSD-3-Clause https://spdx.org/licenses/BSD-3-Clause.html
Cc: Simo Sorce <simo@redhat.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
net/sunrpc/auth_gss/auth_gss.c | 27 +--------------------------
net/sunrpc/auth_gss/gss_krb5_mech.c | 27 +--------------------------
net/sunrpc/auth_gss/gss_mech_switch.c | 27 +--------------------------
net/sunrpc/auth_gss/gss_rpc_upcall.c | 15 +--------------
net/sunrpc/auth_gss/gss_rpc_upcall.h | 16 ++--------------
net/sunrpc/auth_gss/gss_rpc_xdr.c | 15 +--------------
net/sunrpc/auth_gss/gss_rpc_xdr.h | 17 +----------------
net/sunrpc/auth_gss/svcauth_gss.c | 1 +
8 files changed, 9 insertions(+), 136 deletions(-)
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 3d1fbd6..fda454c 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: BSD-3-Clause
/*
* linux/net/sunrpc/auth_gss/auth_gss.c
*
@@ -8,34 +9,8 @@
*
* Dug Song <dugsong@monkey.org>
* Andy Adamson <andros@umich.edu>
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in the
- * documentation and/or other materials provided with the distribution.
- * 3. Neither the name of the University nor the names of its
- * contributors may be used to endorse or promote products derived
- * from this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
- * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
- * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
-
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c b/net/sunrpc/auth_gss/gss_krb5_mech.c
index be31a58..56cc85c 100644
--- a/net/sunrpc/auth_gss/gss_krb5_mech.c
+++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: BSD-3-Clause
/*
* linux/net/sunrpc/gss_krb5_mech.c
*
@@ -6,32 +7,6 @@
*
* Andy Adamson <andros@umich.edu>
* J. Bruce Fields <bfields@umich.edu>
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in the
- * documentation and/or other materials provided with the distribution.
- * 3. Neither the name of the University nor the names of its
- * contributors may be used to endorse or promote products derived
- * from this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
- * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
- * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
*/
#include <crypto/hash.h>
diff --git a/net/sunrpc/auth_gss/gss_mech_switch.c b/net/sunrpc/auth_gss/gss_mech_switch.c
index 379318d..8206009 100644
--- a/net/sunrpc/auth_gss/gss_mech_switch.c
+++ b/net/sunrpc/auth_gss/gss_mech_switch.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: BSD-3-Clause
/*
* linux/net/sunrpc/gss_mech_switch.c
*
@@ -5,32 +6,6 @@
* All rights reserved.
*
* J. Bruce Fields <bfields@umich.edu>
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- *
- * 1. Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in the
- * documentation and/or other materials provided with the distribution.
- * 3. Neither the name of the University nor the names of its
- * contributors may be used to endorse or promote products derived
- * from this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
- * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
- * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
*/
#include <linux/types.h>
diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c
index 73dcda0..0349f45 100644
--- a/net/sunrpc/auth_gss/gss_rpc_upcall.c
+++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c
@@ -1,21 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0+
/*
* linux/net/sunrpc/gss_rpc_upcall.c
*
* Copyright (C) 2012 Simo Sorce <simo@redhat.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#include <linux/types.h>
diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.h b/net/sunrpc/auth_gss/gss_rpc_upcall.h
index 1e542ad..31e9634 100644
--- a/net/sunrpc/auth_gss/gss_rpc_upcall.h
+++ b/net/sunrpc/auth_gss/gss_rpc_upcall.h
@@ -1,21 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
/*
* linux/net/sunrpc/gss_rpc_upcall.h
*
* Copyright (C) 2012 Simo Sorce <simo@redhat.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#ifndef _GSS_RPC_UPCALL_H
@@ -45,4 +32,5 @@ int gssp_accept_sec_context_upcall(struct net *net,
void init_gssp_clnt(struct sunrpc_net *);
int set_gssp_clnt(struct net *);
void clear_gssp_clnt(struct sunrpc_net *);
+
#endif /* _GSS_RPC_UPCALL_H */
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index 006062a..2ff7b70 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -1,21 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0+
/*
* GSS Proxy upcall module
*
* Copyright (C) 2012 Simo Sorce <simo@redhat.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#include <linux/sunrpc/svcauth.h>
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.h b/net/sunrpc/auth_gss/gss_rpc_xdr.h
index 146c310..3f17411 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.h
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.h
@@ -1,21 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
/*
* GSS Proxy upcall module
*
* Copyright (C) 2012 Simo Sorce <simo@redhat.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#ifndef _LINUX_GSS_RPC_XDR_H
@@ -262,6 +249,4 @@ int gssx_dec_accept_sec_context(struct rpc_rqst *rqstp,
#define GSSX_ARG_wrap_size_limit_sz 0
#define GSSX_RES_wrap_size_limit_sz 0
-
-
#endif /* _LINUX_GSS_RPC_XDR_H */
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index f1aabab..0c5d789 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* Neil Brown <neilb@cse.unsw.edu.au>
* J. Bruce Fields <bfields@umich.edu>
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-01 19:57 ` [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants Chuck Lever
@ 2019-02-02 2:30 ` Tom Talpey
2019-02-02 22:46 ` Chuck Lever
2019-02-02 17:02 ` Christoph Hellwig
1 sibling, 1 reply; 31+ messages in thread
From: Tom Talpey @ 2019-02-02 2:30 UTC (permalink / raw)
To: Chuck Lever, linux-nfs; +Cc: simo
On 2/1/2019 2:57 PM, Chuck Lever wrote:
> Byte-swapping causes a CPU pipeline bubble on some processors. When
> a decoder is comparing an on-the-wire value for equality, byte-
> swapping can be avoided by comparing it directly to a pre-byte-
> swapped constant value.
>
> The current set of pre-xdr'd constants is missing some common values
> used in the RPC header. Fill those out.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> include/linux/sunrpc/auth_gss.h | 5 ++-
> include/linux/sunrpc/xdr.h | 66 ++++++++++++++++++++++++---------------
> 2 files changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h
> index 30427b7..adc4be2 100644
> --- a/include/linux/sunrpc/auth_gss.h
> +++ b/include/linux/sunrpc/auth_gss.h
> @@ -19,7 +19,10 @@
> #include <linux/sunrpc/svc.h>
> #include <linux/sunrpc/gss_api.h>
>
> -#define RPC_GSS_VERSION 1
> +enum {
> + RPC_GSS_VERSION = 1,
> + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION)
> +};
>
> #define MAXSEQ 0x80000000 /* maximum legal sequence number, from rfc 2203 */
>
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 787939d..69161cb 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -17,6 +17,7 @@
> #include <asm/byteorder.h>
> #include <asm/unaligned.h>
> #include <linux/scatterlist.h>
> +#include <linux/sunrpc/msg_prot.h>
>
> struct bio_vec;
> struct rpc_rqst;
> @@ -79,31 +80,46 @@ struct xdr_buf {
> buf->buflen = len;
> }
>
> -/*
> - * pre-xdr'ed macros.
> - */
> -
> -#define xdr_zero cpu_to_be32(0)
> -#define xdr_one cpu_to_be32(1)
> -#define xdr_two cpu_to_be32(2)
> -
> -#define rpc_success cpu_to_be32(RPC_SUCCESS)
> -#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
> -#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
> -#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL)
> -#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS)
> -#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ERR)
> -#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPLY)
> -
> -#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK)
> -#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED)
> -#define rpc_autherr_rejectedcred cpu_to_be32(RPC_AUTH_REJECTEDCRED)
> -#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF)
> -#define rpc_autherr_rejectedverf cpu_to_be32(RPC_AUTH_REJECTEDVERF)
> -#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK)
> -#define rpcsec_gsserr_credproblem cpu_to_be32(RPCSEC_GSS_CREDPROBLEM)
> -#define rpcsec_gsserr_ctxproblem cpu_to_be32(RPCSEC_GSS_CTXPROBLEM)
> -#define rpc_autherr_oldseqnum cpu_to_be32(101)
> +enum xdr_be32_equivalents {
> + xdr_zero = cpu_to_be32(0),
> + xdr_one = cpu_to_be32(1),
> + xdr_two = cpu_to_be32(2),
It is clever to use an enum to pre-compute these values, but
it becomes a concern that the type (and size) of an enum may
not be the same as values they may be compared to.
Commonly, code may compare them to int32, uint32, etc. What
guarantees are there that such comparisons will yield the
appropriate result, especially if a < or > test is performed?
Tom.
> +
> + rpc_version = cpu_to_be32(RPC_VERSION),
> +
> + rpc_auth_null = cpu_to_be32(RPC_AUTH_NULL),
> + rpc_auth_unix = cpu_to_be32(RPC_AUTH_UNIX),
> + rpc_auth_short = cpu_to_be32(RPC_AUTH_SHORT),
> + rpc_auth_des = cpu_to_be32(RPC_AUTH_DES),
> + rpc_auth_krb = cpu_to_be32(RPC_AUTH_KRB),
> + rpc_auth_gss = cpu_to_be32(RPC_AUTH_GSS),
> +
> + rpc_call = cpu_to_be32(RPC_CALL),
> + rpc_reply = cpu_to_be32(RPC_REPLY),
> +
> + rpc_msg_accepted = cpu_to_be32(RPC_MSG_ACCEPTED),
> + rpc_msg_denied = cpu_to_be32(RPC_MSG_DENIED),
> +
> + rpc_success = cpu_to_be32(RPC_SUCCESS),
> + rpc_prog_unavail = cpu_to_be32(RPC_PROG_UNAVAIL),
> + rpc_prog_mismatch = cpu_to_be32(RPC_PROG_MISMATCH),
> + rpc_proc_unavail = cpu_to_be32(RPC_PROC_UNAVAIL),
> + rpc_garbage_args = cpu_to_be32(RPC_GARBAGE_ARGS),
> + rpc_system_err = cpu_to_be32(RPC_SYSTEM_ERR),
> + rpc_drop_reply = cpu_to_be32(RPC_DROP_REPLY),
> +
> + rpc_mismatch = cpu_to_be32(RPC_MISMATCH),
> + rpc_auth_error = cpu_to_be32(RPC_AUTH_ERROR),
> +
> + rpc_auth_ok = cpu_to_be32(RPC_AUTH_OK),
> + rpc_autherr_badcred = cpu_to_be32(RPC_AUTH_BADCRED),
> + rpc_autherr_rejectedcred = cpu_to_be32(RPC_AUTH_REJECTEDCRED),
> + rpc_autherr_badverf = cpu_to_be32(RPC_AUTH_BADVERF),
> + rpc_autherr_rejectedverf = cpu_to_be32(RPC_AUTH_REJECTEDVERF),
> + rpc_autherr_tooweak = cpu_to_be32(RPC_AUTH_TOOWEAK),
> + rpcsec_gsserr_credproblem = cpu_to_be32(RPCSEC_GSS_CREDPROBLEM),
> + rpcsec_gsserr_ctxproblem = cpu_to_be32(RPCSEC_GSS_CTXPROBLEM),
> +};
>
> /*
> * Miscellaneous XDR helper functions
>
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-01 19:57 ` [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants Chuck Lever
2019-02-02 2:30 ` Tom Talpey
@ 2019-02-02 17:02 ` Christoph Hellwig
2019-02-02 22:49 ` Chuck Lever
1 sibling, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2019-02-02 17:02 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, simo
On Fri, Feb 01, 2019 at 02:57:47PM -0500, Chuck Lever wrote:
> Byte-swapping causes a CPU pipeline bubble on some processors. When
> a decoder is comparing an on-the-wire value for equality, byte-
> swapping can be avoided by comparing it directly to a pre-byte-
> swapped constant value.
Which ones?
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-02 2:30 ` Tom Talpey
@ 2019-02-02 22:46 ` Chuck Lever
2019-02-03 15:00 ` Trond Myklebust
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-02 22:46 UTC (permalink / raw)
To: Tom Talpey; +Cc: Linux NFS Mailing List, simo
> On Feb 1, 2019, at 9:30 PM, Tom Talpey <tom@talpey.com> wrote:
>
> On 2/1/2019 2:57 PM, Chuck Lever wrote:
>> Byte-swapping causes a CPU pipeline bubble on some processors. When
>> a decoder is comparing an on-the-wire value for equality, byte-
>> swapping can be avoided by comparing it directly to a pre-byte-
>> swapped constant value.
>> The current set of pre-xdr'd constants is missing some common values
>> used in the RPC header. Fill those out.
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/linux/sunrpc/auth_gss.h | 5 ++-
>> include/linux/sunrpc/xdr.h | 66 ++++++++++++++++++++++++---------------
>> 2 files changed, 45 insertions(+), 26 deletions(-)
>> diff --git a/include/linux/sunrpc/auth_gss.h b/include/linux/sunrpc/auth_gss.h
>> index 30427b7..adc4be2 100644
>> --- a/include/linux/sunrpc/auth_gss.h
>> +++ b/include/linux/sunrpc/auth_gss.h
>> @@ -19,7 +19,10 @@
>> #include <linux/sunrpc/svc.h>
>> #include <linux/sunrpc/gss_api.h>
>> -#define RPC_GSS_VERSION 1
>> +enum {
>> + RPC_GSS_VERSION = 1,
>> + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION)
>> +};
>> #define MAXSEQ 0x80000000 /* maximum legal sequence number, from rfc 2203 */
>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>> index 787939d..69161cb 100644
>> --- a/include/linux/sunrpc/xdr.h
>> +++ b/include/linux/sunrpc/xdr.h
>> @@ -17,6 +17,7 @@
>> #include <asm/byteorder.h>
>> #include <asm/unaligned.h>
>> #include <linux/scatterlist.h>
>> +#include <linux/sunrpc/msg_prot.h>
>> struct bio_vec;
>> struct rpc_rqst;
>> @@ -79,31 +80,46 @@ struct xdr_buf {
>> buf->buflen = len;
>> }
>> -/*
>> - * pre-xdr'ed macros.
>> - */
>> -
>> -#define xdr_zero cpu_to_be32(0)
>> -#define xdr_one cpu_to_be32(1)
>> -#define xdr_two cpu_to_be32(2)
>> -
>> -#define rpc_success cpu_to_be32(RPC_SUCCESS)
>> -#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
>> -#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
>> -#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL)
>> -#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS)
>> -#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ERR)
>> -#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPLY)
>> -
>> -#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK)
>> -#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED)
>> -#define rpc_autherr_rejectedcred cpu_to_be32(RPC_AUTH_REJECTEDCRED)
>> -#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF)
>> -#define rpc_autherr_rejectedverf cpu_to_be32(RPC_AUTH_REJECTEDVERF)
>> -#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK)
>> -#define rpcsec_gsserr_credproblem cpu_to_be32(RPCSEC_GSS_CREDPROBLEM)
>> -#define rpcsec_gsserr_ctxproblem cpu_to_be32(RPCSEC_GSS_CTXPROBLEM)
>> -#define rpc_autherr_oldseqnum cpu_to_be32(101)
>> +enum xdr_be32_equivalents {
>> + xdr_zero = cpu_to_be32(0),
>> + xdr_one = cpu_to_be32(1),
>> + xdr_two = cpu_to_be32(2),
>
> It is clever to use an enum to pre-compute these values, but
Perhaps not clever; it is a current Linux kernel coding
practice to use an enum in favor of a C macro for constants.
> it becomes a concern that the type (and size) of an enum may
> not be the same as values they may be compared to.
Indeed, an enum is a variably-sized signed integer, IIUC.
> Commonly, code may compare them to int32, uint32, etc. What
> guarantees are there that such comparisons will yield the
> appropriate result, especially if a < or > test is performed?
I believe for the purposes of assignment and equality comparison
the compiler will promote these to the size and sign of the
variable. We would never perform a greater or less than test with
these values, obviously.
However, they probably should have an obvious and well-defined
type, and I should leave the already-defined macros as they
are.
> Tom.
>
>> +
>> + rpc_version = cpu_to_be32(RPC_VERSION),
>> +
>> + rpc_auth_null = cpu_to_be32(RPC_AUTH_NULL),
>> + rpc_auth_unix = cpu_to_be32(RPC_AUTH_UNIX),
>> + rpc_auth_short = cpu_to_be32(RPC_AUTH_SHORT),
>> + rpc_auth_des = cpu_to_be32(RPC_AUTH_DES),
>> + rpc_auth_krb = cpu_to_be32(RPC_AUTH_KRB),
>> + rpc_auth_gss = cpu_to_be32(RPC_AUTH_GSS),
>> +
>> + rpc_call = cpu_to_be32(RPC_CALL),
>> + rpc_reply = cpu_to_be32(RPC_REPLY),
>> +
>> + rpc_msg_accepted = cpu_to_be32(RPC_MSG_ACCEPTED),
>> + rpc_msg_denied = cpu_to_be32(RPC_MSG_DENIED),
>> +
>> + rpc_success = cpu_to_be32(RPC_SUCCESS),
>> + rpc_prog_unavail = cpu_to_be32(RPC_PROG_UNAVAIL),
>> + rpc_prog_mismatch = cpu_to_be32(RPC_PROG_MISMATCH),
>> + rpc_proc_unavail = cpu_to_be32(RPC_PROC_UNAVAIL),
>> + rpc_garbage_args = cpu_to_be32(RPC_GARBAGE_ARGS),
>> + rpc_system_err = cpu_to_be32(RPC_SYSTEM_ERR),
>> + rpc_drop_reply = cpu_to_be32(RPC_DROP_REPLY),
>> +
>> + rpc_mismatch = cpu_to_be32(RPC_MISMATCH),
>> + rpc_auth_error = cpu_to_be32(RPC_AUTH_ERROR),
>> +
>> + rpc_auth_ok = cpu_to_be32(RPC_AUTH_OK),
>> + rpc_autherr_badcred = cpu_to_be32(RPC_AUTH_BADCRED),
>> + rpc_autherr_rejectedcred = cpu_to_be32(RPC_AUTH_REJECTEDCRED),
>> + rpc_autherr_badverf = cpu_to_be32(RPC_AUTH_BADVERF),
>> + rpc_autherr_rejectedverf = cpu_to_be32(RPC_AUTH_REJECTEDVERF),
>> + rpc_autherr_tooweak = cpu_to_be32(RPC_AUTH_TOOWEAK),
>> + rpcsec_gsserr_credproblem = cpu_to_be32(RPCSEC_GSS_CREDPROBLEM),
>> + rpcsec_gsserr_ctxproblem = cpu_to_be32(RPCSEC_GSS_CTXPROBLEM),
>> +};
>> /*
>> * Miscellaneous XDR helper functions
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-02 17:02 ` Christoph Hellwig
@ 2019-02-02 22:49 ` Chuck Lever
2019-02-04 7:53 ` Christoph Hellwig
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-02 22:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Linux NFS Mailing List, simo
> On Feb 2, 2019, at 12:02 PM, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Feb 01, 2019 at 02:57:47PM -0500, Chuck Lever wrote:
>> Byte-swapping causes a CPU pipeline bubble on some processors. When
>> a decoder is comparing an on-the-wire value for equality, byte-
>> swapping can be avoided by comparing it directly to a pre-byte-
>> swapped constant value.
>
> Which ones?
I assume you mean on which processors have I observed CPU cycle
spikes around bswap instructions. I've seen this behavior only
on Intel processors of various families.
Would you prefer a different justification for this clean-up?
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-02 22:46 ` Chuck Lever
@ 2019-02-03 15:00 ` Trond Myklebust
2019-02-03 16:49 ` Chuck Lever
0 siblings, 1 reply; 31+ messages in thread
From: Trond Myklebust @ 2019-02-03 15:00 UTC (permalink / raw)
To: tom, chuck.lever; +Cc: simo, linux-nfs
On Sat, 2019-02-02 at 17:46 -0500, Chuck Lever wrote:
> > On Feb 1, 2019, at 9:30 PM, Tom Talpey <tom@talpey.com> wrote:
> >
> > On 2/1/2019 2:57 PM, Chuck Lever wrote:
> > > Byte-swapping causes a CPU pipeline bubble on some processors.
> > > When
> > > a decoder is comparing an on-the-wire value for equality, byte-
> > > swapping can be avoided by comparing it directly to a pre-byte-
> > > swapped constant value.
> > > The current set of pre-xdr'd constants is missing some common
> > > values
> > > used in the RPC header. Fill those out.
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > > include/linux/sunrpc/auth_gss.h | 5 ++-
> > > include/linux/sunrpc/xdr.h | 66 ++++++++++++++++++++++++-
> > > --------------
> > > 2 files changed, 45 insertions(+), 26 deletions(-)
> > > diff --git a/include/linux/sunrpc/auth_gss.h
> > > b/include/linux/sunrpc/auth_gss.h
> > > index 30427b7..adc4be2 100644
> > > --- a/include/linux/sunrpc/auth_gss.h
> > > +++ b/include/linux/sunrpc/auth_gss.h
> > > @@ -19,7 +19,10 @@
> > > #include <linux/sunrpc/svc.h>
> > > #include <linux/sunrpc/gss_api.h>
> > > -#define RPC_GSS_VERSION 1
> > > +enum {
> > > + RPC_GSS_VERSION = 1,
> > > + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION)
> > > +};
> > > #define MAXSEQ 0x80000000 /* maximum legal sequence number,
> > > from rfc 2203 */
> > > diff --git a/include/linux/sunrpc/xdr.h
> > > b/include/linux/sunrpc/xdr.h
> > > index 787939d..69161cb 100644
> > > --- a/include/linux/sunrpc/xdr.h
> > > +++ b/include/linux/sunrpc/xdr.h
> > > @@ -17,6 +17,7 @@
> > > #include <asm/byteorder.h>
> > > #include <asm/unaligned.h>
> > > #include <linux/scatterlist.h>
> > > +#include <linux/sunrpc/msg_prot.h>
> > > struct bio_vec;
> > > struct rpc_rqst;
> > > @@ -79,31 +80,46 @@ struct xdr_buf {
> > > buf->buflen = len;
> > > }
> > > -/*
> > > - * pre-xdr'ed macros.
> > > - */
> > > -
> > > -#define xdr_zero cpu_to_be32(0)
> > > -#define xdr_one cpu_to_be32(1)
> > > -#define xdr_two cpu_to_be32(2)
> > > -
> > > -#define rpc_success cpu_to_be32(RPC_SUCCESS)
> > > -#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
> > > -#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
> > > -#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL)
> > > -#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS)
> > > -#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ER
> > > R)
> > > -#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPL
> > > Y)
> > > -
> > > -#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK)
> > > -#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED)
> > > -#define rpc_autherr_rejectedcred
> > > cpu_to_be32(RPC_AUTH_REJECTEDCRED)
> > > -#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF)
> > > -#define rpc_autherr_rejectedverf
> > > cpu_to_be32(RPC_AUTH_REJECTEDVERF)
> > > -#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK)
> > > -#define rpcsec_gsserr_credproblem cpu_to_be32(RPCSEC_GSS_CR
> > > EDPROBLEM)
> > > -#define rpcsec_gsserr_ctxproblem cpu_to_be32(RPCSEC_GSS_CT
> > > XPROBLEM)
> > > -#define rpc_autherr_oldseqnum cpu_to_be32(101)
> > > +enum xdr_be32_equivalents {
> > > + xdr_zero = cpu_to_be32(0),
> > > + xdr_one = cpu_to_be32(1),
> > > + xdr_two = cpu_to_be32(2),
> >
> > It is clever to use an enum to pre-compute these values, but
>
> Perhaps not clever; it is a current Linux kernel coding
> practice to use an enum in favor of a C macro for constants.
Sure, but won't that confuse 'sparse' static checking? These constants
will no longer appear as being big endian.
Doesn't gcc's __builtin_bswap32() already compute the result at compile
time when you feed it a constant value? AFAICS it is supposed to, which
is why we use it directly in include/uapi/linux/swab.h instead of using
a special cased __builtin_constant_p().
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-03 15:00 ` Trond Myklebust
@ 2019-02-03 16:49 ` Chuck Lever
2019-02-03 18:58 ` Trond Myklebust
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-03 16:49 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Tom Talpey, simo, Linux NFS Mailing List
> On Feb 3, 2019, at 10:00 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Sat, 2019-02-02 at 17:46 -0500, Chuck Lever wrote:
>>> On Feb 1, 2019, at 9:30 PM, Tom Talpey <tom@talpey.com> wrote:
>>>
>>> On 2/1/2019 2:57 PM, Chuck Lever wrote:
>>>> Byte-swapping causes a CPU pipeline bubble on some processors.
>>>> When
>>>> a decoder is comparing an on-the-wire value for equality, byte-
>>>> swapping can be avoided by comparing it directly to a pre-byte-
>>>> swapped constant value.
>>>> The current set of pre-xdr'd constants is missing some common
>>>> values
>>>> used in the RPC header. Fill those out.
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> include/linux/sunrpc/auth_gss.h | 5 ++-
>>>> include/linux/sunrpc/xdr.h | 66 ++++++++++++++++++++++++-
>>>> --------------
>>>> 2 files changed, 45 insertions(+), 26 deletions(-)
>>>> diff --git a/include/linux/sunrpc/auth_gss.h
>>>> b/include/linux/sunrpc/auth_gss.h
>>>> index 30427b7..adc4be2 100644
>>>> --- a/include/linux/sunrpc/auth_gss.h
>>>> +++ b/include/linux/sunrpc/auth_gss.h
>>>> @@ -19,7 +19,10 @@
>>>> #include <linux/sunrpc/svc.h>
>>>> #include <linux/sunrpc/gss_api.h>
>>>> -#define RPC_GSS_VERSION 1
>>>> +enum {
>>>> + RPC_GSS_VERSION = 1,
>>>> + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION)
>>>> +};
>>>> #define MAXSEQ 0x80000000 /* maximum legal sequence number,
>>>> from rfc 2203 */
>>>> diff --git a/include/linux/sunrpc/xdr.h
>>>> b/include/linux/sunrpc/xdr.h
>>>> index 787939d..69161cb 100644
>>>> --- a/include/linux/sunrpc/xdr.h
>>>> +++ b/include/linux/sunrpc/xdr.h
>>>> @@ -17,6 +17,7 @@
>>>> #include <asm/byteorder.h>
>>>> #include <asm/unaligned.h>
>>>> #include <linux/scatterlist.h>
>>>> +#include <linux/sunrpc/msg_prot.h>
>>>> struct bio_vec;
>>>> struct rpc_rqst;
>>>> @@ -79,31 +80,46 @@ struct xdr_buf {
>>>> buf->buflen = len;
>>>> }
>>>> -/*
>>>> - * pre-xdr'ed macros.
>>>> - */
>>>> -
>>>> -#define xdr_zero cpu_to_be32(0)
>>>> -#define xdr_one cpu_to_be32(1)
>>>> -#define xdr_two cpu_to_be32(2)
>>>> -
>>>> -#define rpc_success cpu_to_be32(RPC_SUCCESS)
>>>> -#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
>>>> -#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
>>>> -#define rpc_proc_unavail cpu_to_be32(RPC_PROC_UNAVAIL)
>>>> -#define rpc_garbage_args cpu_to_be32(RPC_GARBAGE_ARGS)
>>>> -#define rpc_system_err cpu_to_be32(RPC_SYSTEM_ER
>>>> R)
>>>> -#define rpc_drop_reply cpu_to_be32(RPC_DROP_REPL
>>>> Y)
>>>> -
>>>> -#define rpc_auth_ok cpu_to_be32(RPC_AUTH_OK)
>>>> -#define rpc_autherr_badcred cpu_to_be32(RPC_AUTH_BADCRED)
>>>> -#define rpc_autherr_rejectedcred
>>>> cpu_to_be32(RPC_AUTH_REJECTEDCRED)
>>>> -#define rpc_autherr_badverf cpu_to_be32(RPC_AUTH_BADVERF)
>>>> -#define rpc_autherr_rejectedverf
>>>> cpu_to_be32(RPC_AUTH_REJECTEDVERF)
>>>> -#define rpc_autherr_tooweak cpu_to_be32(RPC_AUTH_TOOWEAK)
>>>> -#define rpcsec_gsserr_credproblem cpu_to_be32(RPCSEC_GSS_CR
>>>> EDPROBLEM)
>>>> -#define rpcsec_gsserr_ctxproblem cpu_to_be32(RPCSEC_GSS_CT
>>>> XPROBLEM)
>>>> -#define rpc_autherr_oldseqnum cpu_to_be32(101)
>>>> +enum xdr_be32_equivalents {
>>>> + xdr_zero = cpu_to_be32(0),
>>>> + xdr_one = cpu_to_be32(1),
>>>> + xdr_two = cpu_to_be32(2),
>>>
>>> It is clever to use an enum to pre-compute these values, but
>>
>> Perhaps not clever; it is a current Linux kernel coding
>> practice to use an enum in favor of a C macro for constants.
>
> Sure, but won't that confuse 'sparse' static checking? These constants
> will no longer appear as being big endian.
Agreed, that is not a desirable situation for these symbolic constants.
> Doesn't gcc's __builtin_bswap32() already compute the result at compile
> time when you feed it a constant value? AFAICS it is supposed to, which
> is why we use it directly in include/uapi/linux/swab.h instead of using
> a special cased __builtin_constant_p().
The return type of __builtin_bswap32 is uint32_t, not __be32.
I will stick with cpu_to_be32(), and simply add the missing constants.
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-03 16:49 ` Chuck Lever
@ 2019-02-03 18:58 ` Trond Myklebust
0 siblings, 0 replies; 31+ messages in thread
From: Trond Myklebust @ 2019-02-03 18:58 UTC (permalink / raw)
To: chuck.lever; +Cc: tom, simo, linux-nfs
On Sun, 2019-02-03 at 11:49 -0500, Chuck Lever wrote:
> > On Feb 3, 2019, at 10:00 AM, Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> >
> > Doesn't gcc's __builtin_bswap32() already compute the result at
> > compile
> > time when you feed it a constant value? AFAICS it is supposed to,
> > which
> > is why we use it directly in include/uapi/linux/swab.h instead of
> > using
> > a special cased __builtin_constant_p().
>
> The return type of __builtin_bswap32 is uint32_t, not __be32.
>
> I will stick with cpu_to_be32(), and simply add the missing
> constants.
>
cpu_to_be32() is a macro that expands to __builtin_bswap32() plus a
cast for most architectures on gcc.
What I'm saying is that as far as I know, the existing #defines should
compile into be32 constants by default, as should any call of the form
cpu_to_b32(<const expression>) and htonl(<const expression>).
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-02 22:49 ` Chuck Lever
@ 2019-02-04 7:53 ` Christoph Hellwig
2019-02-04 14:16 ` Chuck Lever
0 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2019-02-04 7:53 UTC (permalink / raw)
To: Chuck Lever; +Cc: Christoph Hellwig, Linux NFS Mailing List, simo
On Sat, Feb 02, 2019 at 05:49:35PM -0500, Chuck Lever wrote:
> >> Byte-swapping causes a CPU pipeline bubble on some processors. When
> >> a decoder is comparing an on-the-wire value for equality, byte-
> >> swapping can be avoided by comparing it directly to a pre-byte-
> >> swapped constant value.
> >
> > Which ones?
>
> I assume you mean on which processors have I observed CPU cycle
> spikes around bswap instructions.
Yes.
> I've seen this behavior only
> on Intel processors of various families.
Interesting. In general we should not do separate byte swap instructions
on x86, as MOVBE can be used to do a load or store with an included
byteswap, and I thought the whole point for that was that they could
be handled in the same cycle.
In fact https://www.agner.org/optimize/instruction_tables.pdf
says that movbe is generally a single cycle instruction.
> Would you prefer a different justification for this clean-up?
I don't really care about the cleanup, it is just that the explanation
goes against conventional wisdom, which is why I was a little surpised.
And that is not just the cycles, but also as Trond pointed out
that the Linux byte swapping macro on constants should usually be
optimized away at compile time anyway.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-04 7:53 ` Christoph Hellwig
@ 2019-02-04 14:16 ` Chuck Lever
2019-02-04 14:32 ` Christoph Hellwig
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-04 14:16 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Linux NFS Mailing List, simo
> On Feb 4, 2019, at 2:53 AM, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sat, Feb 02, 2019 at 05:49:35PM -0500, Chuck Lever wrote:
>>>> Byte-swapping causes a CPU pipeline bubble on some processors. When
>>>> a decoder is comparing an on-the-wire value for equality, byte-
>>>> swapping can be avoided by comparing it directly to a pre-byte-
>>>> swapped constant value.
>>>
>>> Which ones?
>>
>> I assume you mean on which processors have I observed CPU cycle
>> spikes around bswap instructions.
>
> Yes.
>
>> I've seen this behavior only
>> on Intel processors of various families.
>
> Interesting. In general we should not do separate byte swap instructions
> on x86, as MOVBE can be used to do a load or store with an included
> byteswap, and I thought the whole point for that was that they could
> be handled in the same cycle.
>
> In fact https://www.agner.org/optimize/instruction_tables.pdf
> says that movbe is generally a single cycle instruction.
>
>> Would you prefer a different justification for this clean-up?
>
> I don't really care about the cleanup, it is just that the explanation
> goes against conventional wisdom, which is why I was a little surpised.
>
> And that is not just the cycles, but also as Trond pointed out
> that the Linux byte swapping macro on constants should usually be
> optimized away at compile time anyway.
They are. The problem is that we are byte-swapping the incoming wire
data and then comparing to CPU-endian constants in some hot paths.
It's better to leave the incoming data alone and compare to a pre-
byte-swapped constant. This patch adds some of these constants that
were missing, in preparation for fixing the hot paths.
That is apparently not clear from the patch description, so I will
endeavor to improve it.
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-04 14:16 ` Chuck Lever
@ 2019-02-04 14:32 ` Christoph Hellwig
2019-02-04 14:56 ` Chuck Lever
0 siblings, 1 reply; 31+ messages in thread
From: Christoph Hellwig @ 2019-02-04 14:32 UTC (permalink / raw)
To: Chuck Lever; +Cc: Christoph Hellwig, Linux NFS Mailing List, simo
On Mon, Feb 04, 2019 at 09:16:54AM -0500, Chuck Lever wrote:
> They are. The problem is that we are byte-swapping the incoming wire
> data and then comparing to CPU-endian constants in some hot paths.
> It's better to leave the incoming data alone and compare to a pre-
> byte-swapped constant. This patch adds some of these constants that
> were missing, in preparation for fixing the hot paths.
>
> That is apparently not clear from the patch description, so I will
> endeavor to improve it.
Why do we need new enums / #defines for that?
Just replace:
if (beXX_to_cpu(pkt->field) == SOME_CONSTANT)
with
if (pkt->field == cpu_to_be32(SOME_CONSTANT))
and we are done. The latter is a pretty common pattern through
the kernel.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-04 14:32 ` Christoph Hellwig
@ 2019-02-04 14:56 ` Chuck Lever
2019-02-04 19:37 ` J. Bruce Fields
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-04 14:56 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Linux NFS Mailing List, simo
> On Feb 4, 2019, at 9:32 AM, Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Feb 04, 2019 at 09:16:54AM -0500, Chuck Lever wrote:
>> They are. The problem is that we are byte-swapping the incoming wire
>> data and then comparing to CPU-endian constants in some hot paths.
>> It's better to leave the incoming data alone and compare to a pre-
>> byte-swapped constant. This patch adds some of these constants that
>> were missing, in preparation for fixing the hot paths.
>>
>> That is apparently not clear from the patch description, so I will
>> endeavor to improve it.
>
> Why do we need new enums / #defines for that?
>
> Just replace:
>
> if (beXX_to_cpu(pkt->field) == SOME_CONSTANT)
>
> with
>
> if (pkt->field == cpu_to_be32(SOME_CONSTANT))
>
> and we are done.
True.
> The latter is a pretty common pattern through the kernel.
However the pattern in the NFS server and lockd is to define a
lower-case version of the same macro that is pre-byte-swapped.
I'm attempting to follow existing precedent in this area.
We already have, for example, in various sunrpc headers:
enum rpc_accept_stat {
RPC_SUCCESS = 0,
RPC_PROG_UNAVAIL = 1,
RPC_PROG_MISMATCH = 2,
...
};
#define rpc_success cpu_to_be32(RPC_SUCCESS)
#define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
#define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
Or, for NFS:
enum nfs_stat {
...
NFSERR_SHARE_DENIED = 10015,
...
};
#define nfserr_share_denied cpu_to_be32(NFSERR_SHARE_DENIED)
There are some missing lower-case macros, which I am trying to
add to our existing collection before I rewrite the RPC header
encoding and decoding functions. So I'm adding:
+ rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION),
+ rpc_call = cpu_to_be32(RPC_CALL),
+ rpc_reply = cpu_to_be32(RPC_REPLY),
+
+ rpc_msg_accepted = cpu_to_be32(RPC_MSG_ACCEPTED),
+ rpc_msg_denied = cpu_to_be32(RPC_MSG_DENIED),
Actually since we have decided not to use enum for these, this
smaller addition can simply be squashed into the later patches,
and I can drop this patch, which was intended as a clean-up but
now appears to be unnecessary.
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions
2019-02-01 19:57 ` [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions Chuck Lever
@ 2019-02-04 19:04 ` J. Bruce Fields
2019-02-04 19:07 ` Chuck Lever
0 siblings, 1 reply; 31+ messages in thread
From: J. Bruce Fields @ 2019-02-04 19:04 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, simo
On Fri, Feb 01, 2019 at 02:57:31PM -0500, Chuck Lever wrote:
> Clean up: Reduce dprintk noise by removing dprintk() call sites
> from hot path that do not report exceptions. These are usually
> replaceable with function graph tracing.
Yeah, dprintk's sometimes much to verbose to be useful, thanks for
looking at this. How are you deciding what the hot paths are?
--b.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> net/sunrpc/auth.c | 29 -----------------------------
> net/sunrpc/auth_unix.c | 9 +--------
> 2 files changed, 1 insertion(+), 37 deletions(-)
>
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 8dfab61..275e84e 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -17,10 +17,6 @@
> #include <linux/sunrpc/gss_api.h>
> #include <linux/spinlock.h>
>
> -#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
> -# define RPCDBG_FACILITY RPCDBG_AUTH
> -#endif
> -
> #define RPC_CREDCACHE_DEFAULT_HASHBITS (4)
> struct rpc_cred_cache {
> struct hlist_head *hashtable;
> @@ -267,8 +263,6 @@ static int param_get_hashtbl_sz(char *buffer, const struct kernel_param *kp)
> }
> }
> rcu_read_unlock();
> -
> - dprintk("RPC: %s returns %d\n", __func__, result);
> return result;
> }
> EXPORT_SYMBOL_GPL(rpcauth_list_flavors);
> @@ -636,9 +630,6 @@ struct rpc_cred *
> struct rpc_cred *ret;
> const struct cred *cred = current_cred();
>
> - dprintk("RPC: looking up %s cred\n",
> - auth->au_ops->au_name);
> -
> memset(&acred, 0, sizeof(acred));
> acred.cred = cred;
> ret = auth->au_ops->lookup_cred(auth, &acred, flags);
> @@ -670,8 +661,6 @@ struct rpc_cred *
> };
> struct rpc_cred *ret;
>
> - dprintk("RPC: %5u looking up %s cred\n",
> - task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
> ret = auth->au_ops->lookup_cred(auth, &acred, lookupflags);
> put_cred(acred.cred);
> return ret;
> @@ -688,8 +677,6 @@ struct rpc_cred *
>
> if (!acred.principal)
> return NULL;
> - dprintk("RPC: %5u looking up %s machine cred\n",
> - task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
> return auth->au_ops->lookup_cred(auth, &acred, lookupflags);
> }
>
> @@ -698,8 +685,6 @@ struct rpc_cred *
> {
> struct rpc_auth *auth = task->tk_client->cl_auth;
>
> - dprintk("RPC: %5u looking up %s cred\n",
> - task->tk_pid, auth->au_ops->au_name);
> return rpcauth_lookupcred(auth, lookupflags);
> }
>
> @@ -776,9 +761,6 @@ struct rpc_cred *
> {
> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>
> - dprintk("RPC: %5u marshaling %s cred %p\n",
> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
> -
> return cred->cr_ops->crmarshal(task, p);
> }
>
> @@ -787,9 +769,6 @@ struct rpc_cred *
> {
> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>
> - dprintk("RPC: %5u validating %s cred %p\n",
> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
> -
> return cred->cr_ops->crvalidate(task, p);
> }
>
> @@ -808,8 +787,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
> {
> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>
> - dprintk("RPC: %5u using %s cred %p to wrap rpc data\n",
> - task->tk_pid, cred->cr_ops->cr_name, cred);
> if (cred->cr_ops->crwrap_req)
> return cred->cr_ops->crwrap_req(task, encode, rqstp, data, obj);
> /* By default, we encode the arguments normally. */
> @@ -833,8 +810,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
> {
> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>
> - dprintk("RPC: %5u using %s cred %p to unwrap rpc data\n",
> - task->tk_pid, cred->cr_ops->cr_name, cred);
> if (cred->cr_ops->crunwrap_resp)
> return cred->cr_ops->crunwrap_resp(task, decode, rqstp,
> data, obj);
> @@ -865,8 +840,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
> goto out;
> cred = task->tk_rqstp->rq_cred;
> }
> - dprintk("RPC: %5u refreshing %s cred %p\n",
> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
>
> err = cred->cr_ops->crrefresh(task);
> out:
> @@ -880,8 +853,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
> {
> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>
> - dprintk("RPC: %5u invalidating %s cred %p\n",
> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
> if (cred)
> clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
> }
> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
> index 387f6b3..fc8a591 100644
> --- a/net/sunrpc/auth_unix.c
> +++ b/net/sunrpc/auth_unix.c
> @@ -28,8 +28,6 @@
> static struct rpc_auth *
> unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
> {
> - dprintk("RPC: creating UNIX authenticator for client %p\n",
> - clnt);
> refcount_inc(&unix_auth.au_count);
> return &unix_auth;
> }
> @@ -37,7 +35,6 @@
> static void
> unx_destroy(struct rpc_auth *auth)
> {
> - dprintk("RPC: destroying UNIX authenticator %p\n", auth);
> }
>
> /*
> @@ -48,10 +45,6 @@
> {
> struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
>
> - dprintk("RPC: allocating UNIX cred for uid %d gid %d\n",
> - from_kuid(&init_user_ns, acred->cred->fsuid),
> - from_kgid(&init_user_ns, acred->cred->fsgid));
> -
> rpcauth_init_cred(ret, acred, auth, &unix_credops);
> ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
> return ret;
> @@ -61,7 +54,7 @@
> unx_free_cred_callback(struct rcu_head *head)
> {
> struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
> - dprintk("RPC: unx_free_cred %p\n", rpc_cred);
> +
> put_cred(rpc_cred->cr_cred);
> mempool_free(rpc_cred, unix_pool);
> }
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions
2019-02-04 19:04 ` J. Bruce Fields
@ 2019-02-04 19:07 ` Chuck Lever
0 siblings, 0 replies; 31+ messages in thread
From: Chuck Lever @ 2019-02-04 19:07 UTC (permalink / raw)
To: Bruce Fields; +Cc: Linux NFS Mailing List, simo
> On Feb 4, 2019, at 2:04 PM, bfields@fieldses.org wrote:
>
> On Fri, Feb 01, 2019 at 02:57:31PM -0500, Chuck Lever wrote:
>> Clean up: Reduce dprintk noise by removing dprintk() call sites
>> from hot path that do not report exceptions. These are usually
>> replaceable with function graph tracing.
>
> Yeah, dprintk's sometimes much to verbose to be useful, thanks for
> looking at this. How are you deciding what the hot paths are?
Anything that is called one or more times per RPC.
There are some places where low value dprintks are
removed just because. By and large these can often be
replaced by using the ftrace function plug-in.
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> net/sunrpc/auth.c | 29 -----------------------------
>> net/sunrpc/auth_unix.c | 9 +--------
>> 2 files changed, 1 insertion(+), 37 deletions(-)
>>
>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>> index 8dfab61..275e84e 100644
>> --- a/net/sunrpc/auth.c
>> +++ b/net/sunrpc/auth.c
>> @@ -17,10 +17,6 @@
>> #include <linux/sunrpc/gss_api.h>
>> #include <linux/spinlock.h>
>>
>> -#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>> -# define RPCDBG_FACILITY RPCDBG_AUTH
>> -#endif
>> -
>> #define RPC_CREDCACHE_DEFAULT_HASHBITS (4)
>> struct rpc_cred_cache {
>> struct hlist_head *hashtable;
>> @@ -267,8 +263,6 @@ static int param_get_hashtbl_sz(char *buffer, const struct kernel_param *kp)
>> }
>> }
>> rcu_read_unlock();
>> -
>> - dprintk("RPC: %s returns %d\n", __func__, result);
>> return result;
>> }
>> EXPORT_SYMBOL_GPL(rpcauth_list_flavors);
>> @@ -636,9 +630,6 @@ struct rpc_cred *
>> struct rpc_cred *ret;
>> const struct cred *cred = current_cred();
>>
>> - dprintk("RPC: looking up %s cred\n",
>> - auth->au_ops->au_name);
>> -
>> memset(&acred, 0, sizeof(acred));
>> acred.cred = cred;
>> ret = auth->au_ops->lookup_cred(auth, &acred, flags);
>> @@ -670,8 +661,6 @@ struct rpc_cred *
>> };
>> struct rpc_cred *ret;
>>
>> - dprintk("RPC: %5u looking up %s cred\n",
>> - task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
>> ret = auth->au_ops->lookup_cred(auth, &acred, lookupflags);
>> put_cred(acred.cred);
>> return ret;
>> @@ -688,8 +677,6 @@ struct rpc_cred *
>>
>> if (!acred.principal)
>> return NULL;
>> - dprintk("RPC: %5u looking up %s machine cred\n",
>> - task->tk_pid, task->tk_client->cl_auth->au_ops->au_name);
>> return auth->au_ops->lookup_cred(auth, &acred, lookupflags);
>> }
>>
>> @@ -698,8 +685,6 @@ struct rpc_cred *
>> {
>> struct rpc_auth *auth = task->tk_client->cl_auth;
>>
>> - dprintk("RPC: %5u looking up %s cred\n",
>> - task->tk_pid, auth->au_ops->au_name);
>> return rpcauth_lookupcred(auth, lookupflags);
>> }
>>
>> @@ -776,9 +761,6 @@ struct rpc_cred *
>> {
>> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>>
>> - dprintk("RPC: %5u marshaling %s cred %p\n",
>> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
>> -
>> return cred->cr_ops->crmarshal(task, p);
>> }
>>
>> @@ -787,9 +769,6 @@ struct rpc_cred *
>> {
>> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>>
>> - dprintk("RPC: %5u validating %s cred %p\n",
>> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
>> -
>> return cred->cr_ops->crvalidate(task, p);
>> }
>>
>> @@ -808,8 +787,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
>> {
>> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>>
>> - dprintk("RPC: %5u using %s cred %p to wrap rpc data\n",
>> - task->tk_pid, cred->cr_ops->cr_name, cred);
>> if (cred->cr_ops->crwrap_req)
>> return cred->cr_ops->crwrap_req(task, encode, rqstp, data, obj);
>> /* By default, we encode the arguments normally. */
>> @@ -833,8 +810,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
>> {
>> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>>
>> - dprintk("RPC: %5u using %s cred %p to unwrap rpc data\n",
>> - task->tk_pid, cred->cr_ops->cr_name, cred);
>> if (cred->cr_ops->crunwrap_resp)
>> return cred->cr_ops->crunwrap_resp(task, decode, rqstp,
>> data, obj);
>> @@ -865,8 +840,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
>> goto out;
>> cred = task->tk_rqstp->rq_cred;
>> }
>> - dprintk("RPC: %5u refreshing %s cred %p\n",
>> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
>>
>> err = cred->cr_ops->crrefresh(task);
>> out:
>> @@ -880,8 +853,6 @@ static void rpcauth_wrap_req_encode(kxdreproc_t encode, struct rpc_rqst *rqstp,
>> {
>> struct rpc_cred *cred = task->tk_rqstp->rq_cred;
>>
>> - dprintk("RPC: %5u invalidating %s cred %p\n",
>> - task->tk_pid, cred->cr_auth->au_ops->au_name, cred);
>> if (cred)
>> clear_bit(RPCAUTH_CRED_UPTODATE, &cred->cr_flags);
>> }
>> diff --git a/net/sunrpc/auth_unix.c b/net/sunrpc/auth_unix.c
>> index 387f6b3..fc8a591 100644
>> --- a/net/sunrpc/auth_unix.c
>> +++ b/net/sunrpc/auth_unix.c
>> @@ -28,8 +28,6 @@
>> static struct rpc_auth *
>> unx_create(const struct rpc_auth_create_args *args, struct rpc_clnt *clnt)
>> {
>> - dprintk("RPC: creating UNIX authenticator for client %p\n",
>> - clnt);
>> refcount_inc(&unix_auth.au_count);
>> return &unix_auth;
>> }
>> @@ -37,7 +35,6 @@
>> static void
>> unx_destroy(struct rpc_auth *auth)
>> {
>> - dprintk("RPC: destroying UNIX authenticator %p\n", auth);
>> }
>>
>> /*
>> @@ -48,10 +45,6 @@
>> {
>> struct rpc_cred *ret = mempool_alloc(unix_pool, GFP_NOFS);
>>
>> - dprintk("RPC: allocating UNIX cred for uid %d gid %d\n",
>> - from_kuid(&init_user_ns, acred->cred->fsuid),
>> - from_kgid(&init_user_ns, acred->cred->fsgid));
>> -
>> rpcauth_init_cred(ret, acred, auth, &unix_credops);
>> ret->cr_flags = 1UL << RPCAUTH_CRED_UPTODATE;
>> return ret;
>> @@ -61,7 +54,7 @@
>> unx_free_cred_callback(struct rcu_head *head)
>> {
>> struct rpc_cred *rpc_cred = container_of(head, struct rpc_cred, cr_rcu);
>> - dprintk("RPC: unx_free_cred %p\n", rpc_cred);
>> +
>> put_cred(rpc_cred->cr_cred);
>> mempool_free(rpc_cred, unix_pool);
>> }
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-04 14:56 ` Chuck Lever
@ 2019-02-04 19:37 ` J. Bruce Fields
2019-02-05 1:57 ` Tom Talpey
0 siblings, 1 reply; 31+ messages in thread
From: J. Bruce Fields @ 2019-02-04 19:37 UTC (permalink / raw)
To: Chuck Lever; +Cc: Christoph Hellwig, Linux NFS Mailing List, simo
On Mon, Feb 04, 2019 at 09:56:20AM -0500, Chuck Lever wrote:
>
>
> > On Feb 4, 2019, at 9:32 AM, Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Feb 04, 2019 at 09:16:54AM -0500, Chuck Lever wrote:
> >> They are. The problem is that we are byte-swapping the incoming wire
> >> data and then comparing to CPU-endian constants in some hot paths.
> >> It's better to leave the incoming data alone and compare to a pre-
> >> byte-swapped constant. This patch adds some of these constants that
> >> were missing, in preparation for fixing the hot paths.
> >>
> >> That is apparently not clear from the patch description, so I will
> >> endeavor to improve it.
> >
> > Why do we need new enums / #defines for that?
> >
> > Just replace:
> >
> > if (beXX_to_cpu(pkt->field) == SOME_CONSTANT)
> >
> > with
> >
> > if (pkt->field == cpu_to_be32(SOME_CONSTANT))
> >
> > and we are done.
>
> True.
I'm a little surprised the compiler couldn't do that automatically.
(Disclaimer: I know nothing about compilers.)
> > The latter is a pretty common pattern through the kernel.
>
> However the pattern in the NFS server and lockd is to define a
> lower-case version of the same macro that is pre-byte-swapped.
> I'm attempting to follow existing precedent in this area.
I've never really understood why that was done. (Not saying it was
wrong, just that I don't understand it.) As long as you're reading the
value, how could byte-swapping it actually add a significant expense?
The one thing I do like is that I can look at e.g.:
int nfsd4_get_nfs4_acl ...
__be32 nfsd4_set_nfs4_acl ...
and tell that the former returns a linux errno, the latter an NFS error.
--b.
>
> We already have, for example, in various sunrpc headers:
>
> enum rpc_accept_stat {
> RPC_SUCCESS = 0,
> RPC_PROG_UNAVAIL = 1,
> RPC_PROG_MISMATCH = 2,
> ...
> };
>
> #define rpc_success cpu_to_be32(RPC_SUCCESS)
> #define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
> #define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
>
> Or, for NFS:
>
> enum nfs_stat {
> ...
> NFSERR_SHARE_DENIED = 10015,
> ...
> };
>
> #define nfserr_share_denied cpu_to_be32(NFSERR_SHARE_DENIED)
>
> There are some missing lower-case macros, which I am trying to
> add to our existing collection before I rewrite the RPC header
> encoding and decoding functions. So I'm adding:
>
> + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION),
>
> + rpc_call = cpu_to_be32(RPC_CALL),
> + rpc_reply = cpu_to_be32(RPC_REPLY),
> +
> + rpc_msg_accepted = cpu_to_be32(RPC_MSG_ACCEPTED),
> + rpc_msg_denied = cpu_to_be32(RPC_MSG_DENIED),
>
> Actually since we have decided not to use enum for these, this
> smaller addition can simply be squashed into the later patches,
> and I can drop this patch, which was intended as a clean-up but
> now appears to be unnecessary.
>
> --
> Chuck Lever
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
2019-02-01 19:58 ` [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim() Chuck Lever
@ 2019-02-04 19:46 ` J. Bruce Fields
2019-02-04 19:49 ` Chuck Lever
0 siblings, 1 reply; 31+ messages in thread
From: J. Bruce Fields @ 2019-02-04 19:46 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, simo
On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote:
> The key action of xdr_buf_trim() is that it shortens buf->len, the
> length of the xdr_buf' content. The other actions -- shortening the
> head, pages, and tail components -- are actually not necessary. In
> some cases, changing the size of those components corrupts the RPC
> message contained in the buffer.
That's really burying the lede.... Is there an actual user-visible bug
here?
--b.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> include/linux/sunrpc/xdr.h | 1 -
> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++---
> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> net/sunrpc/xdr.c | 41 -----------------------------------
> 4 files changed, 6 insertions(+), 46 deletions(-)
>
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 69161cb..4ae398c 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le
> extern void xdr_shift_buf(struct xdr_buf *, size_t);
> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int);
> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> index 5cdde6c..14a0aff 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift)
> */
> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len);
> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
> - buf->head[0].iov_len);
> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
> + buf->head[0].iov_len)
> + return GSS_S_FAILURE;
> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
>
> /* Trim off the trailing "extra count" and checksum blob */
> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip;
> +
> return GSS_S_COMPLETE;
> }
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 152790e..f1aabab 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom)
> if (svc_getnl(&buf->head[0]) != seq)
> goto out;
> /* trim off the mic and padding at the end before returning */
> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
> + buf->len -= 4 + round_up_to_quad(mic.len);
> stat = 0;
> out:
> kfree(mic.data);
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 5f0aa53..4bce619 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len)
> }
> EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
>
> -/**
> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf"
> - * @buf: buf to be trimmed
> - * @len: number of bytes to reduce "buf" by
> - *
> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note
> - * that it's possible that we'll trim less than that amount if the xdr_buf is
> - * too small, or if (for instance) it's all in the head and the parser has
> - * already read too far into it.
> - */
> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
> -{
> - size_t cur;
> - unsigned int trim = len;
> -
> - if (buf->tail[0].iov_len) {
> - cur = min_t(size_t, buf->tail[0].iov_len, trim);
> - buf->tail[0].iov_len -= cur;
> - trim -= cur;
> - if (!trim)
> - goto fix_len;
> - }
> -
> - if (buf->page_len) {
> - cur = min_t(unsigned int, buf->page_len, trim);
> - buf->page_len -= cur;
> - trim -= cur;
> - if (!trim)
> - goto fix_len;
> - }
> -
> - if (buf->head[0].iov_len) {
> - cur = min_t(size_t, buf->head[0].iov_len, trim);
> - buf->head[0].iov_len -= cur;
> - trim -= cur;
> - }
> -fix_len:
> - buf->len -= (len - trim);
> -}
> -EXPORT_SYMBOL_GPL(xdr_buf_trim);
> -
> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len)
> {
> unsigned int this_len;
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
2019-02-04 19:46 ` J. Bruce Fields
@ 2019-02-04 19:49 ` Chuck Lever
2019-02-04 20:00 ` Bruce Fields
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-04 19:49 UTC (permalink / raw)
To: Bruce Fields; +Cc: Linux NFS Mailing List, simo
> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote:
>
> On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote:
>> The key action of xdr_buf_trim() is that it shortens buf->len, the
>> length of the xdr_buf' content. The other actions -- shortening the
>> head, pages, and tail components -- are actually not necessary. In
>> some cases, changing the size of those components corrupts the RPC
>> message contained in the buffer.
>
> That's really burying the lede.... Is there an actual user-visible bug
> here?
I don't think so. This is more of the form:
a) the function does fundamentally the wrong thing, so
b) certain changes to this code path result is unexpected and incorrect
behavior
Thus typically only developers hacking on this code run into a problem.
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/linux/sunrpc/xdr.h | 1 -
>> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++---
>> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
>> net/sunrpc/xdr.c | 41 -----------------------------------
>> 4 files changed, 6 insertions(+), 46 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>> index 69161cb..4ae398c 100644
>> --- a/include/linux/sunrpc/xdr.h
>> +++ b/include/linux/sunrpc/xdr.h
>> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le
>> extern void xdr_shift_buf(struct xdr_buf *, size_t);
>> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
>> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
>> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int);
>> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
>> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
>> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
>> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
>> index 5cdde6c..14a0aff 100644
>> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
>> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
>> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift)
>> */
>> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len);
>> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
>> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
>> - buf->head[0].iov_len);
>> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
>> + buf->head[0].iov_len)
>> + return GSS_S_FAILURE;
>> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
>> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
>> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
>>
>> /* Trim off the trailing "extra count" and checksum blob */
>> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
>> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip;
>> +
>> return GSS_S_COMPLETE;
>> }
>>
>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>> index 152790e..f1aabab 100644
>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom)
>> if (svc_getnl(&buf->head[0]) != seq)
>> goto out;
>> /* trim off the mic and padding at the end before returning */
>> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
>> + buf->len -= 4 + round_up_to_quad(mic.len);
>> stat = 0;
>> out:
>> kfree(mic.data);
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 5f0aa53..4bce619 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len)
>> }
>> EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
>>
>> -/**
>> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf"
>> - * @buf: buf to be trimmed
>> - * @len: number of bytes to reduce "buf" by
>> - *
>> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note
>> - * that it's possible that we'll trim less than that amount if the xdr_buf is
>> - * too small, or if (for instance) it's all in the head and the parser has
>> - * already read too far into it.
>> - */
>> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
>> -{
>> - size_t cur;
>> - unsigned int trim = len;
>> -
>> - if (buf->tail[0].iov_len) {
>> - cur = min_t(size_t, buf->tail[0].iov_len, trim);
>> - buf->tail[0].iov_len -= cur;
>> - trim -= cur;
>> - if (!trim)
>> - goto fix_len;
>> - }
>> -
>> - if (buf->page_len) {
>> - cur = min_t(unsigned int, buf->page_len, trim);
>> - buf->page_len -= cur;
>> - trim -= cur;
>> - if (!trim)
>> - goto fix_len;
>> - }
>> -
>> - if (buf->head[0].iov_len) {
>> - cur = min_t(size_t, buf->head[0].iov_len, trim);
>> - buf->head[0].iov_len -= cur;
>> - trim -= cur;
>> - }
>> -fix_len:
>> - buf->len -= (len - trim);
>> -}
>> -EXPORT_SYMBOL_GPL(xdr_buf_trim);
>> -
>> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len)
>> {
>> unsigned int this_len;
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
2019-02-04 19:49 ` Chuck Lever
@ 2019-02-04 20:00 ` Bruce Fields
2019-02-04 20:07 ` Chuck Lever
0 siblings, 1 reply; 31+ messages in thread
From: Bruce Fields @ 2019-02-04 20:00 UTC (permalink / raw)
To: Chuck Lever; +Cc: Linux NFS Mailing List, simo
On Mon, Feb 04, 2019 at 02:49:11PM -0500, Chuck Lever wrote:
>
>
> > On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote:
> >
> > On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote:
> >> The key action of xdr_buf_trim() is that it shortens buf->len, the
> >> length of the xdr_buf' content. The other actions -- shortening the
> >> head, pages, and tail components -- are actually not necessary. In
> >> some cases, changing the size of those components corrupts the RPC
> >> message contained in the buffer.
> >
> > That's really burying the lede.... Is there an actual user-visible bug
> > here?
>
> I don't think so. This is more of the form:
>
> a) the function does fundamentally the wrong thing, so
>
> b) certain changes to this code path result is unexpected and incorrect
> behavior
>
> Thus typically only developers hacking on this code run into a problem.
OK, got it. It'd help just to make it clear in the changelog that that
this is an accident waiting to happen rather than a current bug (as far
as we know).
--b.
>
>
> > --b.
> >
> >>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> include/linux/sunrpc/xdr.h | 1 -
> >> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++---
> >> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> >> net/sunrpc/xdr.c | 41 -----------------------------------
> >> 4 files changed, 6 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> >> index 69161cb..4ae398c 100644
> >> --- a/include/linux/sunrpc/xdr.h
> >> +++ b/include/linux/sunrpc/xdr.h
> >> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le
> >> extern void xdr_shift_buf(struct xdr_buf *, size_t);
> >> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
> >> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
> >> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int);
> >> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
> >> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
> >> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
> >> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> >> index 5cdde6c..14a0aff 100644
> >> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> >> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> >> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift)
> >> */
> >> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len);
> >> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
> >> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
> >> - buf->head[0].iov_len);
> >> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
> >> + buf->head[0].iov_len)
> >> + return GSS_S_FAILURE;
> >> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
> >> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> >> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
> >>
> >> /* Trim off the trailing "extra count" and checksum blob */
> >> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
> >> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip;
> >> +
> >> return GSS_S_COMPLETE;
> >> }
> >>
> >> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> >> index 152790e..f1aabab 100644
> >> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> >> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> >> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom)
> >> if (svc_getnl(&buf->head[0]) != seq)
> >> goto out;
> >> /* trim off the mic and padding at the end before returning */
> >> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
> >> + buf->len -= 4 + round_up_to_quad(mic.len);
> >> stat = 0;
> >> out:
> >> kfree(mic.data);
> >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >> index 5f0aa53..4bce619 100644
> >> --- a/net/sunrpc/xdr.c
> >> +++ b/net/sunrpc/xdr.c
> >> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len)
> >> }
> >> EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
> >>
> >> -/**
> >> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf"
> >> - * @buf: buf to be trimmed
> >> - * @len: number of bytes to reduce "buf" by
> >> - *
> >> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note
> >> - * that it's possible that we'll trim less than that amount if the xdr_buf is
> >> - * too small, or if (for instance) it's all in the head and the parser has
> >> - * already read too far into it.
> >> - */
> >> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
> >> -{
> >> - size_t cur;
> >> - unsigned int trim = len;
> >> -
> >> - if (buf->tail[0].iov_len) {
> >> - cur = min_t(size_t, buf->tail[0].iov_len, trim);
> >> - buf->tail[0].iov_len -= cur;
> >> - trim -= cur;
> >> - if (!trim)
> >> - goto fix_len;
> >> - }
> >> -
> >> - if (buf->page_len) {
> >> - cur = min_t(unsigned int, buf->page_len, trim);
> >> - buf->page_len -= cur;
> >> - trim -= cur;
> >> - if (!trim)
> >> - goto fix_len;
> >> - }
> >> -
> >> - if (buf->head[0].iov_len) {
> >> - cur = min_t(size_t, buf->head[0].iov_len, trim);
> >> - buf->head[0].iov_len -= cur;
> >> - trim -= cur;
> >> - }
> >> -fix_len:
> >> - buf->len -= (len - trim);
> >> -}
> >> -EXPORT_SYMBOL_GPL(xdr_buf_trim);
> >> -
> >> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len)
> >> {
> >> unsigned int this_len;
>
> --
> Chuck Lever
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
2019-02-04 20:00 ` Bruce Fields
@ 2019-02-04 20:07 ` Chuck Lever
2019-02-04 20:11 ` Bruce Fields
0 siblings, 1 reply; 31+ messages in thread
From: Chuck Lever @ 2019-02-04 20:07 UTC (permalink / raw)
To: Bruce Fields; +Cc: Linux NFS Mailing List, simo
> On Feb 4, 2019, at 3:00 PM, Bruce Fields <bfields@fieldses.org> wrote:
>
> On Mon, Feb 04, 2019 at 02:49:11PM -0500, Chuck Lever wrote:
>>
>>
>>> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote:
>>>
>>> On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote:
>>>> The key action of xdr_buf_trim() is that it shortens buf->len, the
>>>> length of the xdr_buf' content. The other actions -- shortening the
>>>> head, pages, and tail components -- are actually not necessary. In
>>>> some cases, changing the size of those components corrupts the RPC
>>>> message contained in the buffer.
>>>
>>> That's really burying the lede.... Is there an actual user-visible bug
>>> here?
>>
>> I don't think so. This is more of the form:
>>
>> a) the function does fundamentally the wrong thing, so
>>
>> b) certain changes to this code path result is unexpected and incorrect
>> behavior
>>
>> Thus typically only developers hacking on this code run into a problem.
>
> OK, got it. It'd help just to make it clear in the changelog that that
> this is an accident waiting to happen rather than a current bug (as far
> as we know).
With said improvement to the changelog, can I add your Acked-by
when I submit this through Anna's tree?
> --b.
>
>>
>>
>>> --b.
>>>
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> include/linux/sunrpc/xdr.h | 1 -
>>>> net/sunrpc/auth_gss/gss_krb5_wrap.c | 8 ++++---
>>>> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
>>>> net/sunrpc/xdr.c | 41 -----------------------------------
>>>> 4 files changed, 6 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>>>> index 69161cb..4ae398c 100644
>>>> --- a/include/linux/sunrpc/xdr.h
>>>> +++ b/include/linux/sunrpc/xdr.h
>>>> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, const void *s, unsigned int le
>>>> extern void xdr_shift_buf(struct xdr_buf *, size_t);
>>>> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
>>>> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
>>>> -extern void xdr_buf_trim(struct xdr_buf *, unsigned int);
>>>> extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
>>>> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
>>>> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
>>>> diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c
>>>> index 5cdde6c..14a0aff 100644
>>>> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
>>>> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
>>>> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct xdr_buf *buf, unsigned int shift)
>>>> */
>>>> movelen = min_t(unsigned int, buf->head[0].iov_len, buf->len);
>>>> movelen -= offset + GSS_KRB5_TOK_HDR_LEN + headskip;
>>>> - BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
>>>> - buf->head[0].iov_len);
>>>> + if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
>>>> + buf->head[0].iov_len)
>>>> + return GSS_S_FAILURE;
>>>> memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
>>>> buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip;
>>>> buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip;
>>>>
>>>> /* Trim off the trailing "extra count" and checksum blob */
>>>> - xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
>>>> + buf->len -= ec + GSS_KRB5_TOK_HDR_LEN + tailskip;
>>>> +
>>>> return GSS_S_COMPLETE;
>>>> }
>>>>
>>>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>>>> index 152790e..f1aabab 100644
>>>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>>>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>>>> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom)
>>>> if (svc_getnl(&buf->head[0]) != seq)
>>>> goto out;
>>>> /* trim off the mic and padding at the end before returning */
>>>> - xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
>>>> + buf->len -= 4 + round_up_to_quad(mic.len);
>>>> stat = 0;
>>>> out:
>>>> kfree(mic.data);
>>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>>> index 5f0aa53..4bce619 100644
>>>> --- a/net/sunrpc/xdr.c
>>>> +++ b/net/sunrpc/xdr.c
>>>> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, unsigned int len)
>>>> }
>>>> EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
>>>>
>>>> -/**
>>>> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf"
>>>> - * @buf: buf to be trimmed
>>>> - * @len: number of bytes to reduce "buf" by
>>>> - *
>>>> - * Trim an xdr_buf by the given number of bytes by fixing up the lengths. Note
>>>> - * that it's possible that we'll trim less than that amount if the xdr_buf is
>>>> - * too small, or if (for instance) it's all in the head and the parser has
>>>> - * already read too far into it.
>>>> - */
>>>> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
>>>> -{
>>>> - size_t cur;
>>>> - unsigned int trim = len;
>>>> -
>>>> - if (buf->tail[0].iov_len) {
>>>> - cur = min_t(size_t, buf->tail[0].iov_len, trim);
>>>> - buf->tail[0].iov_len -= cur;
>>>> - trim -= cur;
>>>> - if (!trim)
>>>> - goto fix_len;
>>>> - }
>>>> -
>>>> - if (buf->page_len) {
>>>> - cur = min_t(unsigned int, buf->page_len, trim);
>>>> - buf->page_len -= cur;
>>>> - trim -= cur;
>>>> - if (!trim)
>>>> - goto fix_len;
>>>> - }
>>>> -
>>>> - if (buf->head[0].iov_len) {
>>>> - cur = min_t(size_t, buf->head[0].iov_len, trim);
>>>> - buf->head[0].iov_len -= cur;
>>>> - trim -= cur;
>>>> - }
>>>> -fix_len:
>>>> - buf->len -= (len - trim);
>>>> -}
>>>> -EXPORT_SYMBOL_GPL(xdr_buf_trim);
>>>> -
>>>> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void *obj, unsigned int len)
>>>> {
>>>> unsigned int this_len;
>>
>> --
>> Chuck Lever
--
Chuck Lever
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
2019-02-04 20:07 ` Chuck Lever
@ 2019-02-04 20:11 ` Bruce Fields
0 siblings, 0 replies; 31+ messages in thread
From: Bruce Fields @ 2019-02-04 20:11 UTC (permalink / raw)
To: Chuck Lever; +Cc: Linux NFS Mailing List, simo
On Mon, Feb 04, 2019 at 03:07:26PM -0500, Chuck Lever wrote:
>
>
> > On Feb 4, 2019, at 3:00 PM, Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Mon, Feb 04, 2019 at 02:49:11PM -0500, Chuck Lever wrote:
> >>
> >>
> >>> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote:
> >>>
> >>> On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote:
> >>>> The key action of xdr_buf_trim() is that it shortens buf->len, the
> >>>> length of the xdr_buf' content. The other actions -- shortening the
> >>>> head, pages, and tail components -- are actually not necessary. In
> >>>> some cases, changing the size of those components corrupts the RPC
> >>>> message contained in the buffer.
> >>>
> >>> That's really burying the lede.... Is there an actual user-visible bug
> >>> here?
> >>
> >> I don't think so. This is more of the form:
> >>
> >> a) the function does fundamentally the wrong thing, so
> >>
> >> b) certain changes to this code path result is unexpected and incorrect
> >> behavior
> >>
> >> Thus typically only developers hacking on this code run into a problem.
> >
> > OK, got it. It'd help just to make it clear in the changelog that that
> > this is an accident waiting to happen rather than a current bug (as far
> > as we know).
>
> With said improvement to the changelog, can I add your Acked-by
> when I submit this through Anna's tree?
Sure, thanks.
--b.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants
2019-02-04 19:37 ` J. Bruce Fields
@ 2019-02-05 1:57 ` Tom Talpey
0 siblings, 0 replies; 31+ messages in thread
From: Tom Talpey @ 2019-02-05 1:57 UTC (permalink / raw)
To: J. Bruce Fields, Chuck Lever
Cc: Christoph Hellwig, Linux NFS Mailing List, simo
On 2/4/2019 2:37 PM, J. Bruce Fields wrote:
> On Mon, Feb 04, 2019 at 09:56:20AM -0500, Chuck Lever wrote:
>>
>>
>>> On Feb 4, 2019, at 9:32 AM, Christoph Hellwig <hch@infradead.org> wrote:
>>>
>>> On Mon, Feb 04, 2019 at 09:16:54AM -0500, Chuck Lever wrote:
>>>> They are. The problem is that we are byte-swapping the incoming wire
>>>> data and then comparing to CPU-endian constants in some hot paths.
>>>> It's better to leave the incoming data alone and compare to a pre-
>>>> byte-swapped constant. This patch adds some of these constants that
>>>> were missing, in preparation for fixing the hot paths.
>>>>
>>>> That is apparently not clear from the patch description, so I will
>>>> endeavor to improve it.
>>>
>>> Why do we need new enums / #defines for that?
>>>
>>> Just replace:
>>>
>>> if (beXX_to_cpu(pkt->field) == SOME_CONSTANT)
>>>
>>> with
>>>
>>> if (pkt->field == cpu_to_be32(SOME_CONSTANT))
>>>
>>> and we are done.
>>
>> True.
>
> I'm a little surprised the compiler couldn't do that automatically.
> (Disclaimer: I know nothing about compilers.)
>
>>> The latter is a pretty common pattern through the kernel.
>>
>> However the pattern in the NFS server and lockd is to define a
>> lower-case version of the same macro that is pre-byte-swapped.
>> I'm attempting to follow existing precedent in this area.
>
> I've never really understood why that was done. (Not saying it was
> wrong, just that I don't understand it.) As long as you're reading the
> value, how could byte-swapping it actually add a significant expense?
Shifts and Rotates are expensive on many processors, and hard to
pipeline for byteawap because each step needs to wait for the result
of the previous.
Maybe this is less of an issue with modern processors, but I'd
certainly not be surprised that embedded processors "did it the
hard way".
Tom.
>
> The one thing I do like is that I can look at e.g.:
>
> int nfsd4_get_nfs4_acl ...
> __be32 nfsd4_set_nfs4_acl ...
>
> and tell that the former returns a linux errno, the latter an NFS error.
>
> --b.
>
>>
>> We already have, for example, in various sunrpc headers:
>>
>> enum rpc_accept_stat {
>> RPC_SUCCESS = 0,
>> RPC_PROG_UNAVAIL = 1,
>> RPC_PROG_MISMATCH = 2,
>> ...
>> };
>>
>> #define rpc_success cpu_to_be32(RPC_SUCCESS)
>> #define rpc_prog_unavail cpu_to_be32(RPC_PROG_UNAVAIL)
>> #define rpc_prog_mismatch cpu_to_be32(RPC_PROG_MISMATCH)
>>
>> Or, for NFS:
>>
>> enum nfs_stat {
>> ...
>> NFSERR_SHARE_DENIED = 10015,
>> ...
>> };
>>
>> #define nfserr_share_denied cpu_to_be32(NFSERR_SHARE_DENIED)
>>
>> There are some missing lower-case macros, which I am trying to
>> add to our existing collection before I rewrite the RPC header
>> encoding and decoding functions. So I'm adding:
>>
>> + rpc_gss_version = cpu_to_be32(RPC_GSS_VERSION),
>>
>> + rpc_call = cpu_to_be32(RPC_CALL),
>> + rpc_reply = cpu_to_be32(RPC_REPLY),
>> +
>> + rpc_msg_accepted = cpu_to_be32(RPC_MSG_ACCEPTED),
>> + rpc_msg_denied = cpu_to_be32(RPC_MSG_DENIED),
>>
>> Actually since we have decided not to use enum for these, this
>> smaller addition can simply be squashed into the later patches,
>> and I can drop this patch, which was intended as a clean-up but
>> now appears to be unnecessary.
>>
>> --
>> Chuck Lever
>>
>>
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2019-02-05 1:57 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-01 19:57 [PATCH RFC 00/10] SUNRPC GSS overhaul Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 01/10] SUNRPC: Remove some dprintk() call sites from auth functions Chuck Lever
2019-02-04 19:04 ` J. Bruce Fields
2019-02-04 19:07 ` Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 02/10] SUNRPC: Remove rpc_xprt::tsh_size Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 03/10] SUNRPC: Add build option to disable support for insecure enctypes Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 04/10] SUNRPC: Add common byte-swapped RPC header constants Chuck Lever
2019-02-02 2:30 ` Tom Talpey
2019-02-02 22:46 ` Chuck Lever
2019-02-03 15:00 ` Trond Myklebust
2019-02-03 16:49 ` Chuck Lever
2019-02-03 18:58 ` Trond Myklebust
2019-02-02 17:02 ` Christoph Hellwig
2019-02-02 22:49 ` Chuck Lever
2019-02-04 7:53 ` Christoph Hellwig
2019-02-04 14:16 ` Chuck Lever
2019-02-04 14:32 ` Christoph Hellwig
2019-02-04 14:56 ` Chuck Lever
2019-02-04 19:37 ` J. Bruce Fields
2019-02-05 1:57 ` Tom Talpey
2019-02-01 19:57 ` [PATCH RFC 05/10] SUNRPC: Use struct xdr_stream when constructing RPC Call header Chuck Lever
2019-02-01 19:57 ` [PATCH RFC 06/10] SUNRPC: Clean up rpc_verify_header() Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 07/10] SUNRPC: Use struct xdr_stream when decoding RPC Reply header Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 08/10] SUNRPC: Introduce trace points in rpc_auth_gss.ko Chuck Lever
2019-02-01 19:58 ` [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim() Chuck Lever
2019-02-04 19:46 ` J. Bruce Fields
2019-02-04 19:49 ` Chuck Lever
2019-02-04 20:00 ` Bruce Fields
2019-02-04 20:07 ` Chuck Lever
2019-02-04 20:11 ` Bruce Fields
2019-02-01 19:58 ` [PATCH RFC 10/10] SUNRPC: Add SPDX IDs to some net/sunrpc/auth_gss/ files Chuck Lever
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).