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 AA77EC77B7A for ; Fri, 26 May 2023 07:15:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 04DFD900002; Fri, 26 May 2023 03:15:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F401C6B0074; Fri, 26 May 2023 03:15:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DE034900002; Fri, 26 May 2023 03:15:23 -0400 (EDT) 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 CF6976B0072 for ; Fri, 26 May 2023 03:15:23 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id A37BFC0D24 for ; Fri, 26 May 2023 07:15:23 +0000 (UTC) X-FDA: 80831545326.23.CE4AFCF Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by imf29.hostedemail.com (Postfix) with ESMTP id BC3B612000D for ; Fri, 26 May 2023 07:15:21 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=oQmuvkML; spf=pass (imf29.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.54 as permitted sender) smtp.mailfrom=lstoakes@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=1685085321; 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=t91DnNfVBB5flL99LpLlavWZRfqPEi+X/+IxN9gTW9U=; b=avX1PW8n08bsSfTytwUgMPTTp00MsIP7QMevvA3zN4IiTtIlxPRokXqv03gh1eyTFmGLdW LMh61tuDnsLfu2zQztWsVO+pZYU9KhGXvv4TrA+I9mLYzkRCIzrz9a41KoOmoCSjkXM37r ztY4eu47D5MLWQKBCz015W1yBI8ft14= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685085321; a=rsa-sha256; cv=none; b=WudQwn5wKc/2/JKT8eSj6zfFNchifAQ2p1A3r8rwWKl0Ma1xXHPUXcpgyNuYTrImBRAVis 4I4ioT2W/qz3oDLXUrOvO7cq+OaJEBMtCYuR57o4rrsASQOVSMiIexh6/lyMsTJA/sP4WT ITZkLDDdo/iKOBB81HScvsij+rVPs04= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=oQmuvkML; spf=pass (imf29.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.54 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wm1-f54.google.com with SMTP id 5b1f17b1804b1-3f60dfc6028so4380995e9.1 for ; Fri, 26 May 2023 00:15:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685085320; x=1687677320; 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=t91DnNfVBB5flL99LpLlavWZRfqPEi+X/+IxN9gTW9U=; b=oQmuvkMLuPCqWiWXelSR5L0QjuJW2qwM/XZI1QKphvIEmO014uKSzBewq0A1YiIFOG p7xQc2cOmv6ZaP41tn7mYaarKiy1WLEIFC8F/tsacir0laXyfGgNWTOEE6Of8yH09TJq V0fxnE5WqzebEPt76quLPG0RTmVa5qnDpGkyhOy6SMxnWE3zRTAnTpSi5zBwkENBJPWq Dl4lBzd5UaRNgJ9kTorojva8j3U9/M81vPTjSpsU42sGf5mBDBxSZ79XAVR5fTAIm2fn VMUdQQLBZWsTVfsPgv2YV24duGe402z6g5LutKBHow36RQeqTbqDi9uCdkJj+YYCMwgu gSzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685085320; x=1687677320; 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=t91DnNfVBB5flL99LpLlavWZRfqPEi+X/+IxN9gTW9U=; b=NKPyMcTsIk+qyztMji91GznrYrZ6ugUvzFX4l8YUTLnTqTX1GOPJz+OoDXGqk1s+dY UzQVQOGbCF3RpMfYVApTqCRgA+tdt85ykfqqEa9pkcPNllYDsnXVhf+uJRKwxfs7eF8D 6ZL84FW7l3c5iZ29YDe3hdc8UFhBFAfUU/AjHdeUNx4IvaqaVv2GhcxqXqwavcMYyShx TneWMJN2ZWa97uE2X4QElMIBM2qXp6kC3trFSJonrwApeEg3ztCmar4TrG00L4sPurP7 bNSSh0HgDqeoZpJM2pMxelpi8PR/Pkv96BmzMBhaUVK2SpS2AhuBR3aUUrFx3gPCy+JO GZcg== X-Gm-Message-State: AC+VfDxQW4HX0v3rAx18AU5RRsuH9d2s2IU3GLVw42cdRP4y+uYiUfW4 z2YahYTCF27/Zec4B/kkUPE= X-Google-Smtp-Source: ACHHUZ79bjFz3JryFplB2bXK1OOazxp1JWNad91KuvzjlVo3CxG5sH5bO4eUf3D/ZIkKQujrE3rH/g== X-Received: by 2002:a7b:ce0f:0:b0:3f6:a65:6ff3 with SMTP id m15-20020a7bce0f000000b003f60a656ff3mr812644wmc.32.1685085319801; Fri, 26 May 2023 00:15:19 -0700 (PDT) Received: from localhost (host81-154-179-160.range81-154.btcentralplus.com. [81.154.179.160]) by smtp.gmail.com with ESMTPSA id t3-20020a1c7703000000b003f17eaae2c9sm4254789wmi.1.2023.05.26.00.15.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 May 2023 00:15:18 -0700 (PDT) Date: Fri, 26 May 2023 08:13:07 +0100 From: Lorenzo Stoakes To: Baoquan He Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Christoph Hellwig , Uladzislau Rezki Subject: Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array Message-ID: References: <20230524082424.10022-1-lstoakes@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Stat-Signature: ux3bnqds5icmhku7c5y8hx6sn78ty1ef X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: BC3B612000D X-Rspam-User: X-HE-Tag: 1685085321-710067 X-HE-Meta: U2FsdGVkX19b8QinSTg3XL+wmOwtGq5SeBxltsYGqzUfV9Fu56csRqcKC81kkM8go5r8iN0Dn5VbXIi9aQvWmqmc7gH4bJEVIz5X3b1f0lzavKk3ybOmkm+tCH9UpHcaZ/Y2dSE+KjYo+2OR7LLar61/KTHtVf/H86Vu4kccN3oArzBu93rRDawjGIU0yAInGXNMottrvs2dh4uV3W1Bfaqg3tCOMbMFX8pgyiztnZ51n87GMfQS5jvqYifpyX1sUp/lehXHIE+gaPIiIqKYGC4hFjxBOO0oOMKfQE3Y2FjH3UYu9vzl7wO3JRcpG/ZCiccWpiiDz1a2SEny0ZozBjdjUIkncsTxYQws9PL43TUwf39WFlatYeObbLkyb7dauopfCVjJLeXSgazYgKb8Q1I8SBewOVfGuaxKfyGIkBKZpUg1vbqk7oDsSAcJPEIWrTn28F9EzXCwV+bLk4gz09+GnfTwehLTsQI0tO/aa1JektY+YDGDpUH5k1LX/EfChR4ioYsvpNsNdOtgGr22rf47dqomv97mbLnW/GSulkHPzXLvqM/vP1VMnGAZ9Ev6f4n//Eo7iBcgpPDVPQsUvF+93qLvQw74U06D4o8pfUGuetBZ7Ompr4KWDwS7CGmYbeIBVpRyG2BEJ7dP7GQOlHdstxbj29PzkAPzaJb8Y0whLi5wKwNymWHPnFleMUNhUkLNzZKEdaMef8v2TBHu5BpqGJi8dqoEmq9xBQ5D3SLNXXq0WR4cXkIG4zOxzRA979b1NHzTW/JCy9F1lo4jHEbcgdLDtmuxmweKwyouQ12zMs35YILOhRJqJAMykeL8M6dBpHDDUiqTNB8DAUVncFMHO2iEO2rRuFNqGdp/Bw4E8VeFr63cnqj9yTSWh1+a80JKZ9v0/HCDl8DTYjgkjd0um5GQhEDALtfEu/Hus1iJQQuxlx9G698ZyT5Yjr15QTGB/AmoVWNyd0qvktm +/Hij8OR cREYh+w0wRCzQh1fc/VhBPoqxBxyoGWk+S6Cgq4zgnpFbaY3fv0VsyIErZaA/sqhho4QH8sF7qSN5fV5Ao2wEFl9q1YUZ+B87LfMQbtQe9kOe048qPwcps5BVSoo3phnoRHJIqHGwsW5sP+l+WBOD1hfId/7j38PKJo2mrT/1JXB21eLdwoaPVlPCf74uOWGcOI78EEBF5uDovdjlh4cFUpWw732pqIIqJNz1NmpzpSSnzXTAYdMVFGNH81f4HnSKdfzicP7G5gad66a6F5A3UTZwW4pG+TOkabe3AbG5PfzYngHYQbNSbYk0strUUq8j3O0pK/Hea6Qsx2GccsJeWiP36G6KRZVIezjhWZTubFsKIIo11+XppNpeZlKA20ZJkWbQp6FH7MzKnqL5uJuU2uiW37ZSqzq/Ff0ztdNg/4w5zJbyO90mzhKpLGqtc31SStvUdBNdkVHqVxma09XGkliAjg== 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: On Fri, May 26, 2023 at 08:08:33AM +0800, Baoquan He wrote: > On 05/24/23 at 09:24am, Lorenzo Stoakes wrote: > > It turns out that alloc_pages_bulk_array() does not treat the page_array > > parameter as an output parameter, but rather reads the array and skips any > > entries that have already been allocated. > > I read __alloc_pages_bulk() carefully, it does store the allocated page > pointers into page_array[] and pass out, not just reads the array and > skips entry alreay allocated. Umm, the function literally opens with:- /* * Skip populated array elements to determine if any pages need * to be allocated before disabling IRQs. */ while (page_array && nr_populated < nr_pages && page_array[nr_populated]) nr_populated++; And then later:- /* Skip existing pages */ if (page_array && page_array[nr_populated]) { nr_populated++; continue; } This explicitly skips populated array entries and reads page_array to see if entries already exist, and literally documents this in the comments above each line, exactly as I describe. > > For the issue this patch is trying to fix, you mean __alloc_pages_bulk() > doesn't initialize page_array intentionally if it doesn't successfully > allocate desired number of pages. we may need add one sentence to notice > user that page_array need be initialized explicitly. It isn't 'trying' to fix it, it fixes it. I have this reproing locally. What you're stating about 'successfully allocate desired number of pages' is irrelevant, we literally check the number of allocated pages in the caller. No sentences need to be added, I explicitly state that the issue is due to the array being uninitialised, the summary lines talks about reading garbage. > > By the way, could you please tell in which line the test was referencing > uninitialized data and causing the PFN to not be valid and trigger the > WANR_ON? Please forgive my dumb head. Well, I showed you the lines above where __alloc_bulk_array() is accessing uninitialised data by reading page_array[]. But ultimately this is called from vm_map_ram_test() in lib/test_vmalloc.c:- for (i = 0; i < test_loop_count; i++) { v_ptr = vm_map_ram(pages, map_nr_pages, NUMA_NO_NODE); ^--- triggers warning because we can't map the invalid PFN *v_ptr = 'a'; ^--- NULL pointer deref vm_unmap_ram(v_ptr, map_nr_pages); } The warning is triggered in:- vm_map_ram() vmap_pages_range() vmap_pages_range_noflush() __vmap_pages_range_noflush() vmap_pages_p4d_range() vmap_pages_pud_range() vmap_pages_pmd_range() vmap_pages_pte_range() In:- if (WARN_ON(!pfn_valid(page_to_pfn(page)))) return -EINVAL; The PFN is invalid because I happen to have garbage in an array entry such that page_to_pfn(garbage) >= max_pfn. > > > > This is somewhat unexpected and breaks this test, as we allocate the pages > > array uninitialised on the assumption it will be overwritten. > > > > As a result, the test was referencing uninitialised data and causing the > > PFN to not be valid and thus a WARN_ON() followed by a null pointer deref > > and panic. > > > > In addition, this is an array of pointers not of struct page objects, so we > > need only allocate an array with elements of pointer size. > > > > We solve both problems by simply using kcalloc() and referencing > > sizeof(struct page *) rather than sizeof(struct page). > > > > Signed-off-by: Lorenzo Stoakes > > --- > > lib/test_vmalloc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c > > index 9dd9745d365f..3718d9886407 100644 > > --- a/lib/test_vmalloc.c > > +++ b/lib/test_vmalloc.c > > @@ -369,7 +369,7 @@ vm_map_ram_test(void) > > int i; > > > > map_nr_pages = nr_pages > 0 ? nr_pages:1; > > - pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL); > > + pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL); > > if (!pages) > > return -1; > > > > -- > > 2.40.1 > > > A broader problem we might want to think about is how little anybody is running this test in order that it wasn't picked up before now... obviously there's an element of luck as to whether the page_array happens to be zeroed or not, but you'd think it'd be garbage filled at least a reasonable amount of the time.