Re: [PATCH] iov_iter: call kmap on each page even for lowmem if CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP is enabled

[Barry Song]

On Sat, Mar 2, 2024 at 5:32 PM Herbert Xu <herbert@gondor.apana.orgau> wrote:
>
> On Sat, Mar 02, 2024 at 12:09:08PM +1300, Barry Song wrote:
> > From: Barry Song <v-songbaohua@xxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxx>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks
Barry