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 36B06C7EE23 for ; Fri, 26 May 2023 08:57:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9EC986B0074; Fri, 26 May 2023 04:56:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 999446B0075; Fri, 26 May 2023 04:56:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8889A900002; Fri, 26 May 2023 04:56:59 -0400 (EDT) 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 782106B0074 for ; Fri, 26 May 2023 04:56:59 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 38F61140E17 for ; Fri, 26 May 2023 08:56:59 +0000 (UTC) X-FDA: 80831801358.26.6715ED1 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf14.hostedemail.com (Postfix) with ESMTP id 466A710000B for ; Fri, 26 May 2023 08:56:57 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=T+YAm6Zb; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf14.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1685091417; 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=/8JS6bjSYYM5b/bJbEXfvPGMCA87+2GLI6ZyQk0Hyfg=; b=CwGH1gU4eRDMaQrVoyogeeFfvjRO1uyglMkZQ8/G7SoLzVVn8sdHSsU/qszQ0fS2MV5ZR/ 6FRmKYSyQX6W2oJhUySbVXvWuJhbOYuScDQfSB+IXuWxOvohHlfr7l3PYqL9HYNG290jIj qjzoQhNl2g+rP8U3Jy/xVxE6dYbBRRM= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=T+YAm6Zb; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf14.hostedemail.com: domain of bhe@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bhe@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685091417; a=rsa-sha256; cv=none; b=2cKRJ26ECtFER7NVv2CXOrJ8gPRjZlh+3GQz8sfO2TPQ7vYHbB3Gr/NAxMKU4+6y33crPf 9ld0QHImMwOx6ErqS5Szq6uj+3ArhHC1lIm94TZ0Q2U6oHrDUb6dxB840u2yarw1Yr1hdv 3MNeznkP9XLVqSq9K5umbhdluGBYUU0= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685091416; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=/8JS6bjSYYM5b/bJbEXfvPGMCA87+2GLI6ZyQk0Hyfg=; b=T+YAm6ZbJ+v5qY8aomo8t20i7wyg4ohKGQ4UxI+V1YWxkd89PCqsJJ4lKYPYhBSF8UNjW7 7BuH/Kx9AqZLXL04yEN5AY+GA6+fP5p15vNrt6FTCUkKsF42igQ2aCC6ZZ/BfAVh3TSlkv nZI/KLboYBianUOfS2dzWCiQhcyHHwI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-671-5G75kQDWO2-_fG69XeKX5A-1; Fri, 26 May 2023 04:56:54 -0400 X-MC-Unique: 5G75kQDWO2-_fG69XeKX5A-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6C72F802355; Fri, 26 May 2023 08:56:54 +0000 (UTC) Received: from localhost (ovpn-12-35.pek2.redhat.com [10.72.12.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 939DA200AD47; Fri, 26 May 2023 08:56:53 +0000 (UTC) Date: Fri, 26 May 2023 16:56:49 +0800 From: Baoquan He To: Lorenzo Stoakes 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-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 466A710000B X-Stat-Signature: hcejef6dg3zkc9tufe54c136ypeq8e13 X-Rspam-User: X-HE-Tag: 1685091417-903034 X-HE-Meta: U2FsdGVkX1+wvNM1vbZbQYZexjMUflfVp3fjSkTzJZH5bAl/x6uCpVJH1T8fTCfqyaJ9JCbaaDMay88ncDeZnEiQIdNoFXB4RKl11Ke2HV+VFfpKcNT9++g9/k5hTGwEgdIVgmpcMU6bfoVX4QqgdKoGEHFpGDyArZpOkxuPyndUPuyvGUgMjCyPBZ1yMdytkTcOVxPP0sAN2RayydgeF2iowyBN2+RAF2/rqXyksuD/K/7FTjcZJPLcfPB3MEJl+ZnOyMQGn+22Vdia4ob72+FFyUpIeQGd0eBFD57ndTieGiXchWGeJ5NiKuk4SZ5lOS+FngDmnBAMySwlWbBy9lej+5VSRU71XGb1pJrkM5M9iuBPnm2wRvZ7cX1Thwlc6fa3Zm/GTItgfHO64T7OxdD9qmktm35lXC/6jI5siBp2HzQjE4vEjT0rG008QbiP85j8vfr3XT0G7Y2N3/ORNqH1JbIwG4BbmVeVik1lKhaFznClIEkcpl3V9d0gdKsBAkHvEGR7ccfhuEhBgzezmVx99lVsO988wLThOAuyouo0Mk5eCOm/8iPfDBY5YwyJfSqg0SuO1iw+BxE2Rahz1z6P53+W1q5bSaqYldilTGWq6BzigpTHtFw3kDzJ4jbo8BQuumAb1v7K8rRP3M4hR4Uncm2GnkRY9Vgbs1RBQ7Y2Rg4eJBi82w3qQlDaEUsz5tHiAZ6g9ZNai6SSqWJJ4Org70hzhbq9BClV27KgG66loiGtxCFXOONuaJTM9Tj/zh/5Twv/SLZAd3qiI7S6xKFAEHKYffx2HqpHu0JQ8TSR4ZLhyq4pK/zsNlEqeo7rOCI0FgHyNocIiUkzYMHzqmO52KKkHhARfUFeuOAvFe5M4KPoTaN5iyGlNF5EwjHTg8SfoAIckxGzjmoGyQcaPzhydUFQXrzrX+UW1Cu8jdPbgyLGJCDh3ukXfn5ns4J8DitEtI1Kd4rjZE6OtlR t8ExsPI/ K3xLwFuDPzi5EUQb5LdhB36PazqRPwd/Q6ZcwtYbG7Hfs2NTAF4+AP6nLMil5K5VqZSb4InZYIkPjM35JIImXm3vPjTobFLCMgxV1ObC9JoiD5vfyM5lrLLyTcogm04MtTYO5Zq4Sqvc0vSzoCDVpVDvoNc0cGzWbGpsn/4fi31GlsVrj6qliql86R5aal8uIpuqZjcL/xfz3iyjZUfvaHk0KDivSeY7XH2HxpNsYbzpLJZHhh648ynxJSuq7WtMlGp+7L0koLRWp1XY= 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 05/26/23 at 08:13am, Lorenzo Stoakes wrote: > 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++; OK, suppose page_array[] alreasy has three pages populated, if not initialized and there's garbage data in page_array[], it could have nr_populated > 3 finally? This is really risky. > > And then later:- > > /* Skip existing pages */ > if (page_array && page_array[nr_populated]) { > nr_populated++; > continue; > } This is interesting, I thought this place of nr_populated checking and updating is meaningless, in fact it's skipping the element with vlaue in the middle of page_array. I realize this when I recheck the code when replying to your mail. Not sure if we should restrict that, or it's really existing reasonablly. [x][x][x][][][][x][x][][] x marks the element pointing to page. > > 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. OK, I misread your words in log. While page_array[] is still output parameter, just not pure output parameter? Not sure if I understand output parameter correctly. > > > > > 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. Right, my wrong expression. > > 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. Well, I meant adding sentence above __alloc_pages_bulk() to tell: page_array[] could have garbage data stored if you don't initialize it explicitly before calling __alloc_pages_bulk(); This could happen in other place if they don't use kcalloc(), kmalloc(GFP_ZERO) or something like this to allocate page_array[]? > > > > > 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[]. I see now, thanks for these details. > > 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. Hmm, that's why we may need notice people that there's risk in __alloc_pages_bulk() if page_array[] is not initialized and the garbage could be mistaken as a effective page pointer. My personal opinion. People may argue it's caller's responsibility to do that. Thanks Baoquan