> On Jul 24, 2020, at 4:39 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> On Fri, Jul 24, 2020 at 10:10:08AM -0400, Chuck Lever wrote:
>>>> I'd like to remove this code, but
>>>> I'd first like to understand how it will effect the code that follows
>>>> immediately after:
>>>>
>>>> offset = xdr_pad_size(buf->head[0].iov_len);
>>>> if (offset) {
>>>> buf->buflen = RPCSVC_MAXPAYLOAD;
>>>> xdr_shift_buf(buf, offset);
>>>> fix_priv_head(buf, pad);
>>>> }
>>
>> So if one of those patches removes "pad = priv_len - buf->len;"
>> then the above code will break.
>>
>> But I'm trying to see when it is possible for gss_unwrap to
>> return a head buffer that is not quad-aligned. Not coming up
>> with any such scenario.
>
> Thinking about it more, even if there was some gss mechanism returning
> misaligned data, the best place to fix that would likely be in the
> mechanism-specific code (partly for reasons noted in the comment right
> here--it'll be more efficient to put the data in the right spot as you
> encrypt it.)
In principal, I totally agree that the GSS mechanism's unwrap method is
the obvious place to handle mis-alignment. The practical challenge in
this code path is that the needs of the client and server receive logic
diverge just enough to make it annoying.
So another remark about this:
static void
fix_priv_head(struct xdr_buf *buf, int pad)
{
if (buf->page_len == 0) {
/* We need to adjust head and buf->len in tandem in this
* case to make svc_defer() work--it finds the original
* buffer start using buf->len - buf->head[0].iov_len. */
buf->head[0].iov_len -= pad;
}
}
The comment complains about svc_defer, and that particular calculation
is still in that code. It seems like it would be better if a pointer
into buf->head was saved somewhere instead of trying to manufacture
it based on buf->len, which seems to be pretty unreliable.
If svc_defer was changed that way, that might help us get rid of at
least the first fix_priv_head call site.
--
Chuck Lever
