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 E393DC021B8 for ; Tue, 4 Mar 2025 12:18:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7D03A6B0085; Tue, 4 Mar 2025 07:18:12 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 759556B0088; Tue, 4 Mar 2025 07:18:12 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5D2616B0089; Tue, 4 Mar 2025 07:18:12 -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 3B7876B0085 for ; Tue, 4 Mar 2025 07:18:12 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id DD89DA85F1 for ; Tue, 4 Mar 2025 12:18:11 +0000 (UTC) X-FDA: 83183770782.14.D887F54 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) by imf01.hostedemail.com (Postfix) with ESMTP id DDD9240011 for ; Tue, 4 Mar 2025 12:18:09 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=UO0HMctn; spf=pass (imf01.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.208.44 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=1741090690; 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=GU6MTFhK6BSYVwInxWpb8MmgiJb1y2ld5zAqpXaXvFo=; b=2/OKU3Shbn5pNu+Vyu7VBUCPn1frGGnLjyctGpF0kCRQ8avOwVanCJ4KiOcJuoJcmS0/9d qu/GgKTzLTMYr+2pyN3/qpY9uYVnuzUOc8xL81xP81jlaBVVdtdDSYlv+7+hRGbU14YrrM BSBvP1OkD8fmFINxuIloPR1g9jzP9m8= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=UO0HMctn; spf=pass (imf01.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.208.44 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=1741090690; a=rsa-sha256; cv=none; b=3eDKQQ58yy3zDAp6NYHD0jj93vDoAQMRBNTJDQMLduLB8RB6LbTyftuIfF8C8ZY0xYTjql AfdaKUIiyamRmWDCtsZFnlIUNVXT9V4oVWRxGn6gwrDItWHSxMZQ++FgKe5RGmpbJJAr3B oFO0eYFPwa0aYrtdWIVSls8uahtGOlg= Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-5e56b229d60so3014344a12.0 for ; Tue, 04 Mar 2025 04:18:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741090688; x=1741695488; 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=GU6MTFhK6BSYVwInxWpb8MmgiJb1y2ld5zAqpXaXvFo=; b=UO0HMctn8F1Mr7GpYuZ43ir/TYrzUnJ3OMOp5EHUOFM+CpYcPqMePqUUIKA8VceHyh KbMI7LIp7cZOtzVB+OPEwnXi06SYVSoLjqjc9GbMlTIJziNOSnijY87X+ubsUZpyyzFX lhqyjv7JBrirGoaadr1DxiMbT6eKTnUEnFRYceSoCvHB95SWtFY13X+mxEW0YWycxfWi 30eVgZrkstMym4b6Yb+3LRLFeqxeJY4XNzpNzZBjCa9oGwFc8wqkteW1QwnH7Q4mEOvB W4QnAt12Nc6mtMDfYQTuohawSarsCLIGe1R3vragkIcvYn5ODORjcnXv7G9PwgPkuGy5 KSig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741090688; x=1741695488; 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=GU6MTFhK6BSYVwInxWpb8MmgiJb1y2ld5zAqpXaXvFo=; b=pTGcYNI5vOR0cSpVY18zOIWZIG8MH3yWUHlfjOfiaQkdUAcGivcXFOm6SyyEWMuRlz 6Ywi/fXXoISDD7mgJiXrbHnjYM08EvX8SiPz+DcvzpUijyuX4txEyupH6q3vq53F7iZw Y0jBJOUpNzvmGlLWz0Fog5VRzVdE9tJgB49bH6DvviWTHNs8B6EfDplUQhoAHbHagR43 YvqKh8JNWhtLoAgjHXDgAWhovan0IyKaFRu0G42WAuzR58S+L5ZWdTth5WBcVCZRCwXL XoQeG1jmydTFfSGWK/jk3mkxJ0Npv/LkRxZ78nljXm5tqHHmmjN9AhoQItj8NvTfpzum TYQg== X-Forwarded-Encrypted: i=1; AJvYcCVD4EEoXSP1qxuPwKtrARIwm2ZFgghY0FDJFxniv/233o8zd/8qMb7Bb5YXhgPYvzXA0JVu7fmyjQ==@kvack.org X-Gm-Message-State: AOJu0YzfJyGm/GNh25eGFw75wMJO33lg9n6jUGbB0lZ7plrLf2ECowsy +/buslANfqx3bJL07GK8dekHmnyBy/blvKhCJyVHLKiNW/07uEkD X-Gm-Gg: ASbGncs4gxNFWWqJL+jTRqdtv6nqWloI2Lup4bfZa8rK8YeBi7hnCzVNSwD7SO8RNWm EYBpE32ujpAcKlO61qe+CVWbBDnt/0RqZJ0+4gE1SWUbLcgucpevyzj5mzywPcSU1/3dIDOWbeh NHAG8AtPPsPnKaQm2AWVIgWi1Hgfzjc+dXg2FvFbEtns6X0Hmj980prfQwHAFjfPoe0TgcS5GPP 5tlykQwFjuiR/+f1K3vdgQJMutfpq6dWkiVY2bBe6JgWWdQxp82A3DDwHHBB97fxgulS8sXi8Jk PbzqfnNeJLAaAjLaiHg+WOeLrplOOQKLkbk8Z7tScAoyq/Rg X-Google-Smtp-Source: AGHT+IH2dUnEJRh/KYalyuG0AzKTquAC8pQCaaFSVR9Wv0ldxlyVTaJtY0Ad1L902z6mMNPPL9MFMQ== X-Received: by 2002:a17:906:f59b:b0:ab2:f6e5:3f1 with SMTP id a640c23a62f3a-ac1f101934bmr328042866b.8.1741090687847; Tue, 04 Mar 2025 04:18:07 -0800 (PST) Received: from localhost ([2a02:587:860d:d0f9:2a79:b9e6:e503:40e9]) by smtp.gmail.com with UTF8SMTPSA id a640c23a62f3a-abf439db928sm684864366b.64.2025.03.04.04.18.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Mar 2025 04:18:07 -0800 (PST) Date: Tue, 4 Mar 2025 14:18:06 +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: X-Rspam-User: X-Stat-Signature: rsejnezc3yi31oesme5ibz584q7eidpt X-Rspamd-Queue-Id: DDD9240011 X-Rspamd-Server: rspam07 X-HE-Tag: 1741090689-593053 X-HE-Meta: U2FsdGVkX19s8jpithV9511xNHKIEPegtLVywoUoT+41hM7ovhSD0LbAZx07vl5w0Ra5ScNsxvSpE1pf7B0koCFnHSrWHlIuR9AlklBhKFqaaGi8wEwbxp3q3I8W9BdwEDPril8BE+GwaaD96ihNFBoHtogIR1XghkCWBpx3142cI5iC3Tey+1W0g7wtmoFdM6XKmlf3DnC8XPX3nfi/37C68CBFspAG3pHc2quE8zZM/CtxH+d0B5CWqshKOcJ6acmMqu68iHApYjgNQTcJsYKoVdWJv2chFw535vNPGBY1+qlM1diJhQTBIxM4ZSQvYiuxCkYS6sjeXNi0GIq9Q5qobUIxQVxY+2l+MRanFCbj2H/Z0W3P94v/Pv6uBhBb2CrFoKmVi1JUPOBHPIHBMGyAQHXlM/88BoNUFloBVLaBi8ilAMO9mKwmVCMqnVfBmsSRYnAa6Df5qrr1ksqQorSDDIiaAEpQBPFE/hjQpsKRAbJq38Y36tgnq7M2jDdoyWZgJYlQ7mVLZND1zYbOczmq9c0sMHXi8iEouCYsbdxYexhWghTbq2RGcO6H8QESnsWN+8D8M7JXKB5NYtsI82RJaXcoyzEVRBJteZAdZu/skB9DVVKeX2W1GgcUdOkMxjr85GeDV6Vpr63KWPLUm2tKIuM40axQbKSELAU7xaTrmtKaN9eWONARbASh+3VU0ujcieRK0mniDt76ctwBNCQd4Ta0+/grSA/ta8qBFwnEFh6+OyHwLaPXi3Q4ByeS7ipwtF44wAyVkhkygyCmgU5OJNbESHwMn8ohdix5YER4elie6fSsqfyaruGvtt21rj+V3BxZpT/6G9SaH+ziE7Tb70XG+0xAdUFPh8Gdz8VNeI/B3rai+vtE7PTfx4vQwLskFq2RBDmk9pZdIZMmo7z4Np03vQ16GJB9C9qrnCPK3AsGRhAiz/fRZN7qs0iIk+fL1E6UAiUmvnzKoR0 3Que4c2k yK4iZxom5gV5+RBvL4lAPSPLLD3ujPcCWO3ZaEdKXU9D+lFIsUz3k85iZNfTjArXxsoV+OGRaYz0Qu/USdXvEUXd0sfj5MGnRi4H/ERYLKE5PRx+xrxeNbGxIEZO8/Yaks2nC0aAoE2HOiOqn4kOja+m5oVW350fD2k+mQfxPVBPCPyHk6iYYw3+56nflG7oKiZVskU6pDeMSe2dXbcDuHScEV9DuOkgGOnbwHjmOPfo1AMuJUiLBs38W2z4FYkOIPFR7Ee3nFDJuXckuLDoOxbraaR9miIHkRJrX9nuXu5TnIFxvuCoEeQw1qzlyj+eq9Nju87r/Oph7Rbl64q2aPHnjMfeTHDKM96DxnRuURIEVeyuFA3lczfNqDQMq+djEtjY3ZfOwUC8hUK8ZuXLhi9ic/hrMJFObxMw0msxA8wdNiLJhnF6yrkZLOSPcsN+fvDvzQ1D62665xCJTsORMRZbPZd1ahGr7Hk4kJPf4RTP3sWlhLcnLiULjnqtSTZj0qV6IPNkY0yxmO4f0VQz1fT5varyxDFxhuALSvEfM/Iy3Fr+1qXKC6QOawKj/fog97hQZ/FkXleGmT6rW8glw9RvY9WRguEfNGFE5F7qfhpgQLBCkI2YAp1Fc7A== 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 12:20:03PM +0100, Vlastimil Babka wrote: > On 3/4/25 12:06, Lilith Gkini wrote: > > On Tue, Mar 04, 2025 at 09:41:23AM +0100, Vlastimil Babka wrote: > > -- > > > > 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; > > Since we break, nr is not incremented to slab->objects + 1. > > > } 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) { > > And thus we should not need to set fp to NULL and test it here? Am I missing > something? Thats true. I still had the return fp == search; in my mind, but with all these changes we can just leave it as return search == NULL; as it was, because we are handing the edge cases. By the time it reaches that return line it should be fine. I was also thinking of fixing two lines to adhere to the "Breaking long lines and strings" (2) from the coding-style. --- mm/slub.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index 1f50129dcfb3..e06b88137705 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,38 +1437,48 @@ 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); + 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; if (slab->objects != max_objects) { - slab_err(s, slab, "Wrong number of objects. Found %d but should be %d", + slab_err(s, slab, + "Wrong number of objects. Found %d but should be %d", slab->objects, max_objects); slab->objects = max_objects; slab_fix(s, "Number of objects adjusted"); } if (slab->inuse != slab->objects - nr) { - slab_err(s, slab, "Wrong object count. Counter is %d but counted were %d", + slab_err(s, slab, + "Wrong object count. Counter is %d but counted were %d", slab->inuse, slab->objects - nr); slab->inuse = slab->objects - nr; slab_fix(s, "Object count adjusted"); -- I do have to note that the last slab_err is of length 81 with my change, but it looks fine. If that one extra character is unacceptable let me know so I can change it to something else. Or if you think it's completely unnecessary I could leave it as it was in the first place. I just thought since we are trying to modernaze I should fix the length as well. Also the CHECKPATCH is complaining about the `fp != NULL` that we can just check fp on it's own, which is technically true, but wouldn't make readability worse? I think its better as it's in my diff cause it's more obvious, but if you prefer the singular fp I can change it. If these changes are acceptable and we don't have anything further to change or add I can send it as a proper commit again, But I should probably break it into multiple patches. Maybe one patch for the lines and another for the rest? Or should I break the bool change in it's own patch?