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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C13C3EE57DF for ; Mon, 11 Sep 2023 15:54:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 058566B02BD; Mon, 11 Sep 2023 11:54:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 008E66B02BE; Mon, 11 Sep 2023 11:54:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DEAA16B02BF; Mon, 11 Sep 2023 11:54:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id CC1CC6B02BD for ; Mon, 11 Sep 2023 11:54:27 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 9FC5CB32E1 for ; Mon, 11 Sep 2023 15:54:27 +0000 (UTC) X-FDA: 81224763774.09.1DF51F6 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by imf23.hostedemail.com (Postfix) with ESMTP id 1D3C4140003 for ; Mon, 11 Sep 2023 15:54:24 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=djbRf8c2; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=sqateda9; spf=pass (imf23.hostedemail.com: domain of vbabka@suse.cz designates 195.135.220.28 as permitted sender) smtp.mailfrom=vbabka@suse.cz; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694447665; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ZdEZzTCi1Qv5M3fqfhRCBlSPg3Eg/DkRRb1C3EMN/Pc=; b=L+P3peBsOIc+dc6b3tA57d+jZO5aV+nmFhOcYC4qDfwO72I9ueolhuG/SwzKT46ahDeO39 JFIYud0OSSGVSdQzk4fVu1BjoUMdg+z/YWzkxV6BhOObFEefNFsKM2+X5aSyjYWte6gxiR S52jFyiN33BUrCwefw1B6C92xrVATo8= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=djbRf8c2; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=sqateda9; spf=pass (imf23.hostedemail.com: domain of vbabka@suse.cz designates 195.135.220.28 as permitted sender) smtp.mailfrom=vbabka@suse.cz; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694447665; a=rsa-sha256; cv=none; b=ztRr61QO7EKWFbzAeV/8VplRf39zpP8WEoM1dOyF5mWdmSXiSJK/AqU0IBurEk5Aa4t9/n opgfLeE0LjlbvoxcYatGFxGc+db7kutrx9NyVSouu6sRG6yweH5I3N+u6ZZMcHPMMmWToV M05N/qlcuO7Znq9AkT6GZ+wwGCgtGM8= Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id B198921858; Mon, 11 Sep 2023 15:54:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1694447662; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=ZdEZzTCi1Qv5M3fqfhRCBlSPg3Eg/DkRRb1C3EMN/Pc=; b=djbRf8c2O3l1ryRnfxT1+EQt++7yt9L5qUAFvEXThBGYjkJ18MeA9PCkZ2t2civfRDv42u AFq4a8UkknLYqb7SHFsj8z+MaEtazy3Llb3lczV5Eeapsthb9arURLVXBWv63LvlEYoosv RhjTnc9wdJvGy6+NKsdmPBMhQpPXVZM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1694447662; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=ZdEZzTCi1Qv5M3fqfhRCBlSPg3Eg/DkRRb1C3EMN/Pc=; b=sqateda9BhsWxsQGACoqerFl89rYE9OOPvXSFqE73X3vPnBs5rvbvPW+YrpdFaOElvCzkX 4Bqou12983rWaYCA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 7DEA313780; Mon, 11 Sep 2023 15:54:22 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Q7GjHS44/2QBeAAAMHmgww (envelope-from ); Mon, 11 Sep 2023 15:54:22 +0000 Message-ID: Date: Mon, 11 Sep 2023 17:54:22 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size Content-Language: en-US To: David Laight , 'Kees Cook' Cc: "linux-kernel@vger.kernel.org" , "'linux-mm@kvack.org'" , 'Christoph Lameter' , 'Pekka Enberg' , 'David Rientjes' , 'Joonsoo Kim' , 'Andrew Morton' , 'Eric Dumazet' , Hyeonggon Yoo <42.hyeyoo@gmail.com>, Roman Gushchin References: <4d31a2bf7eb544749023cf491c0eccc8@AcuMS.aculab.com> <202309071235.CB4F6B2@keescook> <20ca0a567a874052a1161e9be0870463@AcuMS.aculab.com> From: Vlastimil Babka Autocrypt: addr=vbabka@suse.cz; keydata= xsFNBFZdmxYBEADsw/SiUSjB0dM+vSh95UkgcHjzEVBlby/Fg+g42O7LAEkCYXi/vvq31JTB KxRWDHX0R2tgpFDXHnzZcQywawu8eSq0LxzxFNYMvtB7sV1pxYwej2qx9B75qW2plBs+7+YB 87tMFA+u+L4Z5xAzIimfLD5EKC56kJ1CsXlM8S/LHcmdD9Ctkn3trYDNnat0eoAcfPIP2OZ+ 9oe9IF/R28zmh0ifLXyJQQz5ofdj4bPf8ecEW0rhcqHfTD8k4yK0xxt3xW+6Exqp9n9bydiy tcSAw/TahjW6yrA+6JhSBv1v2tIm+itQc073zjSX8OFL51qQVzRFr7H2UQG33lw2QrvHRXqD Ot7ViKam7v0Ho9wEWiQOOZlHItOOXFphWb2yq3nzrKe45oWoSgkxKb97MVsQ+q2SYjJRBBH4 8qKhphADYxkIP6yut/eaj9ImvRUZZRi0DTc8xfnvHGTjKbJzC2xpFcY0DQbZzuwsIZ8OPJCc LM4S7mT25NE5kUTG/TKQCk922vRdGVMoLA7dIQrgXnRXtyT61sg8PG4wcfOnuWf8577aXP1x 6mzw3/jh3F+oSBHb/GcLC7mvWreJifUL2gEdssGfXhGWBo6zLS3qhgtwjay0Jl+kza1lo+Cv BB2T79D4WGdDuVa4eOrQ02TxqGN7G0Biz5ZLRSFzQSQwLn8fbwARAQABzSBWbGFzdGltaWwg QmFia2EgPHZiYWJrYUBzdXNlLmN6PsLBlAQTAQoAPgIbAwULCQgHAwUVCgkICwUWAgMBAAIe AQIXgBYhBKlA1DSZLC6OmRA9UCJPp+fMgqZkBQJhqjfCBQkPDwOlAAoJECJPp+fMgqZkQhUQ AKmadNSB72V3yGL3T1KMpQhvhMDmlsuZRxjIgydOh0n+eq4fSVN6C7CF2nSFALN2rp/GLZSm mOrXlAvIWp8uVXZ2LTjrOu9526xm03QRL7/dwOiG1e51vTIWJ+WwGcpEexosDrIqQuNK6bki a6Pe/rRUM0BCQY09l7jnsOQv9qHAQXMacG+JrfmYctoINEOetsVWOmlE68OjjxQI17djki78 gSA53vPWBg7CJse7+EeyMyEzuQIe2Z9czVtSwjVE76ho/QifLey7ZrC9EZqihan1TWX2C785 RFOqOYEeeS4fYJllYXGUHcFD/oIWhPW8xJ+9eCbsjd6A84s9GAdIABtmd6HlxXhPxGSfpyiF lVGjj8O1jWcQaTEyGwXn3TeFkDlahVgqj2okmkLOvp4CMm8NYuW32P6w7e7b1YKGbNY2efd0 agD0gDIF4u1tC/xy1NrEskCgWpZW61Clhm0aSjIvBB5dx3JIOgruy23cr90TvEl9gZLHlD9B PRSSjOwNaGIjhC60OhAnFsftqJKfsc3dFdyViwCXmSG4ilx8gTINYjlTPsvzF09GgIY0gg+h V7bEiBU5fftmXGemcFPzpPu4HweVrBSbD4VSpzynx/7N8E4sJ4Yt0w9yc0aLvMXW0KijX4UQ K+9UPEsYefg1HeeES2bpsbgB1Mhle9Xh8L+izsBNBFsZNTUBCACfQfpSsWJZyi+SHoRdVyX5 J6rI7okc4+b571a7RXD5UhS9dlVRVVAtrU9ANSLqPTQKGVxHrqD39XSw8hxK61pw8p90pg4G /N3iuWEvyt+t0SxDDkClnGsDyRhlUyEWYFEoBrrCizbmahOUwqkJbNMfzj5Y7n7OIJOxNRkB IBOjPdF26dMP69BwePQao1M8Acrrex9sAHYjQGyVmReRjVEtv9iG4DoTsnIR3amKVk6si4Ea X/mrapJqSCcBUVYUFH8M7bsm4CSxier5ofy8jTEa/CfvkqpKThTMCQPNZKY7hke5qEq1CBk2 wxhX48ZrJEFf1v3NuV3OimgsF2odzieNABEBAAHCwXwEGAEKACYCGwwWIQSpQNQ0mSwujpkQ PVAiT6fnzIKmZAUCYao6gQUJClNsTAAKCRAiT6fnzIKmZOtJEAC07ejmfZUPKdHuk0jGBgJc FJxq1AP+Gv4i6dVb81cT6RT0vgPIhk/+H3XlIqjgmqKWXVDEv9LibG5RgSe9MWfg6zBAPtOe NFCdksRMQGLHu7OGWO84QNSjrgf3MRlQidpXTBEB3AxB3ajrDhoJy5468qkMQvK4khRjrY1X EKVHFWZbf8Vr+LnL3LdmYGs3OxXfuOeLhFlvFSR3iAHX2AFECRnShcRZC0u0+7MEmmq+QCq5 6TPXB6MDaBAZUTM3+5JiAqvjD+574IbdVpUDWyfVvMOwzaOwErCb8FgNfrj5uqO/s9t/dYUk NTpzIw0gHuKKpveLCTzyDRROX6E4JpFn39/WsQJ011D6Df8vkHHsn0HxFs166cXSCuAjnu5h /T6JwVSprwNfNlYIlYXSJoXQUZ9KtGZ1dfco/7CP7u7K8AKx2l6bRbFNbHAANG0xRFySQ1Aa PJMX1FybUn75MZsrqshwGjtCRnlyFp4S2WdIQCqlrEQjwonpEIM7Hw9JuxQgMIqq2HT0cru3 iu8RMeQytfOhEkhhj936xX0CA/fpx+7XaO13vXOGZxI1ArRzZteItLc9SR+IYUPMgDsB4KFl V6Cs0Mfxv/h2nu22pLY7HBkXOchrO4TLhet+GRjUg2OJ4asaF2PCrZaEUi/yZjybnnKjOO61 tR6d+JzM8nFUZA== In-Reply-To: <20ca0a567a874052a1161e9be0870463@AcuMS.aculab.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 1D3C4140003 X-Rspam-User: X-Stat-Signature: gp1edzewwg1diood6yyc1hnt4prhfzuk X-Rspamd-Server: rspam01 X-HE-Tag: 1694447664-669269 X-HE-Meta: U2FsdGVkX1902ABTzyEa1uhFWWLpN8/K7gnzoRV8kga7JQ5nQ1Qdd4W0OywS6Gxq9eOVdrFYaBUydQHBKnAosJwDHxZgOAKxAd6/S7ndVXF9iJAODg3BNHbKodBYpaA+DSoAl7Bj+mhwkjClZeG4/SLyl8d6LVSORbjx8pqHES3naCYvh42Ekq0267Mo9UAmjsPCuJPrWNYl9yWwcwS9amLry2bKF9CXX8BSVtbFASB1UKmPPhKFwIEsjMRU6dxju2F1LkUBOMt1aWY4ueqtkQFWZG7yMelaUm/I79O0U6+XHI/Q9PN+3jiF7eU81JB1OuHMgP3xA2N4AKaf43pLb2gDjewWs9H2QVTo0m3xU73WyeztRRK4QiNcE1FYzZLUBU+nEvCjrj/Un3DWfX/lbaeM1kCmhEOn2bRqf/C9FsKCMQmMmgQ/4m4y1JGiZhg1LzKiuX8Ej2X/OEWUOTbcN8oD1XrOxImDdgXzhm/rppUj2Ls6wPE50300sfbFIx5uwSkTGisjiCKMUD1lUZ4/d1HlZZWSlozexXwAzNo5SI4DTk/NYBo6FQf95Ur9KyGO37uY0nzAgnhS0OJEzfuTMXekch7vpOsQaShEnNf8Lnt5gDZT0F8fsUeYfBgIiweOv8PGTlzTDoJx0fphRDv8le75jRasrmXDDTWgn2JQQTv0VmVwfM/ONrD7enBT/664YUFEvjHjU5b996pPXiRR94pj7du126jTzpkkHzcMXRtupWkOj/QvZusGW7DkV17j4Akt3yKUBkE0RK7WBLdtxW9p0LWwxYqPXe2DfPW+Fd3KOdnXXtEgR9PMdH36ZzPX6DFtBuc0ndbPL2QF+TH2WyWxKFmUcSsD9ZFaSqyu2cMbmANxtrV+Fwgmhf4kjVoAWb6Ddx/xsUERmL1rXTPC9sCQDq29lEJZTNGr0vsPNcw3okUMuuzCZdmeNjgANL6QM4thM1exUhqdoDDkMWg 0US+d3x3 UlKS7Z+OOTuHsBIR2l6gF3X5VoEYztudROyQ9bKbLlDXONR5GfP9JpO0LYDLVHnrj8XIhEHYSuKy8ZjmsDFErIxBTbjRq2LjNvMZEI2rhVJKv8mVvcaY5iwnALxbQbruh9FaR2z5+xYOiRy3VK4EJ9KGi0T4A6wRwpBO+QlJWd6gwDBjwCVUAvkPOYsr04M8KtKoM7JxOAtR8oUSIhYd0PpKbL1zExdS21nYD0ox6BdGuQJP58/hT8kXxZuVePxxCfV8zl3RqMRVicfE6gv6CBHo9+y1FeVbGvg4QduhO1VeOXuqailS0Rqc/K+CeHjS7x+NsIqXCXB2HwBpx5XzqFrdUmEDXT8/wOmIGhLUMy3rOHrfvh/7oE//RQlKcDf2j+Ek/xA4XxVUUg9trvpPJ11cnmeZd04iw14yDB3MTkRDK5T+3LYXykiON4+8UkJK55L3E911c+L8jOvs= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 9/8/23 10:26, David Laight wrote: > From: Kees Cook >> Sent: 07 September 2023 20:38 >> >> On Thu, Sep 07, 2023 at 12:42:20PM +0000, David Laight wrote: >> > The typical use of kmalloc_size_roundup() is: >> > ptr = kmalloc(sz = kmalloc_size_roundup(size), ...); >> > if (!ptr) return -ENOMEM. >> > This means it is vitally important that the returned value isn't >> > less than the argument even if the argument is insane. >> > In particular if kmalloc_slab() fails or the value is above >> > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return >> > it's single zero-length buffer. >> > >> > Fix by returning the input size on error or if the size exceeds >> > a 'sanity' limit. >> > kmalloc() will then return NULL is the size really is too big. >> > >> > >> > Signed-off-by: David Laight >> > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") >> > --- >> > v2: >> > - Use KMALLOC_MAX_SIZE for upper limit. >> > (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) >> > - Invert test for overlarge for consistency. >> > - Put a likely() on result of kmalloc_slab(). >> > >> > mm/slab_common.c | 26 +++++++++++++------------- >> > 1 file changed, 13 insertions(+), 13 deletions(-) >> > >> > diff --git a/mm/slab_common.c b/mm/slab_common.c >> > index cd71f9581e67..0fb7c7e19bad 100644 >> > --- a/mm/slab_common.c >> > +++ b/mm/slab_common.c >> > @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) >> > { >> > struct kmem_cache *c; >> > >> > - /* Short-circuit the 0 size case. */ >> > - if (unlikely(size == 0)) >> > - return 0; >> >> If we want to allow 0, let's just leave this case as-is: the compiler >> will optimize it against the other tests. > > I doubt the compiler will optimise it away - especially with > the unlikely(). Yeah I also think compiler can't do much optimizations except for build-time constant 0 here. > OTOH the explicit checks for (size && size <= LIMIT) do > get optimised to ((size - 1) <= LIMIT - 1) so become > a single compare. > > Then returning 'size' at the bottom means that zero is returned > in the arg is zero - which is fine. > >> >> > - /* Short-circuit saturated "too-large" case. */ >> > - if (unlikely(size == SIZE_MAX)) >> > - return SIZE_MAX; >> > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { >> > + /* >> > + * The flags don't matter since size_index is common to all. >> > + * Neither does the caller for just getting ->object_size. >> > + */ >> > + c = kmalloc_slab(size, GFP_KERNEL, 0); >> > + return likely(c) ? c->object_size : size; >> >> I would like to have this fail "safe". c should never be NULL here, so >> let's return "KMALLOC_MAX_SIZE + 1" to force failures. > > Why even try to force failure here? > The whole function is just an optimisation so that the caller > can use the spare space. > > The only thing it mustn't do is return a smaller value. If "c" is NULL it means either the kernel build must be broken e.g. by somebody breaking the KMALLOC_MAX_CACHE_SIZE value, and we could just ignore c being NULL and let it crash because of that. But I think it can also be NULL due to trying to call kmalloc_size_roundup() too early, when kmalloc_caches array is not yet populated. Note if we call kmalloc() itself too early, we get a NULL as a result, AFAICS. I can imagine two scenarios: - kmalloc_size_roundup() is called with result immediately fed to kmalloc() that happens too early, in that case we best should not crash on c being NULL and make sure the kmalloc() returns NULL. - kmalloc_size_roundup() is called in some init code to get a value that some later kmalloc() call uses. We might want also not crash in that case, but informing the developer that they did something wrong would be also useful? Clearly returning 0 if c == NULL, as done currently, is wrong for both scenarios. Retuning "size" is OK for the first scenario, also valid for the second one, but the caller will silently lose the benefit of kmalloc_size_roundup() and the developer introducing that won't realize it's done too early and could be fixed. So perhaps the best would be to return size for c == NULL, but also do a WARN_ONCE? >> >> > + } >> > + >> > /* Above the smaller buckets, size is a multiple of page size. */ >> > - if (size > KMALLOC_MAX_CACHE_SIZE) >> > + if (size && size <= KMALLOC_MAX_SIZE) >> > return PAGE_SIZE << get_order(size); >> > >> > - /* >> > - * The flags don't matter since size_index is common to all. >> > - * Neither does the caller for just getting ->object_size. >> > - */ >> > - c = kmalloc_slab(size, GFP_KERNEL, 0); >> > - return c ? c->object_size : 0; >> > + /* Return 'size' for 0 and very large - kmalloc() may fail. */ >> >> I want to _be certain_ failure happens. If we get here we need to return >> "KMALLOC_MAX_SIZE + 1" > > Why care? Yeah no need for that. > > David > >> >> -Kees >> >> > + return size; >> > + >> > } >> > EXPORT_SYMBOL(kmalloc_size_roundup); >> > >> > -- >> > 2.17.1 >> > >> > - >> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >> > Registration No: 1397386 (Wales) >> > >> >> -- >> Kees Cook > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >