general protection fault in hugetlb_vma_lock_write

syzbot In review syzbot 7 participants 17 messages Oct 29, 2023 – Nov 8, 2023

Subsystems (2)

Memory management HugeTLB

lore

Share & tools

lore

syzbot

author syzbot

2023-10-29 09:27 UTC #0 lore raw

Hello,

syzbot found the following issue on:

HEAD commit:    888cf78c29e2 Merge tag 'iommu-fix-v6.6-rc7' of git://git.k..
git tree:       upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=10cbee95680000
kernel config:  https://syzkaller.appspot.com/x/.config?x=9ee601744db6e780
dashboard link: https://syzkaller.appspot.com/bug?extid=6ada951e7c0f7bc8a71e
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=10fa2557680000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14285b57680000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/ca8ad4106267/disk-888cf78c.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/aac2e80e0744/vmlinux-888cf78c.xz
kernel image: https://storage.googleapis.com/syzbot-assets/b14533456815/bzImage-888cf78c.xz

The issue was bisected to:

commit bf4916922c60f43efaa329744b3eef539aa6a2b2

Author: Rik van Riel <[email protected]>
Date:   Fri Oct 6 03:59:07 2023 +0000

hugetlbfs: extend hugetlb_vma_lock to private VMAs

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=17d7dab5680000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=1437dab5680000
console output: https://syzkaller.appspot.com/x/log.txt?x=1037dab5680000

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

Fixes bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private VMAs")

general protection fault, probably for non-canonical address 0xdffffc000000001d: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000000e8-0x00000000000000ef]
CPU: 0 PID: 5048 Comm: syz-executor139 Not tainted 6.6.0-rc7-syzkaller-00142-g888cf78c29e2 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:__lock_acquire+0x109/0x5de0 kernel/locking/lockdep.c:5004
Code: 45 85 c9 0f 84 cc 0e 00 00 44 8b 05 c1 1e 42 0b 45 85 c0 0f 84 be 0d 00 00 48 ba 00 00 00 00 00 fc ff df 4c 89 d1 48 c1 e9 03 <80> 3c 11 00 0f 85 e8 40 00 00 49 81 3a a0 d9 5f 90 0f 84 96 0d 00
RSP: 0018:ffffc90003aa7798 EFLAGS: 00010016

RAX: ffff88807a0b9dc0 RBX: 1ffff92000754f23 RCX: 000000000000001d
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 00000000000000e8
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
R10: 00000000000000e8 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 00000000758bf000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 lock_acquire kernel/locking/lockdep.c:5753 [inline]
 lock_acquire+0x1ae/0x510 kernel/locking/lockdep.c:5718
 down_write+0x93/0x200 kernel/locking/rwsem.c:1573
 hugetlb_vma_lock_write mm/hugetlb.c:300 [inline]
 hugetlb_vma_lock_write+0xae/0x100 mm/hugetlb.c:291
 __hugetlb_zap_begin+0x1e9/0x2b0 mm/hugetlb.c:5447
 hugetlb_zap_begin include/linux/hugetlb.h:258 [inline]
 unmap_vmas+0x2f4/0x470 mm/memory.c:1733
 exit_mmap+0x1ad/0xa60 mm/mmap.c:3230
 __mmput+0x12a/0x4d0 kernel/fork.c:1349
 mmput+0x62/0x70 kernel/fork.c:1371
 exit_mm kernel/exit.c:567 [inline]
 do_exit+0x9ad/0x2a20 kernel/exit.c:861
 __do_sys_exit kernel/exit.c:991 [inline]
 __se_sys_exit kernel/exit.c:989 [inline]
 __x64_sys_exit+0x42/0x50 kernel/exit.c:989
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7ff2b7a78ab9
Code: Unable to access opcode bytes at 0x7ff2b7a78a8f.
RSP: 002b:00007fff926ea6b8 EFLAGS: 00000246 ORIG_RAX: 000000000000003c
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ff2b7a78ab9
RDX: 00007ff2b7ab23f3 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 000000000000cfda R08: 0000000000000000 R09: 0000000000000006
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff926ea6cc
R13: 431bde82d7b634db R14: 0000000000000001 R15: 0000000000000001
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:__lock_acquire+0x109/0x5de0 kernel/locking/lockdep.c:5004
Code: 45 85 c9 0f 84 cc 0e 00 00 44 8b 05 c1 1e 42 0b 45 85 c0 0f 84 be 0d 00 00 48 ba 00 00 00 00 00 fc ff df 4c 89 d1 48 c1 e9 03 <80> 3c 11 00 0f 85 e8 40 00 00 49 81 3a a0 d9 5f 90 0f 84 96 0d 00
RSP: 0018:ffffc90003aa7798 EFLAGS: 00010016
RAX: ffff88807a0b9dc0 RBX: 1ffff92000754f23 RCX: 000000000000001d
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 00000000000000e8
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
R10: 00000000000000e8 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 00000000758bf000 CR4: 0000000000350ef0
----------------
Code disassembly (best guess):
   0:	45 85 c9             	test   %r9d,%r9d
   3:	0f 84 cc 0e 00 00    	je     0xed5
   9:	44 8b 05 c1 1e 42 0b 	mov    0xb421ec1(%rip),%r8d        # 0xb421ed1
  10:	45 85 c0             	test   %r8d,%r8d
  13:	0f 84 be 0d 00 00    	je     0xdd7
  19:	48 ba 00 00 00 00 00 	movabs $0xdffffc0000000000,%rdx
  20:	fc ff df
  23:	4c 89 d1             	mov    %r10,%rcx
  26:	48 c1 e9 03          	shr    $0x3,%rcx
* 2a:	80 3c 11 00          	cmpb   $0x0,(%rcx,%rdx,1) <-- trapping instruction
  2e:	0f 85 e8 40 00 00    	jne    0x411c
  34:	49 81 3a a0 d9 5f 90 	cmpq   $0xffffffff905fd9a0,(%r10)
  3b:	0f                   	.byte 0xf
  3c:	84                   	.byte 0x84
  3d:	96                   	xchg   %eax,%esi
  3e:	0d                   	.byte 0xd

---
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 [email protected].

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot. For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the bug is already fixed, 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 bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

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

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

Rik van Riel

2023-10-31 18:39 UTC #1 lore raw

Quoted 7 lines · syzbot

commit bf4916922c60f43efaa329744b3eef539aa6a2b2
Author: Rik van Riel <[email protected]>
Date: Fri Oct 6 03:59:07 2023 +0000
hugetlbfs: extend hugetlb_vma_lock to private VMAs

I've been trying to reproduce the issue here, but the test case has been running for 4+ hours now on a KVM guest, with KASAN and CONFIG_PROVE_LOCKING enabled. No crashes yet.

I'll try adapting the config file from syzkaller so the resulting kernel boots here, but this is not looking like an easy reproducer so far...

The crash is also confusing me somewhat, because it looks like hugetlb_vma_lock_write() is passing a nonsense (very small value) resv_map->rw_sema pointer down to down_write, but the code has some protection against that:

static inline bool __vma_private_lock(struct vm_area_struct *vma)

{               
        return (!(vma->vm_flags & VM_MAYSHARE)) && vma-

Quoted 1 line

vm_private_data;

}

void hugetlb_vma_lock_write(struct vm_area_struct *vma) {
if (__vma_shareable_lock(vma)) {
struct hugetlb_vma_lock *vma_lock = vma-

Quoted 1 line

vm_private_data;

                down_write(&vma_lock->rw_sema);
        } else if (__vma_private_lock(vma)) {
                struct resv_map *resv_map = vma_resv_map(vma);
         
                down_write(&resv_map->rw_sema);
        }               
}

At fork time, vma->vm_private_data gets cleared in the child process for MAP_PRIVATE hugetlb VMAs.

I do not see anything that would leave behind a tiny, but non-zero value in that pointer.

I'll keep poking at this, but I don't know if it will reproduce here.

Quoted 120 lines

general protection fault, probably for non-canonical address
0xdffffc000000001d: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000000e8-
0x00000000000000ef]
CPU: 0 PID: 5048 Comm: syz-executor139 Not tainted 6.6.0-rc7-
syzkaller-00142-g888cf78c29e2 #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 10/09/2023
RIP: 0010:__lock_acquire+0x109/0x5de0 kernel/locking/lockdep.c:5004
Code: 45 85 c9 0f 84 cc 0e 00 00 44 8b 05 c1 1e 42 0b 45 85 c0 0f 84
be 0d 00 00 48 ba 00 00 00 00 00 fc ff df 4c 89 d1 48 c1 e9 03 <80> 3c 11 00 0f 85 e8 40 00 00 49 81 3a a0 d9 5f 90 0f 84 96 0d 00
RSP: 0018:ffffc90003aa7798 EFLAGS: 00010016
RAX: ffff88807a0b9dc0 RBX: 1ffff92000754f23 RCX: 000000000000001d
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 00000000000000e8
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
R10: 00000000000000e8 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 00000000758bf000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 lock_acquire kernel/locking/lockdep.c:5753 [inline]
 lock_acquire+0x1ae/0x510 kernel/locking/lockdep.c:5718
 down_write+0x93/0x200 kernel/locking/rwsem.c:1573
 hugetlb_vma_lock_write mm/hugetlb.c:300 [inline]
 hugetlb_vma_lock_write+0xae/0x100 mm/hugetlb.c:291
 __hugetlb_zap_begin+0x1e9/0x2b0 mm/hugetlb.c:5447
 hugetlb_zap_begin include/linux/hugetlb.h:258 [inline]
 unmap_vmas+0x2f4/0x470 mm/memory.c:1733
 exit_mmap+0x1ad/0xa60 mm/mmap.c:3230
 __mmput+0x12a/0x4d0 kernel/fork.c:1349
 mmput+0x62/0x70 kernel/fork.c:1371
 exit_mm kernel/exit.c:567 [inline]
 do_exit+0x9ad/0x2a20 kernel/exit.c:861
 __do_sys_exit kernel/exit.c:991 [inline]
 __se_sys_exit kernel/exit.c:989 [inline]
 __x64_sys_exit+0x42/0x50 kernel/exit.c:989
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7ff2b7a78ab9
Code: Unable to access opcode bytes at 0x7ff2b7a78a8f.
RSP: 002b:00007fff926ea6b8 EFLAGS: 00000246 ORIG_RAX:
000000000000003c
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ff2b7a78ab9
RDX: 00007ff2b7ab23f3 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 000000000000cfda R08: 0000000000000000 R09: 0000000000000006
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff926ea6cc
R13: 431bde82d7b634db R14: 0000000000000001 R15: 0000000000000001
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:__lock_acquire+0x109/0x5de0 kernel/locking/lockdep.c:5004
Code: 45 85 c9 0f 84 cc 0e 00 00 44 8b 05 c1 1e 42 0b 45 85 c0 0f 84
be 0d 00 00 48 ba 00 00 00 00 00 fc ff df 4c 89 d1 48 c1 e9 03 <80> 3c 11 00 0f 85 e8 40 00 00 49 81 3a a0 d9 5f 90 0f 84 96 0d 00
RSP: 0018:ffffc90003aa7798 EFLAGS: 00010016
RAX: ffff88807a0b9dc0 RBX: 1ffff92000754f23 RCX: 000000000000001d
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 00000000000000e8
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
R10: 00000000000000e8 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 00000000758bf000 CR4: 0000000000350ef0
----------------
Code disassembly (best guess):
   0:   45 85 c9                test   %r9d,%r9d
   3:   0f 84 cc 0e 00 00       je     0xed5
   9:   44 8b 05 c1 1e 42 0b    mov    0xb421ec1(%rip),%r8d        # 0xb421ed1
10:   45 85 c0                test   %r8d,%r8d
13:   0f 84 be 0d 00 00       je     0xdd7
19:   48 ba 00 00 00 00 00    movabs $0xdffffc0000000000,%rdx 20:   fc ff df
23:   4c 89 d1                mov    %r10,%rcx
26:   48 c1 e9 03             shr    $0x3,%rcx
* 2a:   80 3c 11 00             cmpb   $0x0,(%rcx,%rdx,1) <-- trapping instruction
2e:   0f 85 e8 40 00 00       jne    0x411c
34:   49 81 3a a0 d9 5f 90    cmpq   $0xffffffff905fd9a0,(%r10) 3b:   0f                      .byte 0xf
3c:   84                      .byte 0x84
3d:   96                      xchg   %eax,%esi
3e:   0d                      .byte 0xd
---
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 [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot. For information about bisection process see:
https://goo.gl/tpsmEJ#bisection
If the bug is already fixed, 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 bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup

Signature · 1 line

All Rights Reversed.

Aleksandr Nogikh

2023-11-02 12:15 UTC #2 lore raw

Quoted 7 lines · Rik van Riel

Quoted 5 lines · syzbot
commit bf4916922c60f43efaa329744b3eef539aa6a2b2
Author: Rik van Riel <[email protected]>
Date:   Fri Oct 6 03:59:07 2023 +0000

Quoted 6 lines

Quoted 2 lines
hugetlbfs: extend hugetlb_vma_lock to private VMAs
I've been trying to reproduce the issue here, but the test case has been running for 4+ hours now on a KVM guest, with KASAN and CONFIG_PROVE_LOCKING enabled. No crashes yet.

FWIW you may also try to use the syzbot-built kernel shared via the "Downloadable assets" section[1]. I've just run the C repro against it and it crashed immediately.

[   66.222816][ T5095] general protection fault, probably for
non-canonical address 0xdffffc000000001d: 0000 [#1] PREEMPT SMP KASAN
[   66.227224][ T5095] KASAN: null-ptr-deref in range
[0x00000000000000e8-0x00000000000000ef]
[   66.230109][ T5095] CPU: 0 PID: 5095 Comm: repro Not tainted

6.6.0-rc7-syzkaller-00142-g888cf78c29e2 #0

[1] Here are the instructions with commands to copy-paste:
https://github.com/google/syzkaller/blob/master/docs/syzbot_assets.md

Signature · 1 line

Aleksandr

Quoted 167 lines

I'll try adapting the config file from syzkaller so the resulting kernel boots here, but this is not looking like an easy reproducer so far...
The crash is also confusing me somewhat, because it looks like hugetlb_vma_lock_write() is passing a nonsense (very small value) resv_map->rw_sema pointer down to down_write, but the code has some protection against that:
static inline bool __vma_private_lock(struct vm_area_struct *vma)
{
        return (!(vma->vm_flags & VM_MAYSHARE)) && vma-
Quoted 1 line
vm_private_data;
}
void hugetlb_vma_lock_write(struct vm_area_struct *vma) {
if (__vma_shareable_lock(vma)) {
struct hugetlb_vma_lock *vma_lock = vma-
Quoted 1 line
vm_private_data;
                down_write(&vma_lock->rw_sema);
        } else if (__vma_private_lock(vma)) {
                struct resv_map *resv_map = vma_resv_map(vma);

                down_write(&resv_map->rw_sema);
        }
}
At fork time, vma->vm_private_data gets cleared in the child process for MAP_PRIVATE hugetlb VMAs.
I do not see anything that would leave behind a tiny, but non-zero value in that pointer.
I'll keep poking at this, but I don't know if it will reproduce here.
Quoted 114 lines
general protection fault, probably for non-canonical address
0xdffffc000000001d: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000000e8-
0x00000000000000ef]
CPU: 0 PID: 5048 Comm: syz-executor139 Not tainted 6.6.0-rc7-
syzkaller-00142-g888cf78c29e2 #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 10/09/2023
RIP: 0010:__lock_acquire+0x109/0x5de0 kernel/locking/lockdep.c:5004
Code: 45 85 c9 0f 84 cc 0e 00 00 44 8b 05 c1 1e 42 0b 45 85 c0 0f 84
be 0d 00 00 48 ba 00 00 00 00 00 fc ff df 4c 89 d1 48 c1 e9 03 <80> 3c 11 00 0f 85 e8 40 00 00 49 81 3a a0 d9 5f 90 0f 84 96 0d 00
RSP: 0018:ffffc90003aa7798 EFLAGS: 00010016
RAX: ffff88807a0b9dc0 RBX: 1ffff92000754f23 RCX: 000000000000001d
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 00000000000000e8
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
R10: 00000000000000e8 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff8880b9800000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 00000000758bf000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 lock_acquire kernel/locking/lockdep.c:5753 [inline]
 lock_acquire+0x1ae/0x510 kernel/locking/lockdep.c:5718
 down_write+0x93/0x200 kernel/locking/rwsem.c:1573
 hugetlb_vma_lock_write mm/hugetlb.c:300 [inline]
 hugetlb_vma_lock_write+0xae/0x100 mm/hugetlb.c:291
 __hugetlb_zap_begin+0x1e9/0x2b0 mm/hugetlb.c:5447
 hugetlb_zap_begin include/linux/hugetlb.h:258 [inline]
 unmap_vmas+0x2f4/0x470 mm/memory.c:1733
 exit_mmap+0x1ad/0xa60 mm/mmap.c:3230
 __mmput+0x12a/0x4d0 kernel/fork.c:1349
 mmput+0x62/0x70 kernel/fork.c:1371
 exit_mm kernel/exit.c:567 [inline]
 do_exit+0x9ad/0x2a20 kernel/exit.c:861
 __do_sys_exit kernel/exit.c:991 [inline]
 __se_sys_exit kernel/exit.c:989 [inline]
 __x64_sys_exit+0x42/0x50 kernel/exit.c:989
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7ff2b7a78ab9
Code: Unable to access opcode bytes at 0x7ff2b7a78a8f.
RSP: 002b:00007fff926ea6b8 EFLAGS: 00000246 ORIG_RAX:
000000000000003c
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ff2b7a78ab9
RDX: 00007ff2b7ab23f3 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 000000000000cfda R08: 0000000000000000 R09: 0000000000000006
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff926ea6cc
R13: 431bde82d7b634db R14: 0000000000000001 R15: 0000000000000001
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:__lock_acquire+0x109/0x5de0 kernel/locking/lockdep.c:5004
Code: 45 85 c9 0f 84 cc 0e 00 00 44 8b 05 c1 1e 42 0b 45 85 c0 0f 84
be 0d 00 00 48 ba 00 00 00 00 00 fc ff df 4c 89 d1 48 c1 e9 03 <80>
3c 11 00 0f 85 e8 40 00 00 49 81 3a a0 d9 5f 90 0f 84 96 0d 00
RSP: 0018:ffffc90003aa7798 EFLAGS: 00010016
RAX: ffff88807a0b9dc0 RBX: 1ffff92000754f23 RCX: 000000000000001d
RDX: dffffc0000000000 RSI: 0000000000000000 RDI: 00000000000000e8
RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
R10: 00000000000000e8 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff8880b9800000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000280 CR3: 00000000758bf000 CR4: 0000000000350ef0
----------------
Code disassembly (best guess):
   0:   45 85 c9                test   %r9d,%r9d
   3:   0f 84 cc 0e 00 00       je     0xed5
   9:   44 8b 05 c1 1e 42 0b    mov    0xb421ec1(%rip),%r8d        #
0xb421ed1
  10:   45 85 c0                test   %r8d,%r8d
  13:   0f 84 be 0d 00 00       je     0xdd7
  19:   48 ba 00 00 00 00 00    movabs $0xdffffc0000000000,%rdx
  20:   fc ff df
  23:   4c 89 d1                mov    %r10,%rcx
  26:   48 c1 e9 03             shr    $0x3,%rcx
* 2a:   80 3c 11 00             cmpb   $0x0,(%rcx,%rdx,1) <--
trapping instruction
  2e:   0f 85 e8 40 00 00       jne    0x411c
  34:   49 81 3a a0 d9 5f 90    cmpq   $0xffffffff905fd9a0,(%r10)
  3b:   0f                      .byte 0xf
  3c:   84                      .byte 0x84
  3d:   96                      xchg   %eax,%esi
  3e:   0d                      .byte 0xd
---
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 [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot. For information about bisection process see:
https://goo.gl/tpsmEJ#bisection
If the bug is already fixed, 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 bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
Quoted 6 lines
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
--
All Rights Reversed.
--
You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/bce5df0508221ab30a1fb121a219034631abedf5.camel%40surriel.com.

Edward Adam Davis

2023-11-01 06:36 UTC #3 lore raw

PATCH 1/1 mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to syzbot

When obtaining resv_map from vma, it is necessary to simultaneously determine the flag HPAGE_RESV_OWNER of vm_private_data.
Only when they are met simultaneously, resv_map is valid.

Reported-and-tested-by [email protected]

Fixes bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private VMAs")

Signed-off-by Edward Adam Davis <[email protected]>

---
 include/linux/hugetlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

include/linux/hugetlb.h +3 -1

index 47d25a5e1933..1a3ec1aee1a3 100644
@@ -1265,9 +1265,11 @@ static inline bool __vma_shareable_lock(struct vm_area_struct *vma)
 	return (vma->vm_flags & VM_MAYSHARE) && vma->vm_private_data;
 }
 
+#define HPAGE_RESV_OWNER    (1UL << 0)
 static inline bool __vma_private_lock(struct vm_area_struct *vma)
 {
-	return (!(vma->vm_flags & VM_MAYSHARE)) && vma->vm_private_data;
+	return (!(vma->vm_flags & VM_MAYSHARE)) && vma->vm_private_data && 
+		((unsigned long)vma->vm_private_data & HPAGE_RESV_OWNER);
 }
 
 /*

Signature · 1 line

2.25.1

Rik van Riel

2023-11-01 14:28 UTC #4 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

Quoted 27 lines · Edward Adam Davis

When obtaining resv_map from vma, it is necessary to simultaneously determine
the flag HPAGE_RESV_OWNER of vm_private_data.
Only when they are met simultaneously, resv_map is valid.
Reported-and-tested-by:
[email protected]
Fixes bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private
VMAs")
Signed-off-by Edward Adam Davis <[email protected]>
---
include/linux/hugetlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
include/linux/hugetlb.h +0 -0
index 47d25a5e1933..1a3ec1aee1a3 100644
@@ -1265,9 +1265,11 @@ static inline bool __vma_shareable_lock(struct
 vm_area_struct *vma)
         return (vma->vm_flags & VM_MAYSHARE) && vma->vm_private_data;
  }
+#define HPAGE_RESV_OWNER (1UL << 0)
static inline bool __vma_private_lock(struct vm_area_struct *vma) {
- return (!(vma->vm_flags & VM_MAYSHARE)) && vma-

Quoted 6 lines

Quoted 1 line
vm_private_data;
+ return (!(vma->vm_flags & VM_MAYSHARE)) && vma-
Quoted 1 line
vm_private_data &&
+ ((unsigned long)vma->vm_private_data & HPAGE_RESV_OWNER);
}

This could be cleaned up a bit by moving the HPAGE_RESV_OWNER definition (and its friends) into hugetlb.h, as well as the is_vma_resv_set() helper function.

Then __vma_private_lock() can just call is_vma_resv_set(), and open coding a duplicate of the same code.

Not having duplicates of the code will make it much harder to "miss a spot" with future changes.

I am still struggling to find a place where we might leave HPAGE_RESV_OWNER behind on a pointer that is otherwise NULL, but if your tests show this fixes the issue, I'm all for it :)

Signature · 1 line

All Rights Reversed.

Edward Adam Davis

2023-11-02 12:58 UTC #5 lore raw

PATCH 1/1 mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

When obtaining resv_map from vma, it is necessary to simultaneously determine the flag HPAGE_RESV_OWNER of vm_private_data.
Only when they are met simultaneously, resv_map is valid.

Reported-and-tested-by [email protected]

Fixes bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private VMAs")

Signed-off-by Edward Adam Davis <[email protected]>

---
 include/linux/hugetlb.h | 5 ++++-
 mm/hugetlb.c            | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

include/linux/hugetlb.h +4 -1

index 47d25a5e1933..14babc602f14 100644
@@ -179,6 +179,7 @@ struct address_space *hugetlb_page_mapping_lock_write(struct page *hpage);
 
 extern int sysctl_hugetlb_shm_group;
 extern struct list_head huge_boot_pages;
+static int is_vma_resv_set(struct vm_area_struct *vma, unsigned long flag);
 
 /* arch callbacks */

@@ -1265,9 +1266,11 @@ static inline bool __vma_shareable_lock(struct vm_area_struct *vma)
 	return (vma->vm_flags & VM_MAYSHARE) && vma->vm_private_data;
 }
 
+#define HPAGE_RESV_OWNER    (1UL << 0)
 static inline bool __vma_private_lock(struct vm_area_struct *vma)
 {
-	return (!(vma->vm_flags & VM_MAYSHARE)) && vma->vm_private_data;
+	return (!(vma->vm_flags & VM_MAYSHARE)) && vma->vm_private_data && 
+		is_vma_resv_set(vma, HPAGE_RESV_OWNER);
 }
 
 /*

mm/hugetlb.c +0 -1

index 1301ba7b2c9a..97ea782dfba6 100644
@@ -1033,7 +1033,6 @@ __weak unsigned long vma_mmu_pagesize(struct vm_area_struct *vma)
  * bits of the reservation map pointer, which are always clear due to
  * alignment.
  */
-#define HPAGE_RESV_OWNER    (1UL << 0)
 #define HPAGE_RESV_UNMAPPED (1UL << 1)
 #define HPAGE_RESV_MASK (HPAGE_RESV_OWNER | HPAGE_RESV_UNMAPPED)

Signature · 1 line

2.25.1

kernel test robot

build bot

2023-11-02 23:29 UTC #6 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

kernel test robot noticed the following build errors:

Hi Edward,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/mm-hugetlb-fix-null-ptr-defer-in-hugetlb_vma_lock_write/20231103-003152
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/tencent_164A247533766D667C3D873798968236C409%40qq.com
patch subject: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write
config: um-defconfig (https://download.01.org/0day-ci/archive/20231103/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from kernel/fork.c:52:
include/linux/hugetlb.h: In function '__vma_private_lock':

>> include/linux/hugetlb.h:1276:17: error: implicit declaration of function 'is_vma_resv_set' [-Werror=implicit-function-declaration]
    1276 |                 is_vma_resv_set(vma, HPAGE_RESV_OWNER);
         |                 ^~~~~~~~~~~~~~~

cc1: some warnings being treated as errors

vim +/is_vma_resv_set +1276 include/linux/hugetlb.h

1271
1272 #define HPAGE_RESV_OWNER (1UL << 0)
1273 static inline bool __vma_private_lock(struct vm_area_struct *vma)
1274 {
1275 return (!(vma->vm_flags & VM_MAYSHARE)) && vma->vm_private_data &&

> 1276			is_vma_resv_set(vma, HPAGE_RESV_OWNER);

1277 }
1278

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

kernel test robot

build bot

2023-11-02 23:29 UTC #7 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Edward Adam Davis

kernel test robot noticed the following build errors:

Hi Edward,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/mm-hugetlb-fix-null-ptr-defer-in-hugetlb_vma_lock_write/20231103-003152
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/tencent_164A247533766D667C3D873798968236C409%40qq.com
patch subject: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write
config: um-allnoconfig (https://download.01.org/0day-ci/archive/20231103/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/[email protected]/reproduce)

All errors (new ones prefixed by >>):

In file included from kernel/fork.c:34:
In file included from include/linux/mempolicy.h:15:
In file included from include/linux/pagemap.h:11:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]

     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from kernel/fork.c:34:
   In file included from include/linux/mempolicy.h:15:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from kernel/fork.c:34:
   In file included from include/linux/mempolicy.h:15:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     692 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     700 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     708 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     717 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     726 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     735 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   In file included from kernel/fork.c:52:
>> include/linux/hugetlb.h:1276:3: error: call to undeclared function 'is_vma_resv_set'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1276 |                 is_vma_resv_set(vma, HPAGE_RESV_OWNER);
         |                 ^

12 warnings and 1 error generated.
--
In file included from kernel/sysctl.c:24:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]

     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from kernel/sysctl.c:24:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from kernel/sysctl.c:24:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     692 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     700 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     708 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     717 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     726 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     735 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   In file included from kernel/sysctl.c:45:
>> include/linux/hugetlb.h:1276:3: error: call to undeclared function 'is_vma_resv_set'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1276 |                 is_vma_resv_set(vma, HPAGE_RESV_OWNER);
         |                 ^
   In file included from kernel/sysctl.c:53:
   In file included from include/linux/nfs_fs.h:31:
   In file included from include/linux/sunrpc/auth.h:13:
   In file included from include/linux/sunrpc/sched.h:19:
   include/linux/sunrpc/xdr.h:782:46: warning: result of comparison of constant 4611686018427387903 with expression of type '__u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
     782 |         if (U32_MAX >= SIZE_MAX / sizeof(*p) && len > SIZE_MAX / sizeof(*p))
         |                                                 ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~

13 warnings and 1 error generated.
--
In file included from mm/shmem.c:29:
In file included from include/linux/pagemap.h:11:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]

     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from mm/shmem.c:29:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from mm/shmem.c:29:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     692 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     700 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     708 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     717 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     726 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     735 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   In file included from mm/shmem.c:39:
>> include/linux/hugetlb.h:1276:3: error: call to undeclared function 'is_vma_resv_set'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1276 |                 is_vma_resv_set(vma, HPAGE_RESV_OWNER);
         |                 ^
   In file included from mm/shmem.c:58:
   include/linux/mman.h:158:9: warning: division by zero is undefined [-Wdivision-by-zero]
     158 |                _calc_vm_trans(flags, MAP_SYNC,       VM_SYNC      ) |
         |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/mman.h:136:21: note: expanded from macro '_calc_vm_trans'
     136 |    : ((x) & (bit1)) / ((bit1) / (bit2))))
         |                     ^ ~~~~~~~~~~~~~~~~~

13 warnings and 1 error generated.

vim +/is_vma_resv_set +1276 include/linux/hugetlb.h

> 1276			is_vma_resv_set(vma, HPAGE_RESV_OWNER);

1277 }
1278

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Rik van Riel

2023-11-03 00:57 UTC #8 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Edward Adam Davis

Quoted 21 lines · Edward Adam Davis

+++ b/include/linux/hugetlb.h
+#define HPAGE_RESV_OWNER    (1UL << 0)
static inline bool __vma_private_lock(struct vm_area_struct *vma) {
-       return (!(vma->vm_flags & VM_MAYSHARE)) && vma-
Quoted 1 line
vm_private_data;
+       return (!(vma->vm_flags & VM_MAYSHARE)) && vma-
Quoted 1 line
vm_private_data &&
+               is_vma_resv_set(vma, HPAGE_RESV_OWNER); }

/*
mm/hugetlb.c +0 -0
index 1301ba7b2c9a..97ea782dfba6 100644
@@ -1033,7 +1033,6 @@ __weak unsigned long vma_mmu_pagesize(struct
 vm_area_struct *vma)
   * bits of the reservation map pointer, which are always clear due

Quoted 7 lines

to
* alignment.
*/
-#define HPAGE_RESV_OWNER (1UL << 0)
#define HPAGE_RESV_UNMAPPED (1UL << 1)
#define HPAGE_RESV_MASK (HPAGE_RESV_OWNER | HPAGE_RESV_UNMAPPED)

Moving just that one define is less than ideal, and the kernel test robot seems unhappy, too.

It may be cleaner to just move __vma_private_lock into hugetlb.c, where it has all the dependencies it needs.

It isn't being called from anywhere else, anyway.

Hopefully that will keep the kernel test robot happy, too :)

Signature · 1 line

All Rights Reversed.

kernel test robot

build bot

2023-11-03 01:27 UTC #9 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Edward Adam Davis

kernel test robot noticed the following build warnings:

Hi Edward,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/mm-hugetlb-fix-null-ptr-defer-in-hugetlb_vma_lock_write/20231103-003152
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/tencent_164A247533766D667C3D873798968236C409%40qq.com
patch subject: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write
config: loongarch-randconfig-002-20231103 (https://download.01.org/0day-ci/archive/20231103/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231103/[email protected]/reproduce)

All warnings (new ones prefixed by >>):

In file included from include/linux/migrate.h:8,
from include/linux/balloon_compaction.h:41, from drivers/virtio/virtio_balloon.c:16:

>> include/linux/hugetlb.h:182:12: warning: 'is_vma_resv_set' used but never defined
     182 | static int is_vma_resv_set(struct vm_area_struct *vma, unsigned long flag);
         |            ^~~~~~~~~~~~~~~

vim +/is_vma_resv_set +182 include/linux/hugetlb.h

179
180 extern int sysctl_hugetlb_shm_group;
181 extern struct list_head huge_boot_pages;

 > 182	static int is_vma_resv_set(struct vm_area_struct *vma, unsigned long flag);

183

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Mike Kravetz

2023-11-03 02:24 UTC #10 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Edward Adam Davis

Quoted 3 lines · Edward Adam Davis

When obtaining resv_map from vma, it is necessary to simultaneously determine the flag HPAGE_RESV_OWNER of vm_private_data.
Only when they are met simultaneously, resv_map is valid.

Thanks for looking into this!

The check for HPAGE_RESV_OWNER does 'work'. However, I believe root cause is this block of code in __unmap_hugepage_range().

		/*
		 * If a reference page is supplied, it is because a specific
		 * page is being unmapped, not a range. Ensure the page we
		 * are about to unmap is the actual page of interest.
		 */
		if (ref_page) {
			if (page != ref_page) {
				spin_unlock(ptl);
				continue;
			}
			/*
			 * Mark the VMA as having unmapped its page so that
			 * future faults in this VMA will fail rather than
			 * looking like data was lost
			 */
			set_vma_resv_flags(vma, HPAGE_RESV_UNMAPPED);
		}

In the specific case causing the null-ptr-deref, the resv_map pointer (vm_private_data) is NULL. So, set_vma_resv_flags() just sets the lower bit. Because of this, __vma_private_lock returns true.

As mentioned, the check for HPAGE_RESV_OWNER in this patch 'works' because only the HPAGE_RESV_UNMAPPED bit is set in vm_private_data.

I was thinking a more explicit check for this 'NULL pointer' with lower bits set could be made in __vma_private_lock. Below is something I put together. I just open coded the check for 'NULL pointer' instead of moving a bunch of code to the header file. In addition, I changed the #defines from HPAGE_* to HUGETLB_* to avoid any confusion as the header file defines are in the global name space. I thought about also adding an explicit check for the HPAGE_RESV_OWNER as done in this patch. This would catch the case where the pointer is not NULL. I do not believe that is possible in the code today, but might make the check more future proof.

include/linux/hugetlb.h +12 -1

index 47d25a5e1933..7b472432708e 100644
@@ -1260,6 +1260,15 @@ bool want_pmd_share(struct vm_area_struct *vma, unsigned long addr);
 #define flush_hugetlb_tlb_range(vma, addr, end)	flush_tlb_range(vma, addr, end)
 #endif
 
+/*
+ * Flags for MAP_PRIVATE reservations.  These are stored in the bottom
+ * bits of the reservation map pointer, which are always clear due to
+ * alignment.
+ */
+#define HUGETLB_RESV_OWNER    (1UL << 0)
+#define HUGETLB_RESV_UNMAPPED (1UL << 1)
+#define HUGETLB_RESV_MASK (HUGETLB_RESV_OWNER | HUGETLB_RESV_UNMAPPED)
+
 static inline bool __vma_shareable_lock(struct vm_area_struct *vma)
 {
 	return (vma->vm_flags & VM_MAYSHARE) && vma->vm_private_data;

@@ -1267,7 +1276,9 @@ static inline bool __vma_shareable_lock(struct vm_area_struct *vma)
 
 static inline bool __vma_private_lock(struct vm_area_struct *vma)
 {
-	return (!(vma->vm_flags & VM_MAYSHARE)) && vma->vm_private_data;
+	/* Careful - flags may be set in lower bits of pointer */
+	return (!(vma->vm_flags & VM_MAYSHARE)) &&
+		(unsigned long)vma->vm_private_data & ~HUGETLB_RESV_MASK;
 }
 
 /*

mm/hugetlb.c +13 ~~-22~~

index 1301ba7b2c9a..d2215f7647b1 100644
@@ -1028,15 +1028,6 @@ __weak unsigned long vma_mmu_pagesize(struct vm_area_struct *vma)
 	return vma_kernel_pagesize(vma);
 }
 
-/*
- * Flags for MAP_PRIVATE reservations.  These are stored in the bottom
- * bits of the reservation map pointer, which are always clear due to
- * alignment.
- */
-#define HPAGE_RESV_OWNER    (1UL << 0)
-#define HPAGE_RESV_UNMAPPED (1UL << 1)
-#define HPAGE_RESV_MASK (HPAGE_RESV_OWNER | HPAGE_RESV_UNMAPPED)
-
 /*
  * These helpers are used to track how many pages are reserved for
  * faults in a MAP_PRIVATE mapping. Only the process that called mmap()

@@ -1162,7 +1153,7 @@ static struct resv_map *vma_resv_map(struct vm_area_struct *vma)
 
 	} else {
 		return (struct resv_map *)(get_vma_private_data(vma) &
-							~HPAGE_RESV_MASK);
+							~HUGETLB_RESV_MASK);
 	}
 }

@@ -1236,7 +1227,7 @@ void clear_vma_resv_huge_pages(struct vm_area_struct *vma)
 	 */
 	struct resv_map *reservations = vma_resv_map(vma);
 
-	if (reservations && is_vma_resv_set(vma, HPAGE_RESV_OWNER)) {
+	if (reservations && is_vma_resv_set(vma, HUGETLB_RESV_OWNER)) {
 		resv_map_put_hugetlb_cgroup_uncharge_info(reservations);
 		kref_put(&reservations->refs, resv_map_release);
 	}

@@ -1282,7 +1273,7 @@ static bool vma_has_reserves(struct vm_area_struct *vma, long chg)
 	 * Only the process that called mmap() has reserves for
 	 * private mappings.
 	 */
-	if (is_vma_resv_set(vma, HPAGE_RESV_OWNER)) {
+	if (is_vma_resv_set(vma, HUGETLB_RESV_OWNER)) {
 		/*
 		 * Like the shared case above, a hole punch or truncate
 		 * could have been performed on the private mapping.

@@ -2763,7 +2754,7 @@ static long __vma_reservation_common(struct hstate *h,
 	if (vma->vm_flags & VM_MAYSHARE || mode == VMA_DEL_RESV)
 		return ret;
 	/*
-	 * We know private mapping must have HPAGE_RESV_OWNER set.
+	 * We know private mapping must have HUGETLB_RESV_OWNER set.
 	 *
 	 * In most cases, reserves always exist for private mappings.
 	 * However, a file associated with mapping could have been

@@ -4833,7 +4824,7 @@ static void hugetlb_vm_op_open(struct vm_area_struct *vma)
 	struct resv_map *resv = vma_resv_map(vma);
 
 	/*
-	 * HPAGE_RESV_OWNER indicates a private mapping.
+	 * HUGETLB_RESV_OWNER indicates a private mapping.
 	 * This new VMA should share its siblings reservation map if present.
 	 * The VMA will only ever have a valid reservation map pointer where
 	 * it is being copied for another still existing VMA.  As that VMA

@@ -4841,7 +4832,7 @@ static void hugetlb_vm_op_open(struct vm_area_struct *vma)
 	 * after this open call completes.  It is therefore safe to take a
 	 * new reference here without additional locking.
 	 */
-	if (resv && is_vma_resv_set(vma, HPAGE_RESV_OWNER)) {
+	if (resv && is_vma_resv_set(vma, HUGETLB_RESV_OWNER)) {
 		resv_map_dup_hugetlb_cgroup_uncharge_info(resv);
 		kref_get(&resv->refs);
 	}

@@ -4877,7 +4868,7 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)
 	hugetlb_vma_lock_free(vma);
 
 	resv = vma_resv_map(vma);
-	if (!resv || !is_vma_resv_set(vma, HPAGE_RESV_OWNER))
+	if (!resv || !is_vma_resv_set(vma, HUGETLB_RESV_OWNER))
 		return;
 
 	start = vma_hugecache_offset(h, vma, vma->vm_start);

@@ -5394,7 +5385,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 			 * future faults in this VMA will fail rather than
 			 * looking like data was lost
 			 */
-			set_vma_resv_flags(vma, HPAGE_RESV_UNMAPPED);
+			set_vma_resv_flags(vma, HUGETLB_RESV_UNMAPPED);
 		}
 
 		pte = huge_ptep_get_and_clear(mm, address, ptep);

@@ -5544,7 +5535,7 @@ static void unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
 		 * could insert a zeroed page instead of the data existing
 		 * from the time of fork. This would look like data corruption
 		 */
-		if (!is_vma_resv_set(iter_vma, HPAGE_RESV_OWNER))
+		if (!is_vma_resv_set(iter_vma, HUGETLB_RESV_OWNER))
 			unmap_hugepage_range(iter_vma, address,
 					     address + huge_page_size(h), page, 0);
 	}

@@ -5625,7 +5616,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 	 * at the time of fork() could consume its reserves on COW instead
 	 * of the full address range.
 	 */
-	if (is_vma_resv_set(vma, HPAGE_RESV_OWNER) &&
+	if (is_vma_resv_set(vma, HUGETLB_RESV_OWNER) &&
 			old_folio != pagecache_folio)
 		outside_reserve = 1;

@@ -5865,7 +5856,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
 	 * COW/unsharing. Warn that such a situation has occurred as it may not
 	 * be obvious.
 	 */
-	if (is_vma_resv_set(vma, HPAGE_RESV_UNMAPPED)) {
+	if (is_vma_resv_set(vma, HUGETLB_RESV_UNMAPPED)) {
 		pr_warn_ratelimited("PID %d killed due to inadequate hugepage pool\n",
 			   current->pid);
 		goto out;

@@ -6756,7 +6747,7 @@ bool hugetlb_reserve_pages(struct inode *inode,
 		chg = to - from;
 
 		set_vma_resv_map(vma, resv_map);
-		set_vma_resv_flags(vma, HPAGE_RESV_OWNER);
+		set_vma_resv_flags(vma, HUGETLB_RESV_OWNER);
 	}
 
 	if (chg < 0)

@@ -6853,7 +6844,7 @@ bool hugetlb_reserve_pages(struct inode *inode,
 		 */
 		if (chg >= 0 && add < 0)
 			region_abort(resv_map, from, to, regions_needed);
-	if (vma && is_vma_resv_set(vma, HPAGE_RESV_OWNER)) {
+	if (vma && is_vma_resv_set(vma, HUGETLB_RESV_OWNER)) {
 		kref_put(&resv_map->refs, resv_map_release);
 		set_vma_resv_map(vma, NULL);
 	}

Mike Kravetz

2023-11-03 02:28 UTC #11 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

Quoted 28 lines · Mike Kravetz

Quoted 3 lines · Edward Adam Davis
When obtaining resv_map from vma, it is necessary to simultaneously determine the flag HPAGE_RESV_OWNER of vm_private_data.
Only when they are met simultaneously, resv_map is valid.
Thanks for looking into this!
The check for HPAGE_RESV_OWNER does 'work'. However, I believe root cause is this block of code in __unmap_hugepage_range().
		/*
		 * If a reference page is supplied, it is because a specific
		 * page is being unmapped, not a range. Ensure the page we
		 * are about to unmap is the actual page of interest.
		 */
		if (ref_page) {
			if (page != ref_page) {
				spin_unlock(ptl);
				continue;
			}
			/*
			 * Mark the VMA as having unmapped its page so that
			 * future faults in this VMA will fail rather than
			 * looking like data was lost
			 */
			set_vma_resv_flags(vma, HPAGE_RESV_UNMAPPED);
		}

Quoted 3 lines

In the specific case causing the null-ptr-deref, the resv_map pointer (vm_private_data) is NULL. So, set_vma_resv_flags() just sets the lower bit. Because of this, __vma_private_lock returns true.

Ah!

I see Yin, Fengwei already discovered this code path.

Signature · 1 line

Mike Kravetz

Mike Kravetz

2023-11-03 02:37 UTC #12 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Mike Kravetz

Quoted 3 lines · Mike Kravetz

In the specific case causing the null-ptr-deref, the resv_map pointer (vm_private_data) is NULL.

Hi Rik,

In commit bf4916922c60 hugetlbfs: extend hugetlb_vma_lock to private VMAs, it correctly says:

Extend the locking scheme used to protect shared hugetlb mappings from truncate vs page fault races, in order to protect private hugetlb mappings (with resv_map) against MADV_DONTNEED.

That qualification '(with resv_map)' caught my attention originally, and I thought about it again while looking into this. We now cover the common cases, but there are still quite a few cases where resv_map is NULL for private mappings. In such cases, the race between MADV_DONTNEED and page fault still exists. Is that a concern?

With a bit more work we 'could' make sure every hugetlb vma has a lock to participate in this scheme.

Any thhoughts?

Signature · 1 line

Mike Kravetz

Rik van Riel

2023-11-03 03:16 UTC #13 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

Quoted 18 lines · Mike Kravetz

Quoted 4 lines · Mike Kravetz
In the specific case causing the null-ptr-deref, the resv_map pointer
(vm_private_data) is NULL.
Hi Rik,
In commit bf4916922c60 hugetlbfs: extend hugetlb_vma_lock to private VMAs,
it correctly says:
    Extend the locking scheme used to protect shared hugetlb mappings from
    truncate vs page fault races, in order to protect private hugetlb mappings
    (with resv_map) against MADV_DONTNEED.

Quoted 9 lines

That qualification '(with resv_map)' caught my attention originally, and
I thought about it again while looking into this. We now cover the common
cases, but there are still quite a few cases where resv_map is NULL for
private mappings. In such cases, the race between MADV_DONTNEED and page
fault still exists. Is that a concern?

Honestly, I'm not sure. In hugetlb_dup_vma_private, which is called at fork time, we have this comment:

* - For MAP_PRIVATE mappings, this is the reserve map which
does
* not apply to children. Faults generated by the children
are
* not guaranteed to succeed, even if read-only.

That suggests we already have no guarantee of faults succeeding after fork.

Quoted 6 lines

With a bit more work we 'could' make sure every hugetlb vma has a lock
to participate in this scheme.
Any thhoughts?

We can certainly close the race between MADV_DONTNEED and page faults for MAP_PRIVATE mappings in child processes, but that does not guarantee that we actually have hugetlb pages for those processes.

In short, I'm not sure :)

Signature · 1 line

All Rights Reversed.

Mike Kravetz

2023-11-03 04:31 UTC #14 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

Quoted 15 lines · Rik van Riel

Quoted 10 lines · Mike Kravetz
On 11/02/23 19:24, Mike Kravetz wrote:
That qualification '(with resv_map)' caught my attention originally, and
I thought about it again while looking into this. We now cover the common
cases, but there are still quite a few cases where resv_map is NULL for
private mappings. In such cases, the race between MADV_DONTNEED and page
fault still exists. Is that a concern?
Honestly, I'm not sure. In hugetlb_dup_vma_private, which is called at fork time, we have this comment:

Quoted 8 lines

* - For MAP_PRIVATE mappings, this is the reserve map which
does
* not apply to children. Faults generated by the children
are
* not guaranteed to succeed, even if read-only.
That suggests we already have no guarantee of faults succeeding after fork.

Right!

Quoted 6 lines

Quoted 6 lines
With a bit more work we 'could' make sure every hugetlb vma has a lock
to participate in this scheme.

Quoted 8 lines

Quoted 1 line
Any thhoughts?
We can certainly close the race between MADV_DONTNEED and page faults for MAP_PRIVATE mappings in child processes, but that does not guarantee that we actually have hugetlb pages for those processes.
In short, I'm not sure :)

I sort of remember something Dave Hansen added years ago to help a customer allocating LOTs of hugetlb pages dynamically. I seem to recall that this was to get better numa locality. As a result, they did not use reservations.

I guess it sticks with me because it was/is a real example of a customer choosing NOT to use reservations.

I don't have any evidence that this is common. My thought is to leave it as is until someone complains.

Signature · 1 line

Mike Kravetz

Mike Kravetz

2023-11-08 03:23 UTC #15 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Edward Adam Davis

Hello Edward,

Do you plan on following up with a new version of the patch?

My suggestion would be:
- Move __vma_private_lock into hugetlb.c as suggeted by Rik.
- Change __vma_private_lock to check for NULL resv_map even if low bit flags are set as well as HPAGE_RESV_OWNER.

Signature · 1 line

Mike Kravetz

Yin, Fengwei

2023-11-02 13:46 UTC #16 lore raw

Re: [PATCH] mm/hugetlb: fix null ptr defer in hugetlb_vma_lock_write

In reply to Rik van Riel

Quoted 28 lines · Rik van Riel

Quoted 27 lines · Edward Adam Davis
When obtaining resv_map from vma, it is necessary to simultaneously determine
the flag HPAGE_RESV_OWNER of vm_private_data.
Only when they are met simultaneously, resv_map is valid.
Reported-and-tested-by:
[email protected]
Fixes bf4916922c60 ("hugetlbfs: extend hugetlb_vma_lock to private
VMAs")
Signed-off-by Edward Adam Davis <[email protected]>
---
  include/linux/hugetlb.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
include/linux/hugetlb.h +1 -1
index 47d25a5e1933..1a3ec1aee1a3 100644
@@ -1265,9 +1265,11 @@ static inline bool __vma_shareable_lock(struct
 vm_area_struct *vma)
         return (vma->vm_flags & VM_MAYSHARE) && vma->vm_private_data;
  }
 
+#define HPAGE_RESV_OWNER    (1UL << 0)
  static inline bool __vma_private_lock(struct vm_area_struct *vma)
  {
-       return (!(vma->vm_flags & VM_MAYSHARE)) && vma-

Quoted 6 lines

Quoted 6 lines
include/linux/hugetlb.h +1 -0
 > vm_private_data;
+       return (!(vma->vm_flags & VM_MAYSHARE)) && vma-
Quoted 1 line
vm_private_data &&
+ ((unsigned long)vma->vm_private_data & HPAGE_RESV_OWNER);
}

I am wondering whether this line should be:
return (!(vma->vm_flags & VM_MAYSHARE)) &&

	((unsigned long)vma->vm_private_data & HPAGE_RESV_OWNER);

or even:
return (unsigned long)vma->vm_private_data & HPAGE_RESV_OWNER;

because mm/hugetlb.c has:
"We know private mapping must have HPAGE_RESV_OWNER set."

Quoted 8 lines

This could be cleaned up a bit by moving the HPAGE_RESV_OWNER definition (and its friends) into hugetlb.h, as well as the is_vma_resv_set() helper function.
Then __vma_private_lock() can just call is_vma_resv_set(), and open coding a duplicate of the same code.

Quoted 7 lines

Not having duplicates of the code will make it much harder to "miss a spot" with future changes.
I am still struggling to find a place where we might leave HPAGE_RESV_OWNER behind on a pointer that is otherwise NULL, but if your tests show this fixes the issue, I'm all for it :)

Like you said, vma->vm_private_data is cleared during fork().

But I saw following possible code path in child process:
hugetlb_wp()
unmap_ref_private() if alloc_hugetlb_folio fails
unmap_hugepage_range()
__unmap_hugepage_range()
set_vma_resv_flags(vma, HPAGE_RESV_UNMAPPED)

vm_private_data become 0x2 which is none NULL without HPAGE_RESV_OWNER bit.