Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: fix_priv_head
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200724203900.GB9244@fieldses.org>
Date:   Fri, 24 Jul 2020 17:05:05 -0400
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: 7bit
Message-Id: <8005DB66-EC25-4DE2-847B-CEAD9BBE48E3@oracle.com>
References: <3799C9E0-DFF3-450C-A815-14BAFAC97EA8@oracle.com>
 <20200723193811.GG31487@fieldses.org>
 <94381D74-3563-4071-A0CF-4EC016744FEC@oracle.com>
 <20200724011720.GH31487@fieldses.org>
 <7557A354-8396-448D-BFC5-CA5512A4516B@oracle.com>
 <20200724203900.GB9244@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Jul 24, 2020, at 4:39 PM, Bruce Fields <bfields@fieldses.org> 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