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 X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7C389C2BA19 for ; Tue, 14 Apr 2020 13:28:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 60FFB2075E for ; Tue, 14 Apr 2020 13:28:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732019AbgDNN2P (ORCPT ); Tue, 14 Apr 2020 09:28:15 -0400 Received: from foss.arm.com ([217.140.110.172]:55996 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732002AbgDNN2B (ORCPT ); Tue, 14 Apr 2020 09:28:01 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 829A930E; Tue, 14 Apr 2020 06:27:55 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.30.4]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 467793F73D; Tue, 14 Apr 2020 06:27:54 -0700 (PDT) Date: Tue, 14 Apr 2020 14:27:51 +0100 From: Mark Rutland To: Vincenzo Frascino Cc: linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com, will@kernel.org, stable@vger.kernel.org Subject: Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages Message-ID: <20200414132751.GF2486@C02TD0UTHF1T.local> References: <20200414104252.16061-1-mark.rutland@arm.com> <20200414104252.16061-2-mark.rutland@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote: > Hi Mark, > > On 4/14/20 11:42 AM, Mark Rutland wrote: > > The aarch32_vdso_pages[] array never has entries allocated in the C_VVAR > > or C_VDSO slots, and as the array is zero initialized these contain > > NULL. > > > > However in __aarch32_alloc_vdso_pages() when > > aarch32_alloc_kuser_vdso_page() fails we attempt to free the page whose > > struct page is at NULL, which is obviously nonsensical. > > Could you please explain why do you think that free(NULL) is "nonsensical"? Regardless of the below, can you please explain why it is sensical? I'm struggling to follow your argument here. * It serves no legitimate purpose. One cannot free a page without a corresponding struct page. * It is redundant. Removing the code does not detract from the utility of the remainging code, or make that remaing code more complex. * It hinders comprehension of the code. When a developer sees the free_page() they will assume that the page was allocated somewhere, but there is no corresponding allocation as the pointers are never assigned to. Even if the code in question is not harmful to the functional correctness of the code, it is an unnecessary burden to developers. * page_to_virt(NULL) does not have a well-defined result, and page_to_virt() should only be called for a valid struct page pointer. The result of page_to_virt(NULL) may not be a pointer into the linear map as would be expected. * free_page(x) calls free_pages(x, 0), which checks virt_addr_valid(x). As page_to_virt(NULL) is not a valid linear map address, this can trigger a VM_BUG_ON() * Even if page_to_virt(virt_to_page(NULL)) is NULL, within __free_pages(NULL, 0) we'd call put_page_testzero(NULL) which will fault when dereferencing NULL to get at the fields in struct page. Likewise for everything under free_the_page(NULL, 0). > And if this is causing a bug (according to the cover-letter), could > you please provide a stack-trace? I haven't triggered it, as I found it by inspection. As above the behaviour is clearly erroneous and can trigger several failures to occur. Note that this only happens if aarch32_alloc_kuser_vdso_page() fails in the boot path, which is unlikely. Thanks, Mark. > > This patch removes the erroneous page freeing. > > > > Signed-off-by: Mark Rutland > > Cc: Catalin Marinas > > Cc: Vincenzo Frascino > > Cc: Will Deacon > > Cc: stable@vger.kernel.org > > --- > > arch/arm64/kernel/vdso.c | 13 +------------ > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c > > index 354b11e27c07..033a48f30dbb 100644 > > --- a/arch/arm64/kernel/vdso.c > > +++ b/arch/arm64/kernel/vdso.c > > @@ -260,18 +260,7 @@ static int __aarch32_alloc_vdso_pages(void) > > if (ret) > > return ret; > > > > - ret = aarch32_alloc_kuser_vdso_page(); > > - if (ret) { > > - unsigned long c_vvar = > > - (unsigned long)page_to_virt(aarch32_vdso_pages[C_VVAR]); > > - unsigned long c_vdso = > > - (unsigned long)page_to_virt(aarch32_vdso_pages[C_VDSO]); > > - > > - free_page(c_vvar); > > - free_page(c_vdso); > > - } > > - > > - return ret; > > + return aarch32_alloc_kuser_vdso_page(); > > } > > #else > > static int __aarch32_alloc_vdso_pages(void) > > > > -- > Regards, > Vincenzo 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 X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C755C2BA19 for ; Tue, 14 Apr 2020 13:28:04 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D85D32063A for ; Tue, 14 Apr 2020 13:28:03 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="L0wh5cBr" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D85D32063A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=FPBklYY8RIJzgoTONPYU7/2kVdFp3l4AF1O5ncou66g=; b=L0wh5cBre4+99J ch5PxNHMEk55/TNZakTeriCqnf/weaLH0jtSnJNMJCLBWaABsdx4TBZuGlrHkh5BU+3FdeifN3maq RkkBOcxN3mJhDHd/OFpoj6viDVb+8sanTDmlY7E45nzVy/OMFo21knx14Kh7t5nvirWP0jvTAj+JC bVCLOw2L3ipuFlhpB2NPwop6tVcP0PVGkuBFyz2tM6knLWJekcR+87mv8KCyHDkj9ZEceNnX1zKzO tCKRbGiLrctLwvrX6iTqJOgxJavPq7F+82dhL5D8BJqZmcpD5EZnD4YrwxDmTHgKJKR4YLAXVNr4q iLKIar7GybOWcBoMe+Zw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jOLbc-0001mh-8O; Tue, 14 Apr 2020 13:28:00 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jOLbZ-0001m6-29 for linux-arm-kernel@lists.infradead.org; Tue, 14 Apr 2020 13:27:58 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 829A930E; Tue, 14 Apr 2020 06:27:55 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.30.4]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 467793F73D; Tue, 14 Apr 2020 06:27:54 -0700 (PDT) Date: Tue, 14 Apr 2020 14:27:51 +0100 From: Mark Rutland To: Vincenzo Frascino Subject: Re: [PATCH 1/5] arm64: vdso: don't free unallocated pages Message-ID: <20200414132751.GF2486@C02TD0UTHF1T.local> References: <20200414104252.16061-1-mark.rutland@arm.com> <20200414104252.16061-2-mark.rutland@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200414_062757_497623_27E4EA25 X-CRM114-Status: GOOD ( 24.19 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: catalin.marinas@arm.com, will@kernel.org, stable@vger.kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote: > Hi Mark, > > On 4/14/20 11:42 AM, Mark Rutland wrote: > > The aarch32_vdso_pages[] array never has entries allocated in the C_VVAR > > or C_VDSO slots, and as the array is zero initialized these contain > > NULL. > > > > However in __aarch32_alloc_vdso_pages() when > > aarch32_alloc_kuser_vdso_page() fails we attempt to free the page whose > > struct page is at NULL, which is obviously nonsensical. > > Could you please explain why do you think that free(NULL) is "nonsensical"? Regardless of the below, can you please explain why it is sensical? I'm struggling to follow your argument here. * It serves no legitimate purpose. One cannot free a page without a corresponding struct page. * It is redundant. Removing the code does not detract from the utility of the remainging code, or make that remaing code more complex. * It hinders comprehension of the code. When a developer sees the free_page() they will assume that the page was allocated somewhere, but there is no corresponding allocation as the pointers are never assigned to. Even if the code in question is not harmful to the functional correctness of the code, it is an unnecessary burden to developers. * page_to_virt(NULL) does not have a well-defined result, and page_to_virt() should only be called for a valid struct page pointer. The result of page_to_virt(NULL) may not be a pointer into the linear map as would be expected. * free_page(x) calls free_pages(x, 0), which checks virt_addr_valid(x). As page_to_virt(NULL) is not a valid linear map address, this can trigger a VM_BUG_ON() * Even if page_to_virt(virt_to_page(NULL)) is NULL, within __free_pages(NULL, 0) we'd call put_page_testzero(NULL) which will fault when dereferencing NULL to get at the fields in struct page. Likewise for everything under free_the_page(NULL, 0). > And if this is causing a bug (according to the cover-letter), could > you please provide a stack-trace? I haven't triggered it, as I found it by inspection. As above the behaviour is clearly erroneous and can trigger several failures to occur. Note that this only happens if aarch32_alloc_kuser_vdso_page() fails in the boot path, which is unlikely. Thanks, Mark. > > This patch removes the erroneous page freeing. > > > > Signed-off-by: Mark Rutland > > Cc: Catalin Marinas > > Cc: Vincenzo Frascino > > Cc: Will Deacon > > Cc: stable@vger.kernel.org > > --- > > arch/arm64/kernel/vdso.c | 13 +------------ > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c > > index 354b11e27c07..033a48f30dbb 100644 > > --- a/arch/arm64/kernel/vdso.c > > +++ b/arch/arm64/kernel/vdso.c > > @@ -260,18 +260,7 @@ static int __aarch32_alloc_vdso_pages(void) > > if (ret) > > return ret; > > > > - ret = aarch32_alloc_kuser_vdso_page(); > > - if (ret) { > > - unsigned long c_vvar = > > - (unsigned long)page_to_virt(aarch32_vdso_pages[C_VVAR]); > > - unsigned long c_vdso = > > - (unsigned long)page_to_virt(aarch32_vdso_pages[C_VDSO]); > > - > > - free_page(c_vvar); > > - free_page(c_vdso); > > - } > > - > > - return ret; > > + return aarch32_alloc_kuser_vdso_page(); > > } > > #else > > static int __aarch32_alloc_vdso_pages(void) > > > > -- > Regards, > Vincenzo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel