| Message ID | 20240506205245.4455-1-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SUNRPC: Fix gss_free_in_token_pages() | expand |
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
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
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; }