Received-SPF: pass (google.com: domain of linux-kernel+bounces-106653-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <000000000000bbb3d80613f243a6@google.com> <CAKEwX=MAX0km1p43DQmKbeSy2G4dPFHiF+deH_qzqygc2Vnjig@mail.gmail.com>
 <CAGsJ_4y7aFg3FBh_isa_TCqY1B8n64Rro5mVu6=wvk7FP35mWw@mail.gmail.com> <CAJD7tkaPmgsUjPCi0wKCYAmPb+Vnif_zS+ouyd9NGQN=mdhJPw@mail.gmail.com>
In-Reply-To: <CAJD7tkaPmgsUjPCi0wKCYAmPb+Vnif_zS+ouyd9NGQN=mdhJPw@mail.gmail.com>
From: Barry Song <21cnbao@gmail.com>
Date: Tue, 19 Mar 2024 09:50:30 +1300
Message-ID: <CAGsJ_4xyDm0BLYvbW0z9J-Q=sQPYrttva33g2mRFVXcJJ+wimw@mail.gmail.com>
Subject: Re: [syzbot] [mm?] kernel BUG in sg_init_one
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Nhat Pham <nphamcs@gmail.com>, 
	syzbot <syzbot+adbc983a1588b7805de3@syzkaller.appspotmail.com>, 
	akpm@linux-foundation.org, chengming.zhou@linux.dev, hannes@cmpxchg.org, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org, 
	syzkaller-bugs@googlegroups.com, Barry Song <v-songbaohua@oppo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 19, 2024 at 9:35=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com>=
 wrote:
>
> On Mon, Mar 18, 2024 at 1:25=E2=80=AFPM Barry Song <21cnbao@gmail.com> wr=
ote:
> >
> > On Tue, Mar 19, 2024 at 7:00=E2=80=AFAM Nhat Pham <nphamcs@gmail.com> w=
rote:
> > >
> > > On Mon, Mar 18, 2024 at 9:58=E2=80=AFAM syzbot
> > > <syzbot+adbc983a1588b7805de3@syzkaller.appspotmail.com> 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=3D13043ab=
e180000
> > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=3D19bb57c=
23dffc38e
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=3Dadbc983a1=
588b7805de3
> > > > compiler:       arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GN=
U ld (GNU Binutils for Debian) 2.40
> > > > userspace arch: arm
> > > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=3D1706d=
231180000
> > > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=3D13ba795=
9180000
> > > >
> > > > Downloadable assets:
> > > > disk image (non-bootable): https://storage.googleapis.com/syzbot-as=
sets/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/7539441b=
4add/zImage-e5eb28f6.xz
> > > >
> > > > IMPORTANT: if you fix the issue, please add the following tag to th=
e commit:
> > > > Reported-by: syzbot+adbc983a1588b7805de3@syzkaller.appspotmail.com
> > > >
> > > > ------------[ 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 migh=
t
> > > 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 simplifie=
s 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 conc=
ern
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 p=
roblem.
>
> >         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