Received-SPF: pass (google.com: domain of linux-kernel+bounces-106678-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>
 <CAGsJ_4xyDm0BLYvbW0z9J-Q=sQPYrttva33g2mRFVXcJJ+wimw@mail.gmail.com> <CAJD7tkaP_wfkJG_dfsp-jV7QP2JWyMFfNApTFPWbsqmTvfKyLw@mail.gmail.com>
In-Reply-To: <CAJD7tkaP_wfkJG_dfsp-jV7QP2JWyMFfNApTFPWbsqmTvfKyLw@mail.gmail.com>
From: Barry Song <21cnbao@gmail.com>
Date: Tue, 19 Mar 2024 10:12:27 +1300
Message-ID: <CAGsJ_4yKHYsPomhjnmdoWPSes_M5+8ZxvPry+X78pA5Kj_F_Jg@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 10:00=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com=
> wrote:
>
> [..]
> > > > 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_ent=
ry
> > > > *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()?

yes. it is checking if linear_mapping can apply on the buffer.

>
> >
> > >
> > > 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 simpl=
ifies 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.

I think both sg_init_one() and sg_set_page() lack docs. as apparently
sg_init_one() can't take highmem. sg_set_page() can definitely take
highmem as crypto/scompress.c takes care of both high
and low. and scatterwalk_map_and_copy() can handle both.

Thanks
Barry