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 C3004C282D0 for ; Tue, 4 Mar 2025 08:24:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 445F56B0088; Tue, 4 Mar 2025 03:24:21 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3CD846B0089; Tue, 4 Mar 2025 03:24:21 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 26CFD280001; Tue, 4 Mar 2025 03:24:21 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 05B3A6B0088 for ; Tue, 4 Mar 2025 03:24:20 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 8137CA5EB3 for ; Tue, 4 Mar 2025 08:24:20 +0000 (UTC) X-FDA: 83183181480.23.7A905EA Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) by imf25.hostedemail.com (Postfix) with ESMTP id 8EEDEA0011 for ; Tue, 4 Mar 2025 08:24:18 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=iANwU3Cn; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.218.48 as permitted sender) smtp.mailfrom=lilithpgkini@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741076658; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ilSMtdNdsliGxA9UF1qUw5o9oqOJ+G3vMgmcwIAvt+E=; b=S2gjqXd+h+OeVZJgBRHOWndcaIbnB6H9fNTAwo2XhFlqiqAbK9xWJfAUr22bL3jf4+Lhzz xchRNG1nYgFO3zkUC4bb7C1ZlwD1Du6QzNlYfNabYDbPy8Ys7WpOYxu7ueIatslqA40iy8 EztkScULE0YIrwmDqs9Eq5KN51m6ufo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741076658; a=rsa-sha256; cv=none; b=a7kU600LTOlqx/GtJuDOs+GDaCnlWttVQRC5lkaFaLyGxIuTyultZiE9AHOJOX+CzZAwh7 WRvcweEv4IE8A2pMM+tVZ53kP1gxLCpc4CAuRSHGyvP5Gk/BSdDscX0lf+lOCG65URXJ+B I5t4ljXtuQX2etjGVyyp23x1yxnQPzk= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=iANwU3Cn; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf25.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.218.48 as permitted sender) smtp.mailfrom=lilithpgkini@gmail.com Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-aaec111762bso978284066b.2 for ; Tue, 04 Mar 2025 00:24:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741076657; x=1741681457; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ilSMtdNdsliGxA9UF1qUw5o9oqOJ+G3vMgmcwIAvt+E=; b=iANwU3Cngcv8KdK3ekgj7Pm50wN4pLK6R+6aEdYl/dOTwJ5FRD4cry//lCcRocwcpk 29FUgGbAHOahOgpzWsYDk9USOsB9AqztkK57LgFABvb4tOELheyu0MZ9BxOXlqxKKbZf FgKhT2pnkFqkZdszfIJAvm25M6bFDcHcGoqzocXwxAV/04HbuPjuvPagkHJsRM0JUdcZ wzhSVrsu38/aOt6awf9qrEfX6X8Egz7HQYJUeWPg3e8tqCbBPrCpSqllEUmIh8837Lpy uMY0jeKycpc02dmh17mT/yWRhuKgd/Yl6OcNpnSTBIkQ4JaSolgKsmOLWDj1i61dgjhg 1KRw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741076657; x=1741681457; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ilSMtdNdsliGxA9UF1qUw5o9oqOJ+G3vMgmcwIAvt+E=; b=IUZCmqVVjotBLQv6ZWUIfJkZWT6CMMsISctOYRIAVKY2mZIuba4d17jxd2TZ0WhR7y LttRIqvY0xws7prZGPKfWj5BOJ3VHNDdP6p4/jO1bw3/JzkjfqHBWTQ7W+N8h9Q2zGwC RRUTxuki+eZc7L/7VLj8sBUaE1JWuyZ/5be83mUzzdYqnPKSpszJL4nImzYDtv/Ft+1W DPPQISb0e2IPnZAJBQY4kpSrPFbbxKD7bywkuf7I4/wiqR5ATVWMsa+8Rd3Qb1R7iCh7 5EII1YH5V4SJDTzcPMoPzuCl7mKmRbj7i14W2lWz+LRtHYNPYfxYnqoLf3JMNHHnPkjO eujA== X-Forwarded-Encrypted: i=1; AJvYcCXHnLZE1sgg/ZIl51sb/gHA0ckahQiyf1hq1Hkim/WzyjKj9/u6UvnW+loMWDI4dTWPB8gHpvUhqQ==@kvack.org X-Gm-Message-State: AOJu0YzHA0mNTx660H4OLfJkn90qUp4UHBdGt2XPlUfk5BPeGPHdkJj8 sBVpwm6w6Z4CRe/3KF/qHuoj/vlvra/qe/vs4GiycyLG1HgdfLts X-Gm-Gg: ASbGncvrFMajhmOm7i/DNRZQnA1NvBkn3ijNCpFmCPseDeSlRKyDAxyZCsci/6jR2io TRmZxBmMQbf/xzxHjcsfXq+Ith0UegrUvm4ppZrmzjasfPXt7lu4XHmE4hZRGIGlsBZHv3xUsWb uOLV+9uhbanuNNmFL/asB8z9YcsDXORqfAQYRYTDi//PtJ5vBRG9ZwOsR8JUX92XOO0VqFEoEc7 StQF83sPpulOd6YmyO91L4rsvvJARAURljA+WDsH08j9N+lF/d9llhmyIMRdi5ViBB6WVo81+VC yrNe7mc8iJPYMYY5MDRL/2xtlr0R+2ZBPlkssHw/VXABARJq X-Google-Smtp-Source: AGHT+IGAek1A6P3Ha+7ueQirgdhy2Ux6efWtUdnbiEC7nzfgGCERjiuz9cfsSyk1cpCIt1yMr4fXCw== X-Received: by 2002:a17:907:3545:b0:abf:44b1:22e4 with SMTP id a640c23a62f3a-abf44b12416mr1228115966b.11.1741076656579; Tue, 04 Mar 2025 00:24:16 -0800 (PST) Received: from localhost ([2a02:587:860d:d0f9:2a79:b9e6:e503:40e9]) by smtp.gmail.com with UTF8SMTPSA id a640c23a62f3a-abf3f3bbfb3sm675248866b.77.2025.03.04.00.24.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Mar 2025 00:24:16 -0800 (PST) Date: Tue, 4 Mar 2025 10:24:15 +0200 From: Lilith Gkini To: Vlastimil Babka Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, harry.yoo@oracle.com Subject: Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist() Message-ID: References: <8cabcf70-d887-471d-9277-ef29aca1216b@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8cabcf70-d887-471d-9277-ef29aca1216b@suse.cz> X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 8EEDEA0011 X-Stat-Signature: o6cgsqq9ii4s3158dsug4pgetewjox1b X-HE-Tag: 1741076658-565002 X-HE-Meta: U2FsdGVkX19XAiUjZieZcC/44zRHLY7CfYPEaOeApg9g+yh6NPdUHk4Sa/HoI1AlmSDvXymwRGGWPpy53EjOlIkNKmVZaGr/pQZRRjxaJkRhjRF5fxyQxPTNM/UsO/GBzrEnrGoEwORFVIhUSoDAVSJNTZ8+GI+jmg0wg1gR2I/hlH3RGqJMof+YZ4F6Ua6xG9r5D9uLmSxtwfA80SYz32GwNwKTSkhoLQyZnqlEmd80guGRRUQJMNbx/1+DbTA81LdsPj166D1npjtgHl0yNMcXJBu5hq6qdFb1pYYAwZugZAWKSvBZKPZ4NlcK+V6HfcrSVN3V4RKSSS4n/OGiEUcqVmBeXzeP066NWJVei4dkSAeV/lGxPMS36QbLZhwHVJDgFQAV7yGtcKrxe9KoPH90btKNwYUckaEpf3jjTl8sRpsaO5OhDSZ/Aex62CA+i4rvV7yiB8YX+aFZ/vIt7aduCDRdmDNIcdOd51T9zDfLoxzoUcyUxuuI2iyEPYGzoCHmYzp87Q4kqHfDWldHmOtu4C/KsQovd9c/CKgB4lwzTK4wY0RreRwN6jpkkO/mQf/LWT6xiqX8pECg3wDXZfaNPZnT6gDG6kMXuCJXWWAp+KLds+rRP8ZlJXTplgLwPeOCLJ6PrYM7Z88NYLie2PiIc8iZDkLYqJz04ftfVmxx5d2SB7DnJMU1EkbvVH5UghlHcAQBCpaEDjXAsLeiOAVzxF0E5bmysrGPVvs8Qwl12k6RFDglNvUSNwYLoFXT+iCXoKC9unwqRjwnb57XWPbJCT7SqQrCH/AUgh4eQgQh0x0SmOpu0vg0rQagIn0NV1CDLHBwvn1EtHjMGev9wi7zYO3FF0lgM3ebeV1uKlpT2T7swMwl26DaBED9nuHqg1YOVg4J5SGi42O16wjT1dY/zYV/S98Teti1vuwYARNiluvmZfUS5wM7GJmW0sVmKEhj9gchN0ylDe9tsBf hN2cBaW7 dYslSaPqfINbCkQKCM1gL83oHIIKMVDicAWv54dxmmzLm+w2r1yeqU7q5gqN/Z9hAnnXVhE1P4PmcDA7F9BpS3mEq4jtNsK7Y9slXHD1k+x3tojAT8gIzCWhJ61gIWiTtnRpA1XuEta56S2W8dhTYX+GBCMVh0NhXZwOSm3qTJAsd/6XLYXTBhD8JtE8jscnayUYTPkH1pn9I+0awdJC4jVqRfmOEFDAst6tS83fXe3/gIVrDSUywC9kDv1ACQRozXNbTJC+Td3ETbiXegMmDXkzX2YA5fK8Lw3FDMSEH8hXfXS8wHg3cyU7B3n24RqZZGPiuQw/H06Z3AnlsAkOhIam7qz3hGq3wGJO+Eb43fNZR92FS5omGHJsejwCK/pT41eRwNgn8V12uBtyxcvLqdgDU2SsqtjegJkVgUMAzmNbPcPTZDBgAx0WLDgMp/gX7f+7j8KGY6XIIZrS0wdrhZjnISYIRNni/vIvsw9K0gkRNnGE4UR29W8EnS1MWh/CHIJHgulk4/o3miG85YWm/DrL8+TRTqXCFOeaHjz7RTE6u3XRTa1cIFd8NzrNyh4ioRrcPPfdsHm4zJnVok4PSaswjOv9nKVGQG3OYJJuWefepaFXY6vcEQUjnBg== 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: List-Subscribe: List-Unsubscribe: On Mon, Mar 03, 2025 at 08:06:32PM +0100, Vlastimil Babka wrote: > On 3/3/25 17:41, Lilith Gkini wrote: > > On Mon, Mar 03, 2025 at 12:06:58PM +0100, Vlastimil Babka wrote: > >> On 3/2/25 19:01, Lilith Persefoni Gkini wrote: > >> > If the `search` pattern is not found in the freelist then the function > >> > should return `fp == search` where fp is the last freepointer from the > >> > while loop. > >> > > >> > If the caller of the function was searching for NULL and the freelist is > >> > valid it should return True (1), otherwise False (0). > >> > >> This suggests we should change the function return value to bool :) > >> > > > > Alright, If you want to be more technical it's > > `1 (true), otherwise 0 (false).` > > Its just easier to communicate with the true or false concepts, but in C > > we usually don't use bools cause its just 1s or 0s. > > Yeah, I think bools were not used initially int the kernel, but we're fine > with them now and changing a function for other reasons is a good > opportunity to modernize. There are some guidelines in > Documentation/process/coding-style.rst about this (paragraphs 16 and 17). > int is recommended if 0 means success and -EXXX for error, bool for simple > true/false which is the case here. Oh! because of the emote I thought you were being sarcastic that I didnt report it properly. Thank you for clarifying! That should be an easy fix! > >> I think there's a problem that none of this will fix or even report the > >> situation properly. Even worse we'll set slab->inuse to 0 and thus pretend > >> all objects are free. This goes contrary to the other places that respond to > >> slab corruption by setting all objects to used and trying not to touch the > >> slab again at all. > >> > >> So I think after the while loop we could determine there was a cycle if (nr > >> == slab->objects && fp != NULL), right? In that case we could perform the > >> same report and fix as in the "Freepointer corrupt" case? > > > > True! We could either add an if check after the while as you said to > > replicate the "Freepointer corrupt" behavior... > > Or... > > > > I hate to say it, or we could leave the while condition with the equal > > sign intact, as it was, and change that `if` check from > > `if (!check_valid_pointer(s, slab, fp)) {` > > to > > `if (!check_valid_pointer(s, slab, fp) || nr == slab->objects) {` > > You're right! > > > When it reaches nr == slab->objects and we are still in the while loop > > it means that fp != NULL and therefore the freelist is corrupted (note > > that nr starts from 0). > > > > This would add fewer lines of code and there won't be any repeating > > code. > > It will enter in the "Freechain corrupt" branch and set the tail of > > the freelist to NULL, inform us of the error and it won't get a chance > > to do the nr++ part, leaving nr == slab->objects in that particular > > case, because it breaks of the loop afterwards. > > > > But it will not Null-out the freelist and set inuse to objects like you > > suggested. If that is the desired behavior instead then we could do > > something like you suggested. > > We could change if (object) to if (object && nr != slab->objects) to force > it into the "Freepointer corrupt" variant which is better. But then the We could add a ternary operator in addition to you suggestion. Changing this: `slab_err(s, slab, "Freepointer corrupt");` to this (needs adjusting for the proper formating ofc...): `slab_err(s, slab, (nr == slab->objects) ? "Freelist cycle detected" : "Freepointer corrupt");` But this might be too much voodoo... > message should be also adjusted depending on nr... it should really report I m not sure what you have in mind about the adjusting the message on nr. Do we really need to report the nr in the error? Do we need to mention anything besides "Freelist cycle detected" like you mentioned? > "Freelist cycle detected", but that's adding too many conditions just to > reuse the cleanup code so maybe it's more readable to check that outside of > the while loop after all. If the ternary operator is too unreadable we could do something like you suggested ``` if (fp != NULL && nr == slab->objects) { slab_err(s, slab, "Freelist cycle detected"); slab->freelist = NULL; slab->inuse = slab->objects; slab_fix(s, "Freelist cleared"); return false; } ``` What more would you like to add in the error message? In a previous email you mentioned this > >> I think there's a problem that none of this will fix or even report the > >> situation properly. Even worse we'll set slab->inuse to 0 and thus pretend > >> all objects are free. This goes contrary to the other places that respond to > >> slab corruption by setting all objects to used and trying not to touch the > >> slab again at all. If nuking it is how we should hangle corrupted freelists shouldn't we also do the same in the "Freechain corrupt" branch? Otherwise it wouldn't be consistent. Instead the code now just sets the tail to NULL. In that case we'll need to do a lot more rewriting, but it might help out with avoiding the reuse of cleanup code.