Re: [PATCH v3 6/6] SUNRPC: Document validity guarantees of the pointer returned by reserve_space

Chuck Lever
2024-12-27T00:52:55Z

On 12/26/24 5:36 PM, NeilBrown wrote:
> On Fri, 27 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 | 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