Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On 04/04/2018 23:53, David Rientjes wrote: > On Wed, 4 Apr 2018, Laurent Dufour wrote: > >>> I also think the following is needed: >>> >>> diff --git a/fs/exec.c b/fs/exec.c >>> --- a/fs/exec.c >>> +++ b/fs/exec.c >>> @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm) >>> vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | >>> VM_STACK_INCOMPLETE_SETUP; >>> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); >>> INIT_LIST_HEAD(>anon_vma_chain); >>> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT >>> + seqcount_init(>vm_sequence); >>> + atomic_set(>vm_ref_count, 0); >>> +#endif >>> >>> err = insert_vm_struct(mm, vma); >>> if (err) >> >> No, this not needed because the vma is allocated with kmem_cache_zalloc() so >> vm_ref_count is 0, and insert_vm_struc() will later call >> __vma_link_rb() which will call seqcount_init(). >> >> Furhtermore, in case of error, the vma structure is freed without calling >> get_vma() so there is risk of lockdep warning. >> > > Perhaps you're working from a different tree than I am, or you fixed the > lockdep warning differently when adding to dup_mmap() and mmap_region(). > > I got the following two lockdep errors. > > I fixed it locally by doing the seqcount_init() and atomic_set() > everywhere a vma could be initialized. That's weird, I don't get that on my side with lockdep activated. There is a call to seqcount_init() in dup_mmap(), in mmap_region() and __vma_link_rb() and that's enough to cover all the case. That's being said, it'll be better call seqcount_init each time as soon as a vma structure is allocated. For the vm_ref_count value, as most of the time the vma is zero allocated, I don't think this is needed. I just have to check when new_vma = *old_vma is done, but this often just follow a vma allocation. > > INFO: trying to register non-static key. > the code is fine but needs lockdep annotation. > turning off the locking correctness validator. > CPU: 12 PID: 1 Comm: init Not tainted > Call Trace: > [] dump_stack+0x67/0x98 > [] register_lock_class+0x1e6/0x4e0 > [] __lock_acquire+0xb9/0x1710 > [] lock_acquire+0xba/0x200 > [] mprotect_fixup+0x10f/0x310 > [] setup_arg_pages+0x12d/0x230 > [] load_elf_binary+0x44a/0x1740 > [] search_binary_handler+0x9b/0x1e0 > [] load_script+0x206/0x270 > [] search_binary_handler+0x9b/0x1e0 > [] do_execveat_common.isra.32+0x6b5/0x9d0 > [] do_execve+0x2c/0x30 > [] run_init_process+0x2b/0x30 > [] kernel_init+0x54/0x110 > [] ret_from_fork+0x3a/0x50 > > and > > INFO: trying to register non-static key. > the code is fine but needs lockdep annotation. > turning off the locking correctness validator. > CPU: 21 PID: 1926 Comm: mkdir Not tainted > Call Trace: > [] dump_stack+0x67/0x98 > [] register_lock_class+0x1e6/0x4e0 > [] __lock_acquire+0xb9/0x1710 > [] lock_acquire+0xba/0x200 > [] unmap_page_range+0x89/0xaa0 > [] unmap_single_vma+0x8f/0x100 > [] unmap_vmas+0x4b/0x90 > [] exit_mmap+0xa3/0x1c0 > [] mmput+0x73/0x120 > [] do_exit+0x2bd/0xd60 > [] SyS_exit+0x17/0x20 > [] do_syscall_64+0x6d/0x1a0 > [] entry_SYSCALL_64_after_hwframe+0x26/0x9b > > I think it would just be better to generalize vma allocation to initialize > certain fields and init both spf fields properly for > CONFIG_SPECULATIVE_PAGE_FAULT. It's obviously too delicate as is. >
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On Wed, 4 Apr 2018, Laurent Dufour wrote: > > I also think the following is needed: > > > > diff --git a/fs/exec.c b/fs/exec.c > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm) > > vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | > > VM_STACK_INCOMPLETE_SETUP; > > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > > INIT_LIST_HEAD(>anon_vma_chain); > > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > > + seqcount_init(>vm_sequence); > > + atomic_set(>vm_ref_count, 0); > > +#endif > > > > err = insert_vm_struct(mm, vma); > > if (err) > > No, this not needed because the vma is allocated with kmem_cache_zalloc() so > vm_ref_count is 0, and insert_vm_struc() will later call > __vma_link_rb() which will call seqcount_init(). > > Furhtermore, in case of error, the vma structure is freed without calling > get_vma() so there is risk of lockdep warning. > Perhaps you're working from a different tree than I am, or you fixed the lockdep warning differently when adding to dup_mmap() and mmap_region(). I got the following two lockdep errors. I fixed it locally by doing the seqcount_init() and atomic_set() everywhere a vma could be initialized. INFO: trying to register non-static key. the code is fine but needs lockdep annotation. turning off the locking correctness validator. CPU: 12 PID: 1 Comm: init Not tainted Call Trace: [] dump_stack+0x67/0x98 [] register_lock_class+0x1e6/0x4e0 [] __lock_acquire+0xb9/0x1710 [] lock_acquire+0xba/0x200 [] mprotect_fixup+0x10f/0x310 [] setup_arg_pages+0x12d/0x230 [] load_elf_binary+0x44a/0x1740 [] search_binary_handler+0x9b/0x1e0 [] load_script+0x206/0x270 [] search_binary_handler+0x9b/0x1e0 [] do_execveat_common.isra.32+0x6b5/0x9d0 [] do_execve+0x2c/0x30 [] run_init_process+0x2b/0x30 [] kernel_init+0x54/0x110 [] ret_from_fork+0x3a/0x50 and INFO: trying to register non-static key. the code is fine but needs lockdep annotation. turning off the locking correctness validator. CPU: 21 PID: 1926 Comm: mkdir Not tainted Call Trace: [] dump_stack+0x67/0x98 [] register_lock_class+0x1e6/0x4e0 [] __lock_acquire+0xb9/0x1710 [] lock_acquire+0xba/0x200 [] unmap_page_range+0x89/0xaa0 [] unmap_single_vma+0x8f/0x100 [] unmap_vmas+0x4b/0x90 [] exit_mmap+0xa3/0x1c0 [] mmput+0x73/0x120 [] do_exit+0x2bd/0xd60 [] SyS_exit+0x17/0x20 [] do_syscall_64+0x6d/0x1a0 [] entry_SYSCALL_64_after_hwframe+0x26/0x9b I think it would just be better to generalize vma allocation to initialize certain fields and init both spf fields properly for CONFIG_SPECULATIVE_PAGE_FAULT. It's obviously too delicate as is.
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On 04/04/2018 03:03, David Rientjes wrote: > On Tue, 3 Apr 2018, David Rientjes wrote: > > I found the root cause of this lockdep warning. > > In mmap_region(), unmap_region() may be called while vma_link() has not > been > called. This happens during the error path if call_mmap() failed. > > The only to fix that particular case is to call > seqcount_init(>vm_sequence) when initializing the vma in > mmap_region(). > Ack, although that would require a fixup to dup_mmap() as well. >>> >>> You're right, I'll fix that too. >>> >> >> I also think the following is needed: >> >> diff --git a/fs/exec.c b/fs/exec.c >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm) >> vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | >> VM_STACK_INCOMPLETE_SETUP; >> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); >> INIT_LIST_HEAD(>anon_vma_chain); >> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT >> +seqcount_init(>vm_sequence); >> +atomic_set(>vm_ref_count, 0); >> +#endif >> >> err = insert_vm_struct(mm, vma); >> if (err) >> > > Ugh, I think there are a number of other places where this is needed as > well in mm/mmap.c. I think it would be better to just create a new > alloc_vma(unsigned long flags) that all vma allocators can use and for > CONFIG_SPECULATIVE_PAGE_FAULT will initialize the seqcount_t and atomic_t. > I don't think this is really needed, most of the time the vma structure is allocated cleared and is then link to rb tree via vma_link. The only case generating a locked warning is when the vma is referenced in the error path before being linked in the mm tree and that is not usual.
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On 04/04/2018 02:48, David Rientjes wrote: > On Wed, 28 Mar 2018, Laurent Dufour wrote: > >> On 26/03/2018 00:10, David Rientjes wrote: >>> On Wed, 21 Mar 2018, Laurent Dufour wrote: >>> I found the root cause of this lockdep warning. In mmap_region(), unmap_region() may be called while vma_link() has not been called. This happens during the error path if call_mmap() failed. The only to fix that particular case is to call seqcount_init(>vm_sequence) when initializing the vma in mmap_region(). >>> >>> Ack, although that would require a fixup to dup_mmap() as well. >> >> You're right, I'll fix that too. >> > > I also think the following is needed: > > diff --git a/fs/exec.c b/fs/exec.c > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm) > vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | > VM_STACK_INCOMPLETE_SETUP; > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > INIT_LIST_HEAD(>anon_vma_chain); > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > + seqcount_init(>vm_sequence); > + atomic_set(>vm_ref_count, 0); > +#endif > > err = insert_vm_struct(mm, vma); > if (err) No, this not needed because the vma is allocated with kmem_cache_zalloc() so vm_ref_count is 0, and insert_vm_struc() will later call __vma_link_rb() which will call seqcount_init(). Furhtermore, in case of error, the vma structure is freed without calling get_vma() so there is risk of lockdep warning.
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On Tue, 3 Apr 2018, David Rientjes wrote: > > >> I found the root cause of this lockdep warning. > > >> > > >> In mmap_region(), unmap_region() may be called while vma_link() has not > > >> been > > >> called. This happens during the error path if call_mmap() failed. > > >> > > >> The only to fix that particular case is to call > > >> seqcount_init(>vm_sequence) when initializing the vma in > > >> mmap_region(). > > >> > > > > > > Ack, although that would require a fixup to dup_mmap() as well. > > > > You're right, I'll fix that too. > > > > I also think the following is needed: > > diff --git a/fs/exec.c b/fs/exec.c > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm) > vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | > VM_STACK_INCOMPLETE_SETUP; > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > INIT_LIST_HEAD(>anon_vma_chain); > +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT > + seqcount_init(>vm_sequence); > + atomic_set(>vm_ref_count, 0); > +#endif > > err = insert_vm_struct(mm, vma); > if (err) > Ugh, I think there are a number of other places where this is needed as well in mm/mmap.c. I think it would be better to just create a new alloc_vma(unsigned long flags) that all vma allocators can use and for CONFIG_SPECULATIVE_PAGE_FAULT will initialize the seqcount_t and atomic_t.
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On Wed, 28 Mar 2018, Laurent Dufour wrote: > On 26/03/2018 00:10, David Rientjes wrote: > > On Wed, 21 Mar 2018, Laurent Dufour wrote: > > > >> I found the root cause of this lockdep warning. > >> > >> In mmap_region(), unmap_region() may be called while vma_link() has not > >> been > >> called. This happens during the error path if call_mmap() failed. > >> > >> The only to fix that particular case is to call > >> seqcount_init(>vm_sequence) when initializing the vma in > >> mmap_region(). > >> > > > > Ack, although that would require a fixup to dup_mmap() as well. > > You're right, I'll fix that too. > I also think the following is needed: diff --git a/fs/exec.c b/fs/exec.c --- a/fs/exec.c +++ b/fs/exec.c @@ -312,6 +312,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm) vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP; vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); INIT_LIST_HEAD(>anon_vma_chain); +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT + seqcount_init(>vm_sequence); + atomic_set(>vm_ref_count, 0); +#endif err = insert_vm_struct(mm, vma); if (err)
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On 26/03/2018 00:10, David Rientjes wrote: > On Wed, 21 Mar 2018, Laurent Dufour wrote: > >> I found the root cause of this lockdep warning. >> >> In mmap_region(), unmap_region() may be called while vma_link() has not been >> called. This happens during the error path if call_mmap() failed. >> >> The only to fix that particular case is to call >> seqcount_init(>vm_sequence) when initializing the vma in mmap_region(). >> > > Ack, although that would require a fixup to dup_mmap() as well. You're right, I'll fix that too. Thanks a lot. Laurent.
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On Wed, 21 Mar 2018, Laurent Dufour wrote: > I found the root cause of this lockdep warning. > > In mmap_region(), unmap_region() may be called while vma_link() has not been > called. This happens during the error path if call_mmap() failed. > > The only to fix that particular case is to call > seqcount_init(>vm_sequence) when initializing the vma in mmap_region(). > Ack, although that would require a fixup to dup_mmap() as well.
Re: [mm] b1f0502d04: INFO:trying_to_register_non-static_key
On 17/03/2018 08:51, kernel test robot wrote:
> FYI, we noticed the following commit (built with gcc-7):
>
> commit: b1f0502d04537ef55b0c296823affe332b100eb5 ("mm: VMA sequence count")
> url:
> https://github.com/0day-ci/linux/commits/Laurent-Dufour/Speculative-page-faults/20180316-151833
>
>
> in testcase: trinity
> with following parameters:
>
> runtime: 300s
>
> test-description: Trinity is a linux system call fuzz tester.
> test-url: http://codemonkey.org.uk/projects/trinity/
>
>
> on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -m 512M
>
> caused below changes (please refer to attached dmesg/kmsg for entire
> log/backtrace):
>
>
> ++++
> || 6a4ce82339 | b1f0502d04 |
> ++++
> | boot_successes | 8 | 4 |
> | boot_failures | 0 | 4 |
> | INFO:trying_to_register_non-static_key | 0 | 4 |
> ++++
>
>
>
> [ 22.212940] INFO: trying to register non-static key.
> [ 22.213687] the code is fine but needs lockdep annotation.
> [ 22.214459] turning off the locking correctness validator.
> [ 22.227459] CPU: 0 PID: 547 Comm: trinity-main Not tainted
> 4.16.0-rc4-next-20180309-7-gb1f0502 #239
> [ 22.228904] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.10.2-1 04/01/2014
> [ 22.230043] Call Trace:
> [ 22.230409] dump_stack+0x5d/0x79
> [ 22.231025] register_lock_class+0x226/0x45e
> [ 22.231827] ? kvm_clock_read+0x21/0x30
> [ 22.232544] ? kvm_sched_clock_read+0x5/0xd
> [ 22.20] __lock_acquire+0xa2/0x774
> [ 22.234152] lock_acquire+0x4b/0x66
> [ 22.234805] ? unmap_vmas+0x30/0x3d
> [ 22.245680] unmap_page_range+0x56/0x48c
> [ 22.248127] ? unmap_vmas+0x30/0x3d
> [ 22.248741] ? lru_deactivate_file_fn+0x2c6/0x2c6
> [ 22.249537] ? pagevec_lru_move_fn+0x9a/0xa9
> [ 22.250244] unmap_vmas+0x30/0x3d
> [ 22.250791] unmap_region+0xad/0x105
> [ 22.251419] mmap_region+0x3cc/0x455
> [ 22.252011] do_mmap+0x394/0x3e9
> [ 22.261224] vm_mmap_pgoff+0x9c/0xe5
> [ 22.261798] SyS_mmap_pgoff+0x19a/0x1d4
> [ 22.262475] ? task_work_run+0x5e/0x9c
> [ 22.263163] do_syscall_64+0x6d/0x103
> [ 22.263814] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> [ 22.264697] RIP: 0033:0x4573da
> [ 22.267248] RSP: 002b:7fffa22f1398 EFLAGS: 0246 ORIG_RAX:
> 0009
> [ 22.274720] RAX: ffda RBX: 0001 RCX:
> 004573da
> [ 22.276083] RDX: 0001 RSI: 1000 RDI:
>
> [ 22.277343] RBP: 001c R08: 001c R09:
>
> [ 22.278686] R10: 0002 R11: 0246 R12:
>
> [ 22.279930] R13: 1000 R14: 0002 R15:
>
> [ 22.391866] trinity-main uses obsolete (PF_INET,SOCK_PACKET)
> [ 327.566956] sysrq: SysRq : Emergency Sync
> [ 327.567849] Emergency Sync complete
> [ 327.569975] sysrq: SysRq : Resetting
I found the root cause of this lockdep warning.
In mmap_region(), unmap_region() may be called while vma_link() has not been
called. This happens during the error path if call_mmap() failed.
The only to fix that particular case is to call
seqcount_init(>vm_sequence) when initializing the vma in mmap_region().
Thanks,
Laurent.
