On Mon, May 06, 2024 at 09:05:34PM +0000, Trond Myklebust wrote:
> On Mon, 2024-05-06 at 16:52 -0400, cel@xxxxxxxxxx wrote:
> > From: Chuck Lever <chuck.lever@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Fixes: 5866efa8cbfb ("SUNRPC: Fix svcauth_gss_proxy_init()")
> > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > ---
> > 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
