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 C1FCBC021B8 for ; Tue, 4 Mar 2025 11:06:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 553A36B0083; Tue, 4 Mar 2025 06:06:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 503756B008C; Tue, 4 Mar 2025 06:06:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3A3C76B0092; Tue, 4 Mar 2025 06:06:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 17E276B0083 for ; Tue, 4 Mar 2025 06:06:11 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 9841E1A1B8E for ; Tue, 4 Mar 2025 11:06:10 +0000 (UTC) X-FDA: 83183589300.24.DB7CE74 Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by imf06.hostedemail.com (Postfix) with ESMTP id 4A928180002 for ; Tue, 4 Mar 2025 11:06:08 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BpLJ9Ntw; spf=pass (imf06.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=lilithpgkini@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741086368; 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=yxvndgwv4ifV3uRHMIkq4wSTyXoMRUAEuK9dHeEk0yg=; b=Zwiodr5MlACvBsnS/Q78S5ejH8M1pLfbE90j7Qcg92MxjjhWwRwjE+OfkO2LvMRyCPLHJH vCkTXw3QkTfODWIlRIi+9n7557p1lQHb1dAxQb8RXPwTDDJiXxjl90EGqa5krfpS0NIROg HD6oSW3xgdLWlkL3SAEXxFdMsWvJmRE= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BpLJ9Ntw; spf=pass (imf06.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=lilithpgkini@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741086368; a=rsa-sha256; cv=none; b=Gfg2z0hG2IxctsUtGr1bIoQz4vrhhpk3GCFJh8VzGYk2L1GEmPmBZjfkqBYEFT+jGZQpFR XPUI1Yu9ScKzPP+wXNHml8PZSZqviMdNlrKLCUobsygy7BUxVmWHKrHC6AsGVlAZ3B1nCc ujsg8JN3zYdxujeQuNZhB3731Tbb5JM= Received: by mail-ed1-f50.google.com with SMTP id 4fb4d7f45d1cf-5e0573a84fcso7508288a12.2 for ; Tue, 04 Mar 2025 03:06:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741086366; x=1741691166; 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=yxvndgwv4ifV3uRHMIkq4wSTyXoMRUAEuK9dHeEk0yg=; b=BpLJ9NtwimIDU2Jkk9oWgfbNzmhTArF8yHakCo8PEL2sRT2X7+diMrCCqCE3h9968S XR+JRpByhlbc5p3jkuQRgWyOUB/Y4e8TLeuiWi46QDEuCIffoP+ZjTfjtuoLpdF+mvtF m/wmidPhTIani0UdEty5nkyTiI0/q5uNp8YRsOkASrNb9V+5sab68RFiD9xMI1xEVIPo Rlwf4Z6ccMbafi04CmT9qnmMhZB5RDQmE1X1jIFan0N902sC/7tRPm9rqyOf62uo3ryH 34aWX3Q53RxWt0j+tph84+950szgSWikUFHBnM1BTy4i71Hen0z1dUQzpPxYnwDTMMd5 fS+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741086366; x=1741691166; 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=yxvndgwv4ifV3uRHMIkq4wSTyXoMRUAEuK9dHeEk0yg=; b=aBxZuzCvtuVt6I8+IjJfFVC2E3tBF+iLO2lBd4cjQZyfB3jsUosHUKR1adfPZxX8yF pftYtgac1yH4FK8CHzixNRjnoQ1Dc8NzhIEjR6w3+je8CxZHQw6YYTriJOTVA4Utz3tF cj52tlWBXEA/GhXyCxWNihYiF1z/oxMrEeyzjE35BRM034JZEUXyBgE2fEyXbTV/f8+3 +ZhJMHkCKGbfWaSbLulHJsfU1s7su7qKzmzkbstehnoyIrdokMWmRKSChWtx9GnLnnEd 8pLTosxqAV364R9BBQkK1q7ZF188iPLfh1wzNxOEYyQ4Lmba+8Wae/5dkwA3QsRtmaCF h4yg== X-Forwarded-Encrypted: i=1; AJvYcCWSDYXleCra0N3PZfokX6AReaonZ+ryAqJSSOUTk5RO/vI/myDL4LFYwPaCnLBeriBY9MZj+HrE0g==@kvack.org X-Gm-Message-State: AOJu0Yz+DfGjsys2wqKcOnTTcDABjVDkWQN13hx9tf3YTkCl3WJ2Trrm E2xbxZY8uHxdHBX1K8srRBE2KuHaeHoywGd1fV7/Ptry8KMVvbHE X-Gm-Gg: ASbGnctni/yLEBaH59ITUB5W9BV6A8AUBSIvuV7EGzRHFVw/GtyhdN5EQjzhVNaADre v64fr4DTj7F09TH39w1NauURVMr2DU33J18a0w4G8+XQsDx2MZlLIlz7xvQHATjko3vK0+HMylV MZuEmfr/4+u02MOmLrf88wW0VgyoKn7EWFHdOT8CHLvOqMS3Wwf+cPCBGJmrMQH6yor6R4THOYi eb7nlRXYPhiKflxo7E+itSSrO42JNK1gN3Wd9S1mgl0A+hlXZUOGVlLh6CxhdVAKZ+SeccxAvyt KKAhFxeXGU4yRDCHfUpiiel7nLJ76MrMigpC/m/dRLFiEo5P X-Google-Smtp-Source: AGHT+IHKQCxTrixvTJRNOXl1k412tIdlU/Fx3nbL1YBSRrSigkz1sJpeABuqT2KoL02ZS9AZ0iyq1A== X-Received: by 2002:a05:6402:210a:b0:5e0:69b3:441c with SMTP id 4fb4d7f45d1cf-5e4d6b6906fmr16381942a12.26.1741086366196; Tue, 04 Mar 2025 03:06:06 -0800 (PST) Received: from localhost ([2a02:587:860d:d0f9:2a79:b9e6:e503:40e9]) by smtp.gmail.com with UTF8SMTPSA id 4fb4d7f45d1cf-5e4c3b6cfd0sm8055619a12.28.2025.03.04.03.06.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Mar 2025 03:06:05 -0800 (PST) Date: Tue, 4 Mar 2025 13:06:04 +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> <714d353a-49c8-4cbd-88d6-e24ae8f78aaa@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <714d353a-49c8-4cbd-88d6-e24ae8f78aaa@suse.cz> X-Rspam-User: X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 4A928180002 X-Stat-Signature: brsused3s1w41sxwqf54s16n664ecqin X-HE-Tag: 1741086367-432769 X-HE-Meta: U2FsdGVkX1+72NdOlVcBB23jXjb709u/jKU33sDsbsqdx4GTUOiBGVz1dXObfOInUfUXIlJSqWJIkJLL7Q4tFOBDprRofOSyW8DAwQhyfQ1sGVRyho7rbkflIN9x+aS2I/0hAvdNZu17ZlJtN5LNy86s6LjkZma9SUWXpibiMJXYwrp0tskgx0OtdGBDa7C+oJtV3h2s9PAwIBSfZynyk9ofn+nDNWP3Y/pucZ1zQcPKzQdgLJdLMd0zzrIvpk7OaIgDR58GG9gM54HaoMa3uOGN+hJRFtGoi2mCy3BcMnfT6pPBcPfYqbx75GCSGnugLfgU8YPtFuZ0axXL1zmhFva9Kb7GkdrLMLefbbSdV+Lh6fXME3XRFR5Ba28yD55lWxbc50/cDU2PDRrVmh/yKM6t2QDCP5iBtm+2txJKztoFZiPjW57ZWvtLQbVsW0Y8Exo1N7prNQWKxFFDp6DBd7qTWqNBXpM33vF34PsBKJKeZxACKMFbJJ5tJIAEj0p3OBR983/LdBG1v1mtVGrkHDesoxmc+jShALzQfLpf7a6FAaj0EirI7nrxI6xhBsoxAEd2aMemxVyK2X4rVuz95Np/Ger9LOC1HrH97vpVuFt9yGI9+xAhd8JGqs70NcUUf8LjuYy2Q2i9wr2XSei6jz/w5GLsXwfEVLTciatRUlVAbvDZQfXa30F1Fyh8gnL8w5psIVVsIh++lLL7JpWtEbNCO9JNnqgJwjVCAhBoF8F/s4H+Du0Uqhde6OPMLCC/1thYNCD00demt6eg1y1tVlYQqSeaOqor0/xThxSFLFyAn2hx68bEpLVyO2upDcK1v6SnBYt/6BIrQt2BoTnuc0HvBP0TgpoHGlZ46T3QDMvPjDPitZg525khvCv4Et7rAdzsn+FPlB6gDiFYu8ENJ7MTXSRvkL7bagyWuvBNSC9CzmOkei9cFUK6z0qGzDT8cc3SblnbyyLIILlcl0a Z/vvNL89 8fYGRzlrh1jWRMXMObOrLNQ3rc2L5lew2hTbliVq8SpQ4/3QyRwmPil68bMZgpdtz9AY667o/FL73Q6ITkRkn2jJ5urpFr+3caDMtHXiC1wv951N9opyrX25XZamD1Y0xv1dvmDr1RRwRwdEgdmlEZFlFneardnMlmd3GqQ4BW3FO8UNjXKu/xhuTjYWrhiXErHKmqnTSIIiIz1hyLL4zBGSpZXEbWnhW6yqb97icg17oANtrdhnT4fEttLClNJ7sJrXfMKXwM8eOCM1hBHQvvXlDvlF/cT3PCiCzDWBsv1TO8ayEaP0UxNXcEvsj+FarYMkxSD/OVVJ549b3KMmG4AFyIUZp0zIofT6CsAcnfFlxM6amVERkTC2qdmW82Mmorzorg1lpsO5FIYgtt0uSsDpUCYzb5GXEpOEwecOe4ojiqlztHggaOd5APNK5Qrhz1RGzEhRHHBiZ64+936CJuoJIpRUC8nQNt7w0EyP+3SLU6ziDka/2F9DNRQ1nggYKa9rWxUsOjJYHM5SEdnmWJy3ly83AXby8KD+lwMcgowAfU5p9l1iBbrszwoOlPU3GhCB4NX5Yf7BWrfgb5HOeqMq5H8pKn5e2Ss0REBuyi1CQnU9Iz/eboilXtA== 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 Tue, Mar 04, 2025 at 09:41:23AM +0100, Vlastimil Babka wrote: > On 3/4/25 09:24, Lilith Gkini wrote: > > On Mon, Mar 03, 2025 at 08:06:32PM +0100, Vlastimil Babka wrote: > >> 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. > > Ah, sorry about that misunderstanding. > > > Thank you for clarifying! That should be an easy fix! > > Great! > > >> > 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... > > Yeah it means 3 places where we check (nr == slab->objects) so it's not very > readable. > > >> 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? > > I meant just the Freelist cycle detected" message. As "nr" equals > slab->objects it's not so interesting. > > >> "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; > > } > > ``` > > Yeah looks good. > > > > 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. > > It sets the tail to NULL but then also breaks out of the loop (btw that > break; could be moved to the if (object) branch to make it more obvious) to > the code below, which should also set slab->inuse properly. So the result > should be consistent? In that case we're able to salvage at least the > uncorrupted part of the freelist. It's likely corrupted by a use-after-free > of a single object overwriting the freepointer. Yes! You are right! I also just tested this. The "Freelist cycle detected" will get triggered even if there is an invalid address at the tail in the case of a full freelist, which is a bit... inacurate, right? It's technically not a cycle in that case since the freepointer is invalid and it doesn't point back to the slab. - We could avoid this by nulling the fp in that case (as I suggested in v1 in previous emails) inside the "Freechain corrupt" branch, but also reverting the while condition back to it's equal sign like it was and then changing the new if check to: if (fp != NULL && nr > slab->objects) { but it feels a bit messy. - Or we could just change the "Freelist cycle detected" message to something else. - Or we could leave it as "Freelist cycle detected". This is only a problem if the freelist is full and the tail is junk. If the freelist is not full the code will act as you suggested. If this is becoming too hard to follow I'll include the two diffs. For the case were we are fine with the "Freelist cycle detected" message, even in the case of a junk tail: --- mm/slub.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index 1f50129dcfb3..25e972a3b914 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1427,7 +1427,7 @@ static int check_slab(struct kmem_cache *s, struct slab *slab) * Determine if a certain object in a slab is on the freelist. Must hold the * slab lock to guarantee that the chains are in a consistent state. */ -static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search) +static bool on_freelist(struct kmem_cache *s, struct slab *slab, void *search) { int nr = 0; void *fp; @@ -1435,29 +1435,37 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search) int max_objects; fp = slab->freelist; - while (fp && nr <= slab->objects) { + while (fp && nr < slab->objects) { if (fp == search) - return 1; + return true; if (!check_valid_pointer(s, slab, fp)) { if (object) { object_err(s, slab, object, "Freechain corrupt"); set_freepointer(s, object, NULL); + break; } else { slab_err(s, slab, "Freepointer corrupt"); slab->freelist = NULL; slab->inuse = slab->objects; slab_fix(s, "Freelist cleared"); - return 0; + return false; } - break; } object = fp; fp = get_freepointer(s, object); nr++; } - max_objects = order_objects(slab_order(slab), s->size); + 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; + } + + max_objects = order_objects(slab_or0der(slab), s->size); if (max_objects > MAX_OBJS_PER_PAGE) max_objects = MAX_OBJS_PER_PAGE; -- and in the case where we want the code to not display "Freelist cycle detected" we could do something like this: --- mm/slub.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index 1f50129dcfb3..eef879d4feb1 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1427,7 +1427,7 @@ static int check_slab(struct kmem_cache *s, struct slab *slab) * Determine if a certain object in a slab is on the freelist. Must hold the * slab lock to guarantee that the chains are in a consistent state. */ -static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search) +static bool on_freelist(struct kmem_cache *s, struct slab *slab, void *search) { int nr = 0; void *fp; @@ -1437,27 +1437,36 @@ static int on_freelist(struct kmem_cache *s, struct slab *slab, void *search) fp = slab->freelist; while (fp && nr <= slab->objects) { if (fp == search) - return 1; + return true; if (!check_valid_pointer(s, slab, fp)) { if (object) { object_err(s, slab, object, "Freechain corrupt"); set_freepointer(s, object, NULL); + fp = NULL; + break; } else { slab_err(s, slab, "Freepointer corrupt"); slab->freelist = NULL; slab->inuse = slab->objects; slab_fix(s, "Freelist cleared"); - return 0; + return false; } - break; } object = fp; fp = get_freepointer(s, object); nr++; } - max_objects = order_objects(slab_order(slab), s->size); + 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; + } + + max_objects = order_objects(slab_or0der(slab), s->size); if (max_objects > MAX_OBJS_PER_PAGE) max_objects = MAX_OBJS_PER_PAGE; -- Let me know what you think!