Re: [syzbot] [mm?] kernel BUG in sg_init_one

[Ira Weiny <ira.weiny@intel.com>]

Yosry Ahmed wrote:
> On Wed, Feb 26, 2025 at 10:01:29AM -0600, Ira Weiny wrote:
> > Yosry Ahmed wrote:
> > > On Wed, Feb 12, 2025 at 09:20:24AM -0800, Yosry Ahmed wrote:
> > > > On Mon, Mar 18, 2024 at 2:03 PM Barry Song <21cnbao@gmail.com> wrote:
> > > > >
> > > > > On Tue, Mar 19, 2024 at 9:52 AM syzbot
> > > > > <syzbot+adbc983a1588b7805de3@syzkaller.appspotmail.com> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > > > > > WARNING in __kmap_to_page
> > > > > >
> > > > > > ------------[ cut here ]------------
> > > > > > WARNING: CPU: 0 PID: 3529 at mm/highmem.c:167 __kmap_to_page+0x100/0x194 mm/highmem.c:167
> > > > > > Modules linked in:
> > > > >
> > > > > + Ira
> > > > >
> > > > > Hi Ira,
> > > > >
> > > > > I noticed this warning is coming from commit ef6e06b2ef87077.
> > > > >
> > > > > you have a commit message like
> > > > > "    Because it is intended to remove kmap_to_page() add a warn on once to
> > > > >     the kmap checks to flag potential issues early.
> > > > > "
> > > > >
> > > > > Do we have a replacement for kmap_to_page()? The background is that we
> > > > > want to pass a highmem buffer to sg_set_page() but we only know its virt
> > > > > address.
> > > > 
> > > > I am reviving this thread because new zsmalloc changes will make
> > > > mappings sleepable, which will allow zswap to drop the memcpy() in
> > > > zswap_decompress() -- except for the !virt_addr_valid() case. We can
> > > > get rid of that too if we can use kmap_tp_page() in the scatterlist
> > > > code.
> > > > 
> > > > Ira, could you please answer Barry's question above about
> > > > kmap_to_page()? It has been a year and kmap_to_page() is still around.
> > > 
> > > (Trying again with Ira as the main recepient just in case)
> > > 
> > > Ira, could you please help us out here? :)
> > 
> > Apologies,
> > 
> > No there is no alternative to kmap_to_page().  The work I was doing has
> > stalled out and I really don't know if it will resume.
> > 
> > There are a few folks, like me, who would like to remove highmem but every
> > time the subject comes up someone speaks up about a rare (mostly embedded)
> > platform which really needs it.  So I don't see it going away soon.
> > 
> > Removing the warning could be justified by saying that highmem removal can
> > be done completely within the kmap calls and only when that has been
> > completed can these calls go away.  But generally kmap_to_page() is not a
> > popular call and it might be seen as a step backwards by some.
> > 
> > For example:
> > 	https://lore.kernel.org/linux-mm/20221216070621.GA24832@lst.de/
> > 
> > The patch with the warnings was a stop gap to ensure current users did not
> > break.
> > 
> > Do you have evidence that the extra memcpy is bad enough performance that
> > you could justify using kmap_to_page?
> 
> It's not about performance, it's about cleaning up the code. Currently
> we have a special memcpy path [1] with a huge comment in
> zswap_decompress() to handle zsmalloc not being sleepable and the kmap
> case. The zsmalloc case is going away soon, and we'd like to remove the
> special handling completely.
> 
> Using kmap_to_page() will allow us to do so. As you mentioned, this
> would not be blocking to removing highmem. The call can just be replaced
> with virt_to_page() once this is done.
> 
> If this is okay with you I can send a patch removing the warnings in
> __kmap_to_page() when I start using the function.

It is fine with me.

I was just thinking it would be easier to justify adding a caller if there
was something like a performance boost.

Perhaps Christoph is not as concerned these days.  :-D

Ira

> 
> [1]https://elixir.bootlin.com/linux/v6.13.4/source/mm/zswap.c#L1012
>  
> > Ira
> > 
> > > 
> > > > 
> > > > Thanks.
> > > 
> > 
> >