Received-SPF: pass (google.com: domain of linux-kernel+bounces-52018-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <000000000000ae47f90610803260@google.com> <CAKEwX=O1xg=BuLMbmv=n3L_mNRwhrumZ-ig4Y5N4JJ4Mu=d4Hg@mail.gmail.com>
 <5ea0950d-1b2e-47ae-b8a0-8bc0e99479f1@linux.dev>
In-Reply-To: <5ea0950d-1b2e-47ae-b8a0-8bc0e99479f1@linux.dev>
From: Nhat Pham <nphamcs@gmail.com>
Date: Sun, 4 Feb 2024 19:48:38 -0800
Message-ID: <CAKEwX=Nzipr_nWTVPsUC_JKzoKu=6CX7rMSRv+T1YoQ8WheuWw@mail.gmail.com>
Subject: Re: [syzbot] [mm?] WARNING in zswap_folio_swapin
To: Chengming Zhou <chengming.zhou@linux.dev>
Cc: syzbot <syzbot+17a611d10af7d18a7092@syzkaller.appspotmail.com>, 
	akpm@linux-foundation.org, hannes@cmpxchg.org, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, syzkaller-bugs@googlegroups.com, yosryahmed@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Feb 3, 2024 at 6:59=E2=80=AFPM Chengming Zhou <chengming.zhou@linux=
dev> wrote:
>
> On 2024/2/4 09:28, Nhat Pham wrote:
> > On Sat, Feb 3, 2024 at 12:37=E2=80=AFPM syzbot
> > <syzbot+17a611d10af7d18a7092@syzkaller.appspotmail.com> wrote:
> >>
> >> Hello,
> >>
> >> syzbot found the following issue on:
> >>
> >> HEAD commit:    861c0981648f Merge tag 'jfs-6.8-rc3' of github.com:kle=
ikam..
> >> git tree:       upstream
> >> console output: https://syzkaller.appspot.com/x/log.txt?x=3D174537bbe8=
0000
> >> kernel config:  https://syzkaller.appspot.com/x/.config?x=3Db168fa511d=
b3ca08
> >> dashboard link: https://syzkaller.appspot.com/bug?extid=3D17a611d10af7=
d18a7092
> >> compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils fo=
r Debian) 2.40
> >> userspace arch: i386
> >>
> >> Unfortunately, I don't have any reproducer for this issue yet.
> >>
> >> Downloadable assets:
> >> disk image (non-bootable): https://storage.googleapis.com/syzbot-asset=
s/7bc7510fe41f/non_bootable_disk-861c0981.raw.xz
> >> vmlinux: https://storage.googleapis.com/syzbot-assets/b2b204c7b4a0/vml=
inux-861c0981.xz
> >> kernel image: https://storage.googleapis.com/syzbot-assets/170ec316e55=
7/bzImage-861c0981.xz
> >>
> >> IMPORTANT: if you fix the issue, please add the following tag to the c=
ommit:
> >> Reported-by: syzbot+17a611d10af7d18a7092@syzkaller.appspotmail.com
> >>
> >>  kcov_ioctl+0x4f/0x720 kernel/kcov.c:704
> >>  __do_compat_sys_ioctl+0x2bf/0x330 fs/ioctl.c:971
> >>  do_syscall_32_irqs_on arch/x86/entry/common.c:165 [inline]
> >>  __do_fast_syscall_32+0x79/0x110 arch/x86/entry/common.c:321
> >> page has been migrated, last migrate reason: compaction
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 2 PID: 5104 at include/linux/memcontrol.h:775 folio_lruv=
ec include/linux/memcontrol.h:775 [inline]
> >> WARNING: CPU: 2 PID: 5104 at include/linux/memcontrol.h:775 zswap_foli=
o_swapin+0x47d/0x5a0 mm/zswap.c:381
> >> Modules linked in:
> >> CPU: 2 PID: 5104 Comm: syz-fuzzer Not tainted 6.8.0-rc2-syzkaller-0003=
1-g861c0981648f #0
> >> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian=
-1.16.2-1 04/01/2014
> >> RIP: 0010:folio_lruvec include/linux/memcontrol.h:775 [inline]
> >
> > Hmm looks like it's this line:
> > VM_WARN_ON_ONCE_FOLIO(!memcg && !mem_cgroup_disabled(), folio);
> >
> > Looks like memcg was cleared from the folio. Haven't looked too
> > closely yet, but this (and the "page has been migrated" line above)
> > suggests maybe there is some migration business going on -
> > mem_cgroup_migrate() clears the old folio's memcg_data (via
> > old->memcg_data =3D 0).
>
> Yeah, I think it's this case.
>
> >
> > Here's my theory (which could be wrong - someone please fact-check
> > me): swap_read_folio(), which precedes zswap_folio_swapin(), unlocks
>
> And another case is !page_allocated, the returned folio is unlocked, righ=
t?

I think you're correct. That said, it's probably fine to keep the
protection size if we find the folio in the swapcache anyway - IIUC,
we are not performing a swapin in that case (since !page_allocated
means no swap_read_folio() called), which is the scenario that the
heuristics cares about :)

IOW, something like this:

if (unlikely(page_allocated)) {
    zswap_folio_swapin(folio);
    swap_read_folio(folio, false, NULL);
}

make sense to me, both from the correctness POV, and the heuristics POV.


>
> > the folio. Could this be sufficient to allow for migration? If this is
>
> IMHO, folio locked is sufficient to avoid concurrent memcg migration.
>
> > the case, all we need to do is move this to above swap_read_folio(),
> > while the folio is still locked. __read_swap_cache_async() already
> > charges the folio to an memcg, so no need to wait till after
> > swap_read_page() anyway.
>
> Should we call zswap_folio_swapin() in the !page_allocated case?
>
> Thanks.