[PATCH 0/4] quick NFSD-related clean-ups for 6.2

[PATCH 0/4] quick NFSD-related clean-ups for 6.2

[Chuck Lever]

Four unrelated fixes/clean-ups that I'd like to apply to v6.2
Comments welcome.

---

Chuck Lever (4):
      SUNRPC: Don't leak netobj memory when gss_read_proxy_verf() fails
      SUNRPC: Clean up xdr_write_pages()
      NFSD: Use only RQ_DROPME to signal the need to drop a reply
      SUNRPC: Make the svc_authenticate tracepoint conditional


 fs/nfsd/nfsproc.c                 |  4 ++--
 fs/nfsd/nfssvc.c                  |  2 +-
 include/trace/events/sunrpc.h     |  4 +++-
 net/sunrpc/auth_gss/svcauth_gss.c |  9 +++++++--
 net/sunrpc/svc.c                  |  3 +--
 net/sunrpc/xdr.c                  | 22 +++++++++++++---------
 6 files changed, 27 insertions(+), 17 deletions(-)

--
Chuck Lever

[PATCH 1/4] SUNRPC: Don't leak netobj memory when gss_read_proxy_verf() fails

[Chuck Lever]

Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: <stable@vger.kernel.org>
---
 net/sunrpc/auth_gss/svcauth_gss.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index bcd74dddbe2d..9a5db285d4ae 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1162,18 +1162,23 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
 		return res;
 
 	inlen = svc_getnl(argv);
-	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len))
+	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len)) {
+		kfree(in_handle->data);
 		return SVC_DENIED;
+	}
 
 	pages = DIV_ROUND_UP(inlen, PAGE_SIZE);
 	in_token->pages = kcalloc(pages, sizeof(struct page *), GFP_KERNEL);
-	if (!in_token->pages)
+	if (!in_token->pages) {
+		kfree(in_handle->data);
 		return SVC_DENIED;
+	}
 	in_token->page_base = 0;
 	in_token->page_len = inlen;
 	for (i = 0; i < pages; i++) {
 		in_token->pages[i] = alloc_page(GFP_KERNEL);
 		if (!in_token->pages[i]) {
+			kfree(in_handle->data);
 			gss_free_in_token_pages(in_token);
 			return SVC_DENIED;
 		}

Re: [PATCH 1/4] SUNRPC: Don't leak netobj memory when gss_read_proxy_verf() fails

[Jeff Layton]

On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
> Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: <stable@vger.kernel.org>
> ---
>  net/sunrpc/auth_gss/svcauth_gss.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index bcd74dddbe2d..9a5db285d4ae 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1162,18 +1162,23 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
>  		return res;
>  
>  	inlen = svc_getnl(argv);
> -	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len))
> +	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len)) {
> +		kfree(in_handle->data);

I wish this were more obvious in this code. It's not at all evident to
the casual reader that gss_read_common_verf calls dup_netobj here and
that you need to clean up after it. At a bare minimum, we ought to have
a comment to that effect over gss_read_common_verf. While you're in
there, that function is also pretty big to be marked static inline. Can
you change that too? Ditto for gss_read_verf.


>  		return SVC_DENIED;
> +	}
>  
>  	pages = DIV_ROUND_UP(inlen, PAGE_SIZE);
>  	in_token->pages = kcalloc(pages, sizeof(struct page *), GFP_KERNEL);
> -	if (!in_token->pages)
> +	if (!in_token->pages) {
> +		kfree(in_handle->data);
>  		return SVC_DENIED;
> +	}
>  	in_token->page_base = 0;
>  	in_token->page_len = inlen;
>  	for (i = 0; i < pages; i++) {
>  		in_token->pages[i] = alloc_page(GFP_KERNEL);
>  		if (!in_token->pages[i]) {
> +			kfree(in_handle->data);
>  			gss_free_in_token_pages(in_token);
>  			return SVC_DENIED;
>  		}
> 
> 

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 1/4] SUNRPC: Don't leak netobj memory when gss_read_proxy_verf() fails

[Chuck Lever III]

> On Nov 28, 2022, at 8:11 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> 
> On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
>> Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> net/sunrpc/auth_gss/svcauth_gss.c |    9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>> index bcd74dddbe2d..9a5db285d4ae 100644
>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>> @@ -1162,18 +1162,23 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
>> 		return res;
>> 
>> 	inlen = svc_getnl(argv);
>> -	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len))
>> +	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len)) {
>> +		kfree(in_handle->data);
> 
> I wish this were more obvious in this code. It's not at all evident to
> the casual reader that gss_read_common_verf calls dup_netobj here and
> that you need to clean up after it. At a bare minimum, we ought to have
> a comment to that effect over gss_read_common_verf. While you're in
> there, that function is also pretty big to be marked static inline. Can
> you change that too? Ditto for gss_read_verf.

Agreed: I've done that clean up in subsequent patches that are part
of the (yet to be posted) series to replace svc_get/putnl with
xdr_stream.

This seemed like a good fix to apply earlier rather than later. That
should enable it to be backported cleanly.


>> 		return SVC_DENIED;
>> +	}
>> 
>> 	pages = DIV_ROUND_UP(inlen, PAGE_SIZE);
>> 	in_token->pages = kcalloc(pages, sizeof(struct page *), GFP_KERNEL);
>> -	if (!in_token->pages)
>> +	if (!in_token->pages) {
>> +		kfree(in_handle->data);
>> 		return SVC_DENIED;
>> +	}
>> 	in_token->page_base = 0;
>> 	in_token->page_len = inlen;
>> 	for (i = 0; i < pages; i++) {
>> 		in_token->pages[i] = alloc_page(GFP_KERNEL);
>> 		if (!in_token->pages[i]) {
>> +			kfree(in_handle->data);
>> 			gss_free_in_token_pages(in_token);
>> 			return SVC_DENIED;
>> 		}
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>

--
Chuck Lever

Re: [PATCH 1/4] SUNRPC: Don't leak netobj memory when gss_read_proxy_verf() fails

[Jeff Layton]

On Mon, 2022-11-28 at 14:02 +0000, Chuck Lever III wrote:
> 
> > On Nov 28, 2022, at 8:11 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > 
> > On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
> > > Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > Cc: <stable@vger.kernel.org>
> > > ---
> > > net/sunrpc/auth_gss/svcauth_gss.c |    9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > > index bcd74dddbe2d..9a5db285d4ae 100644
> > > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > > @@ -1162,18 +1162,23 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
> > > 		return res;
> > > 
> > > 	inlen = svc_getnl(argv);
> > > -	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len))
> > > +	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len)) {
> > > +		kfree(in_handle->data);
> > 
> > I wish this were more obvious in this code. It's not at all evident to
> > the casual reader that gss_read_common_verf calls dup_netobj here and
> > that you need to clean up after it. At a bare minimum, we ought to have
> > a comment to that effect over gss_read_common_verf. While you're in
> > there, that function is also pretty big to be marked static inline. Can
> > you change that too? Ditto for gss_read_verf.
> 
> Agreed: I've done that clean up in subsequent patches that are part
> of the (yet to be posted) series to replace svc_get/putnl with
> xdr_stream.
> 
> This seemed like a good fix to apply earlier rather than later. That
> should enable it to be backported cleanly.
> 


Fair enough. You can add my Reviewed-by to the whole series. I'll look
forward to seeing the full cleanup.

> 
> > > 		return SVC_DENIED;
> > > +	}
> > > 
> > > 	pages = DIV_ROUND_UP(inlen, PAGE_SIZE);
> > > 	in_token->pages = kcalloc(pages, sizeof(struct page *), GFP_KERNEL);
> > > -	if (!in_token->pages)
> > > +	if (!in_token->pages) {
> > > +		kfree(in_handle->data);
> > > 		return SVC_DENIED;
> > > +	}
> > > 	in_token->page_base = 0;
> > > 	in_token->page_len = inlen;
> > > 	for (i = 0; i < pages; i++) {
> > > 		in_token->pages[i] = alloc_page(GFP_KERNEL);
> > > 		if (!in_token->pages[i]) {
> > > +			kfree(in_handle->data);
> > > 			gss_free_in_token_pages(in_token);
> > > 			return SVC_DENIED;
> > > 		}
> > > 
> > > 
> > 
> > -- 
> > Jeff Layton <jlayton@poochiereds.net>
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/4] SUNRPC: Don't leak netobj memory when gss_read_proxy_verf() fails

[Chuck Lever III]

> On Nov 28, 2022, at 9:13 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-11-28 at 14:02 +0000, Chuck Lever III wrote:
>> 
>>> On Nov 28, 2022, at 8:11 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
>>> 
>>> On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
>>>> Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> Cc: <stable@vger.kernel.org>
>>>> ---
>>>> net/sunrpc/auth_gss/svcauth_gss.c |    9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>>>> index bcd74dddbe2d..9a5db285d4ae 100644
>>>> --- a/net/sunrpc/auth_gss/svcauth_gss.c
>>>> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>>>> @@ -1162,18 +1162,23 @@ static int gss_read_proxy_verf(struct svc_rqst *rqstp,
>>>> 		return res;
>>>> 
>>>> 	inlen = svc_getnl(argv);
>>>> -	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len))
>>>> +	if (inlen > (argv->iov_len + rqstp->rq_arg.page_len)) {
>>>> +		kfree(in_handle->data);
>>> 
>>> I wish this were more obvious in this code. It's not at all evident to
>>> the casual reader that gss_read_common_verf calls dup_netobj here and
>>> that you need to clean up after it. At a bare minimum, we ought to have
>>> a comment to that effect over gss_read_common_verf. While you're in
>>> there, that function is also pretty big to be marked static inline. Can
>>> you change that too? Ditto for gss_read_verf.
>> 
>> Agreed: I've done that clean up in subsequent patches that are part
>> of the (yet to be posted) series to replace svc_get/putnl with
>> xdr_stream.
>> 
>> This seemed like a good fix to apply earlier rather than later. That
>> should enable it to be backported cleanly.
>> 
> 
> 
> Fair enough. You can add my Reviewed-by to the whole series.

Thanks!


> I'll look forward to seeing the full cleanup.

It's several dozen patches, and it's currently based on something
close to what's going into v6.2. So my plan is to rebase it on
v6.2-rc1 when that becomes available and then post the first half
of it (the decode part) at that time.


--
Chuck Lever

[PATCH 2/4] SUNRPC: Clean up xdr_write_pages()

[Chuck Lever]

Make it more evident how xdr_write_pages() updates the tail buffer
by using the convention of naming the iov pointer variable "tail".
I spent more than a couple of hours chasing through code to
understand this, so someone is likely to find this useful later.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xdr.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 336a7c7833e4..f7767bf22406 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1224,30 +1224,34 @@ EXPORT_SYMBOL(xdr_restrict_buflen);
 /**
  * xdr_write_pages - Insert a list of pages into an XDR buffer for sending
  * @xdr: pointer to xdr_stream
- * @pages: list of pages
- * @base: offset of first byte
- * @len: length of data in bytes
+ * @pages: array of pages to insert
+ * @base: starting offset of first data byte in @pages
+ * @len: number of data bytes in @pages to insert
  *
+ * After the @pages are added, the tail iovec is instantiated pointing to
+ * end of the head buffer, and the stream is set up to encode subsequent
+ * items into the tail.
  */
 void xdr_write_pages(struct xdr_stream *xdr, struct page **pages, unsigned int base,
 		 unsigned int len)
 {
 	struct xdr_buf *buf = xdr->buf;
-	struct kvec *iov = buf->tail;
+	struct kvec *tail = buf->tail;
+
 	buf->pages = pages;
 	buf->page_base = base;
 	buf->page_len = len;
 
-	iov->iov_base = (char *)xdr->p;
-	iov->iov_len  = 0;
-	xdr->iov = iov;
+	tail->iov_base = xdr->p;
+	tail->iov_len = 0;
+	xdr->iov = tail;
 
 	if (len & 3) {
 		unsigned int pad = 4 - (len & 3);
 
 		BUG_ON(xdr->p >= xdr->end);
-		iov->iov_base = (char *)xdr->p + (len & 3);
-		iov->iov_len  += pad;
+		tail->iov_base = (char *)xdr->p + (len & 3);
+		tail->iov_len += pad;
 		len += pad;
 		*xdr->p++ = 0;
 	}

Re: [PATCH 2/4] SUNRPC: Clean up xdr_write_pages()

[Jeff Layton]

On Sat, 2022-11-26 at 15:55 -0500, Chuck Lever wrote:
> Make it more evident how xdr_write_pages() updates the tail buffer
> by using the convention of naming the iov pointer variable "tail".
> I spent more than a couple of hours chasing through code to
> understand this, so someone is likely to find this useful later.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  net/sunrpc/xdr.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 336a7c7833e4..f7767bf22406 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1224,30 +1224,34 @@ EXPORT_SYMBOL(xdr_restrict_buflen);
>  /**
>   * xdr_write_pages - Insert a list of pages into an XDR buffer for sending
>   * @xdr: pointer to xdr_stream
> - * @pages: list of pages
> - * @base: offset of first byte
> - * @len: length of data in bytes
> + * @pages: array of pages to insert
> + * @base: starting offset of first data byte in @pages
> + * @len: number of data bytes in @pages to insert
>   *
> + * After the @pages are added, the tail iovec is instantiated pointing to
> + * end of the head buffer, and the stream is set up to encode subsequent
> + * items into the tail.
>   */
>  void xdr_write_pages(struct xdr_stream *xdr, struct page **pages, unsigned int base,
>  		 unsigned int len)
>  {
>  	struct xdr_buf *buf = xdr->buf;
> -	struct kvec *iov = buf->tail;
> +	struct kvec *tail = buf->tail;
> +
>  	buf->pages = pages;
>  	buf->page_base = base;
>  	buf->page_len = len;
>  
> -	iov->iov_base = (char *)xdr->p;
> -	iov->iov_len  = 0;
> -	xdr->iov = iov;
> +	tail->iov_base = xdr->p;
> +	tail->iov_len = 0;
> +	xdr->iov = tail;
>  
>  	if (len & 3) {
>  		unsigned int pad = 4 - (len & 3);
>  
>  		BUG_ON(xdr->p >= xdr->end);
> -		iov->iov_base = (char *)xdr->p + (len & 3);
> -		iov->iov_len  += pad;
> +		tail->iov_base = (char *)xdr->p + (len & 3);
> +		tail->iov_len += pad;
>  		len += pad;
>  		*xdr->p++ = 0;
>  	}
> 
> 

Reviewed-by: Jeff Layton <jlayton@kernel.org>

[PATCH 3/4] NFSD: Use only RQ_DROPME to signal the need to drop a reply

[Chuck Lever]

Clean up: NFSv2 has the only two usages of rpc_drop_reply in the
NFSD code base. Since NFSv2 is going away at some point, replace
these in order to simplify the "drop this reply?" check in
nfsd_dispatch().

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfsproc.c |    4 ++--
 fs/nfsd/nfssvc.c  |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 82b3ddeacc33..24f15ddb68dd 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -211,7 +211,7 @@ nfsd_proc_read(struct svc_rqst *rqstp)
 	if (resp->status == nfs_ok)
 		resp->status = fh_getattr(&resp->fh, &resp->stat);
 	else if (resp->status == nfserr_jukebox)
-		return rpc_drop_reply;
+		__set_bit(RQ_DROPME, &rqstp->rq_flags);
 	return rpc_success;
 }
 
@@ -246,7 +246,7 @@ nfsd_proc_write(struct svc_rqst *rqstp)
 	if (resp->status == nfs_ok)
 		resp->status = fh_getattr(&resp->fh, &resp->stat);
 	else if (resp->status == nfserr_jukebox)
-		return rpc_drop_reply;
+		__set_bit(RQ_DROPME, &rqstp->rq_flags);
 	return rpc_success;
 }
 
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index bfbd9f672f59..00b6eb72d1c9 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -1054,7 +1054,7 @@ int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
 	svcxdr_init_encode(rqstp);
 
 	*statp = proc->pc_func(rqstp);
-	if (*statp == rpc_drop_reply || test_bit(RQ_DROPME, &rqstp->rq_flags))
+	if (test_bit(RQ_DROPME, &rqstp->rq_flags))
 		goto out_update_drop;
 
 	if (!proc->pc_encode(rqstp, &rqstp->rq_res_stream))

[PATCH 4/4] SUNRPC: Make the svc_authenticate tracepoint conditional

[Chuck Lever]

Clean up: Simplify the tracepoint's only call site.

Also, I noticed that when svc_authenticate() returns SVC_COMPLETE,
it leaves rq_auth_stat set to an error value. That doesn't need to
be recorded in the trace log.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/trace/events/sunrpc.h |    4 +++-
 net/sunrpc/svc.c              |    3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/sunrpc.h b/include/trace/events/sunrpc.h
index f48f2ab9d238..e29d99c32891 100644
--- a/include/trace/events/sunrpc.h
+++ b/include/trace/events/sunrpc.h
@@ -1666,11 +1666,13 @@ TRACE_DEFINE_ENUM(SVC_COMPLETE);
 #define SVC_RQST_ENDPOINT_VARARGS \
 		__entry->xid, __get_sockaddr(server), __get_sockaddr(client)
 
-TRACE_EVENT(svc_authenticate,
+TRACE_EVENT_CONDITION(svc_authenticate,
 	TP_PROTO(const struct svc_rqst *rqst, int auth_res),
 
 	TP_ARGS(rqst, auth_res),
 
+	TP_CONDITION(auth_res != SVC_OK && auth_res != SVC_COMPLETE),
+
 	TP_STRUCT__entry(
 		SVC_RQST_ENDPOINT_FIELDS(rqst)
 
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 34383c352bc3..8f1b596db33f 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -1280,8 +1280,7 @@ svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
 	/* Also give the program a chance to reject this call: */
 	if (auth_res == SVC_OK && progp)
 		auth_res = progp->pg_authenticate(rqstp);
-	if (auth_res != SVC_OK)
-		trace_svc_authenticate(rqstp, auth_res);
+	trace_svc_authenticate(rqstp, auth_res);
 	switch (auth_res) {
 	case SVC_OK:
 		break;