From: Barry Song <[email protected]>
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 <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Barry Song <[email protected]>
---
-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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]> wrote:
>
> On Sat, Mar 02, 2024 at 12:09:08PM +1300, Barry Song wrote:
> > From: Barry Song <[email protected]>
> >
> > 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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Thanks
Barry