From: Chuck Lever <[email protected]>
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 <[email protected]>
Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
Signed-off-by: Chuck Lever <[email protected]>
---
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
On Mon, 2024-05-06 at 16:52 -0400, [email protected] wrote:
> From: Chuck Lever <[email protected]>
>
> 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 <[email protected]>
> Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> 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
[email protected]
On Mon, May 06, 2024 at 09:05:34PM +0000, Trond Myklebust wrote:
> On Mon, 2024-05-06 at 16:52 -0400, [email protected] wrote:
> > From: Chuck Lever <[email protected]>
> >
> > 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 <[email protected]>
> > Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
> > Signed-off-by: Chuck Lever <[email protected]>
> > ---
> > ?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