| Message ID | 166128840714.2788.7887913547062461761.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/7] SUNRPC: Fix end-of-buffer calculation | expand |
On Tue, 2022-08-23 at 17:00 -0400, Chuck Lever wrote: > 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. > > Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding > on the server-side") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > include/linux/sunrpc/svc.h | 14 +++++++++++--- > include/linux/sunrpc/xdr.h | 12 ++++++++++++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h > index daecb009c05b..494375313a6f 100644 > --- a/include/linux/sunrpc/svc.h > +++ b/include/linux/sunrpc/svc.h > @@ -544,16 +544,24 @@ 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); > + /* Reset the argument buffer's length, now that the RPC > header > + * has been decoded. */ > + buf->len = xdr_buf_length(buf); > + > + xdr_init_decode(xdr, buf, argv->iov_base, NULL); > xdr_set_scratch_page(xdr, rqstp->rq_scratch_page); > } > > diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h > index 69175029abbb..f6bb215d4029 100644 > --- a/include/linux/sunrpc/xdr.h > +++ b/include/linux/sunrpc/xdr.h > @@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start, > size_t len) > buf->buflen = len; > } > > +/** > + * xdr_buf_length - Return the summed length of @buf's sub-buffers > + * @buf: an instantiated xdr_buf > + * > + * Returns the sum of the lengths of the head kvec, the tail kvec, > + * and the page array. > + */ > +static inline unsigned int xdr_buf_length(const struct xdr_buf *buf) > +{ > + return buf->head->iov_len + buf->page_len + buf->tail- > >iov_len; > +} > + NACK. This function is neither needed nor wanted for the client code, which already does the right thing w.r.t. maintaining an authoritative buf->len. If you need this helper, then stuff under a server-specific mattress where developers looking for client functionality can't find it.
> On Aug 23, 2022, at 5:52 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Tue, 2022-08-23 at 17:00 -0400, Chuck Lever wrote: >> 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. >> >> Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding >> on the server-side") >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> include/linux/sunrpc/svc.h | 14 +++++++++++--- >> include/linux/sunrpc/xdr.h | 12 ++++++++++++ >> 2 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h >> index daecb009c05b..494375313a6f 100644 >> --- a/include/linux/sunrpc/svc.h >> +++ b/include/linux/sunrpc/svc.h >> @@ -544,16 +544,24 @@ 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); >> + /* Reset the argument buffer's length, now that the RPC >> header >> + * has been decoded. */ >> + buf->len = xdr_buf_length(buf); >> + >> + xdr_init_decode(xdr, buf, argv->iov_base, NULL); >> xdr_set_scratch_page(xdr, rqstp->rq_scratch_page); >> } >> >> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h >> index 69175029abbb..f6bb215d4029 100644 >> --- a/include/linux/sunrpc/xdr.h >> +++ b/include/linux/sunrpc/xdr.h >> @@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start, >> size_t len) >> buf->buflen = len; >> } >> >> +/** >> + * xdr_buf_length - Return the summed length of @buf's sub-buffers >> + * @buf: an instantiated xdr_buf >> + * >> + * Returns the sum of the lengths of the head kvec, the tail kvec, >> + * and the page array. >> + */ >> +static inline unsigned int xdr_buf_length(const struct xdr_buf *buf) >> +{ >> + return buf->head->iov_len + buf->page_len + buf->tail- >>> iov_len; >> +} >> + > > NACK. This function is neither needed nor wanted for the client code, > which already does the right thing w.r.t. maintaining an authoritative > buf->len. See patch 7/7 in this series, which updates two functions that are part of client-side call chains. I'm reading into your objection a little, but I think your long term goal is to have the XDR layer manage ::len opaquely to RPC consumers in both the client and the server implementation. I agree that's a good goal, and yes, eventually I will pull the chain and replace the use of svc_getnl() and friends with xdr_stream-based decoding. Just not today. > If you need this helper, then stuff under a server-specific mattress > where developers looking for client functionality can't find it. I don't have to have this helper, it was meant as nothing more than code de-duplication. I'll remove it and drop 7/7. Thanks for taking a look! -- Chuck Lever
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index daecb009c05b..494375313a6f 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -544,16 +544,24 @@ 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); + /* Reset the argument buffer's length, now that the RPC header + * has been decoded. */ + buf->len = xdr_buf_length(buf); + + xdr_init_decode(xdr, buf, argv->iov_base, NULL); xdr_set_scratch_page(xdr, rqstp->rq_scratch_page); } diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h index 69175029abbb..f6bb215d4029 100644 --- a/include/linux/sunrpc/xdr.h +++ b/include/linux/sunrpc/xdr.h @@ -83,6 +83,18 @@ xdr_buf_init(struct xdr_buf *buf, void *start, size_t len) buf->buflen = len; } +/** + * xdr_buf_length - Return the summed length of @buf's sub-buffers + * @buf: an instantiated xdr_buf + * + * Returns the sum of the lengths of the head kvec, the tail kvec, + * and the page array. + */ +static inline unsigned int xdr_buf_length(const struct xdr_buf *buf) +{ + return buf->head->iov_len + buf->page_len + buf->tail->iov_len; +} + /* * pre-xdr'ed macros. */
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. Fixes: 5191955d6fc6 ("SUNRPC: Prepare for xdr_stream-style decoding on the server-side") Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- include/linux/sunrpc/svc.h | 14 +++++++++++--- include/linux/sunrpc/xdr.h | 12 ++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)