On 12/18/18 11:43 PM, Trond Myklebust wrote:
> On Tue, 2018-12-18 at 23:02 +0300, Vasily Averin wrote:
>> On 12/18/18 5:55 PM, Trond Myklebust wrote:
>>>>> It probably also requires us to store a pointer to struct net
>>>>> in
>>>>> the
>>>>> struct svc_rqst so that nfs4_callback_compound() and
>>>>> svcauth_gss_accept() can find it, but that should be OK since
>>>>> the
>>>>> transport already has that referenced.
>>
>> Ok, I can fix these functions and their sub-calls.
>> However rqst->rq_xprt is used in other functions that seems can be
>> called inside svc_process_common()
>> - in trace_svc_process(rqstp, progp->pg_name);
>> - in svc_reserve_auth(rqstp, ...) -> svc_reserve()
>> - svc_authorise() -> svcauth_gss_release()
>>
>> It seems I should fix these places too, it isn't?
>> could you please advise how to fix svc_reserve() ?
>
> We don't want svc_reserve() to run at all for the back channel, so I
> guess that a test for rqstp->rq_xprt != NULL is appropriate there too.
>
> svcauth_gss_release() is just using rqstp->rq_xprt to find the net
> namespace, so if you add a pointer rqstp->rq_net to fix
> nfs4_callback_compound, then that will fix the gss case as well.
>
> For trace_svc_process(), maybe pull rqst->rq_xprt->xpt_remotebuf out of
> the tracepoint definition in include/trace/events/sunrpc.h and make it
> a tracepoint argument that is allowed to be NULL?
This one seems works, could you please check it before formal submit ?
NFSv4 callback-1644 [002] .... 4731.064372: svc_process: addr=(null) xid=0x0b0924e3 service=NFSv4 callback vers=1 proc=1
Frankly speaking I'm afraid that I missed something,
rqstp->rq_xprt is widely used and nobody expect that it can be NULL.
And even I missed nothing -- it's quite tricky anyway.
Future cahnges can add new calls or execute old non-empty-xprt-aware
functions and trigger crash in some exotic configuration.
Thank you,
Vasily Averin
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 73e130a840ce..f87d2ba88869 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -295,9 +295,12 @@ struct svc_rqst {
struct svc_cacherep * rq_cacherep; /* cache info */
struct task_struct *rq_task; /* service thread */
spinlock_t rq_lock; /* per-request lock */
+ struct net * rq_bc_net; /* pointer to backchannel's
+ * net namespace
+ */
};
-#define SVC_NET(svc_rqst) (svc_rqst->rq_xprt->xpt_net)
+#define SVC_NET(rqst) (rqst->rq_xprt ? rqst->rq_xprt->xpt_net : rqst->rq_bc_net)
/*
* Rigorous type checking on sockaddr type conversions
diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index 28e384186c35..df4305be73d6 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -569,7 +569,7 @@ TRACE_EVENT(svc_process,
__field(u32, vers)
__field(u32, proc)
__string(service, name)
- __string(addr, rqst->rq_xprt->xpt_remotebuf)
+ __string(addr, rqst->rq_xprt ? rqst->rq_xprt->xpt_remotebuf : "(null)")
),
TP_fast_assign(
@@ -577,7 +577,7 @@ TRACE_EVENT(svc_process,
__entry->vers = rqst->rq_vers;
__entry->proc = rqst->rq_proc;
__assign_str(service, name);
- __assign_str(addr, rqst->rq_xprt->xpt_remotebuf);
+ __assign_str(addr, rqst->rq_xprt ? rqst->rq_xprt->xpt_remotebuf : "(null)");
),
TP_printk("addr=%s xid=0x%08x service=%s vers=%u proc=%u",
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 1ece4bc3eb8d..152790ed309c 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1142,7 +1142,7 @@ static int svcauth_gss_legacy_init(struct svc_rqst *rqstp,
struct kvec *resv = &rqstp->rq_res.head[0];
struct rsi *rsip, rsikey;
int ret;
- struct sunrpc_net *sn = net_generic(rqstp->rq_xprt->xpt_net, sunrpc_net_id);
+ struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
memset(&rsikey, 0, sizeof(rsikey));
ret = gss_read_verf(gc, argv, authp,
@@ -1253,7 +1253,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,
uint64_t handle;
int status;
int ret;
- struct net *net = rqstp->rq_xprt->xpt_net;
+ struct net *net = SVC_NET(rqstp);
struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
memset(&ud, 0, sizeof(ud));
@@ -1444,7 +1444,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp, __be32 *authp)
__be32 *rpcstart;
__be32 *reject_stat = resv->iov_base + resv->iov_len;
int ret;
- struct sunrpc_net *sn = net_generic(rqstp->rq_xprt->xpt_net, sunrpc_net_id);
+ struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
dprintk("RPC: svcauth_gss: argv->iov_len = %zd\n",
argv->iov_len);
@@ -1734,7 +1734,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)
struct rpc_gss_wire_cred *gc = &gsd->clcred;
struct xdr_buf *resbuf = &rqstp->rq_res;
int stat = -EINVAL;
- struct sunrpc_net *sn = net_generic(rqstp->rq_xprt->xpt_net, sunrpc_net_id);
+ struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);
if (gc->gc_proc != RPC_GSS_PROC_DATA)
goto out;
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index a7264fd1b3db..6ebb0324748f 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1148,7 +1148,8 @@ static __printf(2,3) void svc_printk(struct svc_rqst *rqstp, const char *fmt, ..
* Common routine for processing the RPC request.
*/
static int
-svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
+svc_process_common(struct svc_rqst *rqstp, struct kvec *argv,
+ struct kvec *resv, const struct svc_xprt_ops *xops)
{
struct svc_program *progp;
const struct svc_version *versp = NULL; /* compiler food */
@@ -1172,7 +1173,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
clear_bit(RQ_DROPME, &rqstp->rq_flags);
/* Setup reply header */
- rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp);
+ xops->xpo_prep_reply_hdr(rqstp);
svc_putu32(resv, rqstp->rq_xid);
@@ -1244,7 +1245,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
* for lower versions. RPC_PROG_MISMATCH seems to be the closest
* fit.
*/
- if (versp->vs_need_cong_ctrl &&
+ if (versp->vs_need_cong_ctrl && rqstp->rq_xprt &&
!test_bit(XPT_CONG_CTRL, &rqstp->rq_xprt->xpt_flags))
goto err_bad_vers;
@@ -1336,7 +1337,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
return 0;
close:
- if (test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
+ if (rqstp->rq_xprt && test_bit(XPT_TEMP, &rqstp->rq_xprt->xpt_flags))
svc_close_xprt(rqstp->rq_xprt);
dprintk("svc: svc_process close\n");
return 0;
@@ -1432,7 +1433,8 @@ svc_process(struct svc_rqst *rqstp)
}
/* Returns 1 for send, 0 for drop */
- if (likely(svc_process_common(rqstp, argv, resv)))
+ if (likely(svc_process_common(rqstp, argv, resv,
+ rqstp->rq_xprt->xpt_ops)))
return svc_send(rqstp);
out_drop:
@@ -1465,10 +1467,10 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
goto proc_error;
/* Build the svc_rqst used by the common processing routine */
- rqstp->rq_xprt = s_xprt;
rqstp->rq_xid = req->rq_xid;
rqstp->rq_prot = req->rq_xprt->prot;
rqstp->rq_server = serv;
+ rqstp->rq_bc_net = net;
rqstp->rq_addrlen = sizeof(req->rq_xprt->addr);
memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen);
@@ -1499,8 +1501,8 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
svc_getnl(argv); /* CALLDIR */
/* Parse and execute the bc call */
- proc_error = svc_process_common(rqstp, argv, resv);
- svc_xprt_put(rqstp->rq_xprt);
+ proc_error = svc_process_common(rqstp, argv, resv, s_xprt->xpt_ops);
+ svc_xprt_put(s_xprt);
atomic_inc(&req->rq_xprt->bc_free_slots);
if (!proc_error)
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 51d36230b6e3..51da7c244bee 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -468,10 +468,11 @@ static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
*/
void svc_reserve(struct svc_rqst *rqstp, int space)
{
+ struct svc_xprt *xprt = rqstp->rq_xprt;
+
space += rqstp->rq_res.head[0].iov_len;
- if (space < rqstp->rq_reserved) {
- struct svc_xprt *xprt = rqstp->rq_xprt;
+ if (xprt && (space < rqstp->rq_reserved)) {
atomic_sub((rqstp->rq_reserved - space), &xprt->xpt_reserved);
rqstp->rq_reserved = space;
