* [PATCH] SUNRPC: Fix gss_free_in_token_pages()
@ 2024-05-06 20:52 cel
2024-05-06 21:05 ` Trond Myklebust
0 siblings, 1 reply; 3+ messages in thread
From: cel @ 2024-05-06 20:52 UTC (permalink / raw)
To: linux-nfs; +Cc: Chuck Lever, Dan Carpenter
From: Chuck Lever <chuck.lever@oracle.com>
Commit 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()") from Oct
24, 2019 (linux-next), leads to the following Smatch static checker
warning:
net/sunrpc/auth_gss/svcauth_gss.c:1039 gss_free_in_token_pages()
warn: iterator 'i' not incremented
net/sunrpc/auth_gss/svcauth_gss.c
1034 static void gss_free_in_token_pages(struct gssp_in_token *in_token)
1035 {
1036 u32 inlen;
1037 int i;
1038
--> 1039 i = 0;
1040 inlen = in_token->page_len;
1041 while (inlen) {
1042 if (in_token->pages[i])
1043 put_page(in_token->pages[i]);
^
This puts page zero over and over.
1044 inlen -= inlen > PAGE_SIZE ? PAGE_SIZE : inlen;
1045 }
1046
1047 kfree(in_token->pages);
1048 in_token->pages = NULL;
1049 }
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 24de94184700..bdd8273d74d3 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1040,7 +1040,7 @@ static void gss_free_in_token_pages(struct gssp_in_token *in_token)
inlen = in_token->page_len;
while (inlen) {
if (in_token->pages[i])
- put_page(in_token->pages[i]);
+ put_page(in_token->pages[i++]);
inlen -= inlen > PAGE_SIZE ? PAGE_SIZE : inlen;
}
base-commit: 939cb14d51a150e3c12ef7a8ce0ba04ce6131bd2
--
2.44.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] SUNRPC: Fix gss_free_in_token_pages()
2024-05-06 20:52 [PATCH] SUNRPC: Fix gss_free_in_token_pages() cel
@ 2024-05-06 21:05 ` Trond Myklebust
2024-05-06 21:13 ` Chuck Lever
0 siblings, 1 reply; 3+ messages in thread
From: Trond Myklebust @ 2024-05-06 21:05 UTC (permalink / raw)
To: cel@kernel.org, linux-nfs@vger.kernel.org
Cc: dan.carpenter@linaro.org, chuck.lever@oracle.com
On Mon, 2024-05-06 at 16:52 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
>
> Commit 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()") from Oct
> 24, 2019 (linux-next), leads to the following Smatch static checker
> warning:
>
> net/sunrpc/auth_gss/svcauth_gss.c:1039
> gss_free_in_token_pages()
> warn: iterator 'i' not incremented
>
> net/sunrpc/auth_gss/svcauth_gss.c
> 1034 static void gss_free_in_token_pages(struct gssp_in_token
> *in_token)
> 1035 {
> 1036 u32 inlen;
> 1037 int i;
> 1038
> --> 1039 i = 0;
> 1040 inlen = in_token->page_len;
> 1041 while (inlen) {
> 1042 if (in_token->pages[i])
> 1043 put_page(in_token->pages[i]);
> ^
> This puts page zero over and over.
>
> 1044 inlen -= inlen > PAGE_SIZE ? PAGE_SIZE :
> inlen;
> 1045 }
> 1046
> 1047 kfree(in_token->pages);
> 1048 in_token->pages = NULL;
> 1049 }
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
> b/net/sunrpc/auth_gss/svcauth_gss.c
> index 24de94184700..bdd8273d74d3 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -1040,7 +1040,7 @@ static void gss_free_in_token_pages(struct
> gssp_in_token *in_token)
> inlen = in_token->page_len;
> while (inlen) {
> if (in_token->pages[i])
> - put_page(in_token->pages[i]);
> + put_page(in_token->pages[i++]);
Wouldn't it be both more efficient and transparent just to break out of
the loop once you hit a NULL page? You already know the remainder of
the array will also be NULL.
> inlen -= inlen > PAGE_SIZE ? PAGE_SIZE : inlen;
> }
>
>
> base-commit: 939cb14d51a150e3c12ef7a8ce0ba04ce6131bd2
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] SUNRPC: Fix gss_free_in_token_pages()
2024-05-06 21:05 ` Trond Myklebust
@ 2024-05-06 21:13 ` Chuck Lever
0 siblings, 0 replies; 3+ messages in thread
From: Chuck Lever @ 2024-05-06 21:13 UTC (permalink / raw)
To: Trond Myklebust
Cc: cel@kernel.org, linux-nfs@vger.kernel.org,
dan.carpenter@linaro.org
On Mon, May 06, 2024 at 09:05:34PM +0000, Trond Myklebust wrote:
> On Mon, 2024-05-06 at 16:52 -0400, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> >
> > Commit 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()") from Oct
> > 24, 2019 (linux-next), leads to the following Smatch static checker
> > warning:
> >
> > net/sunrpc/auth_gss/svcauth_gss.c:1039
> > gss_free_in_token_pages()
> > warn: iterator 'i' not incremented
> >
> > net/sunrpc/auth_gss/svcauth_gss.c
> > 1034 static void gss_free_in_token_pages(struct gssp_in_token
> > *in_token)
> > 1035 {
> > 1036 u32 inlen;
> > 1037 int i;
> > 1038
> > --> 1039 i = 0;
> > 1040 inlen = in_token->page_len;
> > 1041 while (inlen) {
> > 1042 if (in_token->pages[i])
> > 1043 put_page(in_token->pages[i]);
> > ^
> > This puts page zero over and over.
> >
> > 1044 inlen -= inlen > PAGE_SIZE ? PAGE_SIZE :
> > inlen;
> > 1045 }
> > 1046
> > 1047 kfree(in_token->pages);
> > 1048 in_token->pages = NULL;
> > 1049 }
> >
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> > net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c
> > b/net/sunrpc/auth_gss/svcauth_gss.c
> > index 24de94184700..bdd8273d74d3 100644
> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > @@ -1040,7 +1040,7 @@ static void gss_free_in_token_pages(struct
> > gssp_in_token *in_token)
> > inlen = in_token->page_len;
> > while (inlen) {
> > if (in_token->pages[i])
> > - put_page(in_token->pages[i]);
> > + put_page(in_token->pages[i++]);
>
> Wouldn't it be both more efficient and transparent just to break out of
> the loop once you hit a NULL page? You already know the remainder of
> the array will also be NULL.
Based on the way that the ->pages[] array is constructed in
gss_read_proxy_verf() ? I guess that is true.
> > inlen -= inlen > PAGE_SIZE ? PAGE_SIZE : inlen;
> > }
> >
> >
> > base-commit: 939cb14d51a150e3c12ef7a8ce0ba04ce6131bd2
--
Chuck Lever
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-06 21:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-06 20:52 [PATCH] SUNRPC: Fix gss_free_in_token_pages() cel
2024-05-06 21:05 ` Trond Myklebust
2024-05-06 21:13 ` Chuck Lever
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.