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
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 <[email protected]>
---
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))
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 <[email protected]>
---
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;
Fixes: 030d794bf498 ("SUNRPC: Use gssproxy upcall for server RPCGSS authentication.")
Signed-off-by: Chuck Lever <[email protected]>
Cc: <[email protected]>
---
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;
}
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 <[email protected]>
> Cc: <[email protected]>
> ---
> 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 <[email protected]>
> On Nov 28, 2022, at 8:11 AM, Jeff Layton <[email protected]> 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 <[email protected]>
>> Cc: <[email protected]>
>> ---
>> 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 <[email protected]>
--
Chuck Lever
On Mon, 2022-11-28 at 14:02 +0000, Chuck Lever III wrote:
>
> > On Nov 28, 2022, at 8:11 AM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > Cc: <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
> On Nov 28, 2022, at 9:13 AM, Jeff Layton <[email protected]> wrote:
>
> On Mon, 2022-11-28 at 14:02 +0000, Chuck Lever III wrote:
>>
>>> On Nov 28, 2022, at 8:11 AM, Jeff Layton <[email protected]> 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 <[email protected]>
>>>> Cc: <[email protected]>
>>>> ---
>>>> 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