Five missing NFSD fixes for v5.10.y
Subsystems (3)
[email protected]
maintainercover Five missing NFSD fixes for v5.10.y
From: Chuck Lever <[email protected]>
Hi-
It was pointed out that the NFSD fixes that went into 5.10.220 were missing a few forward fixes from upstream. These five are the ones I identified. I've run them through the usual NFSD CI tests and found no new issues.
Chuck Lever (3):
SUNRPC: Fix a NULL pointer deref in trace_svc_stats_latency()
SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation
SUNRPC: Fix svcxdr_init_encode's buflen calculation
Jeff Layton (1):
nfsd: hold a lighter-weight client reference over CB_RECALL_ANY
Yunjian Wang (1):
SUNRPC: Fix null pointer dereference in svc_rqst_free()
fs/nfsd/nfs4state.c | 7 ++----- include/linux/sunrpc/svc.h | 20 ++++++++++++++++---- include/trace/events/sunrpc.h | 8 ++++---- net/sunrpc/svc.c | 18 +++++++++++++++++- 4 files changed, 39 insertions(+), 14 deletions(-)
Signature · 1 line
2.45.1
[email protected]
maintainerPATCH 1/5 SUNRPC: Fix null pointer dereference in svc_rqst_free()
From: Yunjian Wang <[email protected]>
[ Upstream commit b9f83ffaa0c096b4c832a43964fe6bff3acffe10 ]
When alloc_pages_node() returns null in svc_rqst_alloc(), the null rq_scratch_page pointer will be dereferenced when calling put_page() in svc_rqst_free(). Fix it by adding a null check.
Addresses-Coverity: ("Dereference after null check")
--- net/sunrpc/svc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/sunrpc/svc.c +2 -1
@@ -845,7 +845,8 @@ void svc_rqst_free(struct svc_rqst *rqstp) {svc_release_buffer (rqstp); -put_page (rqstp->rq_scratch_page); +if (rqstp->rq_scratch_page) +put_page (rqstp->rq_scratch_page);kfree (rqstp->rq_resp);kfree (rqstp->rq_argp);kfree (rqstp->rq_auth_data);
Signature · 1 line
2.45.1
[email protected]
maintainerPATCH 2/5 SUNRPC: Fix a NULL pointer deref in trace_svc_stats_latency()
In reply to [email protected]From: Chuck Lever <[email protected]>
[ Upstream commit 5c11720767f70d34357d00a15ba5a0ad052c40fe ]
Some paths through svc_process() leave rqst->rq_procinfo set to NULL, which triggers a crash if tracing happens to be enabled.
--- include/linux/sunrpc/svc.h | 1 + include/trace/events/sunrpc.h | 8 ++++---- net/sunrpc/svc.c | 15 +++++++++++++++ 3 files changed, 20 insertions(+), 4 deletions(-)
include/linux/sunrpc/svc.h +1 -0
@@ -498,6 +498,7 @@ void svc_wake_up(struct svc_serv *);void svc_reserve (struct svc_rqst *rqstp,int space);struct svc_pool *svc_pool_for_cpu (struct svc_serv *serv,int cpu);char *svc_print_addr (struct svc_rqst *,char *,size_t ); +const char *svc_proc_name (const struct svc_rqst *rqstp);int svc_encode_result_payload (struct svc_rqst *rqstp,unsigned int offset,unsigned int length);
include/trace/events/sunrpc.h +4 -4
@@ -1578,7 +1578,7 @@ TRACE_EVENT(svc_process, __field(u32, vers) __field(u32, proc) __string(service, name) - __string(procedure, rqst->rq_procinfo->pc_name) + __string(procedure, svc_proc_name(rqst))__string (addr, rqst->rq_xprt ? rqst->rq_xprt->xpt_remotebuf :"(null)" ) ),
@@ -1588,7 +1588,7 @@ TRACE_EVENT(svc_process, __entry->vers = rqst->rq_vers; __entry->proc = rqst->rq_proc;__assign_str (service, name); -__assign_str (procedure, rqst->rq_procinfo->pc_name); +__assign_str (procedure,svc_proc_name (rqst));__assign_str (addr, rqst->rq_xprt ? rqst->rq_xprt->xpt_remotebuf :"(null)" ); ),
@@ -1854,7 +1854,7 @@ TRACE_EVENT(svc_stats_latency,TP_STRUCT__entry (__field (u32 ,xid ) __field(unsigned long, execute) - __string(procedure, rqst->rq_procinfo->pc_name) +__string (procedure,svc_proc_name (rqst)) __string(addr, rqst->rq_xprt->xpt_remotebuf) ),
@@ -1862,7 +1862,7 @@ TRACE_EVENT(svc_stats_latency, __entry->xid =be32_to_cpu (rqst->rq_xid); __entry->execute =ktime_to_us (ktime_sub (ktime_get (), rqst->rq_stime)); -__assign_str (procedure, rqst->rq_procinfo->pc_name); +__assign_str (procedure,svc_proc_name (rqst));__assign_str (addr, rqst->rq_xprt->xpt_remotebuf); ),
net/sunrpc/svc.c +15 -0
@@ -1612,6 +1612,21 @@ u32 svc_max_payload(const struct svc_rqst *rqstp) }EXPORT_SYMBOL_GPL (svc_max_payload); +/** +* svc_proc_name - Return RPC procedure name in string form +* @rqstp: svc_rqst to operate on +* +* Return value: +* Pointer to a NUL-terminated string +*/ +const char *svc_proc_name (const struct svc_rqst *rqstp) +{ +if (rqstp && rqstp->rq_procinfo) +return rqstp->rq_procinfo->pc_name; +return "unknown" ; +} + +/** * svc_encode_result_payload - mark a range of bytes as a result payload * @rqstp: svc_rqst to operate on
Signature · 1 line
2.45.1
[email protected]
maintainerPATCH 3/5 SUNRPC: Fix svcxdr_init_decode's end-of-buffer calculation
In reply to [email protected]From: Chuck Lever <[email protected]>
[ Upstream commit 90bfc37b5ab91c1a6165e3e5cfc49bf04571b762 ]
Ensure that stream-based argument decoding can't go past the actual end of the receive buffer. xdr_init_decode's calculation of the value of xdr->end over-estimates the end of the buffer because the Linux kernel RPC server code does not remove the size of the RPC header from rqstp->rq_arg before calling the upper layer's dispatcher.
The server-side still uses the svc_getnl() macros to decode the RPC call header. These macros reduce the length of the head iov but do not update the total length of the message in the buffer (buf->len).
A proper fix for this would be to replace the use of svc_getnl() and friends in the RPC header decoder, but that would be a large and invasive change that would be difficult to backport.
--- include/linux/sunrpc/svc.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
include/linux/sunrpc/svc.h +14 -3
@@ -536,16 +536,27 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space) }/** -* svcxdr_init_decode - Prepare an xdr_stream for svc Call decoding +* svcxdr_init_decode - Prepare an xdr_stream for Call decoding * @rqstp: controlling server RPC transaction context * +* This function currently assumes the RPC header in rq_arg has +* already been decoded. Upon return, xdr->p points to the +* location of the upper layer header. */ static inline void svcxdr_init_decode (struct svc_rqst *rqstp) {struct xdr_stream *xdr = &rqstp->rq_arg_stream; -struct kvec *argv = rqstp->rq_arg.head; +struct xdr_buf *buf = &rqstp->rq_arg; +struct kvec *argv = buf->head; -xdr_init_decode (xdr, &rqstp->rq_arg, argv->iov_base,NULL ); +/* +* svc_getnl() and friends do not keep the xdr_buf's ::len +* field up to date. Refresh that field before initializing +* the argument decoding stream. +*/ + buf->len = buf->head->iov_len + buf->page_len + buf->tail->iov_len; + +xdr_init_decode (xdr, buf, argv->iov_base,NULL );xdr_set_scratch_page (xdr, rqstp->rq_scratch_page); }
Signature · 1 line
2.45.1
[email protected]
maintainerPATCH 4/5 SUNRPC: Fix svcxdr_init_encode's buflen calculation
In reply to [email protected]From: Chuck Lever <[email protected]>
[ Upstream commit 1242a87da0d8cd2a428e96ca68e7ea899b0f4624 ]
Commit 2825a7f90753 ("nfsd4: allow encoding across page boundaries") added an explicit computation of the remaining length in the rq_res XDR buffer.
The computation appears to suffer from an "off-by-one" bug. Because buflen is too large by one page, XDR encoding can run off the end of the send buffer by eventually trying to use the struct page address in rq_page_end, which always contains NULL.
--- include/linux/sunrpc/svc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/sunrpc/svc.h +1 -1
@@ -579,7 +579,7 @@ static inline void svcxdr_init_encode(struct svc_rqst *rqstp) xdr->end = resv->iov_base +PAGE_SIZE - rqstp->rq_auth_slack; buf->len = resv->iov_len; xdr->page_ptr = buf->pages -1 ; - buf->buflen =PAGE_SIZE * (1 + rqstp->rq_page_end - buf->pages); + buf->buflen =PAGE_SIZE * (rqstp->rq_page_end - buf->pages); buf->buflen -= rqstp->rq_auth_slack; xdr->rqst =NULL ; }
Signature · 1 line
2.45.1
[email protected]
maintainerPATCH 5/5 nfsd: hold a lighter-weight client reference over CB_RECALL_ANY
In reply to [email protected]From: Jeff Layton <[email protected]>
[ Upstream commit 10396f4df8b75ff6ab0aa2cd74296565466f2c8d ]
Currently the CB_RECALL_ANY job takes a cl_rpc_users reference to the client. While a callback job is technically an RPC that counter is really more for client-driven RPCs, and this has the effect of preventing the client from being unhashed until the callback completes.
If nfsd decides to send a CB_RECALL_ANY just as the client reboots, we can end up in a situation where the callback can't complete on the (now dead) callback channel, but the new client can't connect because the old client can't be unhashed. This usually manifests as a NFS4ERR_DELAY return on the CREATE_SESSION operation.
The job is only holding a reference to the client so it can clear a flag after the RPC completes. Fix this by having CB_RECALL_ANY instead hold a reference to the cl_nfsdfs.cl_ref. Typically we only take that sort of reference when dealing with the nfsdfs info files, but it should work appropriately here to ensure that the nfs4_client doesn't disappear.
--- fs/nfsd/nfs4state.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
fs/nfsd/nfs4state.c +2 -5
@@ -2888,12 +2888,9 @@ static void nfsd4_cb_recall_any_release(struct nfsd4_callback *cb) {struct nfs4_client *clp = cb->cb_clp; -struct nfsd_net *nn =net_generic (clp->net, nfsd_net_id); -spin_lock (&nn->client_lock);clear_bit (NFSD4_CLIENT_CB_RECALL_ANY , &clp->cl_flags); -put_client_renew_locked (clp); -spin_unlock (&nn->client_lock); +drop_client (clp); }static const struct nfsd4_callback_ops nfsd4_cb_recall_any_ops = {
@@ -6230,7 +6227,7 @@ deleg_reaper(struct nfsd_net *nn)list_add (&clp->cl_ra_cblist, &cblist);/* release in nfsd4_cb_recall_any_release */ -atomic_inc (&clp->cl_rpc_users); +kref_get (&clp->cl_nfsdfs.cl_ref);set_bit (NFSD4_CLIENT_CB_RECALL_ANY , &clp->cl_flags); clp->cl_ra_time =ktime_get_boottime_seconds (); }
Signature · 1 line
2.45.1