Received-SPF: pass (google.com: domain of linux-nfs+bounces-3184-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 6 May 2024 17:13:05 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "cel@kernel.org" <cel@kernel.org>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "dan.carpenter@linaro.org" <dan.carpenter@linaro.org>
Subject: Re: [PATCH] SUNRPC: Fix gss_free_in_token_pages()
Message-ID: <ZjlH4YdsBCYpFQwN@tissot.1015granger.net>
References: <20240506205245.4455-1-cel@kernel.org>
 <528891aeeb2686245cf9665d38c760f4627165fd.camel@hammerspace.com>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <528891aeeb2686245cf9665d38c760f4627165fd.camel@hammerspace.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?iso-8859-1?Q?K/d33KwENaDfceTUTsskFA9zy5smNHsPswD9bbmi5KxGOjm9jUjadFUywS?=
 =?iso-8859-1?Q?pycrbPwuY/mND2e2K5Cc+fngTdBkuwrMoyb/ds/YO7hjyvPORy1T/lWJZl?=
 =?iso-8859-1?Q?E3u1hOR/a0E16T27xjGaA2tgDSVqglsd1zJnNBIBlsd8tmWNbO3copOeUa?=
 =?iso-8859-1?Q?JGZdpRNhS2abRmG046aDnDt92lIPPgNzzGMzKIauFBVjj4eafsNm9uMtRt?=
 =?iso-8859-1?Q?KjcR9dLHOiPzkPAKnzBsyD3TqovU4hSZXHZsmAgRIb1lCx7UKnoon1M2qG?=
 =?iso-8859-1?Q?R//zbrTaIQHbi41ezCh9pyVr71Tx1s/On4ckD6cAT4iVoEfzMXvevObzof?=
 =?iso-8859-1?Q?/lfHQE+S2vErihp96nhD62rTBrk6mHW/CpXj6w4P+XtHjd8tN7Ni4ieJHE?=
 =?iso-8859-1?Q?NAd17tPXjjB9CPlPvKRi1HS4HHxgelHW9FWOySYxJET1vCxtfBf7veWUmj?=
 =?iso-8859-1?Q?uIRylQvejuNeD+/DQBVWPfvD/DsOVKQ8Qxa0LlVxEC5LICwtM07jubS7Y2?=
 =?iso-8859-1?Q?fKGal3xqWFsbzp0Xeuv1XlVHpzLZTzS8bwNIYodgF+x/H+7Z8PIiPX8wtr?=
 =?iso-8859-1?Q?pGXWHc0wacWXgMh9C7ifZeIrfO9u80JC5NvMYbIbxcaYBFSQ+dOLSC4eoR?=
 =?iso-8859-1?Q?zuyBoWkiNOA6FgktNWf7CBPnRwoN+rWb9aAAFcg5SPfuvXUP5QIma+tVRN?=
 =?iso-8859-1?Q?jtePQnkWd4T9aEfMkBl7zpe6v+37iqEu878Wwt+7DUcnxw/1GzJ1kZ42cB?=
 =?iso-8859-1?Q?upHeFlv0uCBJpQXFXVi9ODCmoums4aSBQKc0IdkwzR8PW/DweDX1B/+i/q?=
 =?iso-8859-1?Q?5dYi+SORxaFd2eoiJMzH7N4NowYNuHNEvqVCav2czzuMXW3+dEP2uYZLej?=
 =?iso-8859-1?Q?5LV/Sj2/sTZis6AFDYipF3btoJyBsPzdaFEOcK5N13lBIgoScC557HpY3l?=
 =?iso-8859-1?Q?ZH2C+QyLcBe3HFwop9BR/OWAzfJI8PPYe4B7deF7dlpoRzcrAg4ILBdK7u?=
 =?iso-8859-1?Q?DrEUH1425b0ZIELUb9RtFjoJebMPqFAREM0SZDCx7sFPFkqI7IxtZsTaR1?=
 =?iso-8859-1?Q?UYhh0Ek85ErlY4AHOCh33Wo7cFxsfQpzpyOYy22Fo5OKv/aCOQWT1GtKR0?=
 =?iso-8859-1?Q?wzEyq6vXmFbCN3EOHvY2HbemvcUqzqKfD2USeNVhvd8BITEmUt3lYLQUEf?=
 =?iso-8859-1?Q?2cMPSg9yBZVUvaEyDLz6OJYpjwPaCwHoA3YEpX3dxbeb4isl6aq7OqPLLW?=
 =?iso-8859-1?Q?UORZ6uRAFRn6HmmvOoUCz9Ef9KTZ2Sfr0BCV+EAIQyPJqZUs+3cpKqUH7w?=
 =?iso-8859-1?Q?LcCP8dJNO0OleyLtOZyoxhIgyJGgh9zHlB95qQPjpxYElxPklRqZYpCKVH?=
 =?iso-8859-1?Q?xFG9lpYvUhnUnVqiBDzNjPV/MMFgW31sCXxQTKY0J/VJMvvp8D7dp9e26E?=
 =?iso-8859-1?Q?Hmz0BmVNTVemYy/rtGPUhFJRQKxSm1VRlOPfcKlAwfQw7ocInQ5zEbNJCW?=
 =?iso-8859-1?Q?kLwYvRgBGw2OxF2SBPFLz3OURoyaB68gbnE5FWKJNfPZIQQPN8dsnVPxNj?=
 =?iso-8859-1?Q?U1HDOsKbdWUc6z4r/m34OMn9ikxX10SJNlt31Xt0jPjSku4QOykb7Bgp49?=
 =?iso-8859-1?Q?LmTLiZdnRiyQzWfhjKwIIgcy7Iil7ckzQwTvp4xdPF8axe7v3Lmk01Vg?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	dvRJaPhUK6QZU0pORKD3UcAjF4LUzauLhipk5ZvC0jg5TLetN+EL7sSOthJEtUyYxdgeAR7VzOGpO6dmuJho9cBsFKE0yeSqntgZBW96EYdGQmRz9c49TehRnru3KXrCtZBh+xjFJwqAFH1j6mUPEOkfjd7oCpaAPCiNTA7BiRpWfq7XYFypDASnadfc0frkx5JURvXWpzqBg7NepJRGM3j08bB6igxh0poWIdsVCA/dTlHe3ql7rAFJAn3cl59C//7i9JCPtQTf1MTy+ANSj3CNs0XHquPzkLPiNiHAPjJYqXPO4+Gkph//6xrhilQjBnfGx1G4ZU26paNJw0L2wPtKqonsAvTmR5dxRxT3bLBfeeCwNxQhkzUTDHp92aPTwkIi4JRivsgv8Ks4t24i1S+YWf0WLhTca17Ivc3H1ZnksZ2AsrqjQqYhZFRZ+2nbbmiVFowgCfmj+OXnEGj40Ln6ra+bs/wD4SltMtV8HdFk+0KGAXmWkW8twvu37lvGMkKCQIz2Vc75M3SJ39pDdCjPAjn2NbvwAutIxuQjt1qPS1lVnZTru/KZysdD6rC+dA0ZEpcgtLbZAwnSfluGKr8eBZzWxVnPZWEXrOcczqQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 612ac9d1-de5d-462d-80c0-08dc6e11545a
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 21:13:09.0842
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wSxWERNqWrEM9Nt6RTbBOD15ERssqSMm/5iitu9mz/pAcDh6zZTOTbDKEJMvRNJ/4JoD1E9yufAAwK6jcxZ8CQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR10MB5797
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-06_15,2024-05-06_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 mlxlogscore=999 adultscore=0 spamscore=0 phishscore=0 bulkscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2405060153
X-Proofpoint-GUID: xIhVXiP2UyWo8v2PLTwR0jRCfb0d9tQZ
X-Proofpoint-ORIG-GUID: xIhVXiP2UyWo8v2PLTwR0jRCfb0d9tQZ

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