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 E332EC19F2E for ; Thu, 27 Feb 2025 16:42:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 37C1B6B007B; Thu, 27 Feb 2025 11:42:19 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 32CBB6B0082; Thu, 27 Feb 2025 11:42:19 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1CD70280001; Thu, 27 Feb 2025 11:42:19 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id EBF0C6B007B for ; Thu, 27 Feb 2025 11:42:18 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 978A016198E for ; Thu, 27 Feb 2025 16:42:18 +0000 (UTC) X-FDA: 83166292356.05.DD31F52 Received: from mail-wr1-f47.google.com (mail-wr1-f47.google.com [209.85.221.47]) by imf11.hostedemail.com (Postfix) with ESMTP id 02B214001A for ; Thu, 27 Feb 2025 16:40:19 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=MgP4aU83; spf=pass (imf11.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.221.47 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=1740674420; 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:dkim-signature; bh=tlqa3xJHdsKH2Fvg1chISi5Lg7orvZo0XWjFT2khQYo=; b=HppvFeVP6LK/mAM13vyONOHvvIbLy7pXFKxFXJDEdzEg4WA8CitaKQTcAjkmz9lZ/lobGs GJVEjPHM17MJyZHovqHzbLxgQDtZ5s3FqrmJF7OyrTcLWm+Vq5e4cnT11jEBSuKncwhV51 eOxUSllstRzIQ/M472VjDcfdfa/535s= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=MgP4aU83; spf=pass (imf11.hostedemail.com: domain of lilithpgkini@gmail.com designates 209.85.221.47 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=1740674420; a=rsa-sha256; cv=none; b=TF1t1m/l7HWLpTCJk0fVvqakTj4J9PwlsUK8VAmpj8Hy+daQFJ0hPD2tOxMcpzBxszzvAd gDGLQvkESoj7YRfSUBYQYGcO6VUg5vngnLPvolfEGeSKsuSLJUkISAS8XWLmo0XYMD3fZ6 fXmr9JNgfHDQCTOB+tKQ6T08kIhs3c0= Received: by mail-wr1-f47.google.com with SMTP id ffacd0b85a97d-38f29a1a93bso955620f8f.1 for ; Thu, 27 Feb 2025 08:40:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740674418; x=1741279218; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=tlqa3xJHdsKH2Fvg1chISi5Lg7orvZo0XWjFT2khQYo=; b=MgP4aU83j2MT9mRViMv8uwja7dQrGOZ/2TYzeHuYcpjmcwYNkoOmjtBVxGSoZKt1bg kVXoq8q5KvZXZstA3+rvDlK1x4gAAdod+Wb4hZkkUnaCfYQCY+uL4mC3lbgc8Mkh5I3f rqtqN9P81K/+O4gc//WaCP6jrHU8/ILOOY8RMs7z4TB3OMyMrvVWUTyyIMzF2vva9f2x H10Bg6e/vyekdO3WbqYSafC9kmJ7ZfRV5AlgxsOSpHaN5bUAFIYrOkujVo67MqmZkMgt MBlqeLis63xBQObaJAOd1M7+T+EjV4XnPUzd+oWmY17xarFlpqxbJoBRd42wMrog04oA 3ldw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740674418; x=1741279218; h=in-reply-to:content-transfer-encoding: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=tlqa3xJHdsKH2Fvg1chISi5Lg7orvZo0XWjFT2khQYo=; b=bzco/PEghu3lseh/QVOUjjHJkWHKLywafO8NuwFSQYN20QJFOfxaBEmv092BvclZpX pJTVUJ6AUiJIOODpeVaPsJI+7bTwD6hntYcAc1mc6+9JO+2Ym+XqHHiVQQkt4c72266Z 9uS0roFJxiS1TYnRfSFi83ZxWW0Zb/+BVRpIS6mQOXDhguoDgqH9Bam9Fo75xDWHw/hf 8yO9YTwAul1l68NUARgGIOPMlDZK01o/+uh/YFxKnO4HSU/etWcBtzbNbysUq2I4IP7J 0x21lh7MQAChtnD5JCoyuC9jiOjWLVaCNc9C+OZSE9N/GQmW5OlhTXGcbWb1f8IoUCy3 0pUw== X-Forwarded-Encrypted: i=1; AJvYcCVJnzXlV5hebIsO570WELSJOIJIRt5FP2mmjAWWXesq11TjfKBKJ+hJ0oYUBQ1BMQ3F49oI/xZZgQ==@kvack.org X-Gm-Message-State: AOJu0Yx2hwKJRvYTR95nu3EdSU1XhdIVNuW48eOtUnrWfht45NxSOgk2 1XrUjDgBebmNTBFTcFWU1uwZAHqBe/JnXVXdHvFs6x6NC6cPvpgIcgxxbw== X-Gm-Gg: ASbGncvCUTw9wLDFZ5nir103of+kmrwI51w4gkrTTxkRpuBeBCMoy9SyI/MIOx+2zyQ D8uOan1ke65SA5ca3fm1dwS2FNk1ANxPGXf4evvq2Kas4L39BbSwkJpSxxsckV+Xkkra79167fu D7xmhAwp5/QKJqpeR4ZRHwW7MI8u2K5+CQ7OR6QaTmTCevuwQYRaMoeUoPkR0hCljM7cUQT/EbL S20q9crKKFbeXPvCtFW7NDnA79LlUgXxp3All7KLCX0lI3pgq/JLMGQUr2ah5MRSU7XO2uhTKwN M5VHzhuncMBBEpExrjfQLfNFOGjX8w== X-Google-Smtp-Source: AGHT+IEFsNMCjcv2uWdCqrUOWTTQaKN1QqTUQaSQl389HYTLOqtSHSlCTFqIUxo4mgOaPaYLGrXOiQ== X-Received: by 2002:a5d:47a3:0:b0:385:fc70:7f6 with SMTP id ffacd0b85a97d-390d4f377eemr6710738f8f.7.1740674418042; Thu, 27 Feb 2025 08:40:18 -0800 (PST) Received: from localhost ([2a02:587:860d:d0f9:2a79:b9e6:e503:40e9]) by smtp.gmail.com with UTF8SMTPSA id 5b1f17b1804b1-43aba539466sm63521195e9.18.2025.02.27.08.40.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Feb 2025 08:40:17 -0800 (PST) Date: Thu, 27 Feb 2025 18:40:16 +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=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 02B214001A X-Stat-Signature: k9gpk4mwea6tks66hdunft6zxhwsd5my X-HE-Tag: 1740674419-945950 X-HE-Meta: U2FsdGVkX1+kDcrcDpnSlumz4ULav0M6r7JAu4nJ3c8trelvQZ6BeU2LiaBUJWig4SYegiuO4MCa8Zvq6NllajBfg8UzkGDCqKzsJ0lObWjYIlU5IrdOhocViehadhzavmI3TSq1eMC12L1MVlQeg8MuZqH4Myq9hz180JfcJSlI65yMf2xenNaHzmn/+CPjoOxi6/+iMXSETEBVDreVYKNIgb+4lEi/SIHXLZ3V51qXpCzaOkZfpD6vEvRKkFF6R1c19LXk/ZCHEKMQdwSq0SCaVfXUIaXMmRyCH72NNed5uZrsHmQ51BvgHzScKLCp0TEo8p4lh9JKcDbGFJUckQ3aKrhHE5QAywJdRfQ+R5Yo9yakByWXlSP7vJPLhjqazwr9q5JPwhd+sQqEPYrQQf6kVSeWnfFDPzVf3CNWFoVC4Ujup9tT6pdKq7lQH+aePavB6/pk9F8Rlvew6NczVds3lDfmuRG3Po9cXeoRizNaEr8DsRyY8BalnPmNkpWLs+JaMHYvHb/AsGafq6RFB8Mk61Xyko7AFwERlklJv5jlfemWwM8R1DhFxnHk9w9k1iZZYjdytfMdPksuV6thurET5FZ7ZpgQPh6SCwJujmTX4TtBDNWjcLJL4Do3KNl9BnPi5taxX+bcjqR5YmF9sLNDMCIPIqDsL3zqUv5fgP4UHzxPEpXxpt6gO0seFQ6Z5YMJGlvVQ0saxp6HEdbeMGzghOp7+AuvOEVXh5qs4SFqVh0FAZMp8jVVLKsdV+7VqL2AEaP8c1rc6Bp1xNXMU7odLry8ZuM3ILxe/CSmtpl4k3ZKrDnw2l9s+KlJSqCgavY06sZ3TDaEwN0KZBH6vcTOutHqc8q3mbxD9ewpZkOf7UkUIgTL3wbyTypUxc+OVVWlpVREiX5ipFPO/18GEqe2UJsPhdnhAFoynJroQ4zjHokdYJO2jO37vxwzfPgkjPcafwK72IKYXaDXsJS KWEfTK9L xMPpWxIkcuV44iQrDnGqltMGfmzYY5STSbndE6NU6LZbevfsPvITSEUYoPECtQnMjpZCBc0pIsAQpTsklQvFS85Uznqb0Rv4eGLYUzL6WDCxfrbAOEaFJT4sEJuWu8HqmFyPCZ5bbO2DwM1bSR0qU36eJA2TZAu1oyy3CYUVLPcf2Ah5aQ6uj0g1DSrNbYtlGyFdqvfdjq6rllO8z9brGZuM1LHrOYE2vHcBaNvElku8TuQ4755TnSkN+YrZrkv/6syezs08J5bQb9YMzZsdpB93SIgWN9YJcxW26HdVkNvY0i+NFFpUBSigpNeBqsr5wwQY0glxuoZFx/XyM3OT+nSQsFpaiolwEjJ1IX9QM5SqqKlEjRYti7H1Gs72+nEG+TX11fX4zU6gb7kERE1NazidGmsIRvibr6hC7smg+ihS0Eu75n0hEz6O+hiPcwum8vjl+hp2QzFn24/BmY1+oh1d3SNLmq0dxL5+roi4yTsBXMY5xwpn9LWT1iXWvmReo1OaMJBH/wX1H1rQORJwHMi33LcpaNwNDsXplZIqn4zqXqjccWKH6ygYP3EoZKOMCftF95LiOacr22JpIrUVstgBaxVmoUvk5lGmU5FE9KMY/8Zb5U6jZ9j0NtE352xCEIPAPyLqdtQJJKE7sNIf0R6g0zQ== 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, Feb 25, 2025 at 07:08:09PM +0900, Harry Yoo wrote: > On Mon, Feb 24, 2025 at 02:12:13PM +0200, Lilith Gkini wrote: > > 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 > > Hi, Lilith. > > If you don't mind, could you please avoid bottom posting and > reply with inline comments like how I reply to you? > It makes it easier to follow the conversation. > > > 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. > > Yes. > > > 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. > > Yes. but do we care about the return value of on_freelist() > when the freelist is corrupted? (we don't know if it was null-terminated > or not because it's corrupted.) > > > 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. > > Yes! that makes sense to me. > > > 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. > > FYI in slab terms (slab->inuse == slab->objects) means full slab, > You probably meant empty slab? > > > 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 > > Two falses because when the freepointer of the tail object > is corrupted, the loop stops when nr equals slab->objects? > > > - partial slab > > - correct tail true > > > - corrupted tail with garbage true > > This is true because for partial slabs, the number of objects > plus 1 for the garbage, does not exceed slab->objects? > > > - 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 > > The result seems valid to me. At least, this is the best SLUB can do > while avoiding the risk of infinite loop iteration. > > > I apologize if am going into too many details with my testing, I just > > wanna make sure I didn't miss anything. > > No, it's ok ;-) > > > 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? > > You can either send a new email or reply to this email with > In-Reply-To header. And please cc SLAB ALLOCATOR folks in MAINTAINERS file > including me—I recently changed my name and email (previously Hyeonggon Yoo). > > -- > Cheers, > Harry Hey, Harry. > Yes. but do we care about the return value of on_freelist() > when the freelist is corrupted? (we don't know if it was null-terminated > or not because it's corrupted.) Oh! So we shouldn't care about the return value of on_freelist() if the freelist is corrupted? My thinking was that if the check_valid_pointer() detects a corrupted freelist and fixes it by setting the freepointer to NULL it fixes it, so we would care about the result. But maybe if the whole object_err() triggers, I think that causes an OOPs if I recall, we should consider this as corrupted and we shouldn't care about what it returns, cause something went terribly wrong? I guess that sounds like a sound logic. If that is how we should do this then yeah, my second suggested line is moot and in my later patch I shouldn't include it. > FYI in slab terms (slab->inuse == slab->objects) means full slab, > You probably meant empty slab? Thats fair. I was thinking more like "I allocated everything and then freed everything in the slab, therefore the freelist will be full". But hopefully since I specified what I meant it wasn't too confusing. > Two falses because when the freepointer of the tail object > is corrupted, the loop stops when nr equals slab->objects? Yeah! Since the freelist pointer is corrupted and doesn't end in NULL on_freelist() won't find a NULL, and in the case of the tail having garbage that the check_valid_pointer() would normally catch, the code won't catch it, because it's at the tail which would exceed the slab->objects and the While loop will exit before that, since the freelist* (not slab, as you pointed out) is full. > This is true because for partial slabs, the number of objects > plus 1 for the garbage, does not exceed slab->objects? Yeah. That goes back to that second line I suggested in the previous email. If the number of objects in the freelist is less than the slab->objects and the corrupted freelist has some garbage address that the check_valid_pointer() will catch, it will set it to NULL instead, and as I said since the freelist now includes a NULL, my thinking was that on_freelist() should return true if we were searching for NULL. But if thats not how on_freelist() should work in a case of a corrupted freelist and we don't add my second line that nulls the "fp" then this will return false instead. > The result seems valid to me. At least, this is the best SLUB can do > while avoiding the risk of infinite loop iteration. Yeah! There were no infinite loop iterations in my tests inside on_freelist(). There were in some cases in other functions, but not in on_freelist(). Fortunately the While condition prevents any infinite loopings, even without my patch. > If you don't mind, could you please avoid bottom posting and > reply with inline comments like how I reply to you? > It makes it easier to follow the conversation. Hopefully I did that correctly this time. Should I always include all the previous messages in the reply chain? It was getting rather long and I thought it would look messy. > You can either send a new email or reply to this email with > In-Reply-To header. And please cc SLAB ALLOCATOR folks in MAINTAINERS file > including me—I recently changed my name and email (previously Hyeonggon Yoo). Oh! You are one of the maintainers? That explains a lot, haha. I just assumed you were someone from the mailing list who happened to be really passionate about SLUB.