linux-kernel - Re: [PATCH] iov_iter: call kmap on each page even for lowmem if CONFIG_DEBUG_KMAP_LOCAL_FORCE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGsJ_4y3xmja7RFd2OUP7V+_sJPj9fqfqDc4TMsie5JSyj2Kzg@mail.gmail.com>
Date: Sat, 2 Mar 2024 21:26:34 +1300
From: Barry Song <21cnbao@...il.com>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: viro@...iv.linux.org.uk, akpm@...ux-foundation.org, 
	linux-kernel@...r.kernel.org, Barry Song <v-songbaohua@...o.com>
Subject: Re: [PATCH] iov_iter: call kmap on each page even for lowmem if
 CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP is enabled

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

Thanks
Barry