On 12/26/24 5:36 PM, NeilBrown wrote:
On Fri, 27 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c index 62e07c330a66..f198bb043e2f 100644 --- a/net/sunrpc/xdr.c +++ b/net/sunrpc/xdr.c @@ -1097,6 +1097,9 @@ 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 this stream.There are still several places where the return value of xdr_reserver_space is stored for longer than advised here. - there are several in nfs/callback_xdr.c e.g. encode_compound_hdr - there is attrlen_p in nfsd4_encode_fattr4 - maxcount_p in nfsd4_encode_readlink - flavors_p in nfsd4_do_encode_secinfo - rqstp->rq_accept_statp which is initialied in svcxdr_set_accept_stat and updated in many places. - segcount in rpcrdma_encode_write_list - segcount in rpcrdma_encode_reply_chunk These are all safe because of the "4 octets" rule. Should we include that in the documenting comment, or would you rather all of these be change to use an offset rather than a pointer?
That's getting to be a tall list. I will add another sentence to xdr_reserve_space's kdoc comment for now, something like: "The current implementation guarantees that space reserved for a four-byte data item remains valid until the stream is destroyed." IMO encode_readlink and encode_secinfo need to be updated sooner rather than later. -- Chuck Lever
