| Message ID | 20241231002901.12725-10-cel@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | Fix XDR encoding near page boundaries | expand |
On Tue, 31 Dec 2024, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > A subtlety of this API is that if the @nbytes region traverses a > page boundary, the next __xdr_commit_encode will shift the data item > in the XDR encode buffer. This makes the returned pointer point to > something else, leading to unexpected behavior. > > There are a few cases where the caller saves the returned pointer > and then later uses it to insert a computed value into an earlier > part of the stream. This can be safe only if either: > > - the data item is guaranteed to be in the XDR buffer's head, and > thus is not ever going to be near a page boundary, or > - the data item is no larger than 4 octets, since XDR alignment > rules require all data items to start on 4-octet boundaries > > But that safety is only an artifact of the current implementation. > It would be less brittle if these "safe" uses were eventually > replaced. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/xdr.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > index 62e07c330a66..4e003cb516fe 100644 > --- a/net/sunrpc/xdr.c > +++ b/net/sunrpc/xdr.c > @@ -1097,6 +1097,12 @@ static noinline __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr, > * Checks that we have enough buffer space to encode 'nbytes' more > * bytes of data. If so, update the total xdr_buf length, and > * adjust the length of the current kvec. > + * > + * The returned pointer is valid only until the next call to > + * xdr_reserve_space() or xdr_commit_encode() on @xdr. The current > + * implementation of this API guarantees that space reserved for a > + * four-byte data item remains valid until @xdr is destroyed, but > + * that might not always be true in the future. > */ > __be32 * xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes) > { > -- This series all looks good to me Reviewed-by: NeilBrown <neilb@suse.de> though I do wonder if it would be better make the "four-byte" behaviour a guaranteed part of the API rather than working around a problem that doesn't currently exist and quite possibly never will. Thanks, NeilBrown
On 1/1/25 4:49 PM, NeilBrown wrote: > On Tue, 31 Dec 2024, cel@kernel.org wrote: >> From: Chuck Lever <chuck.lever@oracle.com> >> >> A subtlety of this API is that if the @nbytes region traverses a >> page boundary, the next __xdr_commit_encode will shift the data item >> in the XDR encode buffer. This makes the returned pointer point to >> something else, leading to unexpected behavior. >> >> There are a few cases where the caller saves the returned pointer >> and then later uses it to insert a computed value into an earlier >> part of the stream. This can be safe only if either: >> >> - the data item is guaranteed to be in the XDR buffer's head, and >> thus is not ever going to be near a page boundary, or >> - the data item is no larger than 4 octets, since XDR alignment >> rules require all data items to start on 4-octet boundaries >> >> But that safety is only an artifact of the current implementation. >> It would be less brittle if these "safe" uses were eventually >> replaced. >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> net/sunrpc/xdr.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c >> index 62e07c330a66..4e003cb516fe 100644 >> --- a/net/sunrpc/xdr.c >> +++ b/net/sunrpc/xdr.c >> @@ -1097,6 +1097,12 @@ static noinline __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr, >> * Checks that we have enough buffer space to encode 'nbytes' more >> * bytes of data. If so, update the total xdr_buf length, and >> * adjust the length of the current kvec. >> + * >> + * The returned pointer is valid only until the next call to >> + * xdr_reserve_space() or xdr_commit_encode() on @xdr. The current >> + * implementation of this API guarantees that space reserved for a >> + * four-byte data item remains valid until @xdr is destroyed, but >> + * that might not always be true in the future. >> */ >> __be32 * xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes) >> { >> -- > > This series all looks good to me > Reviewed-by: NeilBrown <neilb@suse.de> Thanks! > though I do wonder if it would be better make the "four-byte" behaviour > a guaranteed part of the API rather than working around a problem that > doesn't currently exist and quite possibly never will. It might be better, but I would like to fix the known problem and document this expectation for the moment. I'm not closing the book on this by any means.
On Mon, 2024-12-30 at 19:29 -0500, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > A subtlety of this API is that if the @nbytes region traverses a > page boundary, the next __xdr_commit_encode will shift the data item > in the XDR encode buffer. This makes the returned pointer point to > something else, leading to unexpected behavior. > > There are a few cases where the caller saves the returned pointer > and then later uses it to insert a computed value into an earlier > part of the stream. This can be safe only if either: > > - the data item is guaranteed to be in the XDR buffer's head, and > thus is not ever going to be near a page boundary, or > - the data item is no larger than 4 octets, since XDR alignment > rules require all data items to start on 4-octet boundaries > > But that safety is only an artifact of the current implementation. > It would be less brittle if these "safe" uses were eventually > replaced. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/xdr.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c > index 62e07c330a66..4e003cb516fe 100644 > --- a/net/sunrpc/xdr.c > +++ b/net/sunrpc/xdr.c > @@ -1097,6 +1097,12 @@ static noinline __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr, > * Checks that we have enough buffer space to encode 'nbytes' more > * bytes of data. If so, update the total xdr_buf length, and > * adjust the length of the current kvec. > + * > + * The returned pointer is valid only until the next call to > + * xdr_reserve_space() or xdr_commit_encode() on @xdr. The current > + * implementation of this API guarantees that space reserved for a > + * four-byte data item remains valid until @xdr is destroyed, but > + * that might not always be true in the future. > */ > __be32 * xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes) > { I agree with Neil that this API could do with less footguns, but this does seem to be an improvement. Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c index 62e07c330a66..4e003cb516fe 100644 --- a/net/sunrpc/xdr.c +++ b/net/sunrpc/xdr.c @@ -1097,6 +1097,12 @@ static noinline __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr, * Checks that we have enough buffer space to encode 'nbytes' more * bytes of data. If so, update the total xdr_buf length, and * adjust the length of the current kvec. + * + * The returned pointer is valid only until the next call to + * xdr_reserve_space() or xdr_commit_encode() on @xdr. The current + * implementation of this API guarantees that space reserved for a + * four-byte data item remains valid until @xdr is destroyed, but + * that might not always be true in the future. */ __be32 * xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes) {