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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 143F4C001E0 for ; Thu, 10 Aug 2023 11:06:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=OYPIgTg6pclSUno6B5IMFsqExKUS4RQYXdFqaDemSXE=; b=uOi2NcybWQ9BlR F3CU8o1Dk/8bP3MHsWuGpZWkMXokMBULVfiV2DTp7uQfSubu4azmvHeJhkDZfsltkn2CBU47o6vI1 L2Owt2KFDaLddA3m1OxcqdbJdbRj3Z2y9YrhUD7SV7P2UvlwSMORKauYG2ZJXjuxVGrG8tyyXJb+L 2K+IuGQYMxM0sqWv5RV7PZPnefyznYcn4Ed6Xyx8xo9XfbQH7Q8AXTB21gsBcgIEiunxB0Ba/pRyg 8wMzde3EcSzNiEjLBccCl0HIWx8MYtjsVhe8/h3LacvrJPL7CN1SMV7kEwet5VPHnZxc2kiI7axxQ L8RctPcZhyp733xa7TpQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qU3Uj-007KsJ-19; Thu, 10 Aug 2023 11:06:21 +0000 Received: from [2001:4d48:ad52:32c8:5054:ff:fe00:142] (helo=pandora.armlinux.org.uk) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qU3Ug-007Kr0-17 for linux-arm-kernel@lists.infradead.org; Thu, 10 Aug 2023 11:06:20 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=9NPriZJP01HOjRIFXLEdP+y7thwGzVL4AEnxIoVS/i8=; b=oEk2AnhOgVr1nINeV3I1FfSFTX Cd9RR2mbpi0kQF+ztEq2tX3qKvAG1wZoGwQbWlbeTjYGKyBW5dkLM3zCB4DNpJLlV/JCIaZRKkqrJ FhY7AcW2ph+kKi5EGKIUixD77nF+uv6COQlTwqj2JrDcC9gH9n0uLgkg1nkThN7/AvWC71gf5M2GI FQgeMyc3ewmxS6kY5CdDre70h+haoXR0Q2zk6xgeuVrlhs2A0M9evndNLkPR7cb3QKQj1oFiksM4B awO/2YVYp0eYOTwhAPXP4V3A1iFpllph420Qge6UVqa9ohrFsUJ2qlCg8CIDl+oPLgLBiZMQBwuhV m9cDTn7w==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:38534) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1qU3Ua-0003rc-1C; Thu, 10 Aug 2023 12:06:12 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1qU3UX-0001it-Nr; Thu, 10 Aug 2023 12:06:09 +0100 Date: Thu, 10 Aug 2023 12:06:09 +0100 From: "Russell King (Oracle)" To: Marek Szyprowski Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Matthew Wilcox (Oracle)" , Andrew Morton , Mike Rapoport Subject: Re: [PATCH] arm: dma-mapping: don't call folio_next() beyond the requested region Message-ID: References: <20230810091955.3579004-1-m.szyprowski@samsung.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230810091955.3579004-1-m.szyprowski@samsung.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230810_040618_389032_3290A228 X-CRM114-Status: GOOD ( 31.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Aug 10, 2023 at 11:19:55AM +0200, Marek Szyprowski wrote: > Add a check for the non-zero offset case to avoid calling folio_next() > beyond the requested region and relying on its parameters. > > Fixes: cc24e9c0895c ("arm: implement the new page table range API") > Signed-off-by: Marek Szyprowski > Suggested-by: Matthew Wilcox > --- > arch/arm/mm/dma-mapping.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 0474840224d9..6c952d6899f2 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -715,6 +715,8 @@ static void __dma_page_dev_to_cpu(struct page *page, unsigned long off, > > if (offset) { > left -= folio_size(folio) - offset; In most cases, "offset" is masked by ~PAGE_MASK, the only exception being when we're walking scatterlists, where it is whatever is in sg->offset. So, what is the range of values that sg->offset could take? If it is guaranteed to be less than PAGE_SIZE (or folio_size() of the first folio) then we're all good. However, consider what happens with the above when offset is larger than the first folio size. To show this, let's rewrite it: left += offset - folio_size(folio); In that case, "left" becomes larger than the original size, which surely is not what we want? This wasn't a problem with the original code, because we guaranteed that "off" was always less than PAGE_SIZE, so left -= PAGE_SIZE - off; would always result in a reduction, but this is no longer the case with folios. The more I'm looking at this, the more I'm convinced that the original conversion is wrong. Let's go back to the original code and see what it is doing: size_t left = size; pfn = page_to_pfn(page) + off / PAGE_SIZE; This positions pfn to be the page frame number to which the page and offset passed into the function reference. off %= PAGE_SIZE; This gives us the offset _within_ that page. if (off) { pfn++; left -= PAGE_SIZE - off; } What this is doing is saying if the first page is a partial page, then we skip marking it clean - only _full_ pages get marked clean. while (left >= PAGE_SIZE) { page = pfn_to_page(pfn++); set_bit(PG_dcache_clean, &page->flags); left -= PAGE_SIZE; } There, we iterate over the size for the number of _whole_ pages only and not a final partial page. Now, if we consider the folio version: + ssize_t left = size; Casts an unsigned to a signed, which will not give expected results if large. + size_t offset = offset_in_folio(folio, paddr); paddr here is the physical address of the page plus the passed in offset. If the offset is larger than a page, then it will be the following pages. What if the offset is larger than the first folio size - bearing in mind that there is no limit on the offset in a scatterlist? If offset is bounded to the size of the folio, then that can truncate the original "offset" that was passed in and we'll end up marking the wrong folios, because: + + if (offset) { + left -= folio_size(folio) - offset; + folio = folio_next(folio); } This only allows us to move to the next folio. Moreover, if offset here _is_ allowed to be bigger than folio_size() then we end up _increasing_ "left" as stated above, so we end up marking _more_ folios as clean than the user of this function requested. + + while (left >= (ssize_t)folio_size(folio)) { + set_bit(PG_dcache_clean, &folio->flags); + left -= folio_size(folio); + folio = folio_next(folio); } So, in all, to me it looks like this conversion is basically wrong, and it needs to be something like: size_t left = size; while (off >= folio_size(folio)) { off -= folio_size(folio); folio = folio_next(folio); } if (off) { /* Partial first folio */ size_t first = folio_size(folio) - off; /* Size doesn't extend the full folio size, so exit */ if (left < first) return; /* Truncate the size and move to the next folio */ left -= first; folio = folio_next(folio); } while (left >= folio_size(folio)) { /* can't become negative */ left -= folio_size(folio); set_bit(PG_dcache_clean, &folio->flags); if (!left) break; folio = folio_next(folio); } -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel