Re: [PATCH v2 0/2] Fix XDR encoding near page boundaries

[Chuck Lever <chuck.lever@oracle.com>]

On 12/23/24 6:22 PM, Rick Macklem wrote:
> On Mon, Dec 23, 2024 at 10:07 AM <cel@kernel.org> wrote:
>>
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> Build out the patch series to address the longstanding bug pointed
>> out by J David and Rick Macklem.
>>
>> At least during NFSv4 COMPOUND encoding, using
>> write_bytes_to_xdr_buf() seems less brittle than saving a pointer
>> into the XDR encoding buffer.
>>
>> I have one more patch to add (not yet included) that addresses the
>> issue in the NFSv4 READ and READ_PLUS encoders.
> It also looks like there is a similar situation in nfsd4_encode_fattr4().
> It uses attrlen_p (only a 4byte xdr_reserve_space(), so safe for now.
> 
> You might just regret choosing to not wire down the "safe to use
> xdr_reserve_space() for 4 bytes" semantic, but it is obviously up to you.

I'm 100% behind the idea of making it hard or impossible to code
things the wrong way.

But I'm not sure what you mean by "wire down". I can't think of a way
to enforce the "only 4 octets or less" rule -- just having a helper
function with that name documents it but doesn't enforce it.

My plan is to replace the obviously unsafe call sites immediately,
document the desired long-term semantics, then replace the other
"safe for now" call sites over time.

I've found a few other potentially unsafe server-side callers:
* gss_wrap_req_integ
* gss_wrap_req_priv
* unx_marshal

I consider these "safe for now" because the reserved space is guaranteed
to be in the XDR buffer's head iovec, far away from page boundaries.

I'm hoping that no-one introduces new unsafe call sites in the meantime.
We should be able to catch that in review.

That also buys some time to come up with something better.


> rick
> 
>>
>> Changes since RFC:
>> - Document the guarantees around pointer returned by xdr_reserve_space()
>> - Use write_bytes_to_xdr_buf() instead
>>
>> Chuck Lever (2):
>>    NFSD: Encode COMPOUND operation status on page boundaries
>>    SUNRPC: Document validity guarantees of the pointer returned by
>>      reserve_space
>>
>>   fs/nfsd/nfs4xdr.c | 20 ++++++++++----------
>>   net/sunrpc/xdr.c  |  3 +++
>>   2 files changed, 13 insertions(+), 10 deletions(-)
>>
>> --
>> 2.47.0
>>


-- 
Chuck Lever