Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: Chuck Lever <chuck.lever@oracle.com>
To: Benjamin Coddington <bcodding@redhat.com>
Cc: trond.myklebust@hammerspace.com,
	Anna Schumaker <anna.schumaker@netapp.com>,
	tibbs@math.uh.edu,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	Bruce Fields <bfields@fieldses.org>,
	km@cm4all.com
Subject: Re: [PATCH 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic
Date: Fri, 13 Sep 2019 13:28:47 -0400
Message-ID: <FC32C8F9-9D59-4D9E-B3B3-A6E57AA46AD5@oracle.com> (raw)
In-Reply-To: <F0DD032B-D7C0-438F-9334-AA7427BA2215@redhat.com>



> On Sep 13, 2019, at 1:26 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> On 13 Sep 2019, at 11:16, Chuck Lever wrote:
> 
>>> On Sep 12, 2019, at 1:07 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
>>> 
>>> Let the name reflect the single user and purpose.
>> 
>> And perhaps the assumption that the MIC is always the last item in the xdr_buf?
>> I'm still reviewing 1/2, but this function might make that assumption.
> 
> It does.. is that a bad assumption, or maybe you'd like a different name?

Just documenting it is enough for now; there is only one user.

In the long run I'd like to see this kind of xdr_buf manipulation replaced
with the use of a scratch buffer.


>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>> ---
>>> include/linux/sunrpc/xdr.h     |  2 +-
>>> net/sunrpc/auth_gss/auth_gss.c |  2 +-
>>> net/sunrpc/xdr.c               | 42 +++++++++++++++++-----------------
>>> 3 files changed, 23 insertions(+), 23 deletions(-)
>>> 
>>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>>> index 9ee3970ba59c..a6b63e47a79b 100644
>>> --- a/include/linux/sunrpc/xdr.h
>>> +++ b/include/linux/sunrpc/xdr.h
>>> @@ -179,7 +179,7 @@ xdr_adjust_iovec(struct kvec *iov, __be32 *p)
>>> extern void xdr_shift_buf(struct xdr_buf *, size_t);
>>> extern void xdr_buf_from_iov(struct kvec *, struct xdr_buf *);
>>> extern int xdr_buf_subsegment(struct xdr_buf *, struct xdr_buf *, unsigned int, unsigned int);
>>> -extern int xdr_buf_read_netobj(struct xdr_buf *, struct xdr_netobj *, unsigned int);
>>> +extern int xdr_buf_read_mic(struct xdr_buf *, struct xdr_netobj *, unsigned int);
>>> extern int read_bytes_from_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
>>> extern int write_bytes_to_xdr_buf(struct xdr_buf *, unsigned int, void *, unsigned int);
>>> 
>>> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
>>> index 4ce42c62458e..d75fddca44c9 100644
>>> --- a/net/sunrpc/auth_gss/auth_gss.c
>>> +++ b/net/sunrpc/auth_gss/auth_gss.c
>>> @@ -1960,7 +1960,7 @@ gss_unwrap_resp_integ(struct rpc_task *task, struct rpc_cred *cred,
>>> 
>>> 	if (xdr_buf_subsegment(rcv_buf, &integ_buf, data_offset, integ_len))
>>> 		goto unwrap_failed;
>>> -	if (xdr_buf_read_netobj(rcv_buf, &mic, mic_offset))
>>> +	if (xdr_buf_read_mic(rcv_buf, &mic, mic_offset))
>>> 		goto unwrap_failed;
>>> 	maj_stat = gss_verify_mic(ctx->gc_gss_ctx, &integ_buf, &mic);
>>> 	if (maj_stat == GSS_S_CONTEXT_EXPIRED)
>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>> index 6e05a9693568..90dfde50f0ef 100644
>>> --- a/net/sunrpc/xdr.c
>>> +++ b/net/sunrpc/xdr.c
>>> @@ -1236,52 +1236,52 @@ xdr_encode_word(struct xdr_buf *buf, unsigned int base, u32 obj)
>>> }
>>> EXPORT_SYMBOL_GPL(xdr_encode_word);
>>> 
>>> -/* If the netobj starting offset bytes from the start of xdr_buf is contained
>>> - * entirely in the head, pages, or tail, set object to point to it; otherwise
>>> - * shift the buffer until it is contained entirely within the pages or tail.
>>> +/* If the mic starting offset bytes from the start of xdr_buf is contained
>>> + * entirely in the head, pages, or tail, set mic to point to it; otherwise
>>> + * shift the buf until it is contained entirely within the pages or tail.
>>> */
>> 
>> Nit: Could the patch convert this into a kernel Doxygen style comment?
> 
> Yes, can do that.
> 
> Ben
> 
>>> -int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned int offset)
>>> +int xdr_buf_read_mic(struct xdr_buf *buf, struct xdr_netobj *mic, unsigned int offset)
>>> {
>>> 	struct xdr_buf subbuf;
>>> 	unsigned int len_to_boundary;
>>> 
>>> -	if (xdr_decode_word(buf, offset, &obj->len))
>>> +	if (xdr_decode_word(buf, offset, &mic->len))
>>> 		return -EFAULT;
>>> 
>>> 	offset += 4;
>>> 
>>> -	/* Is the obj partially in the head? */
>>> +	/* Is the mic partially in the head? */
>>> 	len_to_boundary = buf->head->iov_len - offset;
>>> -	if (len_to_boundary > 0 && len_to_boundary < obj->len)
>>> +	if (len_to_boundary > 0 && len_to_boundary < mic->len)
>>> 		xdr_shift_buf(buf, len_to_boundary);
>>> 
>>> -	/* Is the obj partially in the pages? */
>>> +	/* Is the mic partially in the pages? */
>>> 	len_to_boundary = buf->head->iov_len + buf->page_len - offset;
>>> -	if (len_to_boundary > 0 && len_to_boundary < obj->len)
>>> +	if (len_to_boundary > 0 && len_to_boundary < mic->len)
>>> 		xdr_shrink_pagelen(buf, len_to_boundary);
>>> 
>>> -	if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
>>> +	if (xdr_buf_subsegment(buf, &subbuf, offset, mic->len))
>>> 		return -EFAULT;
>>> 
>>> -	/* Most likely: is the obj contained entirely in the tail? */
>>> -	obj->data = subbuf.tail[0].iov_base;
>>> -	if (subbuf.tail[0].iov_len == obj->len)
>>> +	/* Most likely: is the mic contained entirely in the tail? */
>>> +	mic->data = subbuf.tail[0].iov_base;
>>> +	if (subbuf.tail[0].iov_len == mic->len)
>>> 		return 0;
>>> 
>>> -	/* ..or is the obj contained entirely in the head? */
>>> -	obj->data = subbuf.head[0].iov_base;
>>> -	if (subbuf.head[0].iov_len == obj->len)
>>> +	/* ..or is the mic contained entirely in the head? */
>>> +	mic->data = subbuf.head[0].iov_base;
>>> +	if (subbuf.head[0].iov_len == mic->len)
>>> 		return 0;
>>> 
>>> -	/* obj is in the pages: move to tail */
>>> -	if (obj->len > buf->buflen - buf->len)
>>> +	/* mic is in the pages: move to tail */
>>> +	if (mic->len > buf->buflen - buf->len)
>>> 		return -ENOMEM;
>>> -	obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
>>> -	__read_bytes_from_xdr_buf(&subbuf, obj->data, obj->len);
>>> +	mic->data = buf->head[0].iov_base + buf->head[0].iov_len;
>>> +	__read_bytes_from_xdr_buf(&subbuf, mic->data, mic->len);
>>> 
>>> 	return 0;
>>> }
>>> -EXPORT_SYMBOL_GPL(xdr_buf_read_netobj);
>>> +EXPORT_SYMBOL_GPL(xdr_buf_read_mic);
>>> 
>>> /* Returns 0 on success, or else a negative error code. */
>>> static int
>>> -- 
>>> 2.20.1
>>> 
>> 
>> --
>> Chuck Lever

--
Chuck Lever




  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 17:07 [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack Benjamin Coddington
2019-09-12 17:07 ` [PATCH 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic Benjamin Coddington
2019-09-13 15:16   ` Chuck Lever
2019-09-13 17:26     ` Benjamin Coddington
2019-09-13 17:28       ` Chuck Lever [this message]
2019-09-13 14:41 ` [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack Chuck Lever
2019-09-13 14:49   ` Benjamin Coddington
2019-09-13 16:05 ` Chuck Lever
2019-09-13 17:39   ` Benjamin Coddington
2019-09-15 14:08     ` Benjamin Coddington

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=FC32C8F9-9D59-4D9E-B3B3-A6E57AA46AD5@oracle.com \
    --to=chuck.lever@oracle.com \
    --cc=anna.schumaker@netapp.com \
    --cc=bcodding@redhat.com \
    --cc=bfields@fieldses.org \
    --cc=km@cm4all.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=tibbs@math.uh.edu \
    --cc=trond.myklebust@hammerspace.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org linux-nfs@archiver.kernel.org
	public-inbox-index linux-nfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox