From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3530AC07E9D for ; Mon, 26 Sep 2022 08:24:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234212AbiIZIY1 (ORCPT ); Mon, 26 Sep 2022 04:24:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53448 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234179AbiIZIYU (ORCPT ); Mon, 26 Sep 2022 04:24:20 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 994FE357C2 for ; Mon, 26 Sep 2022 01:24:17 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 2BD6CB8199A for ; Mon, 26 Sep 2022 08:24:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9F169C433D6; Mon, 26 Sep 2022 08:24:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1664180654; bh=c8+4swMzdIG1QtHtMY9tXBxXJZpDGIIEJOhbgPCJ08I=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=LStozAeGmVUIs7vHWYRTprJR5IqB7JIkcDi/A1/JC8ENdicJMCzceQV77PQT6BUDo iqLRWoXtdBBBCrbtN45TAtkEMi9D3IeKsDe+CH9OWWG2slvjvK1YJUfovK8zHNiUnG +R7PY+gDfdWSXugPMpY03iirF/4C+kj5qVrDlZoTjTI+Wauh0MPIiQ0vPnUcVEwLQH ALbRJTVUMLAVYJOZphTXvmXllnTsuZdgRYYuVhPIhjoViMGew4DX6qrU3k4CmTW4Wd UoWWd40u95FTDgTgUokvXKPstACpONSU33zs5odgaZ5nLsF+wmscRoROlXpS3BykPt ZXt/l4py7FzNg== Message-ID: <7248d8bc-8a5b-92f3-abd8-26c3f029251a@kernel.org> Date: Mon, 26 Sep 2022 16:24:08 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Subject: Re: [PATCH v2] mm/slub: clean up create_unique_id() Content-Language: en-US To: Vlastimil Babka , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Hugh Dickins Cc: Chao Yu , Christophe JAILLET , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Roman Gushchin , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20220925071207.13183-1-chao@kernel.org> <514669a4-3ce9-c3b7-b293-ab9514f161b3@suse.cz> From: Chao Yu In-Reply-To: <514669a4-3ce9-c3b7-b293-ab9514f161b3@suse.cz> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2022/9/26 16:22, Vlastimil Babka wrote: > On 9/25/22 09:12, Chao Yu wrote: >> From: Chao Yu >> >> As Christophe JAILLET suggested: >> >> In create_unique_id(), >> >> "looks that ID_STR_LENGTH could even be reduced to 32 or 16. >> >> The 2nd BUG_ON at the end of the function could certainly be just >> removed as well or remplaced by a: >>          if (p > name + ID_STR_LENGTH - 1) { >>                  kfree(name); >>                  return -E; >>          } >> " >> >> According to above suggestion, let's do below cleanups: >> 1. reduce ID_STR_LENGTH to 32, as the buffer size should be enough; >> 2. use WARN_ON instead of BUG_ON() and return error if check condition >> is true; >> 3. use snprintf instead of sprintf to avoid overflow. >> >> Link: https://lore.kernel.org/linux-mm/2025305d-16db-abdf-6cd3-1fb93371c2b4@wanadoo.fr/ >> Fixes: 81819f0fc828 ("SLUB core") >> Suggested-by: Christophe JAILLET >> Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> >> Signed-off-by: Chao Yu >> --- >> v2: >> - add WARN_ON() instead of return error silently; >> - use snprintf instead of sprintf to avoid overflow. >>   mm/slub.c | 10 +++++++--- >>   1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/mm/slub.c b/mm/slub.c >> index 4b98dff9be8e..3d37a8a7b965 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -5890,7 +5890,7 @@ static inline struct kset *cache_kset(struct kmem_cache *s) >>       return slab_kset; >>   } >> -#define ID_STR_LENGTH 64 >> +#define ID_STR_LENGTH 32 >>   /* Create a unique string id for a slab cache: >>    * >> @@ -5924,9 +5924,13 @@ static char *create_unique_id(struct kmem_cache *s) >>           *p++ = 'A'; >>       if (p != name + 1) >>           *p++ = '-'; >> -    p += sprintf(p, "%07u", s->size); >> +    p += snprintf(p, ID_STR_LENGTH - 1 - (p - name), "%07u", s->size); > > I think we don't need "- 1" here as snprintf() says: > @size: The size of the buffer, including the trailing null space Oops, my bad. > >> -    BUG_ON(p > name + ID_STR_LENGTH - 1); >> +    if (p > name + ID_STR_LENGTH - 1) { >> +        WARN_ON(1); > > This would be shorter: if (WARN_ON(p > name...)) Thanks for the comments, will update soon. Thanks, > >> +        kfree(name); >> +        return ERR_PTR(-EINVAL); >> +    } >>       return name; >>   } >