Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH RFC 09/10] SUNRPC: Remove xdr_buf_trim()
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20190204194615.GC1816@fieldses.org>
Date:   Mon, 4 Feb 2019 14:49:11 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>, simo@redhat.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <014F38E7-993C-4FD6-A8C6-9FE113A54A03@oracle.com>
References: <20190201195538.11389.96106.stgit@manet.1015granger.net>
 <20190201195814.11389.4023.stgit@manet.1015granger.net>
 <20190204194615.GC1816@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Feb 4, 2019, at 2:46 PM, bfields@fieldses.org wrote:
>=20
> On Fri, Feb 01, 2019 at 02:58:14PM -0500, Chuck Lever wrote:
>> The key action of xdr_buf_trim() is that it shortens buf->len, the
>> length of the xdr_buf' content. The other actions -- shortening the
>> head, pages, and tail components -- are actually not necessary. In
>> some cases, changing the size of those components corrupts the RPC
>> message contained in the buffer.
>=20
> That's really burying the lede.... Is there an actual user-visible bug
> here?

I don't think so. This is more of the form:

a) the function does fundamentally the wrong thing, so

b) certain changes to this code path result is unexpected and incorrect
   behavior

Thus typically only developers hacking on this code run into a problem.


> --b.
>=20
>>=20
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> include/linux/sunrpc/xdr.h          |    1 -
>> net/sunrpc/auth_gss/gss_krb5_wrap.c |    8 ++++---
>> net/sunrpc/auth_gss/svcauth_gss.c   |    2 +-
>> net/sunrpc/xdr.c                    |   41 =
-----------------------------------
>> 4 files changed, 6 insertions(+), 46 deletions(-)
>>=20
>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>> index 69161cb..4ae398c 100644
>> --- a/include/linux/sunrpc/xdr.h
>> +++ b/include/linux/sunrpc/xdr.h
>> @@ -183,7 +183,6 @@ static inline __be32 *xdr_encode_array(__be32 *p, =
const void *s, unsigned int le
>> 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 void xdr_buf_trim(struct xdr_buf *, unsigned int);
>> extern int xdr_buf_read_netobj(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/gss_krb5_wrap.c =
b/net/sunrpc/auth_gss/gss_krb5_wrap.c
>> index 5cdde6c..14a0aff 100644
>> --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
>> +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
>> @@ -570,14 +570,16 @@ static void rotate_left(u32 base, struct =
xdr_buf *buf, unsigned int shift)
>> 	 */
>> 	movelen =3D min_t(unsigned int, buf->head[0].iov_len, buf->len);
>> 	movelen -=3D offset + GSS_KRB5_TOK_HDR_LEN + headskip;
>> -	BUG_ON(offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
>> -							=
buf->head[0].iov_len);
>> +	if (offset + GSS_KRB5_TOK_HDR_LEN + headskip + movelen >
>> +	    buf->head[0].iov_len)
>> +		return GSS_S_FAILURE;
>> 	memmove(ptr, ptr + GSS_KRB5_TOK_HDR_LEN + headskip, movelen);
>> 	buf->head[0].iov_len -=3D GSS_KRB5_TOK_HDR_LEN + headskip;
>> 	buf->len -=3D GSS_KRB5_TOK_HDR_LEN + headskip;
>>=20
>> 	/* Trim off the trailing "extra count" and checksum blob */
>> -	xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip);
>> +	buf->len -=3D ec + GSS_KRB5_TOK_HDR_LEN + tailskip;
>> +
>> 	return GSS_S_COMPLETE;
>> }
>>=20
>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c =
b/net/sunrpc/auth_gss/svcauth_gss.c
>> index 152790e..f1aabab 100644
>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>> @@ -896,7 +896,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom)
>> 	if (svc_getnl(&buf->head[0]) !=3D seq)
>> 		goto out;
>> 	/* trim off the mic and padding at the end before returning */
>> -	xdr_buf_trim(buf, round_up_to_quad(mic.len) + 4);
>> +	buf->len -=3D 4 + round_up_to_quad(mic.len);
>> 	stat =3D 0;
>> out:
>> 	kfree(mic.data);
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 5f0aa53..4bce619 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -1139,47 +1139,6 @@ void xdr_enter_page(struct xdr_stream *xdr, =
unsigned int len)
>> }
>> EXPORT_SYMBOL_GPL(xdr_buf_subsegment);
>>=20
>> -/**
>> - * xdr_buf_trim - lop at most "len" bytes off the end of "buf"
>> - * @buf: buf to be trimmed
>> - * @len: number of bytes to reduce "buf" by
>> - *
>> - * Trim an xdr_buf by the given number of bytes by fixing up the =
lengths. Note
>> - * that it's possible that we'll trim less than that amount if the =
xdr_buf is
>> - * too small, or if (for instance) it's all in the head and the =
parser has
>> - * already read too far into it.
>> - */
>> -void xdr_buf_trim(struct xdr_buf *buf, unsigned int len)
>> -{
>> -	size_t cur;
>> -	unsigned int trim =3D len;
>> -
>> -	if (buf->tail[0].iov_len) {
>> -		cur =3D min_t(size_t, buf->tail[0].iov_len, trim);
>> -		buf->tail[0].iov_len -=3D cur;
>> -		trim -=3D cur;
>> -		if (!trim)
>> -			goto fix_len;
>> -	}
>> -
>> -	if (buf->page_len) {
>> -		cur =3D min_t(unsigned int, buf->page_len, trim);
>> -		buf->page_len -=3D cur;
>> -		trim -=3D cur;
>> -		if (!trim)
>> -			goto fix_len;
>> -	}
>> -
>> -	if (buf->head[0].iov_len) {
>> -		cur =3D min_t(size_t, buf->head[0].iov_len, trim);
>> -		buf->head[0].iov_len -=3D cur;
>> -		trim -=3D cur;
>> -	}
>> -fix_len:
>> -	buf->len -=3D (len - trim);
>> -}
>> -EXPORT_SYMBOL_GPL(xdr_buf_trim);
>> -
>> static void __read_bytes_from_xdr_buf(struct xdr_buf *subbuf, void =
*obj, unsigned int len)
>> {
>> 	unsigned int this_len;

--
Chuck Lever