Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Nov 2022 22:16:12 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Vlastimil Babka <vbabka@suse.cz>
CC:     John Thomson <lists@johnthomson.fastmail.com.au>,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Christoph Lameter" <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Dmitry Vyukov <dvyukov@google.com>,
        "Jonathan Corbet" <corbet@lwn.net>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>,
        Robin Murphy <robin.murphy@arm.com>,
        John Garry <john.garry@huawei.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        "linux-mips@vger.kernel.org" <linux-mips@vger.kernel.org>
Subject: Re: [PATCH v6 1/4] mm/slub: enable debugging memory wasting of
 kmalloc
Message-ID: <Y2PNLENnxxpqZ74g@feng-clx>
References: <c4285caf-277c-45fd-8fc7-8a1d61685ce8@app.fastmail.com>
 <Y1+0sbQ3R4DB46NX@feng-clx>
 <9b71ae3e-7f53-4c9e-90c4-79d3d649f94c@app.fastmail.com>
 <Y2DReuPHZungAGsU@feng-clx>
 <53b53476-bb1e-402e-9f65-fd7f0ecf94c2@app.fastmail.com>
 <Y2DngwUc7cLB0dG7@hyeyoo>
 <29271a2b-cf19-4af9-bfe5-5bcff8a23fda@app.fastmail.com>
 <097d8fba-bd10-a312-24a3-a4068c4f424c@suse.cz>
 <Y2NXiiAF6V2DnBrB@feng-clx>
 <f88a5d34-de05-25d7-832d-36b3a3eddd72@suse.cz>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <f88a5d34-de05-25d7-832d-36b3a3eddd72@suse.cz>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?mnIW/4hFAJOAMPxxXE4ITUb7mdVLL6qb6FhCnshD8U50Tht/1LXgyL9hYh6F?=
 =?us-ascii?Q?JDjtdv2cZ/4aZjBD2FqDVrRZ1sca/Ix6x7RNQmDijMzPdOpTsE+SwVICETDg?=
 =?us-ascii?Q?uefhomXiG3XEqmNwvWuPdx0pVqOTWmNPc8TAQvWSDUMdrIuo5XEtP+/mAwja?=
 =?us-ascii?Q?TMyvWWqQsal1Olg3oxh5wg7e238xMI9MVrAnGKbrknr1pgXZk+eOFvHkiPul?=
 =?us-ascii?Q?lyLYqHtoOIwoFI/H0HL1MYBmTE7d7RJm3rsIgT5edJvP6ItkVLGvUcuXMf1f?=
 =?us-ascii?Q?RclUXYd+ppnlBZ7frhxTnNXtRvNhR2pFrltyyZ3wXKIbpxGklCov+kmc8rlU?=
 =?us-ascii?Q?vtJFFMnVGapTLIY9oPjfP9zlGj+aodSfu6sdkiOorqetb5m15kbh/5ZMmrxy?=
 =?us-ascii?Q?rJ41kUIJlMbPx16w5QPHnLYtV8cKulE/JaOLCQHKZCtu/mAaruXhKVAVeij/?=
 =?us-ascii?Q?xwDIvf7wEyYDOsTr3+WQcDEM++GypYNZGdbIzQyRIuVYW9wSJgYMiHC4vbQF?=
 =?us-ascii?Q?45zutgcgIedNy9wg/FTmS0d8B2cJk3ul/eWQtkYGtYLpZMtNNlWL7uaiBOyI?=
 =?us-ascii?Q?72M4T3KT01IEEbdHUc7Evhil4Vlf2cqZnbU3Fs5Yhe/Ih6oG8YHuF0qjNvyB?=
 =?us-ascii?Q?tEHDeVs8reGcqMqyE/N/rcwfPkeOZAH5tawj7H63B2j/W/eJ8A24JcYgDs38?=
 =?us-ascii?Q?w/CValfGTrJQHpx3rSaHGOPqGMMS3R3F1h8F+PDnWQdApNObWa89l9/rmK27?=
 =?us-ascii?Q?do2t8r3+UKRPwU8dzA8aIN3PWqgP+TshHxo9mFI7zN7Mk+Z3VW+U0pNNMV2z?=
 =?us-ascii?Q?4JKYY9IWB2xAkyteQIYl20qngKueY7wbYyg7z8B6izx6K68P8yROO4MeesY1?=
 =?us-ascii?Q?oWMM3lIBrZxo6uF/CcT+y5S05LNrX3PQP6sBwzEZdByDBuq92jTw7rhwSfUm?=
 =?us-ascii?Q?l572hNIayWGbRyLs7usFKKJQajAtEnJZU2LfkPOCZ4ROuMy9u4TL3Lkpi4z2?=
 =?us-ascii?Q?34W9nGGfH2eGdoiCHpPKW52Ei+hyg5NsBeJxr2g47auq8Fx4M6DyoAIMcPwR?=
 =?us-ascii?Q?uoVOUikjvn/u3NkuetGpoBRZJ+auklKlGoc56TeKkm9lj4vGnZDycewd5iCs?=
 =?us-ascii?Q?FJLFxpOKTj9Iykq79ZAJWfK209jNTs5oMFSAq5MTz4QaYYDziu2NjMExXNFo?=
 =?us-ascii?Q?42V+N6HSXeVGdN1bAuLwU01Ggg7x09EWMMZ+w8jrjnJNqLlAxzyDs/hSTv+7?=
 =?us-ascii?Q?zlf745NbEUbpJhlEX1MtWe6VDmShnLoT8Rt9yd9H3wLRYF+fC0uWY77SmlYJ?=
 =?us-ascii?Q?/dgan6l84onMiy6WQZ++uwaRU8GE0HWCJtj0/oMrBBLRQpkU5VKSmrGfElcG?=
 =?us-ascii?Q?vCLC/P+VEAS7WZ9zvibv696WQwBYixO8IVnarEJ5Vr38mlXLPICoUSNTl9XM?=
 =?us-ascii?Q?xwr+ETQfvGYSFfnVAahR2BaS/5lN6ZYILqNADL74GV/8bEBHbyoxCyHYoV6p?=
 =?us-ascii?Q?K5RxcK8zxCn3tS7hW4WO0zkzewIXxYq5A/U5F2yTMfksLiMDAMx1y1ByAMEK?=
 =?us-ascii?Q?6xrf2XoSjXTwsJspLY3SAvgzjUUVR2uCnn+wDDCt?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 47db4366-c8e5-4e9d-3b1a-08dabda669f8
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Nov 2022 14:19:26.9424
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xmERxHJrd6kRVGeuWw5IoLBNZKDyHvnbPrA1vzNdWAI7Atmsxn5NRDWNhSu/HbZmXzRgwIM7VvkXM2x90wCW6A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6368
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Nov 03, 2022 at 09:33:28AM +0100, Vlastimil Babka wrote:
[...]
> >> AFAICS before this patch, we "survive" "kmem_cache *s" being NULL as
> >> slab_pre_alloc_hook() will happen to return NULL and we bail out from
> >> slab_alloc_node(). But this is a side-effect, not an intended protection.
> >> Also the CONFIG_TRACING variant of kmalloc_trace() would have called
> >> trace_kmalloc dereferencing s->size anyway even before this patch.
> >> 
> >> I don't think we should add WARNS in the slab hot paths just to prevent this
> >> rare error of using slab too early. At most VM_WARN... would be acceptable
> >> but still not necessary as crashing immediately from a NULL pointer is
> >> sufficient.
> >> 
> >> So IMHO mips should fix their soc init, 
> > 
> > Yes, for the mips fix, John has proposed to defer the calling of prom_soc_init(),
> > which looks reasonable.
> > 
> >> and we should look into the
> >> CONFIG_TRACING=n variant of kmalloc_trace(), to pass orig_size properly.
> > 
> > You mean check if the pointer is NULL and bail out early. 
> 
> No I mean here:
> 
> #else /* CONFIG_TRACING */
> /* Save a function call when CONFIG_TRACING=n */
> static __always_inline __alloc_size(3)                                   
> void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
> {       
>         void *ret = kmem_cache_alloc(s, flags);
>                     
>         ret = kasan_kmalloc(s, ret, size, flags);
>         return ret;
> }
> 
> we call kmem_cache_alloc() and discard the size parameter, so it will assume
> s->object_size (and as the side-effect, crash if s is NULL). We shouldn't
> add "s is NULL?" checks, but fix passing the size - probably switch to
> __kmem_cache_alloc_node()? and in the following kmalloc_node_trace() analogically.
 
Got it, thanks! I might have missed it during some rebasing for the
kmalloc wastage debug patch.

How about the following fix?

Thanks,
Feng

---
From 9f9fa9da8946fd44625f873c0f51167357075be1 Mon Sep 17 00:00:00 2001
From: Feng Tang <feng.tang@intel.com>
Date: Thu, 3 Nov 2022 21:32:10 +0800
Subject: [PATCH] mm/slub: Add missing orig_size parameter for wastage debug

commit 6edf2576a6cc ("mm/slub: enable debugging memory wasting of
kmalloc") was introduced for debugging kmalloc memory wastage,
and it missed to pass the original request size for kmalloc_trace()
and kmalloc_node_trace() in CONFIG_TRACING=n path.

Fix it by using __kmem_cache_alloc_node() with correct original
request size.

Fixes: 6edf2576a6cc ("mm/slub: enable debugging memory wasting of kmalloc")
Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 include/linux/slab.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 90877fcde70b..9691afa569e1 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -469,6 +469,9 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node) __assume_kmalloc_alignm
 							 __alloc_size(1);
 void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node) __assume_slab_alignment
 									 __malloc;
+void *__kmem_cache_alloc_node(struct kmem_cache *s, gfp_t flags, int node,
+				size_t orig_size, unsigned long caller) __assume_slab_alignment
+									 __malloc;
 
 #ifdef CONFIG_TRACING
 void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
@@ -482,7 +485,8 @@ void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
 static __always_inline __alloc_size(3)
 void *kmalloc_trace(struct kmem_cache *s, gfp_t flags, size_t size)
 {
-	void *ret = kmem_cache_alloc(s, flags);
+	void *ret = __kmem_cache_alloc_node(s, flags, NUMA_NO_NODE,
+					    size, _RET_IP_);
 
 	ret = kasan_kmalloc(s, ret, size, flags);
 	return ret;
@@ -492,7 +496,8 @@ static __always_inline __alloc_size(4)
 void *kmalloc_node_trace(struct kmem_cache *s, gfp_t gfpflags,
 			 int node, size_t size)
 {
-	void *ret = kmem_cache_alloc_node(s, gfpflags, node);
+	void *ret = __kmem_cache_alloc_node(s, gfpflags, node,
+					    size, _RET_IP_);
 
 	ret = kasan_kmalloc(s, ret, size, gfpflags);
 	return ret;
-- 
2.34.1