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 C8E04C77B7C for ; Sat, 27 May 2023 10:11:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2A8D96B0071; Sat, 27 May 2023 06:11:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2579A6B0072; Sat, 27 May 2023 06:11:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 11F62900002; Sat, 27 May 2023 06:11:54 -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 034416B0071 for ; Sat, 27 May 2023 06:11:54 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id B9419C01F2 for ; Sat, 27 May 2023 10:11:53 +0000 (UTC) X-FDA: 80835618906.06.82B650B Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf24.hostedemail.com (Postfix) with ESMTP id D31EF180011 for ; Sat, 27 May 2023 10:11:51 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=T49+uk2U; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf24.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.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=1685182311; 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=F/CNOzVzf+mDbz4pPw1e7KenPWxmonVbCLBoJp3uezg=; b=DNaC5xMEUXw/GwXOlR+zBNMt4TlqwgrkkQc/K7P5sNpfJiRohJahmEaFMfOaPLTK3DNR2H Gg7eXiHAuGfJ2yBkJYjzCXp48yoxXjjCV18K59HwJCDIAeyzwSTQR9W5gKDBffnls+YU/L /YX8yUFNAqL+yfMApBhqFhUcFo5BQsM= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=T49+uk2U; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf24.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1685182311; a=rsa-sha256; cv=none; b=I8JXc5PHulRiL6hNlTSodA5Z8kzM3/GqJ5KRq9AGO4c7rZipPS/Q9ex780y9sd64uvNXR1 VZKJqBDDFeVkf4zFO1NZ3xnevtsG+KBrqb5c6RAPwbP0pcIvUe1Pl1LYExOikXqN2kQu05 z6CGmQtWNcHcEOZGMZmnN/NztoKg+oI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685182311; 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=F/CNOzVzf+mDbz4pPw1e7KenPWxmonVbCLBoJp3uezg=; b=T49+uk2UAEDimP3Bm4eFWZ7H5AUYkdzyXaqdfKBBtMvhDkF6td69yt5vW3W8jHk4gdAGG5 zGPMSKn5ZaBBp2Jtlc9U6eT3iXgwCr9OvmmtjQ+leJjTtATbDA7WB88Eqz5ZMb9NhT7+1y 4Vwt0G8Ug3haFcGf9c9+hjlPiqH+br0= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-626-BkoVgVm-NWiDAV7G7SpEBg-1; Sat, 27 May 2023 06:11:34 -0400 X-MC-Unique: BkoVgVm-NWiDAV7G7SpEBg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3560D38117F0; Sat, 27 May 2023 10:11:34 +0000 (UTC) Received: from localhost (ovpn-12-70.pek2.redhat.com [10.72.12.70]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 45376140E954; Sat, 27 May 2023 10:11:32 +0000 (UTC) Date: Sat, 27 May 2023 18:11:29 +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> <368bbc1d-d810-4bc4-8091-7ed55631344f@lucifer.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <368bbc1d-d810-4bc4-8091-7ed55631344f@lucifer.local> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: D31EF180011 X-Stat-Signature: 6mqz65rkcj5u9fx8zmds8ca8g1u57gbe X-HE-Tag: 1685182311-443153 X-HE-Meta: U2FsdGVkX1+sNIOyns3xJjNZJmNK8sc5d5BGfG1hfhClQDGz19ETEpC3HpF/YyQWYF2sb2C/Uv8fusDhanyIICXjCPdAMwzXu+uBZTqzCic4yus3uHze/hPd9ZVn3k4wC+Z9hlyJcGieXx2RfLSHcWt2zEdbHVm3pJjKz11G4SWpQ46YLtbEJlp1L7Mv54NaE/1a2AHgzY+qh5yocFTGj5OYo3/ukw2IU5xrQJezH6TvZDrc/kwYl3e7rj2UrLzkN6jEVat8o7jo8sqJxV5Is6NUIDVFQy10c8h3k07gnoFzqzFW/z8ywXW9rGex34FPMBJZvtUYdySt6/AqXLRiXQYD5ViFKLJNNXOcIIGDn8Glr+TlZ/9P0vhFsnW32MfKWM/Fni7bp6lvfZ3swuHPXLZrvk+OnsDTDldRew7Bv8HCRC0NnQCkEyVntNgar4hF+33HM3OJz7EjUiLk+l6lm+UVshaTNo1ccngd9rGFl52jEgD+tKaWCu/t2NE4XOJkx1rADxRkMWdqjE+z8flhB80idcPyLt66Ci3eHk2vwX03Nt7MBb9+VZzsrv0mEnVOfuxdnkC5LW3gVEM2WEpjJNrkz5lg1GHmt8Aj0aCwXsPg0A5vzJ+/TRe7G9R9swNM6u1x5CIzOQQc1ElQ9zdNcQPDbI9pkqG7OsYvwR6+09e/ZjXURQGAsUuCO+m3ssCO6vUrdL+adHPLKWPXtLc/xL0927MkE8XXpXH4dkR/6ePOvJW1oXIFQFxXDeup9sF27st1gaCnbq2DPFRrx34oklAae9VM63updqm2ohPDlceRyPAPtgFVZ6QptkLzQe6CvN4jZSpWBcHOu0hljw/WFyCcY8c97y634jGv7CPSQ0idJcpu7Gi9BMmOdubglXIzhrnZzWfrte0W+XZitcB781KSHBHYndv2n72sRlvdDz91qVPMEtHHHsAIuqrmc6co/xRzrvma6mM6g6oN0Ty WvvGdhtu DPLeG9jjTPOoXXFWKSyfAm0Nw7Ht/BezLDpzKenWJoEZSWfyecCQk8q/YUrClesrD4ihSG6Or+GqRjajZbM5q6ut3h6/gJ0bvbvXQaqTXLiEoUmzEpgq5EimVXkKRxEOIRIWb2762ifNyzvwG5yP6biEOOOyrh3NGl5vSPRVsy44Zdh9drO5MH5zV6QplORkIlgo/df69V2xpXr3aMVIW3US4poPyOLbqi+QMYMA4oHJ+HoPjofkG4Uh6lA== 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 10:10am, Lorenzo Stoakes wrote: > 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? Yes, I digressed from the patch itself. At the beginning, I truly didn't understand why __alloc_bulk_array() would break the test. Sorry for wasting your time. While I have to say this is a great fix. As you said, people usually don't run this test much. I guess maintainer like Uladzislau or you could run the test to see if the whole code is working well. However, correct testing code is also very important. If people read testing code, she/he will know how the interface is used. Sometime if I am not sure about some code writing, as a vim+ctags+cscope user, I will git grep or use cscope to search, the testing code is also a good example to refer to. > > You're not even giving me a tag because... I'm not auditing all uses of > __alloc_bulk_array() or something? Seriously. I am usually passive when adding a tag as long as the patch has been acked by other people, or have been picked up by maintainer. Especially in MM and kdump I maintained, if Andrew has taken the patch, I will not add tag because I don't want to take up his time to add that tag. Doing this because Andrew has been helping to merge kexec/kdump patches, and he creates a very open and harmonious environment for us to study code and review patch. I always suggest any newcomer to start from MM and post patch to MM because people from maintainers to reviewers are very friendly and very proactive. But now I realize being passive on adidng tag is not good. People could misunderstand I oppose or still have concern. From now on, I will add tag as long as I agree on the patch, then expand the discussion if people would like to.