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 05546EE57E2 for ; Fri, 8 Sep 2023 08:26:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4B3E86B00A4; Fri, 8 Sep 2023 04:26:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 462B36B00A5; Fri, 8 Sep 2023 04:26:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 32BEC6B00A6; Fri, 8 Sep 2023 04:26:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 20A076B00A4 for ; Fri, 8 Sep 2023 04:26:50 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id D8076140EFB for ; Fri, 8 Sep 2023 08:26:49 +0000 (UTC) X-FDA: 81212749338.05.D206310 Received: from eu-smtp-delivery-151.mimecast.com (eu-smtp-delivery-151.mimecast.com [185.58.85.151]) by imf04.hostedemail.com (Postfix) with ESMTP id 83EC94001D for ; Fri, 8 Sep 2023 08:26:46 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=aculab.com; spf=pass (imf04.hostedemail.com: domain of david.laight@aculab.com designates 185.58.85.151 as permitted sender) smtp.mailfrom=david.laight@aculab.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694161607; 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; bh=EeamgIL1k9G70ybBOCsOamJ2CBeMf3v7cR2Qhr7d8iA=; b=d+86XIqmZpd/cgnw5tTjIQ7v1KRfGPlF605cfhzBBiBydxQAd569JwLhGmJ4N+KGWwwD5S aGbkJpBoXsXv4Y2E8v8DXJhMua/eq+xjZYNviD0zVEPSVIzaqreA6vj7UjpMfXNZF7i4/4 j3EqdE7MAZzVPB5vyjeIqQhluTlZfHk= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=aculab.com; spf=pass (imf04.hostedemail.com: domain of david.laight@aculab.com designates 185.58.85.151 as permitted sender) smtp.mailfrom=david.laight@aculab.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694161607; a=rsa-sha256; cv=none; b=z5g0NXhnrmphGSyRIYD5hIttDbAiVpWuoxClFdt5lM3favTuuHcV69zYz5aavv2mkP1STo LnOt9jINhQBbcmVg1cfso0eOTggfN3PSXpG3cZ7mRwDWr2JbVUDLo3PBOHY3XxVkavAEEb FAaovbwTMl13p18qDCnI3BADGVE7Ba0= Received: from AcuMS.aculab.com (156.67.243.121 [156.67.243.121]) by relay.mimecast.com with ESMTP with both STARTTLS and AUTH (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id uk-mta-162-3H32lV7WNIegh-Ql8HGViw-1; Fri, 08 Sep 2023 09:26:43 +0100 X-MC-Unique: 3H32lV7WNIegh-Ql8HGViw-1 Received: from AcuMS.Aculab.com (10.202.163.4) by AcuMS.aculab.com (10.202.163.4) with Microsoft SMTP Server (TLS) id 15.0.1497.48; Fri, 8 Sep 2023 09:26:40 +0100 Received: from AcuMS.Aculab.com ([::1]) by AcuMS.aculab.com ([::1]) with mapi id 15.00.1497.048; Fri, 8 Sep 2023 09:26:40 +0100 From: David Laight To: 'Kees Cook' CC: 'Vlastimil Babka' , "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" Subject: RE: Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size Thread-Topic: Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size Thread-Index: Adnhh8rbtLpHk7QBQE+HpPR0NWDZ5gAMq8SAABymU5A= Date: Fri, 8 Sep 2023 08:26:40 +0000 Message-ID: <20ca0a567a874052a1161e9be0870463@AcuMS.aculab.com> References: <4d31a2bf7eb544749023cf491c0eccc8@AcuMS.aculab.com> <202309071235.CB4F6B2@keescook> In-Reply-To: <202309071235.CB4F6B2@keescook> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: aculab.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 83EC94001D X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: hfokt3ug3ctaeipmmthc136y5isy6ozk X-HE-Tag: 1694161606-294989 X-HE-Meta: U2FsdGVkX1+lpFNfeojG8FQt2wxLoGPy1WQ+Row00pJVE+sFDvL9SaQT+iwtbTuZcEjESTWxEBzxWSRxBG+YZs6DVdQUWy0Tlh3Y4GBHhU4fcFuz8Va+ZvFNkUDyTEHSS9aFmwhWjUFFlFENd0y6QQXymmIGQHU3ZgY0aEnfIqTbAuz7NnGf6fmzPmnVasb7ClKkZz92/o9yGlM5BixGRfqUnF2GzSZ5956gNJrzn5XoWEF2BfqLzCEg6uvZdBEOmPy/BpI44V6z/AaPy5cW6sewh2fm9iqLZe+Dgf7sjtUVPRmcDOoROXwOV95qrdYtSzsIIMHeutOoqHxGZO4WNSRXR4TVHFl5CZonCl2ngunhyZ5YzhlA5cT4goTKPQrQ1jEHuqvN7Y3vGZRpewWxv1LxmkcUdVhAH0fzErQgSYwlKEjBlV5+OYVm0ibOSj0YrBpZ8biHO8MQONA4bJcRyJ0MldybPftA2dPYpNofHAn0KlyrXdVh2kp5FTI5RCPSEraYcramJIe9C9B6gigNZECojYAOTr7IgxVjUMtoM4dUo5YxcdYXKZyUhkSMft0EdpujvtGmJ7vyB9Ortd5+xrzc4g7VvHEZSCoj9Oxq7MNNOAB5L7dOs6t+ZDfWgezaUwo1MxQcKWqvqQ2w47ffBk0/upwUEANp1Ps6oTSObu/3s7yJEN0kdM3DwnZR6dlKHel2LV9Bgh6fpM3hExVdgHRTniz9nLm2iU0zLFuyiBjy/9Dfm87tpX5F8Ppmwfalf1YhP/5mCqCu+PRB5PawOjE00mEN8gpMeNBn9tC3s6HBD30FMwNsCngN6/amzG8MPEw02/4fHR+658GDQ2EF98Tkq95Btj6mNMq0HIJb51nBcE5ia/MXX57/SWYF7cOmzv0mjEhOcTTwt3Sf1ongzN6xCOryekAHgzitTlj9v5ZkL3JtKb3g6U6DHwrUAEkNmm1yGKLk7t5rs1fthBt pNabN2ce 7Kb7+xxOhTx6sV4EdO9p7tt+vGJSC3mDKnnTolOIbOgYC9sKslkY5P/H0ZVtId56nm0KPgXzuHEWgyO9OgFog3ambo3fJevyDjj7hE9zioIsLgzZfS/yntSwOAiavnSH2C18c2HwYdsw2keYjPopZljMhTQKWZ0QUP61hX3TuiCwvkNIQo0OMA+gwdOWuGIqnEJ2wjHEeL9OwSMrzOu/nb8VENXVJFXjcfQO+AP3xDUWlrV2dAoVaoEM7jUZvho0BxMMZLOzy0JKPhi+aYqq8JtE4HGdjylUUm/JKOQwBKV7+NVBnA4R/IM+cevSFNAcAX6Teg8e+JPCX1OwaY56KnhY9kLsbydtRg8+6m5zsghBK5uZjVI3j/iXuXpNVAuIRlU34XqNOTmLU+bxXu3DrR4K8f/PV3DSCR2fX 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: From: Kees Cook > Sent: 07 September 2023 20:38 >=20 > On Thu, Sep 07, 2023 at 12:42:20PM +0000, David Laight wrote: > > The typical use of kmalloc_size_roundup() is: > > =09ptr =3D kmalloc(sz =3D kmalloc_size_roundup(size), ...); > > =09if (!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) > > { > > =09struct kmem_cache *c; > > > > -=09/* Short-circuit the 0 size case. */ > > -=09if (unlikely(size =3D=3D 0)) > > -=09=09return 0; >=20 > 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(). OTOH the explicit checks for (size && size <=3D LIMIT) do get optimised to ((size - 1) <=3D 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. >=20 > > -=09/* Short-circuit saturated "too-large" case. */ > > -=09if (unlikely(size =3D=3D SIZE_MAX)) > > -=09=09return SIZE_MAX; > > +=09if (size && size <=3D KMALLOC_MAX_CACHE_SIZE) { > > +=09=09/* > > +=09=09 * The flags don't matter since size_index is common to all. > > +=09=09 * Neither does the caller for just getting ->object_size. > > +=09=09 */ > > +=09=09c =3D kmalloc_slab(size, GFP_KERNEL, 0); > > +=09=09return likely(c) ? c->object_size : size; >=20 > 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. >=20 > > +=09} > > + > > =09/* Above the smaller buckets, size is a multiple of page size. */ > > -=09if (size > KMALLOC_MAX_CACHE_SIZE) > > +=09if (size && size <=3D KMALLOC_MAX_SIZE) > > =09=09return PAGE_SIZE << get_order(size); > > > > -=09/* > > -=09 * The flags don't matter since size_index is common to all. > > -=09 * Neither does the caller for just getting ->object_size. > > -=09 */ > > -=09c =3D kmalloc_slab(size, GFP_KERNEL, 0); > > -=09return c ? c->object_size : 0; > > +=09/* Return 'size' for 0 and very large - kmalloc() may fail. */ >=20 > I want to _be certain_ failure happens. If we get here we need to return > "KMALLOC_MAX_SIZE + 1" Why care? =09David >=20 > -Kees >=20 > > +=09return size; > > + > > } > > EXPORT_SYMBOL(kmalloc_size_roundup); > > > > -- > > 2.17.1 > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, M= K1 1PT, UK > > Registration No: 1397386 (Wales) > > >=20 > -- > Kees Cook - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1= PT, UK Registration No: 1397386 (Wales)