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 12AA2C77B7C for ; Fri, 26 May 2023 09:13:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A0B29900004; Fri, 26 May 2023 05:13:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9BAE5900002; Fri, 26 May 2023 05:13:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8835E900004; Fri, 26 May 2023 05:13:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 724CF900002 for ; Fri, 26 May 2023 05:13:12 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 442F2400C8 for ; Fri, 26 May 2023 09:13:12 +0000 (UTC) X-FDA: 80831842224.13.129F1F6 Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by imf18.hostedemail.com (Postfix) with ESMTP id 6150D1C000B for ; Fri, 26 May 2023 09:13:10 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=ivDTv96j; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.42 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1685092390; 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=R1U6XpxDvSih4ymgTpgV3kFrDpgdRbG2VFnRYHWBNJs=; b=f0sYLq0+xrAmc+l74vgId7FWst56V1zWU32Onto/S4t2iG6ERSraaMrNa6TkNO2qKUvdwm Ht3sXbZOVOC2ft4GXC3jixL3nA3HoGhph8pSPL8zeHQ7rxADEIBxsxw78PWXp6xMfuJvOQ UbHgLvuC1FrzpBNgv/mGwuh/m3CKb0I= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=ivDTv96j; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.42 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685092390; a=rsa-sha256; cv=none; b=OVJulkOQGI3/2voghBlpErs3IVq0BaidX3QcRT+noc4jRPN+JRI7umKRuVCtgS4cqToUqW b9sWv6HEOesMQyyV9DnPUFVS4m0aFSj8YP7J/CZMmUQvm9kpbsaJWJuTbSBelFd9GtQfLr P7TlEz7QBXWbelXevCoZrwdbYg7a7WI= Received: by mail-wr1-f42.google.com with SMTP id ffacd0b85a97d-3093a6311dcso438889f8f.1 for ; Fri, 26 May 2023 02:13:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685092389; x=1687684389; 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=R1U6XpxDvSih4ymgTpgV3kFrDpgdRbG2VFnRYHWBNJs=; b=ivDTv96j8kH0dG6OxnbkkGSED/St7SA8wh2JOdYc36V0GOaDPoX+B7W4mdUzo/UBhJ Wo5yXXRaHSyRUP3nMMDT+QLgN1vSQ4Ivr/VgLhTufKswkJmAEqMtC3Ky74vOo4WIbkKk sdJpJWiIh+TgPvLh04ZMZ+Wk5lfSeMnKu00wRNSJkIrwv9FnkTra2n/E9INP13/yzde8 rfGkfiGVVsl1wFbV8YrSebJrrOgz6t62DjufKg9rz3X+3OwlIROpxwHnLLZBUC3ywNLi ISdUIxGMWRXVcfBWNQO9mETwwb2EhHW3+mJ3UzaQWCeJxeCtV/aoTrdceUaT31pntrY5 iIdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685092389; x=1687684389; 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=R1U6XpxDvSih4ymgTpgV3kFrDpgdRbG2VFnRYHWBNJs=; b=ee52P1CRuhvPSmE8lSFPXIYDWISQIpKs8XRN2yHMdFsaUfTv0qzj6Y64/GuIv/U5JE AganAPGcQNecGOysT4otTSjH9o3vrx4omdLjjIOhcjmWsuVdSRKMefsdFdV3vtTuarhv Wf4+5XZohZZaJx/+NO0gOclM92gQWY7+CLlDK9wA7fWbE6wgObsGXrl7wFfoPycxhAtt roPNn5njXAjm0+7hTj0nA7cpFnvwFlUrlkMFSWADYL/HjsSEF06iJ1dVElu/AOZC5T5t 7FDJlnJrJGUSbhLmdyQd6dzVBTmiVHgINHVWCO8VZf6UOM5GoMqm+Lrzs9r0Q8ckrFrZ eOWQ== X-Gm-Message-State: AC+VfDwEO/bbnV7tdlYM0c4Eq3tA0Jan0qvRgWlG0eaKkLlRIJIVHzOC L/i+JZWuxXG6mUEFs2NGXRBj8GFY6m8= X-Google-Smtp-Source: ACHHUZ7DIbuyHeioFWko7fZWbQQq71ahSBtGwNaANF/Weuj5m55L4H0xvPykQ8Rf3L5W/YBa+rCveg== X-Received: by 2002:a5d:6e53:0:b0:30a:ab38:30fe with SMTP id j19-20020a5d6e53000000b0030aab3830femr937975wrz.43.1685092388679; Fri, 26 May 2023 02:13:08 -0700 (PDT) Received: from localhost (host81-154-179-160.range81-154.btcentralplus.com. [81.154.179.160]) by smtp.gmail.com with ESMTPSA id r2-20020a5d4e42000000b003095bd71159sm989789wrt.7.2023.05.26.02.13.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 May 2023 02:13:07 -0700 (PDT) Date: Fri, 26 May 2023 10:10:56 +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: <368bbc1d-d810-4bc4-8091-7ed55631344f@lucifer.local> 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-Rspamd-Queue-Id: 6150D1C000B X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: ra6f8kgnd18qhf357y8wiq5u8frcmtmd X-HE-Tag: 1685092390-14628 X-HE-Meta: U2FsdGVkX1/NyUhzJ+EBKFeo3dbmBIwrvjIDvFP/hTlaRX4UZDGqbiv+2V2X/8TQ7reNITF+BgPTgn9YuyXu9yPdrh4fboFgWaOb0tnvAToVkLbR8350lrTw050tfcZnRBwpgnKD4HkIxQOHPCg3DkiXVl5XHhluQbWmQDbyx2Djk7KPqsngvLWUSe91g2cytb+GX0Su4c0bxwRGj95LMV2Zqov+6zHidJbAvwK3E2aa1XDkXgJLgrdOc+pyJ+w0r3+f2bLovJPp7t1Bp6aPbM3CuSAUDpgcxTo6kRWvSYnTNs7huwVXiE5D7m9QUvaycIZj7sdxmBA1Ue+Fc7u/XuJAsUz8VO/WO0K/DGl9Wr8K0gPeD6e4GPGDZsqUhiDPVtRU5kcA5Or3opkKXUd4T8UCki9vo1rqWkK0lRJkiCcEC5DeieRYPPQmEfAvgrS4ebOfkI+tSW6dzFC9yKNNrSxNZfxJ/Iz4pJ+90Z/mlEltYGDwKFrQJoUlxmvUgVBLSMaVsXQCIRaFsbipqNZoJ0e8HJCrvlN3ANBWPUl3LKgWrHACLWrQBzD1lm3bzouP3/ST6PCu14eKIaxk7eBLRvjvK0OjU89Fn9h5Vmkq7v4C3BEcdeo/ymt22AL5nqzBZVc0dB3BAIB9TiuH+tYfR3WZn25Ab7fHSnOVQwexqNnW3W76D8AK10WMRCDidE8AIhf0A4NUYqP7pqQCr1GStkxVAXr3gyX5p3I3c8z+H/gZCDa/l3R2nbMEKKY5Z+GrwI6qp1bFOEM38rwf1ToOsPBZ8IOUWkmDQNYjulKkFmh/9lWD2La1Td71BA3V7RVxZiNSTzFFxRj6xgBXzu+PaEeIKtgRBW0iuVhd5MY9CXbuWGSvomQnyNqTFk3Dp4E1BGaNgIANbghFaOClOUD8vKz6myUrT46ZbBJ3mGwz1q8q6RnEDO6PeUeDTfowj3qzJiNEO4hzJJkZ4mhl9cg NzwLJTI3 FHVK6ifXjbFNj67cm4IBlGtFZ4EH2yoorscGSOC1d9SMMMfBNh+xK6pke2/gxkC0W2dYRuOlolyXVze1voyV54sytj+SdccDMD+QFOStGFowT7UUA+RMNFFKx82odffauqjAXNXCPnT58rF/Fdmf9w8oxmmtIcdREgX9jGyWm4biHvhvmIVh0bJAWXt7Di6e/C0mOZOJ+530i4W8XGWNyG7dqLBx7B16g15T8W5EaUmJavoFL2LI4W6NTx/ggYMqaV1bdHz+FzkZHa0NW4ri9E6pT2whooSb0aCS2Ie44YYGCYNOsmWqBbGMGd3zRWCt32Db9x9pTc7DX5LzduSfONLeG2nc4XztPUCXvzHjRUFiC6iNm2KUOfd/ZeZoguAf0FF9S5BPU6OowZCa0XM5mMnyHIlaUyOfFl4PqmDWYqQY+iGxXFhLxlsuIGg== 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 04:56:49PM +0800, Baoquan He wrote: > > 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. All of this is fine, the caller is expected to provide a zeroed array or an array that contains existing elements. We only need to use it with a zeroed array. We zero the array. Problem solved. Other users use the 'already allocated pages' functionality, we don't care. > > > > > 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. Well, output implies output i.e. writing to something. If you also read it, it's not just an output parameter is it? I don't really want to get into semantics here, the point is the test's expectation is that it'd be write-only and it's not so we have to zero the array, that's it. > > 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(); > As I said I literally state in multiple places this is about needing to initialise the array:- lib/test_vmalloc.c: avoid garbage in page array ... This is somewhat unexpected and breaks this test, as we allocate the pages array uninitialised on the assumption it will be overwritten. ... We solve both problems by simply using kcalloc() and referencing sizeof(struct page *) rather than sizeof(struct page). So I completely disagree we need to add anything more. > This could happen in other place if they don't use kcalloc(), > kmalloc(GFP_ZERO) or something like this to allocate page_array[]? We don't care? I'm fixing a test here not auditing __alloc_bulk_array(). > > 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 > That's just irrelevant to this change. You're also not replying to my point here (that we're clearly not running this test very much). I find this review super bikesheddy. Let's try not to bog things down in lengthily discussions when literally all I am doing here is:- 1. Function expects a zeroed array 2. Change the code to zero the array 3. Change the array to be smaller since it only needs to store pointers It's a two line change, can we try to be a little proportionate here? You're not even giving me a tag because... I'm not auditing all uses of __alloc_bulk_array() or something? Seriously.