| Message ID | 166949611830.106845.15345645610329421030.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | quick NFSD-related clean-ups for 6.2 | expand |
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; > } > >
> 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
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 > > >
> 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
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; }
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(-)