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

[Barry Song]

On Tue, Mar 19, 2024 at 9:35 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Mon, Mar 18, 2024 at 1:25 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
> >
> > On Tue, Mar 19, 2024 at 7:00 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> > >
> > > On Mon, Mar 18, 2024 at 9:58 AM syzbot
> > > <syzbot+adbc983a1588b7805de3@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > Hello,
> > > >
> > > > syzbot found the following issue on:
> > > >
> > > > HEAD commit:    e5eb28f6d1af Merge tag 'mm-nonmm-stable-2024-03-14-09-36' ..
> > > > git tree:       upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=13043abe180000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=19bb57c23dffc38e
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=adbc983a1588b7805de3
> > > > compiler:       arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> > > > userspace arch: arm
> > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1706d231180000
> > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13ba7959180000
> > > >
> > > > Downloadable assets:
> > > > disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/8ead8862021c/non_bootable_disk-e5eb28f6.raw.xz
> > > > vmlinux: https://storage.googleapis.com/syzbot-assets/0a7371c63ff2/vmlinux-e5eb28f6.xz
> > > > kernel image: https://storage.googleapis.com/syzbot-assets/7539441b4add/zImage-e5eb28f6.xz
> > > >
> > > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > > Reported-by: syzbot+adbc983a1588b7805de3@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > >
> > > > ------------[ cut here ]------------
> > > > kernel BUG at include/linux/scatterlist.h:187!
> > >
> > > Looks like the provided buffer is invalid:
> > >
> > > #ifdef CONFIG_DEBUG_SG
> > > BUG_ON(!virt_addr_valid(buf));
> > > #endif
> > >
> > > which is "src" from:
> > >
> > > sg_init_one(&input, src, entry->length);
> > >
> > > Looking at the surrounding code and recent history, there's this
> > > commit that stands out:
> > >
> > > mm/zswap: remove the memcpy if acomp is not sleepable
> > > (sha: 270700dd06ca41a4779c19eb46608f076bb7d40e)
> > >
> > > which has the effect of, IIUC, using the zpool mapped memory directly
> > > as src, instead of acomp_ctx->buffer (which was previously the case,
> > > as zsmalloc was not sleepable).
> > >
> > > This might not necessarily be a bug with that commit itself, but might
> > > have revealed another bug elsewhere.
> > >
> > > Anyway, cc-ing the author, Barry Song, to fact check me :) Will take a
> > > closer look later.
> >
> > 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.

> 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.

>
> 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

>
> 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.

>
> Also FWIW, I think you can use "#sys test" to check if a diff fixes the problem.
>
> >         sg_init_table(&output, 1);
> >         sg_set_page(&output, page, PAGE_SIZE, 0);
> >         acomp_request_set_params(acomp_ctx->req, &input, &output,
> > entry->length, PAGE_SIZE);
> >
> >
> >
Thanks
Barry