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 EB24AC021A4 for ; Mon, 24 Feb 2025 12:12:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7092C6B0085; Mon, 24 Feb 2025 07:12:49 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6B9616B0088; Mon, 24 Feb 2025 07:12:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 580AF6B0089; Mon, 24 Feb 2025 07:12:49 -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 3DC6D6B0085 for ; Mon, 24 Feb 2025 07:12:49 -0500 (EST) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id BA4B0A35BC for ; Mon, 24 Feb 2025 12:12:19 +0000 (UTC) X-FDA: 83154725598.29.17E4C68 Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) by imf07.hostedemail.com (Postfix) with ESMTP id CAF5640012 for ; Mon, 24 Feb 2025 12:12:17 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=D4YzKyB3; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf07.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.218.54 as permitted sender) smtp.mailfrom=lilithpgkini@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740399137; a=rsa-sha256; cv=none; b=x/0HTHU/RqCbkvt/E9DEWCW5il3RVWvUm+rQ7x10k1KMt2Pw1LxkL4RcQtdR6wDYBp9/Ge Ng5cUy8sytGs/DFLrvBnajNIyADDD1keZMPHGcAnLL6rU4VEbelmvH2KCZZj7L65oYSw5i R5zSpOkcFa4rTWNU07qhF08CuKVMM6U= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=D4YzKyB3; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf07.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.218.54 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=1740399137; 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=Rrddu9WJI4Zglua4XbLNnvYF1mA4HBiQV2N19UniiUI=; b=l1k9qbn3R4yjanaRFs/MyNNiNYeRzYNRcvbIlQIQRsO4T865g5eJnKiu/+TyIKf+6spjPY NX5uRT+Wy0t9+w1kgcO50JVcPPkYmaKUnQSn9bJ5aOmHNOwOpnJ/JEppLQUPFnstoCzFoG SXkLnw5EuXN411lQwlVpYchvRuwZI9I= Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-aaf900cc7fbso664773166b.3 for ; Mon, 24 Feb 2025 04:12:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740399136; x=1741003936; 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=Rrddu9WJI4Zglua4XbLNnvYF1mA4HBiQV2N19UniiUI=; b=D4YzKyB39SjQxk9fZRSCtdAfcx5k4ddgaOTiZQFbCp+HmMcSSeajZJgZt4Pn4ySdm3 y5H0TWFbQGmNtoegCS0JKdd96063tDHO7mZwguVuvL6c5qTiphCwnTSTjNPOHsCRFbAO HAv2tXdh9V2+PAEnsIgKXzv7gkWZFkqlM5nadKzW5aerhikavV1PzTN3kJBnRIYfmKIx fKDEq5u1vBxA5DaPbBQApWzJMpsPrkl+hFVjSSRH45GRHErjMxyr0W9Z7in/1SVYKrD3 HDHmTPcubwI+X8feHrOloGd28FJKhrT48AhaKSwf/CQG27jyZnSDuYmJz0lhMjDgpWQp rL0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740399136; x=1741003936; 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=Rrddu9WJI4Zglua4XbLNnvYF1mA4HBiQV2N19UniiUI=; b=E14vl55rOQny0vO/MqAPVNoq97LF0B8aMoUA9xoG2IUyidBOZ0Ei5okrBzLEZWPvlb Bn+cUmVGLTwlYFuy+Ffa0cma/D8yZHUmcCWWmHRrkZbHwTTaw3mLLduoDZttdqPbdQZD N8Mhug211CdTomsSCzfUY2SkTAeTldmgnyFPFXwnP2lifuThjQqmzSs+jOHw1l597T1l ALZGfiOrJt5/d1chOtJhrJg4PA98OORaSR4UPoshrOweP/VDrq0JOjN5jHpxB5+xzQEs 8i43QqP0qtoPilClAMjiv2SntEEg30rFoq7EMr8WrbsbFuj2zTuGOTL0cQNajGYeJL8M m9xA== X-Forwarded-Encrypted: i=1; AJvYcCV/CKUDeO/0/gKCiE1ZeOwEaSeBNdv4dMDF8AItC2FnEi0BxTJRvpEtYoso8XgNSHCjLBVBp+Bdrw==@kvack.org X-Gm-Message-State: AOJu0YxdLVkMhq40lAk+yPWNgoC4KaT51Fd8XYAJAdrliPSCfnurlurF p4OYzM6095Sa4lYmHYBbhwtvwtvTZbwr+3NpUdqQWox1gs4gnmmC X-Gm-Gg: ASbGncsxhD9JfKGb8BNqT92yCaxZffx9UQbb1Oy+mMZ93zaZZxaQ87YZh9jzNekYO4I d4vEiallx2K2ENtIrndJh6LM1qLlLBx/f5xZuFuyeDLxrj+9OV+ZBYzr0bGGSdRgJnW8WHyQJQf CfJo/BkuI0qcPqTinUaOk6LWKewNKO6Ww5OLGHT+AV33qU3qiB1ClhANcfMkFAzdwHa/i8m7hdD Egq1OJn6tgU86l0XiJcJp+sN6MCDAs7t6EvYSEb50UELejZGrc/pIfw1jGQHllzXT+f/aBO9WWo DYlxcwkBoJv2DOXAIw3W4CZFaVL/ X-Google-Smtp-Source: AGHT+IHv1Tnoo5T4imaW7W/oB5X5aEzmHsKmzB619oTOb3oJ0n/kqClK7OlPUzdKUAwCKjy698Zj0g== X-Received: by 2002:a17:907:3f16:b0:aac:619:e914 with SMTP id a640c23a62f3a-abc0d9d9740mr1312556966b.16.1740399135764; Mon, 24 Feb 2025 04:12:15 -0800 (PST) Received: from localhost ([2a02:587:860d:d0f9:2a79:b9e6:e503:40e9]) by smtp.gmail.com with UTF8SMTPSA id a640c23a62f3a-abb843451b1sm1722365666b.42.2025.02.24.04.12.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Feb 2025 04:12:15 -0800 (PST) Date: Mon, 24 Feb 2025 14:12:13 +0200 From: Lilith Gkini To: Harry Yoo Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Vlastimil Babka , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] slub: Fix Off-By-One in the While condition in on_freelist() Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: CAF5640012 X-Stat-Signature: iemuog1qkttygucxiu833jpdwece39uj X-Rspam-User: X-HE-Tag: 1740399137-946844 X-HE-Meta: U2FsdGVkX191/bTWehUdxknArT0DEwLJUfZUdqqAkM7Wb3SAdPiYYqlkTPSDel58Rw8OUihCdaEVcHQ0EpIRK27oqAUQMWu0Jnik1Z5GR2lAcM62yRUYbPMp9nQ6NITN+FUdmiufDDkuSZiy2r1BCqXdI/AHNcalQ8WmNPi5AKGZoFsyHHhTG2yVjB5fu79FcqVTzNrD28VMzJtXQdjt6xHI6GJUa686ne4UE/WgNetu/BybrHvENRvMkMY6yybyTSDOAMYcS0/5fVS0CIi25a1iLCDlqv19Huff6rfz4tyn88s6bG3C4rgg8hrL2ppybWdlXiD4K1KyyE6FaQeTPeZJCDSZDJP96QkxYDrkUecHTsTUlkh+S1jPf+AuOLOWZ++Hvq3FdDJAsVTFDP8K1bmoe9Gq6TUvuVatny/A3WFgUiLx7boyLsIStyLUDdTZWu+o2FnAigqGIEA2L31VW2ecxn2TDEpi+025M7iFHMG6+VQZliq8oHbgyTnxFEzbqo6+KxZr1towJ+Z7/AVuI/9L/WTVx0GAve3G2mXavtLu7DT3sFu/vaplSlplp1/zUSfc4iQ8mFODEhkr7VFfY7yOjUhifxJlewGLMEfef1g3/gffaO0wAxkNS4vbBJt4D6eTrN0NZ/z7Zq/KpqjrsaHlo6hJ2+P/xsCu29a49GXquBmiQEm2NfqSVm4IZ8bK11/LLrRWsfIuC62W2qOlk/WkMTZy1ngD84y4Z9cbeiMHTizXL2fRXgj38zBlrW8Ol48s7uF9nHSKktPJoHLnGeJyu+69c37sq8ZxC2u5oXVqiYGlgWcDp22paK5QRW6sLYkZHPC9t4BpHk6OfodUjjPxcx/MSC+qJ5vCvaNTjmde0RC4mNBvdMxSAzMcdZi0cqdr65fh84BW2+ZALXfqn/l7LDh9u5aDZNi1DBiWAfqjHYmVWRDqL5HFNAET2JNePZOQIYJbX4CctX2kDq+ mh+xFGrR HIwfx7xZNhAVMh78DjAoe5gMeXb5HW4BP/lJANE2hk2c7L3XEDIY5Bx9pbDvGV1RPf6kMhA0cMvERiZGD+ZTOK5kI3SN1qFZt4TU3C8xyWz8iugypFuoQXqJertdto9u7Nmt2UGdGfwTUnR6cdwZGM1I4VOOwcvu8fiPkjDwDS1faF1abVDpSxmHU3RDTPiVyauS3WKZ5U6+OUVi8Hjf3D5f8OPtqidR2Rep1JFvURCnKVH3uGt5iGwgkPKzyFKxOWeKsCfdxscqgzrrjhaf2Rvc/v/nB9onrtNKBMjh2t0DBEarU6Y4VnoqfPP037/ba9LsurFDBNjbTuKHfe2thklwQdnrJ3YisGlXuRmLkugzXDcXG1kS/sWSJVyQoW+58FnTlHc1z17bRSa34TQnzD44x1xFPi2F1h4MCgWUG02pu7YBT66RMjSqLhwUanCk9h4AxbUXvoBIfTO/cwr0pJM9kB9kLdEAq/4WrtRbB2HpfRIwFelV8lDYKAWkxVa6T/Ot+isziG8VFsVhUt0CdXwJfowwDw1m6a9Fl17Eff+lbSOk6Vgq9++G0KgeOmndky2mA75dLNbu3YyhDumoEdrtbB8q42xqizeD0N0RwrjTVnyIs938W9B1RWg== 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, Feb 24, 2025 at 09:00:14AM +0900, Harry Yoo wrote: > On second thought (after reading your email), > I think it should be (fp == NULL) && (search == NULL)? > > When fp is NULL after the loop, it means (the end of the freelist > is NULL) AND (there were equal to or less than (slab->objects - nr) objects > on the freelist). > > If the loop has ended after observing fp == NULL, > on_freelist() should return true only when search == NULL. > > If fp != NULL, it means there were more objects than it should have on > the free list. In that case, we can't take risk of iterating the loop > forever and it's reasonable to assume that the freelist does not > end with NULL. > > > The reason I m saying this is cause the While loop is looking through > > every non-NULL freelist pointer for the "search" pattern. If someone > > wants to search for NULL in the freelist that return 1 will never > > trigger, even for normal freelists. If "fp" was ever null it wouldn't re > > enter the loop. Thus the result of the function would be search == NULL > > because the original writers assumed the freelist will always end with > > NULL. > > Agreed. > > > As you correctly pointed out there might be a case where the freelist > > is corrupted and it doesnt end in NULL. In that case two things could happen: > > > > 1) The check_valid_pointer() catches it and fixes it, restoring the > > corrupted freelist. > > > > 2) The check_valid_pointer() fails to catch it and there isn't any NULL. > > > > > > In the first case the problem fixes itself and thats probably why > > validate_slab() worked fine all this time. > > > > The second case is pretty rare, but thats the case that validate_slab() > > wants to rout out when it checks the `!on_freelist(s, slab, NULL)`, > > right? > > Yes. > > > Also to make my added suggestion of `return fp == NULL` work in the first > > case (since it does corrrect the freelist we want it to now return TRUE) > > we could also add a line that nulls out the fp right after the > > `set_freepointer(s, object, NULL);`. > > Why? > fp = get_freepionter() will observe NULL anyway. > > > But words are cheap, I should test it out dynamically rather than just > > reading the code to see how it behaves. Feel free to also test it > > yourself. > > Yes, testing is important, and you should test > to some extent before submitting a patch. > > > I know I am supposed to send a proper Patch with the multiple added > > lines, but for now we are mostly brainstorming solutions. It will be > > better to see its behavior first in a debugger I think. > > I think it's generally considered good practice to discuss before > writing any code. > > > I hope I am making sense with the thought process I outlined for the > > return thing. I probably shouldn't be writing emails ealry Saturday > > morning, haha. > > > > Also I really appreciate the kind feedback! The Linux Kernel is infamously > > known for how rude and unhinged people can be, which can make it a bit > > stressful and intimidating sending any emails, especially for someone > > starting out such as myself. > > You're welcome ;-) > > -- > Cheers, > Harry Alright, I managed to run some tests through a debugger. You are right, my code isn't correct, `return fp == search` should be more appropriate. So I think the right way would be to do these changes: - while (fp && nr < slab->objects) { - fp = NULL; - return fp == search; The first line removes the "=" so it doesnt iterate more times than slab->objects. The second line sets fp to NULL for the case where the code caught a corrupted freelist (check_valid_pointer) and fixes it by setting the freepointer to NULL (set_freepointer). Now NULL will be in the freelist. The third line is the return value: TRUE if the final fp we got happens to be the search value the caller was looking for in the freelist. FALSE if the final fp we got was not the same as the search value. This should make the validate_slab() work right and if anyone ever uses the on_freelist() again with some other search value other than NULL it should also work as intended. For my tests I wrote a kernel module that creates an isolated cache with 8 objects per slab, allocated all 8 of them and freed them. Then called validate_slab_cache() with my cache and set a breakpoint at on_freelist(). >From there I could set any value I wanted and observe its behavior through GDB (I used QEMU and GDB-ed remotely from my host). This way I didn't have to set a bunch of EXPORT_SYMBOL() and change stuff to not static; It made testing a lot easier. Here are the tests I did with this new change I just mentioned. Note: By "full slab" I mean that I allocated every object in the slab and freed them. By "partial" I mean that I only allocated and freed some objects, but not every object in the slab, ie if the total objects can be 8 I only alloc-ed and freed 7. search == NULL - full slab - correct tail true - corrupted tail with garbage false - corrupted tail with valid address false - partial slab - correct tail true - corrupted tail with garbage true - corrupted tail with valid address false search == some fake address - full slab - correct tail false - corrupted tail with garbage false - corrupted tail with valid address false - partial slab - correct tail false - corrupted tail with garbage false - corrupted tail with valid address false search == some address in the freelist - full slab - correct tail true - corrupted tail with garbage true - corrupted tail with valid address true - partial slab - correct tail true - corrupted tail with garbage true - corrupted tail with valid address true I apologize if am going into too many details with my testing, I just wanna make sure I didn't miss anything. If my proposed changes look confusing I can send a proper patch. Should I send it in this chain as a reply, or send a new email and add you as well to the cc?