linux-kernel - Re: [syzbot] [mm?] kernel BUG in sg_init

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAJD7tkaP_wfkJG_dfsp-jV7QP2JWyMFfNApTFPWbsqmTvfKyLw@mail.gmail.com>
Date: Mon, 18 Mar 2024 13:59:51 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Barry Song <21cnbao@...il.com>
Cc: Nhat Pham <nphamcs@...il.com>, 
	syzbot <syzbot+adbc983a1588b7805de3@...kaller.appspotmail.com>, 
	akpm@...ux-foundation.org, chengming.zhou@...ux.dev, hannes@...xchg.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	syzkaller-bugs@...glegroups.com, Barry Song <v-songbaohua@...o.com>
Subject: Re: [syzbot] [mm?] kernel BUG in sg_init_one

[..]
> > > I guess that is because on arm32 , we have highmem but
> > > sg_init_one supports lowmem only. the below should be
> > > able to fix?
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 9dec853647c8..47c0386caba2 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1086,7 +1086,8 @@ static void zswap_decompress(struct zswap_entry
> > > *entry, struct page *page)
> > >                 zpool_unmap_handle(zpool, entry->handle);
> > >         }
> > >
> > > -       sg_init_one(&input, src, entry->length);
> > > +       sg_init_table(&input, 1);
> > > +       sg_set_page(&input, kmap_to_page(src), entry->length,
> > > offset_in_page(src));
> >
> > Is this working around the debug check in sg_init_one()? IIUC, only
>
> I wouldn't characterize it as a workaround; it's more of a solution.

I assumed that the debug check in sg_set_buf() is because
sg_set_page() cannot handle highmem pages, sorry if that isn't the
case. Apparently we are hitting a warning with kmap_to_page() though
as syzbot just reported.

>
> > lowmem pages are supported. We may be passing in a highmem page to
> > sg_set_page() now, right?
>
> we can pass highmem to sg_set_page(). This is perfectly fine.

So the debug check is only because we are using virt_to_page() in sg_set_buf()?

>
> >
> > Also, it seems like if src is a lowmem address kmap_to_page() will be
> > doing unnecessary checks (assuming it's working correctly)?
>
> In practice, we consistently use kmap and kunmap even on systems with
> low memory.
> However, it's worth noting that for low memory scenarios, kmap
> essentially returns
> page_to_virt(page_address). Thus, the overhead of kmap_to_page shouldn't be
> significant on low memory systems, especially considering that it simplifies to
> virt_to_page().
>
> Another approach is to consistently employ page_to_virt() for low
> memory situations
> and reserve kmap for high memory scenarios. However, since we always
> utilize kmap
> regardless of whether the page is low or high memory, we don't need to concern
> ourselves with this distinction

I see. Thanks for elaborating.

>
> >
> > Would it be more robust to just use the temporary buffer if src is a
> > kmap address?
>
> I don't think so because we will need a memcpy then.

I thought that was necessary because sg_set_page() cannot take in
highmem pages, but you mentioned that this isn't the case.