* [PATCH 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic
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 ` Benjamin Coddington
2019-09-13 15:16 ` Chuck Lever
2019-09-13 14:41 ` [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack Chuck Lever
2019-09-13 16:05 ` Chuck Lever
2 siblings, 1 reply; 10+ messages in thread
From: Benjamin Coddington @ 2019-09-12 17:07 UTC (permalink / raw)
To: trond.myklebust, anna.schumaker, tibbs
Cc: linux-nfs, bfields, chuck.lever, km
Let the name reflect the single user and purpose.
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.
*/
-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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic
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
0 siblings, 1 reply; 10+ messages in thread
From: Chuck Lever @ 2019-09-13 15:16 UTC (permalink / raw)
To: Benjamin Coddington
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
> 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.
> 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?
> -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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic
2019-09-13 15:16 ` Chuck Lever
@ 2019-09-13 17:26 ` Benjamin Coddington
2019-09-13 17:28 ` Chuck Lever
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Coddington @ 2019-09-13 17:26 UTC (permalink / raw)
To: Chuck Lever
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
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?
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] SUNRPC: Rename xdr_buf_read_netobj to xdr_buf_read_mic
2019-09-13 17:26 ` Benjamin Coddington
@ 2019-09-13 17:28 ` Chuck Lever
0 siblings, 0 replies; 10+ messages in thread
From: Chuck Lever @ 2019-09-13 17:28 UTC (permalink / raw)
To: Benjamin Coddington
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack
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 14:41 ` Chuck Lever
2019-09-13 14:49 ` Benjamin Coddington
2019-09-13 16:05 ` Chuck Lever
2 siblings, 1 reply; 10+ messages in thread
From: Chuck Lever @ 2019-09-13 14:41 UTC (permalink / raw)
To: Benjamin Coddington
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
> On Sep 12, 2019, at 1:07 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
>
> The GSS Message Integrity Check data for krb5i may lie partially in the XDR
> reply buffer's pages and tail. If so, we try to copy the entire MIC into
> free space in the tail. But as the estimations of the slack space required
> for authentication and verification have improved there may be less free
> space in the tail to complete this copy -- see commit 2c94b8eca1a2
> ("SUNRPC: Use au_rslack when computing reply buffer size"). In fact, there
> may only be room in the tail for a single copy of the MIC, and not part of
> the MIC and then another complete copy.
>
> The real world failure reported is that `ls` of a directory on NFS may
> sometimes return -EIO, which can be traced back to xdr_buf_read_netobj()
> failing to find available free space in the tail to copy the MIC.
>
> Fix this by checking for the case of the MIC crossing the boundaries of
> head, pages, and tail. If so, shift the buffer until the MIC is contained
> completely within the pages or tail. This allows the remainder of the
> function to create a sub buffer that directly address the complete MIC.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> Cc: stable@vger.kernel.org
# v5.1 ?
> ---
> net/sunrpc/xdr.c | 45 +++++++++++++++++++++++++++------------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 48c93b9e525e..6e05a9693568 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1237,39 +1237,48 @@ 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 or the tail, set object to point to it; otherwise
> - * try to find space for it at the end of the tail, copy it there, and
> - * set obj to point to it. */
> + * 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.
> + */
> int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned int offset)
> {
> struct xdr_buf subbuf;
> + unsigned int len_to_boundary;
>
> if (xdr_decode_word(buf, offset, &obj->len))
> return -EFAULT;
> - if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
> +
> + offset += 4;
> +
> + /* Is the obj partially in the head? */
> + len_to_boundary = buf->head->iov_len - offset;
> + if (len_to_boundary > 0 && len_to_boundary < obj->len)
> + xdr_shift_buf(buf, len_to_boundary);
> +
> + /* Is the obj 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)
> + xdr_shrink_pagelen(buf, len_to_boundary);
Do you need to check if the obj is entirely in ->pages but crosses a page boundary?
> +
> + if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
> return -EFAULT;
>
> - /* Is the obj contained entirely in the head? */
> - obj->data = subbuf.head[0].iov_base;
> - if (subbuf.head[0].iov_len == obj->len)
> - return 0;
> - /* ..or is the obj contained entirely in the tail? */
> + /* 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)
> return 0;
>
> - /* use end of tail as storage for obj:
> - * (We don't copy to the beginning because then we'd have
> - * to worry about doing a potentially overlapping copy.
> - * This assumes the object is at most half the length of the
> - * tail.) */
> + /* ..or is the obj contained entirely in the head? */
> + obj->data = subbuf.head[0].iov_base;
> + if (subbuf.head[0].iov_len == obj->len)
> + return 0;
> +
> + /* obj is in the pages: move to tail */
> if (obj->len > buf->buflen - buf->len)
> return -ENOMEM;
> - if (buf->tail[0].iov_len != 0)
> - obj->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
> - else
> - obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
> + obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
> __read_bytes_from_xdr_buf(&subbuf, obj->data, obj->len);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(xdr_buf_read_netobj);
> --
> 2.20.1
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack
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
0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Coddington @ 2019-09-13 14:49 UTC (permalink / raw)
To: Chuck Lever
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
On 13 Sep 2019, at 10:41, Chuck Lever wrote:
>> On Sep 12, 2019, at 1:07 PM, Benjamin Coddington
>> <bcodding@redhat.com> wrote:
>>
>> The GSS Message Integrity Check data for krb5i may lie partially in
>> the XDR
>> reply buffer's pages and tail. If so, we try to copy the entire MIC
>> into
>> free space in the tail. But as the estimations of the slack space
>> required
>> for authentication and verification have improved there may be less
>> free
>> space in the tail to complete this copy -- see commit 2c94b8eca1a2
>> ("SUNRPC: Use au_rslack when computing reply buffer size"). In fact,
>> there
>> may only be room in the tail for a single copy of the MIC, and not
>> part of
>> the MIC and then another complete copy.
>>
>> The real world failure reported is that `ls` of a directory on NFS
>> may
>> sometimes return -EIO, which can be traced back to
>> xdr_buf_read_netobj()
>> failing to find available free space in the tail to copy the MIC.
>>
>> Fix this by checking for the case of the MIC crossing the boundaries
>> of
>> head, pages, and tail. If so, shift the buffer until the MIC is
>> contained
>> completely within the pages or tail. This allows the remainder of
>> the
>> function to create a sub buffer that directly address the complete
>> MIC.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> Cc: stable@vger.kernel.org
>
> # v5.1 ?
That makes sense to match the changes to rslack.
>> ---
>> net/sunrpc/xdr.c | 45 +++++++++++++++++++++++++++------------------
>> 1 file changed, 27 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 48c93b9e525e..6e05a9693568 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -1237,39 +1237,48 @@ 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 or the tail, set object to point to it;
>> otherwise
>> - * try to find space for it at the end of the tail, copy it there,
>> and
>> - * set obj to point to it. */
>> + * 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.
>> + */
>> int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj,
>> unsigned int offset)
>> {
>> struct xdr_buf subbuf;
>> + unsigned int len_to_boundary;
>>
>> if (xdr_decode_word(buf, offset, &obj->len))
>> return -EFAULT;
>> - if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
>> +
>> + offset += 4;
>> +
>> + /* Is the obj partially in the head? */
>> + len_to_boundary = buf->head->iov_len - offset;
>> + if (len_to_boundary > 0 && len_to_boundary < obj->len)
>> + xdr_shift_buf(buf, len_to_boundary);
>> +
>> + /* Is the obj 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)
>> + xdr_shrink_pagelen(buf, len_to_boundary);
>
> Do you need to check if the obj is entirely in ->pages but crosses a
> page boundary?
We're going to copy it out into the tail in that case. I'm assuming
read_bytes_from_xdr_buf() can handle reading across page boundaries.
So unless I'm missing something, I don't think we need to check.
Ben
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack
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 14:41 ` [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack Chuck Lever
@ 2019-09-13 16:05 ` Chuck Lever
2019-09-13 17:39 ` Benjamin Coddington
2 siblings, 1 reply; 10+ messages in thread
From: Chuck Lever @ 2019-09-13 16:05 UTC (permalink / raw)
To: Benjamin Coddington
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
Hi Ben-
A few review comments below.
> On Sep 12, 2019, at 1:07 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
>
> The GSS Message Integrity Check data for krb5i may lie partially in the XDR
> reply buffer's pages and tail. If so, we try to copy the entire MIC into
> free space in the tail. But as the estimations of the slack space required
> for authentication and verification have improved there may be less free
> space in the tail to complete this copy -- see commit 2c94b8eca1a2
> ("SUNRPC: Use au_rslack when computing reply buffer size"). In fact, there
> may only be room in the tail for a single copy of the MIC, and not part of
> the MIC and then another complete copy.
>
> The real world failure reported is that `ls` of a directory on NFS may
> sometimes return -EIO, which can be traced back to xdr_buf_read_netobj()
> failing to find available free space in the tail to copy the MIC.
>
> Fix this by checking for the case of the MIC crossing the boundaries of
> head, pages, and tail. If so, shift the buffer until the MIC is contained
> completely within the pages or tail. This allows the remainder of the
> function to create a sub buffer that directly address the complete MIC.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> Cc: stable@vger.kernel.org
> ---
> net/sunrpc/xdr.c | 45 +++++++++++++++++++++++++++------------------
> 1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 48c93b9e525e..6e05a9693568 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1237,39 +1237,48 @@ 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 or the tail, set object to point to it; otherwise
> - * try to find space for it at the end of the tail, copy it there, and
> - * set obj to point to it. */
> + * 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.
> + */
Nit: I would explicitly note in this comment that there is no need
for the caller to free @obj, and perhaps it should be noted that
the organization of @buf can be changed by this function.
Maybe more appropriate for 2/2.
> int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj, unsigned int offset)
> {
> struct xdr_buf subbuf;
> + unsigned int len_to_boundary;
>
> if (xdr_decode_word(buf, offset, &obj->len))
> return -EFAULT;
> - if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
> +
> + offset += 4;
Nit: No blank line before "offset += 4;" would help me understand
how the offset bump is related to xdr_decode_word(). It took me
a few blinks to see.
> +
> + /* Is the obj partially in the head? */
> + len_to_boundary = buf->head->iov_len - offset;
> + if (len_to_boundary > 0 && len_to_boundary < obj->len)
I'm not especially excited about the integer underflow when offset
is larger than buf->head->iov_len. This might be more explicit:
if (offset < buf->head[0].iov_len &&
offset + obj->len > buf->head[0].iov_len)
> + xdr_shift_buf(buf, len_to_boundary);
> +
> + /* Is the obj 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)
Ditto.
> + xdr_shrink_pagelen(buf, len_to_boundary);
> +
> + if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
> return -EFAULT;
>
> - /* Is the obj contained entirely in the head? */
> - obj->data = subbuf.head[0].iov_base;
> - if (subbuf.head[0].iov_len == obj->len)
> - return 0;
> - /* ..or is the obj contained entirely in the tail? */
> + /* 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)
> return 0;
>
> - /* use end of tail as storage for obj:
> - * (We don't copy to the beginning because then we'd have
> - * to worry about doing a potentially overlapping copy.
> - * This assumes the object is at most half the length of the
> - * tail.) */
> + /* ..or is the obj contained entirely in the head? */
> + obj->data = subbuf.head[0].iov_base;
> + if (subbuf.head[0].iov_len == obj->len)
> + return 0;
It looks like you're reversing these two tests as a micro-optimization?
Maybe that should be left for another patch, since this is supposed to
be a narrow fix.
Also, I found the new comments confusing: here they refer to the head
and tail of @subbuf; above they refer to head and tail of @buf. Note for
2/2, I guess.
> +
> + /* obj is in the pages: move to tail */
> if (obj->len > buf->buflen - buf->len)
> return -ENOMEM;
> - if (buf->tail[0].iov_len != 0)
> - obj->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
> - else
> - obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
> + obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
Not sure this is a safe change. It's possible that the head buffer
and tail buffer are not contiguous, which is what the buf->tail.iov_len
check is looking for, IMO. Can this hunk be left out?
> __read_bytes_from_xdr_buf(&subbuf, obj->data, obj->len);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(xdr_buf_read_netobj);
> --
> 2.20.1
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack
2019-09-13 16:05 ` Chuck Lever
@ 2019-09-13 17:39 ` Benjamin Coddington
2019-09-15 14:08 ` Benjamin Coddington
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Coddington @ 2019-09-13 17:39 UTC (permalink / raw)
To: Chuck Lever
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
On 13 Sep 2019, at 12:05, Chuck Lever wrote:
> Hi Ben-
>
> A few review comments below.
>
>
>> On Sep 12, 2019, at 1:07 PM, Benjamin Coddington
>> <bcodding@redhat.com> wrote:
>>
>> The GSS Message Integrity Check data for krb5i may lie partially in
>> the XDR
>> reply buffer's pages and tail. If so, we try to copy the entire MIC
>> into
>> free space in the tail. But as the estimations of the slack space
>> required
>> for authentication and verification have improved there may be less
>> free
>> space in the tail to complete this copy -- see commit 2c94b8eca1a2
>> ("SUNRPC: Use au_rslack when computing reply buffer size"). In fact,
>> there
>> may only be room in the tail for a single copy of the MIC, and not
>> part of
>> the MIC and then another complete copy.
>>
>> The real world failure reported is that `ls` of a directory on NFS
>> may
>> sometimes return -EIO, which can be traced back to
>> xdr_buf_read_netobj()
>> failing to find available free space in the tail to copy the MIC.
>>
>> Fix this by checking for the case of the MIC crossing the boundaries
>> of
>> head, pages, and tail. If so, shift the buffer until the MIC is
>> contained
>> completely within the pages or tail. This allows the remainder of
>> the
>> function to create a sub buffer that directly address the complete
>> MIC.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> Cc: stable@vger.kernel.org
>> ---
>> net/sunrpc/xdr.c | 45 +++++++++++++++++++++++++++------------------
>> 1 file changed, 27 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 48c93b9e525e..6e05a9693568 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -1237,39 +1237,48 @@ 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 or the tail, set object to point to it;
>> otherwise
>> - * try to find space for it at the end of the tail, copy it there,
>> and
>> - * set obj to point to it. */
>> + * 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.
>> + */
>
> Nit: I would explicitly note in this comment that there is no need
> for the caller to free @obj, and perhaps it should be noted that
> the organization of @buf can be changed by this function.
>
> Maybe more appropriate for 2/2.
Ok.. yes.. though I don't feel strongly about noting that *obj doesn't
need to be freed.
>> int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj,
>> unsigned int offset)
>> {
>> struct xdr_buf subbuf;
>> + unsigned int len_to_boundary;
>>
>> if (xdr_decode_word(buf, offset, &obj->len))
>> return -EFAULT;
>> - if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
>> +
>> + offset += 4;
>
> Nit: No blank line before "offset += 4;" would help me understand
> how the offset bump is related to xdr_decode_word(). It took me
> a few blinks to see.
>
>
>> +
>> + /* Is the obj partially in the head? */
>> + len_to_boundary = buf->head->iov_len - offset;
>> + if (len_to_boundary > 0 && len_to_boundary < obj->len)
>
> I'm not especially excited about the integer underflow when offset
> is larger than buf->head->iov_len. This might be more explicit:
>
> if (offset < buf->head[0].iov_len &&
> offset + obj->len > buf->head[0].iov_len)
Yep, makes sense - and I prefer the clarity.
>> + xdr_shift_buf(buf, len_to_boundary);
>> +
>> + /* Is the obj 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)
>
> Ditto.
>
>
>> + xdr_shrink_pagelen(buf, len_to_boundary);
>> +
>> + if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
>> return -EFAULT;
>>
>> - /* Is the obj contained entirely in the head? */
>> - obj->data = subbuf.head[0].iov_base;
>> - if (subbuf.head[0].iov_len == obj->len)
>> - return 0;
>> - /* ..or is the obj contained entirely in the tail? */
>> + /* 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)
>> return 0;
>>
>> - /* use end of tail as storage for obj:
>> - * (We don't copy to the beginning because then we'd have
>> - * to worry about doing a potentially overlapping copy.
>> - * This assumes the object is at most half the length of the
>> - * tail.) */
>> + /* ..or is the obj contained entirely in the head? */
>> + obj->data = subbuf.head[0].iov_base;
>> + if (subbuf.head[0].iov_len == obj->len)
>> + return 0;
>
> It looks like you're reversing these two tests as a
> micro-optimization?
> Maybe that should be left for another patch, since this is supposed to
> be a narrow fix.
Yes, if not done here - is it even worth another patch?
> Also, I found the new comments confusing: here they refer to the head
> and tail of @subbuf; above they refer to head and tail of @buf. Note
> for
> 2/2, I guess.
I can clarify in 2/2.
>> +
>> + /* obj is in the pages: move to tail */
>> if (obj->len > buf->buflen - buf->len)
>> return -ENOMEM;
>> - if (buf->tail[0].iov_len != 0)
>> - obj->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
>> - else
>> - obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
>> + obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
>
> Not sure this is a safe change. It's possible that the head buffer
> and tail buffer are not contiguous, which is what the
> buf->tail.iov_len
> check is looking for, IMO. Can this hunk be left out?
That's something I missed somehow, thanks for pointing it out. I see
now
that the transport can allocate them any way it likes.
Thanks for the review, I'll send a v2.
Ben
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Fix buffer handling of GSS MIC with less slack
2019-09-13 17:39 ` Benjamin Coddington
@ 2019-09-15 14:08 ` Benjamin Coddington
0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Coddington @ 2019-09-15 14:08 UTC (permalink / raw)
To: Chuck Lever
Cc: trond.myklebust, Anna Schumaker, tibbs, Linux NFS Mailing List,
Bruce Fields, km
On 13 Sep 2019, at 13:39, Benjamin Coddington wrote:
> On 13 Sep 2019, at 12:05, Chuck Lever wrote:
>
>> Hi Ben-
>>
>> A few review comments below.
>>
>>
>>> On Sep 12, 2019, at 1:07 PM, Benjamin Coddington
>>> <bcodding@redhat.com> wrote:
>>>
>>> The GSS Message Integrity Check data for krb5i may lie partially in
>>> the XDR
>>> reply buffer's pages and tail. If so, we try to copy the entire MIC
>>> into
>>> free space in the tail. But as the estimations of the slack space
>>> required
>>> for authentication and verification have improved there may be less
>>> free
>>> space in the tail to complete this copy -- see commit 2c94b8eca1a2
>>> ("SUNRPC: Use au_rslack when computing reply buffer size"). In
>>> fact, there
>>> may only be room in the tail for a single copy of the MIC, and not
>>> part of
>>> the MIC and then another complete copy.
>>>
>>> The real world failure reported is that `ls` of a directory on NFS
>>> may
>>> sometimes return -EIO, which can be traced back to
>>> xdr_buf_read_netobj()
>>> failing to find available free space in the tail to copy the MIC.
>>>
>>> Fix this by checking for the case of the MIC crossing the boundaries
>>> of
>>> head, pages, and tail. If so, shift the buffer until the MIC is
>>> contained
>>> completely within the pages or tail. This allows the remainder of
>>> the
>>> function to create a sub buffer that directly address the complete
>>> MIC.
>>>
>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>> net/sunrpc/xdr.c | 45 +++++++++++++++++++++++++++------------------
>>> 1 file changed, 27 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>>> index 48c93b9e525e..6e05a9693568 100644
>>> --- a/net/sunrpc/xdr.c
>>> +++ b/net/sunrpc/xdr.c
>>> @@ -1237,39 +1237,48 @@ 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 or the tail, set object to point to it;
>>> otherwise
>>> - * try to find space for it at the end of the tail, copy it there,
>>> and
>>> - * set obj to point to it. */
>>> + * 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.
>>> + */
>>
>> Nit: I would explicitly note in this comment that there is no need
>> for the caller to free @obj, and perhaps it should be noted that
>> the organization of @buf can be changed by this function.
>>
>> Maybe more appropriate for 2/2.
>
> Ok.. yes.. though I don't feel strongly about noting that *obj doesn't
> need to be freed.
>
>
>>> int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj *obj,
>>> unsigned int offset)
>>> {
>>> struct xdr_buf subbuf;
>>> + unsigned int len_to_boundary;
>>>
>>> if (xdr_decode_word(buf, offset, &obj->len))
>>> return -EFAULT;
>>> - if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
>>> +
>>> + offset += 4;
>>
>> Nit: No blank line before "offset += 4;" would help me understand
>> how the offset bump is related to xdr_decode_word(). It took me
>> a few blinks to see.
>>
>>
>>> +
>>> + /* Is the obj partially in the head? */
>>> + len_to_boundary = buf->head->iov_len - offset;
>>> + if (len_to_boundary > 0 && len_to_boundary < obj->len)
>>
>> I'm not especially excited about the integer underflow when offset
>> is larger than buf->head->iov_len. This might be more explicit:
>>
>> if (offset < buf->head[0].iov_len &&
>> offset + obj->len > buf->head[0].iov_len)
>
> Yep, makes sense - and I prefer the clarity.
>
>>> + xdr_shift_buf(buf, len_to_boundary);
>>> +
>>> + /* Is the obj 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)
>>
>> Ditto.
>>
>>
>>> + xdr_shrink_pagelen(buf, len_to_boundary);
>>> +
>>> + if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
>>> return -EFAULT;
>>>
>>> - /* Is the obj contained entirely in the head? */
>>> - obj->data = subbuf.head[0].iov_base;
>>> - if (subbuf.head[0].iov_len == obj->len)
>>> - return 0;
>>> - /* ..or is the obj contained entirely in the tail? */
>>> + /* 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)
>>> return 0;
>>>
>>> - /* use end of tail as storage for obj:
>>> - * (We don't copy to the beginning because then we'd have
>>> - * to worry about doing a potentially overlapping copy.
>>> - * This assumes the object is at most half the length of the
>>> - * tail.) */
>>> + /* ..or is the obj contained entirely in the head? */
>>> + obj->data = subbuf.head[0].iov_base;
>>> + if (subbuf.head[0].iov_len == obj->len)
>>> + return 0;
>>
>> It looks like you're reversing these two tests as a
>> micro-optimization?
>> Maybe that should be left for another patch, since this is supposed
>> to
>> be a narrow fix.
>
> Yes, if not done here - is it even worth another patch?
>
>> Also, I found the new comments confusing: here they refer to the head
>> and tail of @subbuf; above they refer to head and tail of @buf. Note
>> for
>> 2/2, I guess.
>
> I can clarify in 2/2.
>
>>> +
>>> + /* obj is in the pages: move to tail */
>>> if (obj->len > buf->buflen - buf->len)
>>> return -ENOMEM;
>>> - if (buf->tail[0].iov_len != 0)
>>> - obj->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
>>> - else
>>> - obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
>>> + obj->data = buf->head[0].iov_base + buf->head[0].iov_len;
>>
>> Not sure this is a safe change. It's possible that the head buffer
>> and tail buffer are not contiguous, which is what the
>> buf->tail.iov_len
>> check is looking for, IMO. Can this hunk be left out?
>
> That's something I missed somehow, thanks for pointing it out. I see
> now
> that the transport can allocate them any way it likes.
Just looking at this again today -- we can definitely keep the check,
but
the second half of the statement also assumes a contiguous head/tail
range.
I think it's safe to just remove the test altogether and place the
netobj at
the end of the tail. Then in 2/2, we'll just place it at the beginning
of
the tail because the function is specialized for the mic.
Ben
^ permalink raw reply [flat|nested] 10+ messages in thread