On Tue, 31 Dec 2024, cel@xxxxxxxxxx wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxx>
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
