From: Barry Song <v-songbaohua@oppo.com>

copy_page_from_iter_atomic() has the assumption lowmem will only
need one kmap to get start page_address() for all pages. This is
wrong if the debug option CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP is
enabled. This patch fixes it in the same way with skbuff.h by
always applying kmap one by one even for lowmem,

 static inline bool skb_frag_must_loop(struct page *p)
 {
 #if defined(CONFIG_HIGHMEM)
 	if (IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(p))
  		return true;
 #endif
 	return false;
 }

Reported-by: Herbert Xu <herbert@gondor.apana.org.au>
Closes: https://lore.kernel.org/all/ZeGtrvm9WGv5cxQV@gondor.apana.org.au/
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 -v1:
 Herbert found and pointed out this problem when he reviewed my patch
 in crypto/scompress.c;
 99.9% of the credit for this patch goes to Herbert.

 lib/iov_iter.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index e0aa6b440ca5..2e8a5b32f152 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -490,7 +490,7 @@ size_t copy_page_from_iter_atomic(struct page *page, size_t offset,
 		char *p;
 
 		n = bytes - copied;
-		if (PageHighMem(page)) {
+		if (IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(page)) {
 			page += offset / PAGE_SIZE;
 			offset %= PAGE_SIZE;
 			n = min_t(size_t, n, PAGE_SIZE - offset);
@@ -501,7 +501,8 @@ size_t copy_page_from_iter_atomic(struct page *page, size_t offset,
 		kunmap_atomic(p);
 		copied += n;
 		offset += n;
-	} while (PageHighMem(page) && copied != bytes && n > 0);
+	} while ((IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(page))
+		 && copied != bytes && n > 0);
 
 	return copied;
 }
-- 
2.34.1

On Sat, Mar 02, 2024 at 12:09:08PM +1300, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
> 
> copy_page_from_iter_atomic() has the assumption lowmem will only
> need one kmap to get start page_address() for all pages. This is
> wrong if the debug option CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP is
> enabled. This patch fixes it in the same way with skbuff.h by
> always applying kmap one by one even for lowmem,
> 
>  static inline bool skb_frag_must_loop(struct page *p)
>  {
>  #if defined(CONFIG_HIGHMEM)
>  	if (IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(p))
>   		return true;
>  #endif
>  	return false;
>  }

Thanks for the patch.  Perhaps this could be moved into highmem.h
as a helper (kmap_is_highmem)?
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Sat, Mar 2, 2024 at 5:32 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Sat, Mar 02, 2024 at 12:09:08PM +1300, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > copy_page_from_iter_atomic() has the assumption lowmem will only
> > need one kmap to get start page_address() for all pages. This is
> > wrong if the debug option CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP is
> > enabled. This patch fixes it in the same way with skbuff.h by
> > always applying kmap one by one even for lowmem,
> >
> >  static inline bool skb_frag_must_loop(struct page *p)
> >  {
> >  #if defined(CONFIG_HIGHMEM)
> >       if (IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(p))
> >               return true;
> >  #endif
> >       return false;
> >  }
>
> Thanks for the patch.  Perhaps this could be moved into highmem.h
> as a helper (kmap_is_highmem)?

makes sense. OTOH,  is skb_frag_must_loop even correct as
DEBUG_KMAP_LOCAL_FORCE_MAP
is for non-highmem pages and on non-highmem systems.

config DEBUG_KMAP_LOCAL_FORCE_MAP
        bool "Enforce kmap_local temporary mappings"
        depends on DEBUG_KERNEL && ARCH_SUPPORTS_KMAP_LOCAL_FORCE_MAP
        select KMAP_LOCAL
        select DEBUG_KMAP_LOCAL
        help
          This option enforces temporary mappings through the kmap_local
          mechanism for non-highmem pages and on non-highmem systems.
          Disable this for production systems!

And highmem.c also shows it doesn't depend on CONFIG_HIGHMEM at all,

void *__kmap_local_page_prot(struct page *page, pgprot_t prot)
{
        void *kmap;
        /*
         * To broaden the usage of the actual kmap_local() machinery always map
         * pages when debugging is enabled and the architecture has no problems
         * with alias mappings.
         */
        if (!IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) &&
!PageHighMem(page))
                return page_address(page);
        ...
}
EXPORT_SYMBOL(__kmap_local_page_prot);


but skb_frag_must_loop is checking it under if defined(CONFIG_HIGHMEM).
so I guess it is wrong too. Never has a bug been reported.  Probably that is
because nobody is really using CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP,
I guess :-)

I think the correct skb_frag_must_loop() should be

static inline bool skb_frag_must_loop(struct page *p)
{
       return IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) || PageHighMem(p);
}

Thus, kmap_is_highmem() can entirely replace it :-)

> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks
Barry