Re: Bug with report THP eligibility for each vma
On Mon 24-12-18 14:12:51, Mike Rapoport wrote: > On Mon, Dec 24, 2018 at 08:49:16AM +0100, Michal Hocko wrote: > > [Cc-ing mailing list and people involved in the original patch] > > > > On Fri 21-12-18 13:42:24, Paul Oppenheimer wrote: > > > Hello! I've never reported a kernel bug before, and since its on the > > > "next" tree I was told to email the author of the relevant commit. > > > Please redirect me to the correct place if I've made a mistake. > > > > > > When opening firefox or chrome, and using it for a good 7 seconds, it > > > hangs in "uninterruptible sleep" and I recieve a "BUG" in dmesg. This > > > doesn't occur when reverting this commit: > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=48cf516f8c. > > > Ive attached the output of decode_stacktrace.sh and the relevant dmesg > > > log to this email. > > > > > > Thanks > > > > > BUG: unable to handle kernel NULL pointer dereference at 00e8 > > > > Thanks for the bug report! This is offset 232 and that matches > > file->f_mapping as per pahole > > pahole -C file ./vmlinux | grep f_mapping > > struct address_space * f_mapping;/* 232 8 */ > > > > I thought that each file really has to have a mapping. But the following > > should heal the issue and add an extra care. > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index f64733c23067..fc9d70a9fbd1 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -66,6 +66,8 @@ bool transparent_hugepage_enabled(struct vm_area_struct > > *vma) > > { > > if (vma_is_anonymous(vma)) > > return __transparent_hugepage_enabled(vma); > > + if (!vma->vm_file || !vma->vm_file->f_mapping) > > + return false; > > if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma)) > > return __transparent_hugepage_enabled(vma); > > We have vma_is_shmem(), it can be used to replace shmem_mapping() without > adding the check for !vma->vm_file Yes, this looks like a much better choice. Thanks! Andrew, could you fold this in instead. diff --git a/mm/huge_memory.c b/mm/huge_memory.c index f64733c23067..e093cf5e4640 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -66,7 +66,7 @@ bool transparent_hugepage_enabled(struct vm_area_struct *vma) { if (vma_is_anonymous(vma)) return __transparent_hugepage_enabled(vma); - if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma)) + if (vma_is_shmem(vma) && shmem_huge_enabled(vma)) return __transparent_hugepage_enabled(vma); return false; -- Michal Hocko SUSE Labs
Re: Bug with report THP eligibility for each vma
On Mon, Dec 24, 2018 at 08:49:16AM +0100, Michal Hocko wrote: > [Cc-ing mailing list and people involved in the original patch] > > On Fri 21-12-18 13:42:24, Paul Oppenheimer wrote: > > Hello! I've never reported a kernel bug before, and since its on the > > "next" tree I was told to email the author of the relevant commit. > > Please redirect me to the correct place if I've made a mistake. > > > > When opening firefox or chrome, and using it for a good 7 seconds, it > > hangs in "uninterruptible sleep" and I recieve a "BUG" in dmesg. This > > doesn't occur when reverting this commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=48cf516f8c. > > Ive attached the output of decode_stacktrace.sh and the relevant dmesg > > log to this email. > > > > Thanks > > > BUG: unable to handle kernel NULL pointer dereference at 00e8 > > Thanks for the bug report! This is offset 232 and that matches > file->f_mapping as per pahole > pahole -C file ./vmlinux | grep f_mapping > struct address_space * f_mapping;/* 232 8 */ > > I thought that each file really has to have a mapping. But the following > should heal the issue and add an extra care. > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index f64733c23067..fc9d70a9fbd1 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -66,6 +66,8 @@ bool transparent_hugepage_enabled(struct vm_area_struct > *vma) > { > if (vma_is_anonymous(vma)) > return __transparent_hugepage_enabled(vma); > + if (!vma->vm_file || !vma->vm_file->f_mapping) > + return false; > if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma)) > return __transparent_hugepage_enabled(vma); We have vma_is_shmem(), it can be used to replace shmem_mapping() without adding the check for !vma->vm_file > > Andrew, could you fold it to the original patch please? > > Keeping the rest for the reference. > > > #PF error: [normal kernel read fault] > > PGD 0 P4D 0 > > Oops: [#1] PREEMPT SMP PTI > > CPU: 7 PID: 2687 Comm: StreamTrans #56 Tainted: G U > > 4.20.0-rc7-next-20181221-beppy+ #15 > > Hardware name: Dell Inc. XPS 13 9360/0TPN17, BIOS 2.10.0 09/27/2018 > > RIP: 0010:transparent_hugepage_enabled (??:?) > > Code: 17 fd 00 e9 20 ff ff ff 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 53 48 > > 89 fb 48 83 bf 90 00 00 00 00 74 27 48 8b 87 a0 00 00 00 <48> 8b b8 e8 00 > > 00 00 e8 7a cc fa ff 84 c0 75 04 31 c0 5b c3 48 89 > > All code > > > >0: 17 (bad) > >1: fd std > >2: 00 e9 add%ch,%cl > >4: 20 ff and%bh,%bh > >6: ff (bad) > >7: ff 0f decl (%rdi) > >9: 1f (bad) > >a: 84 00 test %al,(%rax) > >c: 00 00 add%al,(%rax) > >e: 00 00 add%al,(%rax) > > 10: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) > > 15: 53 push %rbx > > 16: 48 89 fbmov%rdi,%rbx > > 19: 48 83 bf 90 00 00 00cmpq $0x0,0x90(%rdi) > > 20: 00 > > 21: 74 27 je 0x4a > > 23: 48 8b 87 a0 00 00 00mov0xa0(%rdi),%rax > > 2a:* 48 8b b8 e8 00 00 00mov0xe8(%rax),%rdi <-- > > trapping instruction > > 31: e8 7a cc fa ff callq 0xfffaccb0 > > 36: 84 c0 test %al,%al > > 38: 75 04 jne0x3e > > 3a: 31 c0 xor%eax,%eax > > 3c: 5b pop%rbx > > 3d: c3 retq > > 3e: 48 rex.W > > 3f: 89 .byte 0x89 > > > > Code starting with the faulting instruction > > === > >0: 48 8b b8 e8 00 00 00mov0xe8(%rax),%rdi > >7: e8 7a cc fa ff callq 0xfffacc86 > >c: 84 c0 test %al,%al > >e: 75 04 jne0x14 > > 10: 31 c0 xor%eax,%eax > > 12: 5b pop%rbx > > 13: c3 retq > > 14: 48 rex.W > > 15: 89 .byte 0x89 > > RSP: 0018:b79744f17d28 EFLAGS: 00010282 > > RAX: RBX: 8948c17aff00 RCX: > > RDX: 0004 RSI: ab1165ba RDI: 8948c17aff00 > > RBP: 8948c17aff00 R08: 0007 R09: 894927e547b2 > > R10: R11: 894927e549da R12: b79744f17d38 > > R13: 8948c17aff00 R14: 89489bef9400 R15: 89488b775a80 > > FS:
Re: Bug with report THP eligibility for each vma
> On Dec 24, 2018, at 12:49 AM, Michal Hocko wrote: > > [Cc-ing mailing list and people involved in the original patch] > > On Fri 21-12-18 13:42:24, Paul Oppenheimer wrote: >> Hello! I've never reported a kernel bug before, and since its on the >> "next" tree I was told to email the author of the relevant commit. >> Please redirect me to the correct place if I've made a mistake. >> >> When opening firefox or chrome, and using it for a good 7 seconds, it >> hangs in "uninterruptible sleep" and I recieve a "BUG" in dmesg. This >> doesn't occur when reverting this commit: >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=48cf516f8c. >> Ive attached the output of decode_stacktrace.sh and the relevant dmesg >> log to this email. >> >> Thanks > >> BUG: unable to handle kernel NULL pointer dereference at 00e8 > > Thanks for the bug report! This is offset 232 and that matches > file->f_mapping as per pahole > pahole -C file ./vmlinux | grep f_mapping >struct address_space * f_mapping;/* 232 8 */ > > I thought that each file really has to have a mapping. But the following > should heal the issue and add an extra care. > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index f64733c23067..fc9d70a9fbd1 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -66,6 +66,8 @@ bool transparent_hugepage_enabled(struct vm_area_struct > *vma) > { > if (vma_is_anonymous(vma)) > return __transparent_hugepage_enabled(vma); > + if (!vma->vm_file || !vma->vm_file->f_mapping) > + return false; > if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma)) > return __transparent_hugepage_enabled(vma); From what I see in code in mm/mmap.c, it seems if vma->vm_file is non-zero vma->vm_file->f_mapping may be assumed to be non-NULL; see unlink_file_vma() and __vma_link_file() for two examples, which both use the construct: file = vma->vm_file; if (file) { struct address_space *mapping = file->f_mapping; [ ... ] [ code that dereferences "mapping" without further checks ] } I see nothing wrong with your second check but a few extra instructions performed, but depending upon how often transparent_hugepage_enabled() is called there may be at least theoretical performance concerns. William Kucharski william.kuchar...@oracle.com
Re: Bug with report THP eligibility for each vma
[Cc-ing mailing list and people involved in the original patch] On Fri 21-12-18 13:42:24, Paul Oppenheimer wrote: > Hello! I've never reported a kernel bug before, and since its on the > "next" tree I was told to email the author of the relevant commit. > Please redirect me to the correct place if I've made a mistake. > > When opening firefox or chrome, and using it for a good 7 seconds, it > hangs in "uninterruptible sleep" and I recieve a "BUG" in dmesg. This > doesn't occur when reverting this commit: > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=48cf516f8c. > Ive attached the output of decode_stacktrace.sh and the relevant dmesg > log to this email. > > Thanks > BUG: unable to handle kernel NULL pointer dereference at 00e8 Thanks for the bug report! This is offset 232 and that matches file->f_mapping as per pahole pahole -C file ./vmlinux | grep f_mapping struct address_space * f_mapping;/* 232 8 */ I thought that each file really has to have a mapping. But the following should heal the issue and add an extra care. diff --git a/mm/huge_memory.c b/mm/huge_memory.c index f64733c23067..fc9d70a9fbd1 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -66,6 +66,8 @@ bool transparent_hugepage_enabled(struct vm_area_struct *vma) { if (vma_is_anonymous(vma)) return __transparent_hugepage_enabled(vma); + if (!vma->vm_file || !vma->vm_file->f_mapping) + return false; if (shmem_mapping(vma->vm_file->f_mapping) && shmem_huge_enabled(vma)) return __transparent_hugepage_enabled(vma); Andrew, could you fold it to the original patch please? Keeping the rest for the reference. > #PF error: [normal kernel read fault] > PGD 0 P4D 0 > Oops: [#1] PREEMPT SMP PTI > CPU: 7 PID: 2687 Comm: StreamTrans #56 Tainted: G U > 4.20.0-rc7-next-20181221-beppy+ #15 > Hardware name: Dell Inc. XPS 13 9360/0TPN17, BIOS 2.10.0 09/27/2018 > RIP: 0010:transparent_hugepage_enabled (??:?) > Code: 17 fd 00 e9 20 ff ff ff 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 53 48 89 > fb 48 83 bf 90 00 00 00 00 74 27 48 8b 87 a0 00 00 00 <48> 8b b8 e8 00 00 00 > e8 7a cc fa ff 84 c0 75 04 31 c0 5b c3 48 89 > All code > >0: 17 (bad) >1: fd std >2: 00 e9 add%ch,%cl >4: 20 ff and%bh,%bh >6: ff (bad) >7: ff 0f decl (%rdi) >9: 1f (bad) >a: 84 00 test %al,(%rax) >c: 00 00 add%al,(%rax) >e: 00 00 add%al,(%rax) > 10: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) > 15: 53 push %rbx > 16: 48 89 fbmov%rdi,%rbx > 19: 48 83 bf 90 00 00 00cmpq $0x0,0x90(%rdi) > 20: 00 > 21: 74 27 je 0x4a > 23: 48 8b 87 a0 00 00 00mov0xa0(%rdi),%rax > 2a:*48 8b b8 e8 00 00 00mov0xe8(%rax),%rdi <-- > trapping instruction > 31: e8 7a cc fa ff callq 0xfffaccb0 > 36: 84 c0 test %al,%al > 38: 75 04 jne0x3e > 3a: 31 c0 xor%eax,%eax > 3c: 5b pop%rbx > 3d: c3 retq > 3e: 48 rex.W > 3f: 89 .byte 0x89 > > Code starting with the faulting instruction > === >0: 48 8b b8 e8 00 00 00mov0xe8(%rax),%rdi >7: e8 7a cc fa ff callq 0xfffacc86 >c: 84 c0 test %al,%al >e: 75 04 jne0x14 > 10: 31 c0 xor%eax,%eax > 12: 5b pop%rbx > 13: c3 retq > 14: 48 rex.W > 15: 89 .byte 0x89 > RSP: 0018:b79744f17d28 EFLAGS: 00010282 > RAX: RBX: 8948c17aff00 RCX: > RDX: 0004 RSI: ab1165ba RDI: 8948c17aff00 > RBP: 8948c17aff00 R08: 0007 R09: 894927e547b2 > R10: R11: 894927e549da R12: b79744f17d38 > R13: 8948c17aff00 R14: 89489bef9400 R15: 89488b775a80 > FS: 7fa54ad43700() GS:8949363c() knlGS: > CS: 0010 DS: ES: CR0: 80050033 > CR2: 00e8 CR3: 00025c0ee003 CR4: 003606e0 > Call Trace: > show_smap (/home/bep/.opt/kernel/linux-e/fs/proc/task_mmu.c:805) > seq_read (/home/bep/.opt/kernel/linux-e/fs/seq_file.c:269) > __vfs_read (/home/bep/.opt/kernel/linux-e/fs/read_write.c:421) > vfs_read (/home/bep/.opt/kernel/linux-e/fs/read_write.c:452 > /home/bep/.opt/kernel/linux-e/fs/read_write.c:437) > ksys_read (/home/bep/.opt/kernel/linux-e/fs/rea