Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753465AbdDMXnK (ORCPT ); Thu, 13 Apr 2017 19:43:10 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:42596 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753296AbdDMXnI (ORCPT ); Thu, 13 Apr 2017 19:43:08 -0400 Subject: Re: [PATCH] ARM: dma-mapping: add check for coherent DMA memory without struct page To: Russell King - ARM Linux References: <20170413214756.21265-1-shuahkh@osg.samsung.com> <20170413221055.GE17774@n2100.armlinux.org.uk> Cc: gregkh@linuxfoundation.org, jroedel@suse.de, will.deacon@arm.com, Robin.Murphy@arm.com, vgupta@synopsys.com, m.szyprowski@samsung.com, bart.vanassche@sandisk.com, benjamin.gaignard@linaro.org, krzk@kernel.org, niklas.soderlund+renesas@ragnatech.se, sricharan@codeaurora.org, vinod.koul@intel.com, akpm@linux-foundation.org, dledford@redhat.com, alexander.h.duyck@intel.com, mauricfo@linux.vnet.ibm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Shuah Khan From: Shuah Khan Message-ID: <10ccf249-8f2e-a6b3-2536-b78b417300a9@osg.samsung.com> Date: Thu, 13 Apr 2017 17:42:56 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170413221055.GE17774@n2100.armlinux.org.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3154 Lines: 76 On 04/13/2017 04:10 PM, Russell King - ARM Linux wrote: > On Thu, Apr 13, 2017 at 03:47:56PM -0600, Shuah Khan wrote: >> When coherent DMA memory without struct page is shared, importer >> fails to find the page and runs into kernel page fault when it >> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found >> in the sg_table. >> >> Add a new dma_check_dev_coherent() interface to check if memory is >> from the device coherent area. There is no way to tell where the >> memory returned by dma_alloc_attrs() came from. >> >> arm_dma_get_sgtable() checks for invalid pages, however this check >> could pass even for memory obtained the coherent allocator. Add an >> additional check to call dma_check_dev_coherent() to confirm that it >> is indeed the coherent DMA memory and fail the sgtable creation with >> -EINVAL. > > Sorry, this doesn't make much sense to me. > > pfn_valid(pfn) must *never* return true if 'pfn' does not have a struct > page associated with it. If it returns true (so we allow > arm_dma_get_sgtable() to succeed) then we know we have a valid struct > page in the supplied scatterlist. > >> Signed-off-by: Shuah Khan >> --- >> arch/arm/mm/dma-mapping.c | 11 ++++++++--- >> drivers/base/dma-coherent.c | 25 +++++++++++++++++++++++++ >> include/linux/dma-mapping.h | 2 ++ >> 3 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c >> index 475811f..27c7d9a 100644 >> --- a/arch/arm/mm/dma-mapping.c >> +++ b/arch/arm/mm/dma-mapping.c >> @@ -954,9 +954,14 @@ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt, >> struct page *page; >> int ret; >> >> - /* If the PFN is not valid, we do not have a struct page */ >> - if (!pfn_valid(pfn)) >> - return -ENXIO; >> + /* >> + * If the PFN is not valid, we do not have a struct page >> + * As this check can pass even for memory obtained through >> + * the coherent allocator, do an additional check to determine >> + * if this is coherent DMA memory. >> + */ >> + if (!pfn_valid(pfn) && dma_check_dev_coherent(dev, handle, cpu_addr)) >> + return -EINVAL; > > Right, so what this says is: > > if we do not haev a valid PFN > _and_ if the memory is from the coherent section > _then_ fail > > Why the extra check? Under what circunstances do we end up with memory > where the PFN is valid, but we do not have a valid struct page. It > seems to me that such a scenario is a bug in pfn_valid() and not > something that should be worked around like this. > DMA_ATTR_NO_KERNEL_MAPPING case is the one I am concerned about. pfn_valid() would fail on this if I am understanding it correctly. A few drm drivers set this attr and use sg_table for passing buffers. My reasoning behind adding this check is to not have this fail on NO_KERNEL_MAPPING cases. So I thought adding a restrictive check for just the per-device memory would help. However, I don't have a good understanding of the drm case, hence I could be trying to address a case that doesn't need to be addressed. In any case, thanks for your patience and a quick reply. -- Shuah