Received-SPF: pass (google.com: domain of linux-kernel+bounces-89380-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240301230908.7507-1-21cnbao@gmail.com> <ZeKrx8g3DSHaVWhv@gondor.apana.org.au>
In-Reply-To: <ZeKrx8g3DSHaVWhv@gondor.apana.org.au>
From: Barry Song <21cnbao@gmail.com>
Date: Sat, 2 Mar 2024 21:26:34 +1300
Message-ID: <CAGsJ_4y3xmja7RFd2OUP7V+_sJPj9fqfqDc4TMsie5JSyj2Kzg@mail.gmail.com>
Subject: Re: [PATCH] iov_iter: call kmap on each page even for lowmem if
 CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP is enabled
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: viro@zeniv.linux.org.uk, akpm@linux-foundation.org, 
	linux-kernel@vger.kernel.org, Barry Song <v-songbaohua@oppo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Mar 2, 2024 at 5:32=E2=80=AFPM 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 prob=
lems
         * 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 i=
s
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