[syzbot] [mm?] kernel BUG in sg_init_one

[syzbot]

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+adbc98...@syzkaller.appspotmail.com

------------[ cut here ]------------
kernel BUG at include/linux/scatterlist.h:187!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 2997 Comm: syz-executor198 Not tainted 6.8.0-syzkaller #0
Hardware name: ARM-Versatile Express
PC is at sg_set_buf include/linux/scatterlist.h:187 [inline]
PC is at sg_init_one+0x9c/0xa8 lib/scatterlist.c:143
LR is at sg_init_table+0x2c/0x40 lib/scatterlist.c:128
pc : [<807e1748>] lr : [<807dfb3c>] psr: 80000113
sp : df955c38 ip : df955c70 fp : df955c54
r10: 00000000 r9 : ffefd004 r8 : ff7e7f14
r7 : 00000751 r6 : df955c58 r5 : 844847d0 r4 : ffefd004
r3 : df000000 r2 : ffffffd8 r1 : 00000000 r0 : df955c58
Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
Control: 30c5387d Table: 8446ed00 DAC: fffffffd
Register r0 information: 2-page vmalloc region starting at 0xdf954000 allocated at kernel_clone+0xac/0x3cc kernel/fork.c:2796
Register r1 information: NULL pointer
Register r2 information: non-paged memory
Register r3 information: non-paged memory
Register r4 information: non-paged memory
Register r5 information: slab vmap_area start 844847d0 pointer offset 0 size 40
Register r6 information: 2-page vmalloc region starting at 0xdf954000 allocated at kernel_clone+0xac/0x3cc kernel/fork.c:2796
Register r7 information: non-paged memory
Register r8 information: 0-page vmalloc region starting at 0xff7d8000 allocated at pcpu_get_vm_areas+0x0/0x12c8 mm/vmalloc.c:3064
Register r9 information: non-paged memory
Register r10 information: NULL pointer
Register r11 information: 2-page vmalloc region starting at 0xdf954000 allocated at kernel_clone+0xac/0x3cc kernel/fork.c:2796
Register r12 information: 2-page vmalloc region starting at 0xdf954000 allocated at kernel_clone+0xac/0x3cc kernel/fork.c:2796
Process syz-executor198 (pid: 2997, stack limit = 0xdf954000)
Stack: (0xdf955c38 to 0xdf956000)
5c20: ff7e7ef4 844847d0
5c40: def6d08c 83471c80 df955cb4 df955c58 804c1824 807e16b8 00000002 00000000
5c60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
5c80: 00000000 00000000 00000001 cc78d8d0 844847d0 00000001 def6d08c 8446eb84
5ca0: 8446eb80 8446eb80 df955cdc df955cb8 804c4468 804c1774 def6d08c 00000001
5cc0: df955d4c 00000000 835e6c00 844ac800 df955d2c df955ce0 804b9644 804c4318
5ce0: 804bab68 802e2238 00000000 00000000 00100cca 00000000 00000000 cc78d8d0
5d00: 00000102 00000001 00100cca 00000000 00000000 df955d4b 00000007 00000000
5d20: df955da4 df955d30 804bb064 804b95a8 df955d4b 00000000 00000100 def6d08c
5d40: 00000001 00000001 01955d6c 00000000 00000000 00000000 00000000 00000000
5d60: 00000001 00000000 df955d68 df955d68 8185c338 cc78d8d0 00000406 00000001
5d80: 00000000 00000001 84476480 00100cca 00000000 df955eb8 df955e1c df955da8
5da0: 804bb3b8 804baeac 00000000 cc78d8d0 00000001 df955eb8 00000000 00000000
5dc0: df955df4 df955dd0 8042c648 8042c49c df955eb8 8260cac8 84476480 7eb0d000
5de0: 844ac800 00000000 df955e1c cc78d8d0 804ba838 df955eb8 00000000 00000001
5e00: 84476480 844ac800 00000000 00000040 df955e7c df955e20 8047cde0 804bb35c
5e20: 80491ed4 80477794 df955eec 835e6c00 00000000 00000000 7eb0d000 842b3900
5e40: df955e7c df955e50 844ac800 80491e5c fe4d5003 00000254 835e6c00 7eb0d000
5e60: 84476480 7eb0d000 842b3900 00000040 df955f2c df955e80 8047e6c4 8047cbec
5e80: 842b3940 ffffffff df955ef0 7eb0d9b4 81c64fd4 8376240c 842b3940 7eaed000
5ea0: 7eb0dfff 8376240c 00000000 ffffffff df955eb8 df955fb0 84476480 00000cc0
5ec0: 0007efff 7eb0d000 7eb0d000 00000a54 845a4fa8 8446ed08 00000180 00000000
5ee0: 00000000 00000000 00000000 defbbe08 00000000 00000000 df955f2c cc78d8d0
5f00: 8047dd80 df955fb0 7eb0d9b4 00000254 00000207 7eb0d000 842b3900 00000007
5f20: df955f74 df955f30 80215d28 8047e2f8 835e6c00 00000109 df955fac df955f48
5f40: 8020bbe8 835e6c00 80306e18 8261d0e0 00000207 7eb0d9b4 df955fb0 80215be0
5f60: 00000000 7eb0da7c df955fac df955f78 80216170 80215bec 00000000 cc78d8d0
5f80: 00000000 cc78d8d0 00000000 00066bd4 00000010 ffffffff 835e6c00 824a9044
5fa0: 00000000 df955fb0 80200e3c 80216144 00000000 00000000 22d5f800 0008d158
5fc0: 00000000 7eb0d9a4 00000000 00000109 00000000 00000000 7eb0da7c 7eb0da3c
5fe0: 00000000 7eb0d9a0 00000001 00066bd4 00000010 ffffffff 00000000 00000000
Backtrace:
[<807e16ac>] (sg_init_one) from [<804c1824>] (zswap_decompress+0xbc/0x208 mm/zswap.c:1089)
r7:83471c80 r6:def6d08c r5:844847d0 r4:ff7e7ef4
[<804c1768>] (zswap_decompress) from [<804c4468>] (zswap_load+0x15c/0x198 mm/zswap.c:1637)
r9:8446eb80 r8:8446eb80 r7:8446eb84 r6:def6d08c r5:00000001 r4:844847d0
[<804c430c>] (zswap_load) from [<804b9644>] (swap_read_folio+0xa8/0x498 mm/page_io.c:518)
r9:844ac800 r8:835e6c00 r7:00000000 r6:df955d4c r5:00000001 r4:def6d08c
[<804b959c>] (swap_read_folio) from [<804bb064>] (swap_cluster_readahead+0x1c4/0x34c mm/swap_state.c:684)
r10:00000000 r9:00000007 r8:df955d4b r7:00000000 r6:00000000 r5:00100cca
r4:00000001
[<804baea0>] (swap_cluster_readahead) from [<804bb3b8>] (swapin_readahead+0x68/0x4a8 mm/swap_state.c:904)
r10:df955eb8 r9:00000000 r8:00100cca r7:84476480 r6:00000001 r5:00000000
r4:00000001
[<804bb350>] (swapin_readahead) from [<8047cde0>] (do_swap_page+0x200/0xcc4 mm/memory.c:4046)
r10:00000040 r9:00000000 r8:844ac800 r7:84476480 r6:00000001 r5:00000000
r4:df955eb8
[<8047cbe0>] (do_swap_page) from [<8047e6c4>] (handle_pte_fault mm/memory.c:5301 [inline])
[<8047cbe0>] (do_swap_page) from [<8047e6c4>] (__handle_mm_fault mm/memory.c:5439 [inline])
[<8047cbe0>] (do_swap_page) from [<8047e6c4>] (handle_mm_fault+0x3d8/0x12b8 mm/memory.c:5604)
r10:00000040 r9:842b3900 r8:7eb0d000 r7:84476480 r6:7eb0d000 r5:835e6c00
r4:00000254
[<8047e2ec>] (handle_mm_fault) from [<80215d28>] (do_page_fault+0x148/0x3a8 arch/arm/mm/fault.c:326)
r10:00000007 r9:842b3900 r8:7eb0d000 r7:00000207 r6:00000254 r5:7eb0d9b4
r4:df955fb0
[<80215be0>] (do_page_fault) from [<80216170>] (do_DataAbort+0x38/0xa8 arch/arm/mm/fault.c:558)
r10:7eb0da7c r9:00000000 r8:80215be0 r7:df955fb0 r6:7eb0d9b4 r5:00000207
r4:8261d0e0
[<80216138>] (do_DataAbort) from [<80200e3c>] (__dabt_usr+0x5c/0x60 arch/arm/kernel/entry-armv.S:427)
Exception stack(0xdf955fb0 to 0xdf955ff8)
5fa0: 00000000 00000000 22d5f800 0008d158
5fc0: 00000000 7eb0d9a4 00000000 00000109 00000000 00000000 7eb0da7c 7eb0da3c
5fe0: 00000000 7eb0d9a0 00000001 00066bd4 00000010 ffffffff
r8:824a9044 r7:835e6c00 r6:ffffffff r5:00000010 r4:00066bd4
Code: 1a000004 e1822003 e8860094 e89da8f0 (e7f001f2)
---[ end trace 0000000000000000 ]---
----------------
Code disassembly (best guess):
0: 1a000004 bne 0x18
4: e1822003 orr r2, r2, r3
8: e8860094 stm r6, {r2, r4, r7}
c: e89da8f0 ldm sp, {r4, r5, r6, r7, fp, sp, pc}
* 10: e7f001f2 udf #18 <-- trapping instruction


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Nhat Pham]

On Mon, Mar 18, 2024 at 9:58 AM syzbot
<syzbot+adbc98...@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=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+adbc98...@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 might
have revealed another bug elsewhere.

Anyway, cc-ing the author, Barry Song, to fact check me :) Will take a
closer look later.

[Barry Song]

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



>

[Barry Song]

> 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+adbc98...@syzkaller.appspotmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e5eb28f6d1afebed4bb7d740a797d0390bd3a357

[Barry Song]

On Tue, Mar 19, 2024 at 9:35 AM Yosry Ahmed <yosry...@google.com> wrote:

> 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

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in __kmap_to_page

------------[ cut here ]------------
WARNING: CPU: 0 PID: 3529 at mm/highmem.c:167 __kmap_to_page+0x100/0x194 mm/highmem.c:167
Modules linked in:
Kernel panic - not syncing: kernel: panic_on_warn set ...
CPU: 0 PID: 3529 Comm: syz-executor.0 Not tainted 6.8.0-syzkaller #0
Hardware name: ARM-Versatile Express
Backtrace:
[<8185fe64>] (dump_backtrace) from [<8185ff60>] (show_stack+0x18/0x1c arch/arm/kernel/traps.c:256)
r7:00000000 r6:82622e44 r5:00000000 r4:81fc00fc
[<8185ff48>] (show_stack) from [<8187d720>] (__dump_stack lib/dump_stack.c:88 [inline])
[<8185ff48>] (show_stack) from [<8187d720>] (dump_stack_lvl+0x54/0x7c lib/dump_stack.c:114)
[<8187d6cc>] (dump_stack_lvl) from [<8187d760>] (dump_stack+0x18/0x1c lib/dump_stack.c:123)
r5:00000000 r4:82857d18
[<8187d748>] (dump_stack) from [<81860a08>] (panic+0x120/0x358 kernel/panic.c:348)
[<818608e8>] (panic) from [<80243844>] (check_panic_on_warn kernel/panic.c:241 [inline])
[<818608e8>] (panic) from [<80243844>] (print_tainted+0x0/0xa0 kernel/panic.c:236)
r3:8260c584 r2:00000001 r1:81fa8e48 r0:81fb09f0
r7:80477264
[<802437d0>] (check_panic_on_warn) from [<80243a38>] (__warn+0x7c/0x180 kernel/panic.c:694)
[<802439bc>] (__warn) from [<80243cb4>] (warn_slowpath_fmt+0x178/0x1f4 kernel/panic.c:719)
r8:00000009 r7:81fd71bc r6:df9b1bf4 r5:83682400 r4:00000000
[<80243b40>] (warn_slowpath_fmt) from [<80477264>] (__kmap_to_page+0x100/0x194 mm/highmem.c:167)
r10:00000000 r9:ff7e7f14 r8:83e402c0 r7:dedf0b48 r6:800fd004 r5:ffefd000
r4:7fefd004
[<80477164>] (__kmap_to_page) from [<804c248c>] (kmap_to_page include/linux/highmem-internal.h:63 [inline])
[<80477164>] (__kmap_to_page) from [<804c248c>] (zswap_decompress+0xc0/0x23c mm/zswap.c:1090)
r9:ff7e7f14 r8:83e402c0 r7:dedf0b48 r6:ffefd004 r5:840fb8e8 r4:ff7e7ef4
[<804c23cc>] (zswap_decompress) from [<804c449c>] (zswap_load+0x15c/0x198 mm/zswap.c:1638)
r9:846e3240 r8:846e3240 r7:846e3244 r6:dedf0b48 r5:00000010 r4:840fb8e8
[<804c4340>] (zswap_load) from [<804b9644>] (swap_read_folio+0xa8/0x498 mm/page_io.c:518)
r9:8420fa00 r8:83682400 r7:00000000 r6:df9b1d4c r5:00000000 r4:dedf0b48

[<804b959c>] (swap_read_folio) from [<804bb064>] (swap_cluster_readahead+0x1c4/0x34c mm/swap_state.c:684)

r10:00000000 r9:00000017 r8:df9b1d4b r7:00000000 r6:00000000 r5:00100cca
r4:00000010

[<804baea0>] (swap_cluster_readahead) from [<804bb3b8>] (swapin_readahead+0x68/0x4a8 mm/swap_state.c:904)

r10:df9b1eb8 r9:00000000 r8:00100cca r7:8371e2a0 r6:00000012 r5:00000000

r4:00000001
[<804bb350>] (swapin_readahead) from [<8047cde0>] (do_swap_page+0x200/0xcc4 mm/memory.c:4046)

r10:00000040 r9:00000000 r8:8420fa00 r7:8371e2a0 r6:00000012 r5:00000000
r4:df9b1eb8

[<8047cbe0>] (do_swap_page) from [<8047e6c4>] (handle_pte_fault mm/memory.c:5301 [inline])
[<8047cbe0>] (do_swap_page) from [<8047e6c4>] (__handle_mm_fault mm/memory.c:5439 [inline])
[<8047cbe0>] (do_swap_page) from [<8047e6c4>] (handle_mm_fault+0x3d8/0x12b8 mm/memory.c:5604)

r10:00000040 r9:83dfb900 r8:83682400 r7:8371e2a0 r6:001403b8 r5:83682400
r4:00001254
[<8047e2ec>] (handle_mm_fault) from [<80215da8>] (do_page_fault+0x1c8/0x3a8 arch/arm/mm/fault.c:292)
r10:00000007 r9:83dfb900 r8:83682400 r7:00000207 r6:00000254 r5:001403b8
r4:df9b1fb0

[<80215be0>] (do_page_fault) from [<80216170>] (do_DataAbort+0x38/0xa8 arch/arm/mm/fault.c:558)

r10:7eded670 r9:00000000 r8:80215be0 r7:df9b1fb0 r6:001403b8 r5:00000207

r4:8261d0e0
[<80216138>] (do_DataAbort) from [<80200e3c>] (__dabt_usr+0x5c/0x60 arch/arm/kernel/entry-armv.S:427)

Exception stack(0xdf9b1fb0 to 0xdf9b1ff8)
1fa0: 00000000 00000000 00000000 00000000
1fc0: 00000002 7eded61c 00000000 000001f4 00140000 00000000 7eded670 00001238
1fe0: 00000000 7eded5a8 00000001 00021804 60000010 ffffffff
r8:824a9044 r7:83682400 r6:ffffffff r5:60000010 r4:00021804
Rebooting in 86400 seconds..


Tested on:

commit: e5eb28f6 Merge tag 'mm-nonmm-stable-2024-03-14-09-36' ..
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1491ea6e180000

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=12651546180000

[Barry Song]

On Tue, Mar 19, 2024 at 9:52 AM syzbot
<syzbot+adbc98...@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> WARNING in __kmap_to_page
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 3529 at mm/highmem.c:167 __kmap_to_page+0x100/0x194 mm/highmem.c:167
> Modules linked in:

+ Ira

Hi Ira,

I noticed this warning is coming from commit ef6e06b2ef87077.

you have a commit message like
" Because it is intended to remove kmap_to_page() add a warn on once to
the kmap checks to flag potential issues early.
"

Do we have a replacement for kmap_to_page()? The background is that we
want to pass a highmem buffer to sg_set_page() but we only know its virt
address.

Thanks
Barry

[Johannes Weiner]

On Mon, Mar 18, 2024 at 01:17:19PM -0700, Yosry Ahmed wrote:

> I am not a highmem expert, but the reproducer has CONFIG_HIGHMEM=y,
> and it seems like zs_map_object() may return a highmem address if the
> compressed object is entirely in a single page to avoid copying to a
> buffer:
>
> if (off + class->size <= PAGE_SIZE) {
> /* this object is contained entirely within a page */
> area->vm_addr = kmap_atomic(page);
> ret = area->vm_addr + off;
> goto out;
> }
>
> The virt_addr_valid() check seems to indicate that we expect a direct
> map address in sg_init_one(), right?

If the page is highmem, kmap_atomic() establishes a temporary mapping
to it in the direct map, such that we have a legit kernel pointer to
the memory. Otherwise the memcpy() in zswap also wouldn't work... Am I
missing something?

[Barry Song]

On Tue, Mar 19, 2024 at 10:00 AM Yosry Ahmed <yosry...@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_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()?

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

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

[Barry Song]

Right, we built a map but it is not a linear mapping. so we can't use
virt_to_page
on this kind of non-linear mapping.
kmap_to_page can handle both linear and non-linear, but Ira's commit
added a WARN_ON_ONCE in it for non-linear mapping case.

[Barry Song]

On Tue, Mar 19, 2024 at 10:19 AM Yosry Ahmed <yosry...@google.com> wrote:

> IIUC kmap_atomic() establishes a mapping in the kernel portion of the
> address space, but not a direct map mapping (i.e. not a linear
> mapping), right?
>
> Does virt_addr_valid() check for addresses being in the kernel portion
> of the address space, or it being a linear mapping? I thought it
> checks for the latter.

the latter, right.

>

[Johannes Weiner]

Ah, I misread what virt_addr_valid() does. It actually excludes
kmap. Which, yes, makes sense, if the next line does virt_to_page()...

Sorry about the noise.

[Barry Song]

no worries. I just wonder why Ira's commit ef6e06b2ef870 has added a
WARN_ON_ONCE
in kmap_to_page() given we still have many users :-)

drivers/fpga/fpga-mgr.c: pages[index] =
kmap_to_page((void *)p);
drivers/spi/spi.c: vm_page = kmap_to_page(buf);
drivers/vfio/pci/pds/lm.c: pages[i] =
kmap_to_page((void *)p);
fs/erofs/data.c: .page = kmap_to_page(ptr),
fs/smb/server/transport_rdma.c: page = kmap_to_page(buf);
net/9p/trans_virtio.c: (*pages)[index] =
kmap_to_page(p);

[Yosry Ahmed]

On Mon, Mar 18, 2024 at 11:00 AM Nhat Pham <nph...@gmail.com> wrote:
>

[Yosry Ahmed]

On Mon, Mar 18, 2024 at 1:25 PM Barry Song <21c...@gmail.com> wrote:
>

Is this working around the debug check in sg_init_one()? IIUC, only

lowmem pages are supported. We may be passing in a highmem page to
sg_set_page() now, right?

Also, it seems like if src is a lowmem address kmap_to_page() will be
doing unnecessary checks (assuming it's working correctly)?

Would it be more robust to just use the temporary buffer if src is a
kmap address?

Also FWIW, I think you can use "#sys test" to check if a diff fixes the problem.

[Yosry Ahmed]

[..]

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

[Yosry Ahmed]

[Barry Song]

> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> WARNING in __kmap_to_page
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 3529 at mm/highmem.c:167 __kmap_to_page+0x100/0x194 mm/highmem.c:167
> Modules linked in:
> Kernel panic - not syncing: kernel: panic_on_warn set ...
>

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e5eb28f6d1afebed4bb7d740a797d0390bd3a357

diff --git a/mm/zswap.c b/mm/zswap.c
index 9dec853647c8..17bf6d87b274 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1080,7 +1080,8 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page)
mutex_lock(&acomp_ctx->mutex);

src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
- if (acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) {
+ if ((acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) ||
+ !virt_addr_valid(src)) {
memcpy(acomp_ctx->buffer, src, entry->length);
src = acomp_ctx->buffer;
zpool_unmap_handle(zpool, entry->handle);
@@ -1094,7 +1095,7 @@ static void zswap_decompress(struct zswap_entry *entry, struct page *page)
BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE);
mutex_unlock(&acomp_ctx->mutex);

- if (!acomp_ctx->is_sleepable || zpool_can_sleep_mapped(zpool))
+ if (src != acomp_ctx->buffer)
zpool_unmap_handle(zpool, entry->handle);
}

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+adbc98...@syzkaller.appspotmail.com

Tested on:

commit: e5eb28f6 Merge tag 'mm-nonmm-stable-2024-03-14-09-36' ..
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=109a75a5180000

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

patch: https://syzkaller.appspot.com/x/patch.diff?x=177266be180000

Note: testing is done by a robot and is best-effort only.