From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:32:56
The purpose of this series is to replace the svc_put* macros in the
Linux kernel server's RPC reply header construction code with
xdr_stream helpers. I've measured no change in CPU utilization after
the overhaul.
Memory safety: Buffer bounds checking after encoding each XDR item
is more memory-safe than the current mechanism. Subsequent memory
safety improvements to the common xdr_stream helpers will benefit
all who use them.
Audit friendliness: The new code has additional comments and other
clean-up to help align it with the relevant RPC protocol
specifications. The use of common helpers also makes the encoders
easier to audit and maintain.
I've split the full series in half to make it easier to review. The
patches posted here are the second half, handling RPC reply header
encoding.
Note that another benefit of this work is that we are taking one or
two more strides closer to greater commonality between the client
and server implementations of RPCSEC GSS.
---
Chuck Lever (27):
SUNRPC: Clean up svcauth_gss_release()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
SUNRPC: Check rq_auth_stat when preparing to wrap a response
SUNRPC: Remove the rpc_stat variable in svc_process_common()
SUNRPC: Add XDR encoding helper for opaque_auth
SUNRPC: Push svcxdr_init_encode() into svc_process_common()
SUNRPC: Move svcxdr_init_encode() into ->accept methods
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
SUNRPC: Convert unwrap data paths to use xdr_stream for replies
SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
SUNRPC: Use xdr_stream for encoding GSS reply verifiers
SUNRPC: Hoist init_encode out of svc_authenticate()
SUNRPC: Convert RPC Reply header encoding to use xdr_stream
SUNRPC: Final clean-up of svc_process_common()
SUNRPC: Remove no-longer-used helper functions
SUNRPC: Refactor RPC server dispatch method
SUNRPC: Set rq_accept_statp inside ->accept methods
SUNRPC: Go back to using gsd->body_start
fs/lockd/svc.c | 5 +-
fs/nfs/callback_xdr.c | 6 +-
fs/nfsd/nfscache.c | 4 +-
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 10 +-
include/linux/sunrpc/svc.h | 116 +++----
include/linux/sunrpc/xdr.h | 23 ++
include/trace/events/rpcgss.h | 22 ++
net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
net/sunrpc/svc.c | 91 +++---
net/sunrpc/svcauth_unix.c | 40 ++-
net/sunrpc/xdr.c | 29 ++
12 files changed, 451 insertions(+), 402 deletions(-)
--
Chuck Lever
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:28:58
From: Chuck Lever <redacted>
Replace finicky logic: Instead of trying to find scratch space in
the response buffer, use the scratch buffer from struct
gss_svc_data.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
@@ -1778,40 +1778,39 @@ svcauth_gss_prepare_to_wrap(struct xdr_buf *resbuf, struct gss_svc_data *gsd)staticintsvcauth_gss_wrap_integ(structsvc_rqst*rqstp){structgss_svc_data*gsd=rqstp->rq_auth_data;+structxdr_stream*xdr=&rqstp->rq_res_stream;structrpc_gss_wire_cred*gc=&gsd->clcred;-structxdr_buf*buf=&rqstp->rq_res;+structxdr_buf*buf=xdr->buf;structxdr_bufdatabody_integ;structxdr_netobjchecksum;u32offset,len,maj_stat;-structkvec*resv;__be32*p;p=svcauth_gss_prepare_to_wrap(buf,gsd);if(p==NULL)gotoout;+offset=(u8*)(p+1)-(u8*)buf->head[0].iov_base;len=buf->len-offset;-if(len&3)-gotoout;-*p++=htonl(len);-*p++=htonl(gc->gc_seq);if(xdr_buf_subsegment(buf,&databody_integ,offset,len))gotowrap_failed;+/* Buffer space for these has already been reserved in+*svcauth_gss_accept().*/+*p++=cpu_to_be32(len);+*p=cpu_to_be32(gc->gc_seq);checksum.data=gsd->gsd_scratch;maj_stat=gss_get_mic(gsd->rsci->mechctx,&databody_integ,&checksum);if(maj_stat!=GSS_S_COMPLETE)gotobad_mic;-svc_putnl(resv,checksum.len);-memset(checksum.data+checksum.len,0,-round_up_to_quad(checksum.len)-checksum.len);-resv->iov_len+=XDR_QUADLEN(checksum.len)<<2;-/* not strictly required: */-buf->len+=XDR_QUADLEN(checksum.len)<<2;-if(resv->iov_len>PAGE_SIZE)++if(xdr_stream_encode_opaque(xdr,checksum.data,checksum.len)<0)gotowrap_failed;+xdr_commit_encode(xdr);+out:return0;+bad_mic:trace_rpcgss_svc_get_mic(rqstp,maj_stat);wrap_failed:
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:08
From: Chuck Lever <redacted>
Clean up variable names to match the other unwrap and wrap
functions.
Additionally, the explicit type cast on @gsd in unnecessary; and
@resbuf is renamed to match the variable naming in the unwrap
functions.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 72 ++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 30 deletions(-)
@@ -1817,24 +1817,35 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)return-EINVAL;}-staticinlineint-svcauth_gss_wrap_resp_priv(structsvc_rqst*rqstp)+/*+*RFC2203,Section5.3.2.3+*+*structrpc_gss_priv_data{+*opaquedatabody_priv<>+*};+*+*structrpc_gss_data_t{+*unsignedintseq_num;+*proc_req_arg_targ;+*};+*/+staticintsvcauth_gss_wrap_priv(structsvc_rqst*rqstp){-structgss_svc_data*gsd=(structgss_svc_data*)rqstp->rq_auth_data;+structgss_svc_data*gsd=rqstp->rq_auth_data;structrpc_gss_wire_cred*gc=&gsd->clcred;-structxdr_buf*resbuf=&rqstp->rq_res;+structxdr_buf*buf=&rqstp->rq_res;structpage**inpages=NULL;-__be32*p,*len;+__be32*p,*lenp;intoffset;intpad;-p=svcauth_gss_prepare_to_wrap(resbuf,gsd);+p=svcauth_gss_prepare_to_wrap(buf,gsd);if(p==NULL)return0;-len=p++;-offset=(u8*)p-(u8*)resbuf->head[0].iov_base;+lenp=p++;+offset=(u8*)p-(u8*)buf->head[0].iov_base;*p++=htonl(gc->gc_seq);-inpages=resbuf->pages;+inpages=buf->pages;/* XXX: Would be better to write some xdr helper functions for*nfs{2,3,4}xdr.cthatplacethedataright,insteadofcopying:*/
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:16
From: Chuck Lever <redacted>
Match the error reporting in the other unwrap and wrap functions.
Signed-off-by: Chuck Lever <redacted>
---
include/trace/events/rpcgss.h | 21 +++++++++++++++++++++
net/sunrpc/auth_gss/svcauth_gss.c | 29 +++++++++++++++++++----------
2 files changed, 40 insertions(+), 10 deletions(-)
@@ -1813,7 +1813,9 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)bad_mic:trace_rpcgss_svc_get_mic(rqstp,maj_stat);+return-EINVAL;wrap_failed:+trace_rpcgss_svc_wrap_failed(rqstp);return-EINVAL;}
@@ -1834,18 +1836,16 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)structgss_svc_data*gsd=rqstp->rq_auth_data;structrpc_gss_wire_cred*gc=&gsd->clcred;structxdr_buf*buf=&rqstp->rq_res;-structpage**inpages=NULL;+u32offset,pad,maj_stat;__be32*p,*lenp;-intoffset;-intpad;p=svcauth_gss_prepare_to_wrap(buf,gsd);if(p==NULL)return0;+lenp=p++;offset=(u8*)p-(u8*)buf->head[0].iov_base;*p++=htonl(gc->gc_seq);-inpages=buf->pages;/* XXX: Would be better to write some xdr helper functions for*nfs{2,3,4}xdr.cthatplacethedataright,insteadofcopying:*/
@@ -1859,12 +1859,12 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)if(buf->tail[0].iov_base){if(buf->tail[0].iov_base>=buf->head[0].iov_base+PAGE_SIZE)-return-EINVAL;+gotowrap_failed;if(buf->tail[0].iov_base<buf->head[0].iov_base)-return-EINVAL;+gotowrap_failed;if(buf->tail[0].iov_len+buf->head[0].iov_len+2*RPC_MAX_AUTH_SIZE>PAGE_SIZE)-return-ENOMEM;+gotowrap_failed;memmove(buf->tail[0].iov_base+RPC_MAX_AUTH_SIZE,buf->tail[0].iov_base,buf->tail[0].iov_len);
@@ -1879,13 +1879,16 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)*/if(!buf->tail[0].iov_base){if(buf->head[0].iov_len+2*RPC_MAX_AUTH_SIZE>PAGE_SIZE)-return-ENOMEM;+gotowrap_failed;buf->tail[0].iov_base=buf->head[0].iov_base+buf->head[0].iov_len+RPC_MAX_AUTH_SIZE;buf->tail[0].iov_len=0;}-if(gss_wrap(gsd->rsci->mechctx,offset,buf,inpages))-return-ENOMEM;++maj_stat=gss_wrap(gsd->rsci->mechctx,offset,buf,buf->pages);+if(maj_stat!=GSS_S_COMPLETE)+gotobad_wrap;+*lenp=htonl(buf->len-offset);pad=3-((buf->len-offset-1)&3);p=(__be32*)(buf->tail[0].iov_base+buf->tail[0].iov_len);
@@ -1894,6 +1897,12 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)buf->len+=pad;return0;+wrap_failed:+trace_rpcgss_svc_wrap_failed(rqstp);+return-EINVAL;+bad_wrap:+trace_rpcgss_svc_wrap(rqstp,maj_stat);+return-ENOMEM;}/**
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:26
From: Chuck Lever <redacted>
Simplify the references to the head and tail iovecs for readability.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
@@ -1836,6 +1836,8 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)structgss_svc_data*gsd=rqstp->rq_auth_data;structrpc_gss_wire_cred*gc=&gsd->clcred;structxdr_buf*buf=&rqstp->rq_res;+structkvec*head=buf->head;+structkvec*tail=buf->tail;u32offset,pad,maj_stat;__be32*p,*lenp;
@@ -1844,7 +1846,7 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)return0;lenp=p++;-offset=(u8*)p-(u8*)buf->head[0].iov_base;+offset=(u8*)p-(u8*)head->iov_base;*p++=htonl(gc->gc_seq);/* XXX: Would be better to write some xdr helper functions for*nfs{2,3,4}xdr.cthatplacethedataright,insteadofcopying:*/
@@ -1856,19 +1858,17 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)*thereisRPC_MAX_AUTH_SIZEslackspaceavailablein*boththeheadandtail.*/-if(buf->tail[0].iov_base){-if(buf->tail[0].iov_base>=-buf->head[0].iov_base+PAGE_SIZE)+if(tail->iov_base){+if(tail->iov_base>=head->iov_base+PAGE_SIZE)gotowrap_failed;-if(buf->tail[0].iov_base<buf->head[0].iov_base)+if(tail->iov_base<head->iov_base)gotowrap_failed;-if(buf->tail[0].iov_len+buf->head[0].iov_len+if(tail->iov_len+head->iov_len+2*RPC_MAX_AUTH_SIZE>PAGE_SIZE)gotowrap_failed;-memmove(buf->tail[0].iov_base+RPC_MAX_AUTH_SIZE,-buf->tail[0].iov_base,-buf->tail[0].iov_len);-buf->tail[0].iov_base+=RPC_MAX_AUTH_SIZE;+memmove(tail->iov_base+RPC_MAX_AUTH_SIZE,tail->iov_base,+tail->iov_len);+tail->iov_base+=RPC_MAX_AUTH_SIZE;}/**Ifthereisnocurrenttaildata,makesurethereis
@@ -1877,12 +1877,12 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)*isRPC_MAX_AUTH_SIZEslackspaceavailableinboththe*headandtail.*/-if(!buf->tail[0].iov_base){-if(buf->head[0].iov_len+2*RPC_MAX_AUTH_SIZE>PAGE_SIZE)+if(!tail->iov_base){+if(head->iov_len+2*RPC_MAX_AUTH_SIZE>PAGE_SIZE)gotowrap_failed;-buf->tail[0].iov_base=buf->head[0].iov_base-+buf->head[0].iov_len+RPC_MAX_AUTH_SIZE;-buf->tail[0].iov_len=0;+tail->iov_base=head->iov_base++head->iov_len+RPC_MAX_AUTH_SIZE;+tail->iov_len=0;}maj_stat=gss_wrap(gsd->rsci->mechctx,offset,buf,buf->pages);
@@ -1891,9 +1891,9 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)*lenp=htonl(buf->len-offset);pad=3-((buf->len-offset-1)&3);-p=(__be32*)(buf->tail[0].iov_base+buf->tail[0].iov_len);+p=(__be32*)(tail->iov_base+tail->iov_len);memset(p,0,pad);-buf->tail[0].iov_len+=pad;+tail->iov_len+=pad;buf->len+=pad;return0;
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:27
From: Chuck Lever <redacted>
Actually xdr_stream does not add value here because of how
gss_wrap() works. This is just a clean-up patch.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
@@ -1830,6 +1830,11 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)*unsignedintseq_num;*proc_req_arg_targ;*};+*+*gss_wrap()expandsthesizeoftheRPCmessagepayloadinthe+*responsebuffer.Themainpurposeofsvcauth_gss_wrap_priv()+*istoensurethereisadequatespaceintheresponsebufferto+*avoidoverflowduringthewrap.*/staticintsvcauth_gss_wrap_priv(structsvc_rqst*rqstp){
@@ -1847,9 +1852,9 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)lenp=p++;offset=(u8*)p-(u8*)head->iov_base;-*p++=htonl(gc->gc_seq);-/* XXX: Would be better to write some xdr helper functions for-*nfs{2,3,4}xdr.cthatplacethedataright,insteadofcopying:*/+/* Buffer space for this field has already been reserved+*insvcauth_gss_accept().*/+*p=cpu_to_be32(gc->gc_seq);/**Ifthereiscurrentlytaildata,makesurethereis
@@ -1889,8 +1894,8 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)if(maj_stat!=GSS_S_COMPLETE)gotobad_wrap;-*lenp=htonl(buf->len-offset);-pad=3-((buf->len-offset-1)&3);+*lenp=cpu_to_be32(buf->len-offset);+pad=xdr_pad_size(buf->len-offset);p=(__be32*)(tail->iov_base+tail->iov_len);memset(p,0,pad);tail->iov_len+=pad;
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:40
From: Chuck Lever <redacted>
Commit 5b304bc5bfcc ("[PATCH] knfsd: svcrpc: gss: fix failure on
SVC_DENIED in integrity case") added a check to prevent wrapping an
RPC response if reply_stat == MSG_DENIED, assuming that the only way
to get to svcauth_gss_release() with that reply_stat value was if
the reject_stat was AUTH_ERROR (reject_stat == MISMATCH is handled
earlier in svc_process_common()).
The code there is somewhat confusing. For one thing, rpc_success is
an accept_stat value, not a reply_stat value. The correct reply_stat
value to look for is RPC_MSG_DENIED. It happens to be the same value
as rpc_success, so it all works out, but it's not terribly readable.
Since commit 438623a06bac ("SUNRPC: Add svc_rqst::rq_auth_stat"),
the actual auth_stat value is stored in the svc_rqst, so that value
is now available to svcauth_gss_prepare_to_wrap() to make its
decision to wrap, based on direct information about the
authentication status of the RPC caller.
No behavior change is intended, this simply replaces some old code
with something that should be more self-documenting.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
@@ -1732,17 +1732,19 @@ svcauth_gss_accept(struct svc_rqst *rqstp)}static__be32*-svcauth_gss_prepare_to_wrap(structxdr_buf*resbuf,structgss_svc_data*gsd)+svcauth_gss_prepare_to_wrap(structsvc_rqst*rqstp,structgss_svc_data*gsd){+structxdr_buf*resbuf=&rqstp->rq_res;__be32*p;u32verf_len;p=gsd->verf_start;gsd->verf_start=NULL;-/* If the reply stat is nonzero, don't wrap: */-if(*(p-1)!=rpc_success)+/* AUTH_ERROR replies are not wrapped. */+if(rqstp->rq_auth_stat!=rpc_auth_ok)returnNULL;+/* Skip the verifier: */p+=1;verf_len=ntohl(*p++);
@@ -1786,7 +1788,7 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)u32offset,len,maj_stat;__be32*p;-p=svcauth_gss_prepare_to_wrap(buf,gsd);+p=svcauth_gss_prepare_to_wrap(rqstp,gsd);if(p==NULL)gotoout;
@@ -1846,7 +1848,7 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)u32offset,pad,maj_stat;__be32*p,*lenp;-p=svcauth_gss_prepare_to_wrap(buf,gsd);+p=svcauth_gss_prepare_to_wrap(rqstp,gsd);if(p==NULL)return0;
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:42
From: Chuck Lever <redacted>
There's no RPC header field called rpc_stat; more precisely, the
variable appears to be recording an accept_stat value. But it looks
like we don't need to preserve this value at all, actually, so
simply remove the variable.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/svc.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
@@ -1232,12 +1232,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)structsvc_serv*serv=rqstp->rq_server;structsvc_process_infoprocess;__be32*p,*statp;-__be32rpc_stat;intauth_res,rc;__be32*reply_statp;-rpc_stat=rpc_success;-/* Will be turned off by GSS integrity and privacy services */__set_bit(RQ_SPLICE_OK,&rqstp->rq_flags);/* Will be turned off only when NFSv4 Sessions are used */
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:45
From: Chuck Lever <redacted>
RFC 5531 defines an MSG_ACCEPTED Reply message like this:
struct accepted_reply {
opaque_auth verf;
union switch (accept_stat stat) {
case SUCCESS:
...
In the current server code, struct opaque_auth encoding is open-
coded. Introduce a helper that encodes an opaque_auth data item
within the context of a xdr_stream.
Done as part of hardening the server-side RPC header decoding and
encoding paths.
Signed-off-by: Chuck Lever <redacted>
---
include/linux/sunrpc/xdr.h | 2 ++
net/sunrpc/xdr.c | 29 +++++++++++++++++++++++++++++
2 files changed, 31 insertions(+)
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:50
From: Chuck Lever <redacted>
Now that all vs_dispatch functions invoke svcxdr_init_encode(), it
is common code and can be pushed down into the generic RPC server.
Signed-off-by: Chuck Lever <redacted>
---
fs/lockd/svc.c | 1 -
fs/nfs/callback_xdr.c | 2 --
fs/nfsd/nfscache.c | 2 +-
fs/nfsd/nfssvc.c | 6 ------
include/linux/sunrpc/xdr.h | 21 +++++++++++++++++++++
net/sunrpc/svc.c | 1 +
6 files changed, 23 insertions(+), 10 deletions(-)
@@ -1321,6 +1321,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)*/if(procp->pc_xdrressize)svc_reserve_auth(rqstp,procp->pc_xdrressize<<2);+svcxdr_init_encode(rqstp);/* Call the function that processes the request. */rc=process.dispatch(rqstp,statp);
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:29:55
From: Chuck Lever <redacted>
Refactor: So that the overhaul of each ->accept method can be done
in separate smaller patches, temporarily move the
svcxdr_init_encode() call into those methods.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 5 +++++
net/sunrpc/svc.c | 6 ++----
net/sunrpc/svcauth_unix.c | 3 +++
3 files changed, 10 insertions(+), 4 deletions(-)
@@ -1312,16 +1312,14 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)serv->sv_stats->rpccnt++;trace_svc_process(rqstp,progp->pg_name);-/* Build the reply header. */-statp=resv->iov_base+resv->iov_len;-svc_putnl(resv,RPC_SUCCESS);+statp=xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT);+*statp=rpc_success;/* un-reserve some of the out-queue now that we have a*betterideaofreplysize*/if(procp->pc_xdrressize)svc_reserve_auth(rqstp,procp->pc_xdrressize<<2);-svcxdr_init_encode(rqstp);/* Call the function that processes the request. */rc=process.dispatch(rqstp,statp);
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:30:48
From: Chuck Lever <redacted>
We're now moving svcxdr_init_encode() to /before/ the flavor's
->accept method has set rq_auth_slack. Add a helper that can
set rq_auth_slack /after/ svcxdr_init_encode() has been called.
Signed-off-by: Chuck Lever <redacted>
---
include/linux/sunrpc/svc.h | 26 ++++++++++++++++++++++++--
net/sunrpc/auth_gss/svcauth_gss.c | 18 ++++++++----------
2 files changed, 32 insertions(+), 12 deletions(-)
@@ -1685,24 +1685,22 @@ svcauth_gss_accept(struct svc_rqst *rqstp)svcxdr_init_encode(rqstp);break;caseRPC_GSS_SVC_INTEGRITY:-/* placeholders for length and seq. number: */-svc_putnl(resv,0);-svc_putnl(resv,0);+svcxdr_init_encode(rqstp);+/* placeholders for body length and seq. number: */+xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT*2);if(svcauth_gss_unwrap_integ(rqstp,gc->gc_seq,rsci->mechctx))gotogarbage_args;-rqstp->rq_auth_slack=RPC_MAX_AUTH_SIZE;-svcxdr_init_encode(rqstp);+svcxdr_set_auth_slack(rqstp,RPC_MAX_AUTH_SIZE);break;caseRPC_GSS_SVC_PRIVACY:-/* placeholders for length and seq. number: */-svc_putnl(resv,0);-svc_putnl(resv,0);+svcxdr_init_encode(rqstp);+/* placeholders for body length and seq. number: */+xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT*2);if(svcauth_gss_unwrap_priv(rqstp,gc->gc_seq,rsci->mechctx))gotogarbage_args;-rqstp->rq_auth_slack=RPC_MAX_AUTH_SIZE*2;-svcxdr_init_encode(rqstp);+svcxdr_set_auth_slack(rqstp,RPC_MAX_AUTH_SIZE*2);break;default:gotoauth_err;
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:30:50
From: Chuck Lever <redacted>
This code constructs replies to the decorated NULL procedure calls
that establish GSS contexts. Convert this code path to use struct
xdr_stream to encode such responses.
Done as part of hardening the server-side RPC header encoding path.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 144 +++++++++++++++++++++++--------------
1 file changed, 90 insertions(+), 54 deletions(-)
@@ -1240,17 +1275,17 @@ svcauth_gss_legacy_init(struct svc_rqst *rqstp,returnSVC_CLOSE;ret=SVC_CLOSE;-/* Got an answer to the upcall; use it: */-if(gss_write_init_verf(sn->rsc_cache,rqstp,-&rsip->out_handle,&rsip->major_status))+if(!svcauth_gss_proc_init_verf(sn->rsc_cache,rqstp,&rsip->out_handle,+&rsip->major_status,GSS_SEQ_WIN))+gotoout;+if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)gotoout;-if(gss_write_resv(resv,PAGE_SIZE,-&rsip->out_handle,&rsip->out_token,-rsip->major_status,rsip->minor_status))+if(!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream,&rsip->out_handle,+&rsip->out_token,rsip->major_status,+rsip->minor_status,GSS_SEQ_WIN))gotoout;ret=SVC_COMPLETE;-svcxdr_init_encode(rqstp);out:cache_put(&rsip->h,sn->rsi_cache);returnret;
@@ -1331,7 +1366,6 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,staticintsvcauth_gss_proxy_init(structsvc_rqst*rqstp,structrpc_gss_wire_cred*gc){-structkvec*resv=&rqstp->rq_res.head[0];structxdr_netobjcli_handle;structgssp_upcall_dataud;uint64_thandle;
@@ -1369,17 +1403,17 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,gotoout;}-/* Got an answer to the upcall; use it: */-if(gss_write_init_verf(sn->rsc_cache,rqstp,-&cli_handle,&ud.major_status))+if(!svcauth_gss_proc_init_verf(sn->rsc_cache,rqstp,&cli_handle,+&ud.major_status,GSS_SEQ_WIN))+gotoout;+if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)gotoout;-if(gss_write_resv(resv,PAGE_SIZE,-&cli_handle,&ud.out_token,-ud.major_status,ud.minor_status))+if(!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream,&cli_handle,+&ud.out_token,ud.major_status,+ud.minor_status,GSS_SEQ_WIN))gotoout;ret=SVC_COMPLETE;-svcxdr_init_encode(rqstp);out:gss_free_in_token_pages(&ud.in_token);gssp_free_upcall_data(&ud);
@@ -1700,28 +1640,25 @@ svcauth_gss_accept(struct svc_rqst *rqstp)/* now act upon the command: */switch(gc->gc_proc){caseRPC_GSS_PROC_DESTROY:-if(gss_write_verf(rqstp,rsci->mechctx,gc->gc_seq))+if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;/* Delete the entry from the cache_list and call cache_put */sunrpc_cache_unhash(sn->rsc_cache,&rsci->h);-if(resv->iov_len+4>PAGE_SIZE)-gotodrop;-svc_putnl(resv,RPC_SUCCESS);+if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)+gotoauth_err;gotocomplete;caseRPC_GSS_PROC_DATA:rqstp->rq_auth_stat=rpcsec_gsserr_ctxproblem;-svcdata->verf_start=resv->iov_base+resv->iov_len;-if(gss_write_verf(rqstp,rsci->mechctx,gc->gc_seq))+svcdata->verf_start=xdr_reserve_space(&rqstp->rq_res_stream,0);+if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;rqstp->rq_cred=rsci->cred;get_group_info(rsci->cred.cr_group_info);rqstp->rq_auth_stat=rpc_autherr_badcred;switch(gc->gc_svc){caseRPC_GSS_SVC_NONE:-svcxdr_init_encode(rqstp);break;caseRPC_GSS_SVC_INTEGRITY:-svcxdr_init_encode(rqstp);/* placeholders for body length and seq. number: */xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT*2);if(svcauth_gss_unwrap_integ(rqstp,gc->gc_seq,
@@ -1730,7 +1667,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)svcxdr_set_auth_slack(rqstp,RPC_MAX_AUTH_SIZE);break;caseRPC_GSS_SVC_PRIVACY:-svcxdr_init_encode(rqstp);/* placeholders for body length and seq. number: */xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT*2);if(svcauth_gss_unwrap_priv(rqstp,gc->gc_seq,
@@ -1755,8 +1691,7 @@ svcauth_gss_accept(struct svc_rqst *rqstp)ret=SVC_GARBAGE;gotoout;auth_err:-/* Restore write pointer to its original value: */-xdr_ressize_check(rqstp,reject_stat);+xdr_truncate_encode(&rqstp->rq_res_stream,XDR_UNIT*2);ret=SVC_DENIED;gotoout;complete:
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:30:54
From: Chuck Lever <redacted>
Now that each ->accept method has been converted, the
svcxdr_init_encode() calls can be hoisted back up into the generic
RPC server code.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 2 --
net/sunrpc/svc.c | 2 ++
net/sunrpc/svcauth_unix.c | 3 ---
3 files changed, 2 insertions(+), 5 deletions(-)
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:30:55
From: Chuck Lever <redacted>
The main part of RPC header encoding and the formation of error
responses are now done using the xdr_stream helpers. Bounds checking
before each XDR data item is encoded makes the server's encoding
path safer against accidental buffer overflows.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/svc.c | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
@@ -1227,13 +1227,14 @@ EXPORT_SYMBOL_GPL(svc_generic_init_request);staticintsvc_process_common(structsvc_rqst*rqstp,structkvec*resv){+structxdr_stream*xdr=&rqstp->rq_res_stream;structsvc_program*progp;conststructsvc_procedure*procp=NULL;structsvc_serv*serv=rqstp->rq_server;structsvc_process_infoprocess;__be32*p,*statp;intauth_res,rc;-__be32*reply_statp;+unsignedintaoffset;/* Will be turned off by GSS integrity and privacy services */__set_bit(RQ_SPLICE_OK,&rqstp->rq_flags);
@@ -1242,9 +1243,9 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *resv)__clear_bit(RQ_DROPME,&rqstp->rq_flags);/* Construct the first words of the reply: */-svc_putu32(resv,rqstp->rq_xid);-svc_putnl(resv,RPC_REPLY);-reply_statp=resv->iov_base+resv->iov_len;+svcxdr_init_encode(rqstp);+xdr_stream_encode_be32(xdr,rqstp->rq_xid);+xdr_stream_encode_be32(xdr,rpc_reply);p=xdr_inline_decode(&rqstp->rq_arg_stream,XDR_UNIT*4);if(unlikely(!p))
@@ -1509,22 +1508,21 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,rqstp->rq_arg.len=rqstp->rq_arg.head[0].iov_len+rqstp->rq_arg.page_len;-/* reset result send buffer "put" position */-resv->iov_len=0;--svcxdr_init_decode(rqstp);+/* Reset the response buffer */+rqstp->rq_res.head[0].iov_len=0;/**SkiptheXIDandcalldirfieldsbecausethey'vealready*beenprocessedbythecaller.*/+svcxdr_init_decode(rqstp);if(!xdr_inline_decode(&rqstp->rq_arg_stream,XDR_UNIT*2)){error=-EINVAL;gotoout;}/* Parse and execute the bc call */-proc_error=svc_process_common(rqstp,resv);+proc_error=svc_process_common(rqstp);atomic_dec(&req->rq_xprt->bc_slot_count);if(!proc_error){
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:31:51
From: Chuck Lever <redacted>
Now that svcauth_gss_prepare_to_wrap() no longer computes the
location of RPC header fields in the response buffer,
svcauth_gss_accept() can save the location of the databody
rather than the location of the verifier.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 78 +++++++++++++++++--------------------
1 file changed, 36 insertions(+), 42 deletions(-)
@@ -71,9 +71,7 @@structgss_svc_data{/* decoded gss client cred: */structrpc_gss_wire_credclcred;-/* save a pointer to the beginning of the encoded verifier,-*foruseinencryption/checksumminginsvcauth_gss_release:*/-__be32*verf_start;+u32gsd_databody_offset;structrsc*rsci;/* for temporary results */
@@ -1705,30 +1703,24 @@ svcauth_gss_accept(struct svc_rqst *rqstp)returnret;}-static__be32*+staticu32svcauth_gss_prepare_to_wrap(structsvc_rqst*rqstp,structgss_svc_data*gsd){-__be32*p;-u32verf_len;+u32offset;-p=gsd->verf_start;-gsd->verf_start=NULL;+/* Release can be called twice, but we only wrap once. */+offset=gsd->gsd_databody_offset;+gsd->gsd_databody_offset=0;/* AUTH_ERROR replies are not wrapped. */if(rqstp->rq_auth_stat!=rpc_auth_ok)-returnNULL;--/* Skip the verifier: */-p+=1;-verf_len=ntohl(*p++);-p+=XDR_QUADLEN(verf_len);+return0;/* Also don't wrap if the accept_stat is nonzero: */if(*rqstp->rq_accept_statp!=rpc_success)-returnNULL;+return0;-p++;-returnp;+returnoffset;}/*
@@ -1756,21 +1748,21 @@ static int svcauth_gss_wrap_integ(struct svc_rqst *rqstp)structxdr_buf*buf=xdr->buf;structxdr_bufdatabody_integ;structxdr_netobjchecksum;-u32offset,len,maj_stat;-__be32*p;+u32offset,maj_stat;-p=svcauth_gss_prepare_to_wrap(rqstp,gsd);-if(p==NULL)+offset=svcauth_gss_prepare_to_wrap(rqstp,gsd);+if(!offset)gotoout;-offset=(u8*)(p+1)-(u8*)buf->head[0].iov_base;-len=buf->len-offset;-if(xdr_buf_subsegment(buf,&databody_integ,offset,len))+if(xdr_buf_subsegment(buf,&databody_integ,offset+XDR_UNIT,+buf->len-offset-XDR_UNIT))gotowrap_failed;/* Buffer space for these has already been reserved in*svcauth_gss_accept().*/-*p++=cpu_to_be32(len);-*p=cpu_to_be32(gc->gc_seq);+if(xdr_encode_word(buf,offset,databody_integ.len))+gotowrap_failed;+if(xdr_encode_word(buf,offset+XDR_UNIT,gc->gc_seq))+gotowrap_failed;checksum.data=gsd->gsd_scratch;maj_stat=gss_get_mic(gsd->rsci->mechctx,&databody_integ,&checksum);
@@ -1817,17 +1809,19 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)structkvec*head=buf->head;structkvec*tail=buf->tail;u32offset,pad,maj_stat;-__be32*p,*lenp;+__be32*p;-p=svcauth_gss_prepare_to_wrap(rqstp,gsd);-if(p==NULL)+offset=svcauth_gss_prepare_to_wrap(rqstp,gsd);+if(!offset)return0;-lenp=p++;-offset=(u8*)p-(u8*)head->iov_base;-/* Buffer space for this field has already been reserved-*insvcauth_gss_accept().*/-*p=cpu_to_be32(gc->gc_seq);+/*+*Bufferspaceforthisfieldhasalreadybeenreserved+*insvcauth_gss_accept().NotethattheGSSsequence+*numberisencryptedalongwiththeRPCreplypayload.+*/+if(xdr_encode_word(buf,offset+XDR_UNIT,gc->gc_seq))+gotowrap_failed;/**Ifthereiscurrentlytaildata,makesurethereis
@@ -1863,12 +1857,15 @@ static int svcauth_gss_wrap_priv(struct svc_rqst *rqstp)tail->iov_len=0;}-maj_stat=gss_wrap(gsd->rsci->mechctx,offset,buf,buf->pages);+maj_stat=gss_wrap(gsd->rsci->mechctx,offset+XDR_UNIT,buf,+buf->pages);if(maj_stat!=GSS_S_COMPLETE)gotobad_wrap;-*lenp=cpu_to_be32(buf->len-offset);-pad=xdr_pad_size(buf->len-offset);+/* Wrapping can change the size of databody_priv. */+if(xdr_encode_word(buf,offset,buf->len-offset-XDR_UNIT))+gotowrap_failed;+pad=xdr_pad_size(buf->len-offset-XDR_UNIT);p=(__be32*)(tail->iov_base+tail->iov_len);memset(p,0,pad);tail->iov_len+=pad;
@@ -1908,9 +1905,6 @@ svcauth_gss_release(struct svc_rqst *rqstp)gc=&gsd->clcred;if(gc->gc_proc!=RPC_GSS_PROC_DATA)gotoout;-/* Release can be called twice, but we only wrap once. */-if(gsd->verf_start==NULL)-gotoout;switch(gc->gc_svc){caseRPC_GSS_SVC_NONE:
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:31:51
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Signed-off-by: Chuck Lever <redacted>
---
include/linux/sunrpc/svc.h | 19 +++++++++++++++++++
net/sunrpc/auth_gss/svcauth_gss.c | 21 ++++++++++-----------
net/sunrpc/svc.c | 2 --
net/sunrpc/svcauth_unix.c | 6 ++++++
4 files changed, 35 insertions(+), 13 deletions(-)
@@ -1348,7 +1348,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,if(!svcauth_gss_proc_init_verf(sn->rsc_cache,rqstp,&cli_handle,&ud.major_status,GSS_SEQ_WIN))gotoout;-if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)+if(!svcxdr_set_accept_stat(rqstp))gotoout;if(!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream,&cli_handle,&ud.out_token,ud.major_status,
@@ -1640,16 +1640,18 @@ svcauth_gss_accept(struct svc_rqst *rqstp)caseRPC_GSS_PROC_DESTROY:if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;+if(!svcxdr_set_accept_stat(rqstp))+gotoauth_err;/* Delete the entry from the cache_list and call cache_put */sunrpc_cache_unhash(sn->rsc_cache,&rsci->h);-if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)-gotoauth_err;gotocomplete;caseRPC_GSS_PROC_DATA:rqstp->rq_auth_stat=rpcsec_gsserr_ctxproblem;svcdata->verf_start=xdr_reserve_space(&rqstp->rq_res_stream,0);if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;+if(!svcxdr_set_accept_stat(rqstp))+gotoauth_err;rqstp->rq_cred=rsci->cred;get_group_info(rsci->cred.cr_group_info);rqstp->rq_auth_stat=rpc_autherr_badcred;
@@ -1721,13 +1722,11 @@ svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)p+=1;verf_len=ntohl(*p++);p+=XDR_QUADLEN(verf_len);-/* move accept_stat to right place: */-memcpy(p,p+2,4);-/* Also don't wrap if the accept stat is nonzero: */-if(*p!=rpc_success){-resbuf->head[0].iov_len-=2*4;++/* Also don't wrap if the accept_stat is nonzero: */+if(*rqstp->rq_accept_statp!=rpc_success)returnNULL;-}+p++;returnp;}
@@ -1314,8 +1314,6 @@ svc_process_common(struct svc_rqst *rqstp)trace_svc_process(rqstp,progp->pg_name);aoffset=xdr_stream_pos(xdr);-rqstp->rq_accept_statp=xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT);-*rqstp->rq_accept_statp=rpc_success;/* un-reserve some of the out-queue now that we have a*betterideaofreplysize
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:31:51
From: Chuck Lever <redacted>
Currently, svcauth_gss_accept() pre-reserves response buffer space
for the RPC payload length and GSS sequence number before returning
to the dispatcher, which then adds the header's accept_stat field.
The problem is the accept_stat field is supposed to go before the
length and seq_num fields. So svcauth_gss_release() has to relocate
the accept_stat value (see svcauth_gss_prepare_to_wrap()).
To enable these fields to be added to the response buffer in the
correct (final) order, the pointer to the accept_stat has to be made
available to svcauth_gss_accept() so that it can set it before
reserving space for the length and seq_num fields.
As a first step, move the pointer to the location of the accept_stat
field into struct svc_rqst.
Signed-off-by: Chuck Lever <redacted>
---
fs/lockd/svc.c | 4 ++--
fs/nfs/callback_xdr.c | 4 ++--
fs/nfsd/nfscache.c | 2 +-
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 4 ++--
include/linux/sunrpc/svc.h | 5 +++--
net/sunrpc/svc.c | 10 +++++-----
7 files changed, 16 insertions(+), 15 deletions(-)
@@ -1232,9 +1232,9 @@ svc_process_common(struct svc_rqst *rqstp)conststructsvc_procedure*procp=NULL;structsvc_serv*serv=rqstp->rq_server;structsvc_process_infoprocess;-__be32*p,*statp;intauth_res,rc;unsignedintaoffset;+__be32*p;/* Will be turned off by GSS integrity and privacy services */__set_bit(RQ_SPLICE_OK,&rqstp->rq_flags);
@@ -1314,8 +1314,8 @@ svc_process_common(struct svc_rqst *rqstp)trace_svc_process(rqstp,progp->pg_name);aoffset=xdr_stream_pos(xdr);-statp=xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT);-*statp=rpc_success;+rqstp->rq_accept_statp=xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT);+*rqstp->rq_accept_statp=rpc_success;/* un-reserve some of the out-queue now that we have a*betterideaofreplysize
@@ -1324,7 +1324,7 @@ svc_process_common(struct svc_rqst *rqstp)svc_reserve_auth(rqstp,procp->pc_xdrressize<<2);/* Call the function that processes the request. */-rc=process.dispatch(rqstp,statp);+rc=process.dispatch(rqstp);if(procp->pc_release)procp->pc_release(rqstp);if(!rc)
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:32:56
From: Chuck Lever <redacted>
An error computing the checksum here is an exceptional event.
Signed-off-by: Chuck Lever <redacted>
---
include/trace/events/rpcgss.h | 1 +
net/sunrpc/auth_gss/svcauth_gss.c | 26 +++++++++++++-------------
2 files changed, 14 insertions(+), 13 deletions(-)
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:32:58
From: Chuck Lever <redacted>
Now that upper layers use an xdr_stream to track the construction
of each RPC Reply message, resbuf->len is kept up-to-date
automatically. There's no need to recompute it in svc_gss_release().
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
@@ -1899,10 +1904,7 @@ svcauth_gss_release(struct svc_rqst *rqstp)/* Release can be called twice, but we only wrap once. */if(gsd->verf_start==NULL)gotoout;-/* normally not set till svc_send, but we need it here: */-/* XXX: what for? Do we mess it up the moment we call svc_putu32-*orwhatever?*/-resbuf->len=total_buf_len(resbuf);+switch(gc->gc_svc){caseRPC_GSS_SVC_NONE:break;
From: Chuck Lever <cel@kernel.org> Date: 2023-01-08 16:32:58
From: Chuck Lever <redacted>
Clean up: To help orient readers, name the stack variables to match
the XDR field names.
Additionally, the explicit type cast on @gsd is unnecessary; and
@resbuf is renamed to match the variable naming in the unwrap
functions.
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 70 +++++++++++++++++++++++--------------
1 file changed, 43 insertions(+), 27 deletions(-)
From: Jeff Layton <jlayton@kernel.org> Date: 2023-01-10 14:01:57
On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
quoted hunk
From: Chuck Lever <redacted>
Now that upper layers use an xdr_stream to track the construction
of each RPC Reply message, resbuf->len is kept up-to-date
automatically. There's no need to recompute it in svc_gss_release().
Signed-off-by: Chuck Lever <redacted>
---
net/sunrpc/auth_gss/svcauth_gss.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
As an aside...
It looks like the only place ->release is called is in svc_authorise,
and its callers either ignore the return, or just test whether it
succeeded (returned 0). If it fails then it looks like the xprt is
closed.
The actual return code doesn't matter at all. We could make ->release a
bool return to make that clear.
That's not directly related to this set though.
quoted hunk
static int svcauth_gss_release(struct svc_rqst *rqstp) {- struct gss_svc_data *gsd = (struct gss_svc_data *)rqstp->rq_auth_data;- struct rpc_gss_wire_cred *gc;- struct xdr_buf *resbuf = &rqstp->rq_res;- int stat = -EINVAL; struct sunrpc_net *sn = net_generic(SVC_NET(rqstp), sunrpc_net_id);+ struct gss_svc_data *gsd = rqstp->rq_auth_data;+ struct rpc_gss_wire_cred *gc;+ int stat; if (!gsd) goto out;
@@ -1899,10 +1904,7 @@ svcauth_gss_release(struct svc_rqst *rqstp) /* Release can be called twice, but we only wrap once. */ if (gsd->verf_start == NULL) goto out;- /* normally not set till svc_send, but we need it here: */- /* XXX: what for? Do we mess it up the moment we call svc_putu32- * or whatever? */- resbuf->len = total_buf_len(resbuf);+ switch (gc->gc_svc) { case RPC_GSS_SVC_NONE: break;
The patch itself looks fine.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
From: Jeff Layton <jlayton@kernel.org> Date: 2023-01-10 14:53:42
On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
The purpose of this series is to replace the svc_put* macros in the
Linux kernel server's RPC reply header construction code with
xdr_stream helpers. I've measured no change in CPU utilization after
the overhaul.
Memory safety: Buffer bounds checking after encoding each XDR item
is more memory-safe than the current mechanism. Subsequent memory
safety improvements to the common xdr_stream helpers will benefit
all who use them.
Audit friendliness: The new code has additional comments and other
clean-up to help align it with the relevant RPC protocol
specifications. The use of common helpers also makes the encoders
easier to audit and maintain.
I've split the full series in half to make it easier to review. The
patches posted here are the second half, handling RPC reply header
encoding.
Note that another benefit of this work is that we are taking one or
two more strides closer to greater commonality between the client
and server implementations of RPCSEC GSS.
---
Chuck Lever (27):
SUNRPC: Clean up svcauth_gss_release()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
SUNRPC: Check rq_auth_stat when preparing to wrap a response
SUNRPC: Remove the rpc_stat variable in svc_process_common()
SUNRPC: Add XDR encoding helper for opaque_auth
SUNRPC: Push svcxdr_init_encode() into svc_process_common()
SUNRPC: Move svcxdr_init_encode() into ->accept methods
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
SUNRPC: Convert unwrap data paths to use xdr_stream for replies
SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
SUNRPC: Use xdr_stream for encoding GSS reply verifiers
SUNRPC: Hoist init_encode out of svc_authenticate()
SUNRPC: Convert RPC Reply header encoding to use xdr_stream
SUNRPC: Final clean-up of svc_process_common()
SUNRPC: Remove no-longer-used helper functions
SUNRPC: Refactor RPC server dispatch method
SUNRPC: Set rq_accept_statp inside ->accept methods
SUNRPC: Go back to using gsd->body_start
fs/lockd/svc.c | 5 +-
fs/nfs/callback_xdr.c | 6 +-
fs/nfsd/nfscache.c | 4 +-
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 10 +-
include/linux/sunrpc/svc.h | 116 +++----
include/linux/sunrpc/xdr.h | 23 ++
include/trace/events/rpcgss.h | 22 ++
net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
net/sunrpc/svc.c | 91 +++---
net/sunrpc/svcauth_unix.c | 40 ++-
net/sunrpc/xdr.c | 29 ++
12 files changed, 451 insertions(+), 402 deletions(-)
--
Chuck Lever
I went through the whole set and this all looks like good stuff to me.
The result is a lot more readable, and there is a lot less manual
fiddling with buffer lengths and such.
Do you have a public branch with the current state of this set?
You can add:
Reviewed-by: Jeff Layton <jlayton@kernel.org>
From: Chuck Lever III <hidden> Date: 2023-01-10 15:17:06
On Jan 10, 2023, at 9:53 AM, Jeff Layton [off-list ref] wrote:
On Sun, 2023-01-08 at 11:28 -0500, Chuck Lever wrote:
quoted
The purpose of this series is to replace the svc_put* macros in the
Linux kernel server's RPC reply header construction code with
xdr_stream helpers. I've measured no change in CPU utilization after
the overhaul.
Memory safety: Buffer bounds checking after encoding each XDR item
is more memory-safe than the current mechanism. Subsequent memory
safety improvements to the common xdr_stream helpers will benefit
all who use them.
Audit friendliness: The new code has additional comments and other
clean-up to help align it with the relevant RPC protocol
specifications. The use of common helpers also makes the encoders
easier to audit and maintain.
I've split the full series in half to make it easier to review. The
patches posted here are the second half, handling RPC reply header
encoding.
Note that another benefit of this work is that we are taking one or
two more strides closer to greater commonality between the client
and server implementations of RPCSEC GSS.
---
Chuck Lever (27):
SUNRPC: Clean up svcauth_gss_release()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_integ()
SUNRPC: Record gss_get_mic() errors in svcauth_gss_wrap_integ()
SUNRPC: Replace checksum construction in svcauth_gss_wrap_integ()
SUNRPC: Convert svcauth_gss_wrap_integ() to use xdr_stream()
SUNRPC: Rename automatic variables in svcauth_gss_wrap_resp_priv()
SUNRPC: Record gss_wrap() errors in svcauth_gss_wrap_priv()
SUNRPC: Add @head and @tail variables in svcauth_gss_wrap_priv()
SUNRPC: Convert svcauth_gss_wrap_priv() to use xdr_stream()
SUNRPC: Check rq_auth_stat when preparing to wrap a response
SUNRPC: Remove the rpc_stat variable in svc_process_common()
SUNRPC: Add XDR encoding helper for opaque_auth
SUNRPC: Push svcxdr_init_encode() into svc_process_common()
SUNRPC: Move svcxdr_init_encode() into ->accept methods
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_null_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_unix_accept()
SUNRPC: Use xdr_stream to encode Reply verifier in svcauth_tls_accept()
SUNRPC: Convert unwrap data paths to use xdr_stream for replies
SUNRPC: Use xdr_stream to encode replies in server-side GSS upcall helpers
SUNRPC: Use xdr_stream for encoding GSS reply verifiers
SUNRPC: Hoist init_encode out of svc_authenticate()
SUNRPC: Convert RPC Reply header encoding to use xdr_stream
SUNRPC: Final clean-up of svc_process_common()
SUNRPC: Remove no-longer-used helper functions
SUNRPC: Refactor RPC server dispatch method
SUNRPC: Set rq_accept_statp inside ->accept methods
SUNRPC: Go back to using gsd->body_start
fs/lockd/svc.c | 5 +-
fs/nfs/callback_xdr.c | 6 +-
fs/nfsd/nfscache.c | 4 +-
fs/nfsd/nfsd.h | 2 +-
fs/nfsd/nfssvc.c | 10 +-
include/linux/sunrpc/svc.h | 116 +++----
include/linux/sunrpc/xdr.h | 23 ++
include/trace/events/rpcgss.h | 22 ++
net/sunrpc/auth_gss/svcauth_gss.c | 505 +++++++++++++++---------------
net/sunrpc/svc.c | 91 +++---
net/sunrpc/svcauth_unix.c | 40 ++-
net/sunrpc/xdr.c | 29 ++
12 files changed, 451 insertions(+), 402 deletions(-)
--
Chuck Lever
I went through the whole set and this all looks like good stuff to me.
The result is a lot more readable, and there is a lot less manual
fiddling with buffer lengths and such.
Do you have a public branch with the current state of this set?
These are in the topic-rpcsec-gss-krb5-enhancements branch in
my repo at kernel.org, although I'm about to push them to nfsd's
for-next.
You can add:
Reviewed-by: Jeff Layton <jlayton@kernel.org>
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8) as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
Thanks.
@@ -1348,7 +1348,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,if(!svcauth_gss_proc_init_verf(sn->rsc_cache,rqstp,&cli_handle,&ud.major_status,GSS_SEQ_WIN))gotoout;-if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)+if(!svcxdr_set_accept_stat(rqstp))gotoout;if(!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream,&cli_handle,&ud.out_token,ud.major_status,
@@ -1640,16 +1640,18 @@ svcauth_gss_accept(struct svc_rqst *rqstp)caseRPC_GSS_PROC_DESTROY:if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;+if(!svcxdr_set_accept_stat(rqstp))+gotoauth_err;/* Delete the entry from the cache_list and call cache_put */sunrpc_cache_unhash(sn->rsc_cache,&rsci->h);-if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)-gotoauth_err;gotocomplete;caseRPC_GSS_PROC_DATA:rqstp->rq_auth_stat=rpcsec_gsserr_ctxproblem;svcdata->verf_start=xdr_reserve_space(&rqstp->rq_res_stream,0);if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;+if(!svcxdr_set_accept_stat(rqstp))+gotoauth_err;rqstp->rq_cred=rsci->cred;get_group_info(rsci->cred.cr_group_info);rqstp->rq_auth_stat=rpc_autherr_badcred;
@@ -1721,13 +1722,11 @@ svcauth_gss_prepare_to_wrap(struct svc_rqst *rqstp, struct gss_svc_data *gsd)p+=1;verf_len=ntohl(*p++);p+=XDR_QUADLEN(verf_len);-/* move accept_stat to right place: */-memcpy(p,p+2,4);-/* Also don't wrap if the accept stat is nonzero: */-if(*p!=rpc_success){-resbuf->head[0].iov_len-=2*4;++/* Also don't wrap if the accept_stat is nonzero: */+if(*rqstp->rq_accept_statp!=rpc_success)returnNULL;-}+p++;returnp;}
@@ -1314,8 +1314,6 @@ svc_process_common(struct svc_rqst *rqstp)trace_svc_process(rqstp,progp->pg_name);aoffset=xdr_stream_pos(xdr);-rqstp->rq_accept_statp=xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT);-*rqstp->rq_accept_statp=rpc_success;/* un-reserve some of the out-queue now that we have a*betterideaofreplysize
From: Chuck Lever III <hidden> Date: 2023-05-02 14:14:53
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
On 08. 01. 23, 17:31, Chuck Lever wrote:
quoted
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8)
Assuming you did the bisect on the NFS server's kernel?
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:
nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.
@@ -1348,7 +1348,7 @@ static int svcauth_gss_proxy_init(struct svc_rqst *rqstp,if(!svcauth_gss_proc_init_verf(sn->rsc_cache,rqstp,&cli_handle,&ud.major_status,GSS_SEQ_WIN))gotoout;-if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)+if(!svcxdr_set_accept_stat(rqstp))gotoout;if(!svcxdr_encode_gss_init_res(&rqstp->rq_res_stream,&cli_handle,&ud.out_token,ud.major_status,
@@ -1640,16 +1640,18 @@ svcauth_gss_accept(struct svc_rqst *rqstp)caseRPC_GSS_PROC_DESTROY:if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;+if(!svcxdr_set_accept_stat(rqstp))+gotoauth_err;/* Delete the entry from the cache_list and call cache_put */sunrpc_cache_unhash(sn->rsc_cache,&rsci->h);-if(xdr_stream_encode_u32(&rqstp->rq_res_stream,RPC_SUCCESS)<0)-gotoauth_err;gotocomplete;caseRPC_GSS_PROC_DATA:rqstp->rq_auth_stat=rpcsec_gsserr_ctxproblem;svcdata->verf_start=xdr_reserve_space(&rqstp->rq_res_stream,0);if(!svcauth_gss_encode_verf(rqstp,rsci->mechctx,gc->gc_seq))gotoauth_err;+if(!svcxdr_set_accept_stat(rqstp))+gotoauth_err;rqstp->rq_cred=rsci->cred;get_group_info(rsci->cred.cr_group_info);rqstp->rq_auth_stat=rpc_autherr_badcred;
@@ -1706,7 +1708,6 @@ svcauth_gss_accept(struct svc_rqst *rqstp)static__be32*svcauth_gss_prepare_to_wrap(structsvc_rqst*rqstp,structgss_svc_data*gsd){-structxdr_buf*resbuf=&rqstp->rq_res;__be32*p;u32verf_len;@@-1721,13+1722,11@@svcauth_gss_prepare_to_wrap(structsvc_rqst*rqstp,structgss_svc_data*gsd)p+=1;verf_len=ntohl(*p++);p+=XDR_QUADLEN(verf_len);-/* move accept_stat to right place: */-memcpy(p,p+2,4);-/* Also don't wrap if the accept stat is nonzero: */-if(*p!=rpc_success){-resbuf->head[0].iov_len-=2*4;++/* Also don't wrap if the accept_stat is nonzero: */+if(*rqstp->rq_accept_statp!=rpc_success)returnNULL;-}+p++;returnp;}
@@ -1314,8 +1314,6 @@ svc_process_common(struct svc_rqst *rqstp)trace_svc_process(rqstp,progp->pg_name);aoffset=xdr_stream_pos(xdr);-rqstp->rq_accept_statp=xdr_reserve_space(&rqstp->rq_res_stream,XDR_UNIT);-*rqstp->rq_accept_statp=rpc_success;/* un-reserve some of the out-queue now that we have a*betterideaofreplysize
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
quoted
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
Fixed in nfs-utils-2-3-4-rc1~7
Commit: d68be5d6ae50 ("nfs.conf: fail to disable major NFS version 4 using "vers4=n" in /etc/nfs.conf")
NeilBrown
From: Jeff Layton <jlayton@kernel.org> Date: 2023-05-16 19:23:48
On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
quoted
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
On 08. 01. 23, 17:31, Chuck Lever wrote:
quoted
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8)
Assuming you did the bisect on the NFS server's kernel?
quoted
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
quoted
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:
quoted
nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.
Alex hit this problem in testing too, and I took a quick look.
In the attached capture, the client should have gotten back a
RPC_PROG_MISMATCH error, but the server has recorded an extra successful
accept state before encoding the RPC_PROG_MISMATCH error, leading to a
malformed reply.
I think that the problem is that encoding the accept status too early
means that we can't properly handle failures from the pg_init_request
call.
Chuck, any thoughts on how you'd like to handle this?
--
Jeff Layton [off-list ref]
From: Chuck Lever III <hidden> Date: 2023-05-16 19:26:15
On May 16, 2023, at 3:23 PM, Jeff Layton [off-list ref] wrote:
On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
quoted
quoted
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
On 08. 01. 23, 17:31, Chuck Lever wrote:
quoted
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8)
Assuming you did the bisect on the NFS server's kernel?
quoted
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
quoted
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:
quoted
nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.
Alex hit this problem in testing too, and I took a quick look.
In the attached capture, the client should have gotten back a
RPC_PROG_MISMATCH error, but the server has recorded an extra successful
accept state before encoding the RPC_PROG_MISMATCH error, leading to a
malformed reply.
I think that the problem is that encoding the accept status too early
means that we can't properly handle failures from the pg_init_request
call.
Chuck, any thoughts on how you'd like to handle this?
From: Jeff Layton <jlayton@kernel.org> Date: 2023-05-16 21:25:25
On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
quoted
On May 16, 2023, at 3:23 PM, Jeff Layton [off-list ref] wrote:
On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
quoted
quoted
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
On 08. 01. 23, 17:31, Chuck Lever wrote:
quoted
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8)
Assuming you did the bisect on the NFS server's kernel?
quoted
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
quoted
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:
quoted
nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.
Alex hit this problem in testing too, and I took a quick look.
In the attached capture, the client should have gotten back a
RPC_PROG_MISMATCH error, but the server has recorded an extra successful
accept state before encoding the RPC_PROG_MISMATCH error, leading to a
malformed reply.
I think that the problem is that encoding the accept status too early
means that we can't properly handle failures from the pg_init_request
call.
Chuck, any thoughts on how you'd like to handle this?
From: Chuck Lever III <hidden> Date: 2023-05-16 21:28:47
On May 16, 2023, at 5:25 PM, Jeff Layton [off-list ref] wrote:
On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
quoted
quoted
On May 16, 2023, at 3:23 PM, Jeff Layton [off-list ref] wrote:
On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
quoted
quoted
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
On 08. 01. 23, 17:31, Chuck Lever wrote:
quoted
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8)
Assuming you did the bisect on the NFS server's kernel?
quoted
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
quoted
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:
quoted
nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.
Alex hit this problem in testing too, and I took a quick look.
In the attached capture, the client should have gotten back a
RPC_PROG_MISMATCH error, but the server has recorded an extra successful
accept state before encoding the RPC_PROG_MISMATCH error, leading to a
malformed reply.
I think that the problem is that encoding the accept status too early
means that we can't properly handle failures from the pg_init_request
call.
Chuck, any thoughts on how you'd like to handle this?
From: Jeff Layton <jlayton@kernel.org> Date: 2023-05-16 22:28:50
On Tue, 2023-05-16 at 21:27 +0000, Chuck Lever III wrote:
quoted
On May 16, 2023, at 5:25 PM, Jeff Layton [off-list ref] wrote:
On Tue, 2023-05-16 at 19:25 +0000, Chuck Lever III wrote:
quoted
quoted
On May 16, 2023, at 3:23 PM, Jeff Layton [off-list ref] wrote:
On Tue, 2023-05-02 at 14:14 +0000, Chuck Lever III wrote:
quoted
quoted
On May 2, 2023, at 7:01 AM, Jiri Slaby [off-list ref] wrote:
On 08. 01. 23, 17:31, Chuck Lever wrote:
quoted
From: Chuck Lever <redacted>
To navigate around the space that svcauth_gss_accept() reserves
for the RPC payload body length and sequence number fields,
svcauth_gss_release() does a little dance with the reply's
accept_stat, moving the accept_stat value in the response buffer
down by two words.
Instead, let's have the ->accept() methods each set the proper
final location of the accept_stat to avoid having to move
things.
Hi,
I bisected to this (4bcf0343e8)
Assuming you did the bisect on the NFS server's kernel?
quoted
as it breaks nfs3-only servers in 6.3. I.e. /etc/nfs.conf containing:
[nfsd]
vers4=no
Note: Changing the settings in /etc/nfs.conf had no effect
on my server, so I effected the change by stopping the
server and poking values into /proc/fs/nfsd/versions by
hand.
Steve?
quoted
The client sees:
mount("10.0.2.15:/tmp", "/mnt", "nfs", 0, "vers=4.2,addr=10.0.2.15,clientad"...) = -1 EIO (Input/output error)
write(2, "mount.nfs: mount system call fai"..., 45
mount.nfs: mount system call failed for /mnt
And the kernel says:
nfs4_discover_server_trunking unhandled error -5. Exiting with error EIO
I reported in downstream as:
https://bugzilla.suse.com/show_bug.cgi?id=1210995
It cannot be reverted cleanly on the top of 6.3.
Any ideas?
I can reproduce a similar problem. Network capture shows
that the server is responding with NFS4ERR_NOENT to the
EXCHANGE_ID operation, and the client kernel log says:
quoted
nfs4_discover_server_trunking unhandled error -121. Exiting with error EIO
That's not the failure mode I expected given the commit
you bisected to, so it might not be the same problem you've
hit. I'll troubleshoot this and send a fix for testing.
Alex hit this problem in testing too, and I took a quick look.
In the attached capture, the client should have gotten back a
RPC_PROG_MISMATCH error, but the server has recorded an extra successful
accept state before encoding the RPC_PROG_MISMATCH error, leading to a
malformed reply.
I think that the problem is that encoding the accept status too early
means that we can't properly handle failures from the pg_init_request
call.
Chuck, any thoughts on how you'd like to handle this?