[PATCH 5.15 1/2] dma-mapping: remove bogus test for pfn_valid from dma_map_resource

[PATCH 5.15 1/2] dma-mapping: remove bogus test for pfn_valid from dma_map_resource

[Georgi Djakov]

From: Mike Rapoport <rppt@linux.ibm.com>

[ Upstream commit a9c38c5d267cb94871dfa2de5539c92025c855d7 ]

dma_map_resource() uses pfn_valid() to ensure the range is not RAM.
However, pfn_valid() only checks for availability of the memory map for a
PFN but it does not ensure that the PFN is actually backed by RAM.

As dma_map_resource() is the only method in DMA mapping APIs that has this
check, simply drop the pfn_valid() test from dma_map_resource().

Link: https://lore.kernel.org/all/20210824173741.GC623@arm.com/
Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: David Hildenbrand <david@redhat.com>
Link: https://lore.kernel.org/r/20210930013039.11260-2-rppt@kernel.org
Signed-off-by: Will Deacon <will@kernel.org>
Fixes: 859a85ddf90e ("mm: remove pfn_valid_within() and CONFIG_HOLES_IN_ZONE")
Link: https://lore.kernel.org/r/Yl0IZWT2nsiYtqBT@linux.ibm.com
Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
---
 kernel/dma/mapping.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 8349a9f2c345..9478eccd1c8e 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -296,10 +296,6 @@ dma_addr_t dma_map_resource(struct device *dev, phys_addr_t phys_addr,
 	if (WARN_ON_ONCE(!dev->dma_mask))
 		return DMA_MAPPING_ERROR;
 
-	/* Don't allow RAM to be mapped */
-	if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
-		return DMA_MAPPING_ERROR;
-
 	if (dma_map_direct(dev, ops))
 		addr = dma_direct_map_resource(dev, phys_addr, size, dir, attrs);
 	else if (ops->map_resource)

[PATCH 5.15 2/2] arm64/mm: drop HAVE_ARCH_PFN_VALID

[Georgi Djakov]

From: Anshuman Khandual <anshuman.khandual@arm.com>

[ Upstream commit 3de360c3fdb34fbdbaf6da3af94367d3fded95d3 ]

CONFIG_SPARSEMEM_VMEMMAP is now the only available memory model on arm64
platforms and free_unused_memmap() would just return without creating any
holes in the memmap mapping.  There is no need for any special handling in
pfn_valid() and HAVE_ARCH_PFN_VALID can just be dropped.  This also moves
the pfn upper bits sanity check into generic pfn_valid().

[rppt: rebased on v5.15-rc3]

Link: https://lkml.kernel.org/r/1621947349-25421-1-git-send-email-anshuman.khandual@arm.com
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Acked-by: David Hildenbrand <david@redhat.com>
Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Mike Rapoport <rppt@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
Link: https://lore.kernel.org/r/20210930013039.11260-3-rppt@kernel.org
Signed-off-by: Will Deacon <will@kernel.org>
Fixes: 859a85ddf90e ("mm: remove pfn_valid_within() and CONFIG_HOLES_IN_ZONE")
Link: https://lore.kernel.org/r/Yl0IZWT2nsiYtqBT@linux.ibm.com
Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
---
 arch/arm64/Kconfig            |  1 -
 arch/arm64/include/asm/page.h |  1 -
 arch/arm64/mm/init.c          | 37 -----------------------------------
 3 files changed, 39 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1a18c9045773..8b6f090e0364 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -154,7 +154,6 @@ config ARM64
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_MMAP_RND_BITS
 	select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
-	select HAVE_ARCH_PFN_VALID
 	select HAVE_ARCH_PREL32_RELOCATIONS
 	select HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET
 	select HAVE_ARCH_SECCOMP_FILTER
diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h
index f98c91bbd7c1..993a27ea6f54 100644
--- a/arch/arm64/include/asm/page.h
+++ b/arch/arm64/include/asm/page.h
@@ -41,7 +41,6 @@ void tag_clear_highpage(struct page *to);
 
 typedef struct page *pgtable_t;
 
-int pfn_valid(unsigned long pfn);
 int pfn_is_map_memory(unsigned long pfn);
 
 #include <asm/memory.h>
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index c59cb2efd554..3b269c756798 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -184,43 +184,6 @@ static void __init zone_sizes_init(unsigned long min, unsigned long max)
 	free_area_init(max_zone_pfns);
 }
 
-int pfn_valid(unsigned long pfn)
-{
-	phys_addr_t addr = PFN_PHYS(pfn);
-	struct mem_section *ms;
-
-	/*
-	 * Ensure the upper PAGE_SHIFT bits are clear in the
-	 * pfn. Else it might lead to false positives when
-	 * some of the upper bits are set, but the lower bits
-	 * match a valid pfn.
-	 */
-	if (PHYS_PFN(addr) != pfn)
-		return 0;
-
-	if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
-		return 0;
-
-	ms = __pfn_to_section(pfn);
-	if (!valid_section(ms))
-		return 0;
-
-	/*
-	 * ZONE_DEVICE memory does not have the memblock entries.
-	 * memblock_is_map_memory() check for ZONE_DEVICE based
-	 * addresses will always fail. Even the normal hotplugged
-	 * memory will never have MEMBLOCK_NOMAP flag set in their
-	 * memblock entries. Skip memblock search for all non early
-	 * memory sections covering all of hotplug memory including
-	 * both normal and ZONE_DEVICE based.
-	 */
-	if (!early_section(ms))
-		return pfn_section_valid(ms, pfn);
-
-	return memblock_is_memory(addr);
-}
-EXPORT_SYMBOL(pfn_valid);
-
 int pfn_is_map_memory(unsigned long pfn)
 {
 	phys_addr_t addr = PFN_PHYS(pfn);

Re: [PATCH 5.15 2/2] arm64/mm: drop HAVE_ARCH_PFN_VALID

[Sasha Levin]

On Wed, Apr 20, 2022 at 05:43:41AM -0700, Georgi Djakov wrote:
>From: Anshuman Khandual <anshuman.khandual@arm.com>
>
>[ Upstream commit 3de360c3fdb34fbdbaf6da3af94367d3fded95d3 ]
>
>CONFIG_SPARSEMEM_VMEMMAP is now the only available memory model on arm64
>platforms and free_unused_memmap() would just return without creating any
>holes in the memmap mapping.  There is no need for any special handling in
>pfn_valid() and HAVE_ARCH_PFN_VALID can just be dropped.  This also moves
>the pfn upper bits sanity check into generic pfn_valid().

It's not clear why we need this patch in stable.

-- 
Thanks,
Sasha

Re: [PATCH 5.15 2/2] arm64/mm: drop HAVE_ARCH_PFN_VALID

[Georgi Djakov]

Hi Sasha,

On 20.04.22 17:49, Sasha Levin wrote:
> On Wed, Apr 20, 2022 at 05:43:41AM -0700, Georgi Djakov wrote:
>> From: Anshuman Khandual <anshuman.khandual@arm.com>
>>
>> [ Upstream commit 3de360c3fdb34fbdbaf6da3af94367d3fded95d3 ]
>>
>> CONFIG_SPARSEMEM_VMEMMAP is now the only available memory model on arm64
>> platforms and free_unused_memmap() would just return without creating any
>> holes in the memmap mapping.  There is no need for any special handling in
>> pfn_valid() and HAVE_ARCH_PFN_VALID can just be dropped.  This also moves
>> the pfn upper bits sanity check into generic pfn_valid().
> 
> It's not clear why we need this patch in stable.
> 

I added a Link: tag, pointing to the discussion where the problem is explained
in details and the suggestion to backport these patches to 5.15. But probably
this is not enough and i should have mentioned it in the commit message too.

So in short:
This is fixing a crash on systems where the RAM base address is not aligned
to the pageblock size. Only v5.15 is affected.

 > Fixes: 859a85ddf90e ("mm: remove pfn_valid_within() and CONFIG_HOLES_IN_ZONE")
 > Link: https://lore.kernel.org/r/Yl0IZWT2nsiYtqBT@linux.ibm.com

Thanks,
Georgi

Re: [PATCH 5.15 1/2] dma-mapping: remove bogus test for pfn_valid from dma_map_resource

[Greg KH]

On Wed, Apr 20, 2022 at 05:43:40AM -0700, Georgi Djakov wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> [ Upstream commit a9c38c5d267cb94871dfa2de5539c92025c855d7 ]
> 
> dma_map_resource() uses pfn_valid() to ensure the range is not RAM.
> However, pfn_valid() only checks for availability of the memory map for a
> PFN but it does not ensure that the PFN is actually backed by RAM.
> 
> As dma_map_resource() is the only method in DMA mapping APIs that has this
> check, simply drop the pfn_valid() test from dma_map_resource().
> 
> Link: https://lore.kernel.org/all/20210824173741.GC623@arm.com/
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Acked-by: David Hildenbrand <david@redhat.com>
> Link: https://lore.kernel.org/r/20210930013039.11260-2-rppt@kernel.org
> Signed-off-by: Will Deacon <will@kernel.org>
> Fixes: 859a85ddf90e ("mm: remove pfn_valid_within() and CONFIG_HOLES_IN_ZONE")
> Link: https://lore.kernel.org/r/Yl0IZWT2nsiYtqBT@linux.ibm.com
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> ---
>  kernel/dma/mapping.c | 4 ----
>  1 file changed, 4 deletions(-)

I took this, but I do not understand why patch 2/2 in this series is
needed, as Sasha points out.  Cleanups are nice, but is it necessary
here?

thanks,

greg k-h

Re: [PATCH 5.15 1/2] dma-mapping: remove bogus test for pfn_valid from dma_map_resource

[Georgi Djakov]

On 21.04.22 9:43, Greg KH wrote:
> On Wed, Apr 20, 2022 at 05:43:40AM -0700, Georgi Djakov wrote:
>> From: Mike Rapoport <rppt@linux.ibm.com>
>>
>> [ Upstream commit a9c38c5d267cb94871dfa2de5539c92025c855d7 ]
>>
>> dma_map_resource() uses pfn_valid() to ensure the range is not RAM.
>> However, pfn_valid() only checks for availability of the memory map for a
>> PFN but it does not ensure that the PFN is actually backed by RAM.
>>
>> As dma_map_resource() is the only method in DMA mapping APIs that has this
>> check, simply drop the pfn_valid() test from dma_map_resource().
>>
>> Link: https://lore.kernel.org/all/20210824173741.GC623@arm.com/
>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Link: https://lore.kernel.org/r/20210930013039.11260-2-rppt@kernel.org
>> Signed-off-by: Will Deacon <will@kernel.org>
>> Fixes: 859a85ddf90e ("mm: remove pfn_valid_within() and CONFIG_HOLES_IN_ZONE")
>> Link: https://lore.kernel.org/r/Yl0IZWT2nsiYtqBT@linux.ibm.com
>> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
>> ---
>>   kernel/dma/mapping.c | 4 ----
>>   1 file changed, 4 deletions(-)
> 
> I took this, but I do not understand why patch 2/2 in this series is
> needed, as Sasha points out.  Cleanups are nice, but is it necessary
> here?

It's needed as it removes the "select HAVE_ARCH_PFN_VALID" from the arm64/Kconfig.
This will make us use the generic pfn_valid() function in mmzone.h, instead of the
arch-specific one, that we are dropping.

Thanks,
Georgi

Re: [PATCH 5.15 1/2] dma-mapping: remove bogus test for pfn_valid from dma_map_resource

[Greg KH]

On Thu, Apr 21, 2022 at 10:51:39AM +0300, Georgi Djakov wrote:
> 
> On 21.04.22 9:43, Greg KH wrote:
> > On Wed, Apr 20, 2022 at 05:43:40AM -0700, Georgi Djakov wrote:
> > > From: Mike Rapoport <rppt@linux.ibm.com>
> > > 
> > > [ Upstream commit a9c38c5d267cb94871dfa2de5539c92025c855d7 ]
> > > 
> > > dma_map_resource() uses pfn_valid() to ensure the range is not RAM.
> > > However, pfn_valid() only checks for availability of the memory map for a
> > > PFN but it does not ensure that the PFN is actually backed by RAM.
> > > 
> > > As dma_map_resource() is the only method in DMA mapping APIs that has this
> > > check, simply drop the pfn_valid() test from dma_map_resource().
> > > 
> > > Link: https://lore.kernel.org/all/20210824173741.GC623@arm.com/
> > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > Acked-by: David Hildenbrand <david@redhat.com>
> > > Link: https://lore.kernel.org/r/20210930013039.11260-2-rppt@kernel.org
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > Fixes: 859a85ddf90e ("mm: remove pfn_valid_within() and CONFIG_HOLES_IN_ZONE")
> > > Link: https://lore.kernel.org/r/Yl0IZWT2nsiYtqBT@linux.ibm.com
> > > Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> > > ---
> > >   kernel/dma/mapping.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > 
> > I took this, but I do not understand why patch 2/2 in this series is
> > needed, as Sasha points out.  Cleanups are nice, but is it necessary
> > here?
> 
> It's needed as it removes the "select HAVE_ARCH_PFN_VALID" from the arm64/Kconfig.
> This will make us use the generic pfn_valid() function in mmzone.h, instead of the
> arch-specific one, that we are dropping.

Ah, that is not obvious at all.  Ok, I'll queue this up, but you should
make sure that this doesn't break anything on your systems as I thought
they required this to be a function for some reason...

thanks,

greg k-h