| Message ID | 20200327225102.25061-6-walken@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a new mmap locking API wrapping mmap_sem calls | expand |
> Convert the last few remaining mmap_sem rwsem calls to use the new > mmap locking API. These were missed by coccinelle for some reason Will the clarification of this software situation become more interesting? > (I think coccinelle does not support some of the preprocessor > constructs in these files ?) I suggest to omit this information from the final change description. Would you like to help any more to find nicer solutions for remaining open issues? Regards, Markus
Hi Markus, On Sat, Mar 28, 2020 at 12:37 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > Convert the last few remaining mmap_sem rwsem calls to use the new > > mmap locking API. These were missed by coccinelle for some reason > > Will the clarification of this software situation become more interesting? > > > (I think coccinelle does not support some of the preprocessor > > constructs in these files ?) > > I suggest to omit this information from the final change description. > Would you like to help any more to find nicer solutions > for remaining open issues? So, from a practical perspective I think coccinelle has filled its purpose for me - it got 99% of the job done, and I had to do the last 1% by hand which is not ideal, but really not too bad either. Also, by using coccinelle I think reviewers can appreciate that the change is purely mechanical, and reproduce it on their end if needed, which facilitates the review process greatly. I would be interested to find out why coccinelle wasn't able to do the last 1%, but only as part of a long-term learning process on getting better with coccinelle - I don't consider it a blocker for short-term progress on this patchset.
> I would be interested to find out why coccinelle wasn't able to do the > last 1%, but only as part of a long-term learning process on getting > better with coccinelle - … How will corresponding software development resources evolve? Regards, Markus
On Sat, Mar 28, 2020 at 1:39 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > I would be interested to find out why coccinelle wasn't able to do the > > last 1%, but only as part of a long-term learning process on getting > > better with coccinelle - … > > How will corresponding software development resources evolve? I don't think I understand the question, or, actually, are you asking me or the coccinelle developers ?
>>> I would be interested to find out why coccinelle wasn't able to do the >>> last 1%, but only as part of a long-term learning process on getting >>> better with coccinelle - … >> >> How will corresponding software development resources evolve? > > I don't think I understand the question, Various aspects can be reconsidered. > or, actually, are you asking me or the coccinelle developers ? I am curious how much the change interests will grow by involved contributors and organisations. Would you like to discuss affected implementation details any further? Regards, Markus
>> How will corresponding software development resources evolve? > > I don't think I understand the question, or, actually, are you asking > me or the coccinelle developers ? I hope that another communication approach can eventually increase the chances for a better common understanding of development challenges. The code from a mentioned source file can be reduced to the following test file. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/mm/fault.c?id=69c5eea3128e775fd3c70ecf0098105d96dee330#n34 // deleted part static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, unsigned long address) { struct vm_area_struct * vma = NULL; struct task_struct *tsk = current; struct mm_struct *mm = tsk->mm; // deleted part retry: down_read(&mm->mmap_sem); vma = find_vma(mm, address); if (!vma) goto bad_area; // deleted part } // deleted part Application of the software “Coccinelle 1.0.8-00029-ga549b9f0” (OCaml 4.10.0) elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-c do_page_fault-excerpt3.c … NB total files = 1; perfect = 1; pbs = 0; timeout = 0; =========> 100% nb good = 15, nb passed = 1 =========> 6.25% passed nb good = 15, nb bad = 0 =========> 100.00% good or passed The discussed transformation approach can also be reduced for a test to the following script for the semantic patch language. @replacement@ expression x; @@ -down_read +mmap_read_lock ( - & x - ->mmap_sem ) elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch use_mmap_locking_API_3.cocci do_page_fault-excerpt3.c The desired diff is not generated so far. How would you like to fix this situation? Regards, Markus
On Sat, 28 Mar 2020, Markus Elfring wrote: > >> How will corresponding software development resources evolve? > > > > I don't think I understand the question, or, actually, are you asking > > me or the coccinelle developers ? > > I hope that another communication approach can eventually increase > the chances for a better common understanding of development challenges. > > The code from a mentioned source file can be reduced to the following > test file. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/mm/fault.c?id=69c5eea3128e775fd3c70ecf0098105d96dee330#n34 > > // deleted part > static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, > unsigned long address) > { > struct vm_area_struct * vma = NULL; > struct task_struct *tsk = current; > struct mm_struct *mm = tsk->mm; > // deleted part > retry: > down_read(&mm->mmap_sem); > vma = find_vma(mm, address); > if (!vma) > goto bad_area; > // deleted part > } > // deleted part > > > Application of the software “Coccinelle 1.0.8-00029-ga549b9f0” (OCaml 4.10.0) > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-c do_page_fault-excerpt3.c > … > NB total files = 1; perfect = 1; pbs = 0; timeout = 0; =========> 100% > nb good = 15, nb passed = 1 =========> 6.25% passed > nb good = 15, nb bad = 0 =========> 100.00% good or passed > > > The discussed transformation approach can also be reduced for a test > to the following script for the semantic patch language. > > @replacement@ > expression x; > @@ > -down_read > +mmap_read_lock > ( > - & > x > - ->mmap_sem > ) > > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch use_mmap_locking_API_3.cocci do_page_fault-excerpt3.c > > > The desired diff is not generated so far. > How would you like to fix this situation? Who exactly do you think "you" is? I will look at it, but it is not very polite to ask a user of Coccinelle such a question. julia > > Regards, > Markus > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci >
>> How would you like to fix this situation? > > Who exactly do you think "you" is? Every contributor with helpful software development resources for this issue. > I will look at it, Thanks for another promising feedback. > but it is not very polite to ask a user of Coccinelle such a question. We come along different views for the handling of such a bug report. Regards, Markus
On Sat, 28 Mar 2020, Markus Elfring wrote: > >> How will corresponding software development resources evolve? > > > > I don't think I understand the question, or, actually, are you asking > > me or the coccinelle developers ? > > I hope that another communication approach can eventually increase > the chances for a better common understanding of development challenges. > > The code from a mentioned source file can be reduced to the following > test file. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/mm/fault.c?id=69c5eea3128e775fd3c70ecf0098105d96dee330#n34 > > // deleted part > static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, > unsigned long address) > { > struct vm_area_struct * vma = NULL; > struct task_struct *tsk = current; > struct mm_struct *mm = tsk->mm; > // deleted part > retry: > down_read(&mm->mmap_sem); > vma = find_vma(mm, address); > if (!vma) > goto bad_area; > // deleted part > } > // deleted part > > > Application of the software “Coccinelle 1.0.8-00029-ga549b9f0” (OCaml 4.10.0) > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-c do_page_fault-excerpt3.c > … > NB total files = 1; perfect = 1; pbs = 0; timeout = 0; =========> 100% > nb good = 15, nb passed = 1 =========> 6.25% passed > nb good = 15, nb bad = 0 =========> 100.00% good or passed > > > The discussed transformation approach can also be reduced for a test > to the following script for the semantic patch language. > > @replacement@ > expression x; > @@ > -down_read > +mmap_read_lock > ( > - & > x > - ->mmap_sem > ) > > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch use_mmap_locking_API_3.cocci do_page_fault-excerpt3.c > > > The desired diff is not generated so far. > How would you like to fix this situation? The problem is due to a preceding goto where the destination is expressed as a macro name. Maybe there should be a warning in this case. julia > > Regards, > Markus > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci >
> // deleted part > retry: > down_read(&mm->mmap_sem); > vma = find_vma(mm, address); > if (!vma) > goto bad_area; > // deleted part > } > // deleted part > > > Application of the software “Coccinelle 1.0.8-00029-ga549b9f0” (OCaml 4.10.0) > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-c do_page_fault-excerpt3.c > … > NB total files = 1; perfect = 1; pbs = 0; timeout = 0; =========> 100% > nb good = 15, nb passed = 1 =========> 6.25% passed > nb good = 15, nb bad = 0 =========> 100.00% good or passed > > > The discussed transformation approach can also be reduced for a test > to the following script for the semantic patch language. > > @replacement@ > expression x; > @@ > -down_read > +mmap_read_lock > ( > - & > x > - ->mmap_sem > ) > > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch use_mmap_locking_API_3.cocci do_page_fault-excerpt3.c > > > The desired diff is not generated so far. > How would you like to fix this situation? The problem can be seen with the --debug option: FLOW: can't jump to VMALLOC_FAULT_TARGET: because we can't find this label It's not apparent with the --parse-c option because it's not a parsing problem. julia
> The problem can be seen with the --debug option: > > FLOW: can't jump to VMALLOC_FAULT_TARGET: because we can't find this label > > It's not apparent with the --parse-c option because it's not a parsing problem. Thanks for such information. Can the example be transformed even if extra source code was intentionally deleted for the easier clarification of the shown software test? Regards, Markus
>> How will corresponding software development resources evolve? > > I don't think I understand the question, or, actually, are you asking > me or the coccinelle developers ? I hope that more development challenges will be picked up. The code from a mentioned source file can be reduced to the following test file. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/mmu/paging_tmpl.h?id=7111951b8d4973bda27ff663f2cf18b663d15b48#n122 // deleted part static inline int FNAME(is_present_gpte)(unsigned long pte) { #if PTTYPE != PTTYPE_EPT return pte & PT_PRESENT_MASK; #else return pte & 7; #endif } // deleted part Application of the software “Coccinelle 1.0.8-00029-ga549b9f0” (OCaml 4.10.0) elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-c paging_tmpl-excerpt1.h … (ONCE) CPP-MACRO: found known macro = FNAME … parse error = File "paging_tmpl-excerpt1.h", line 2, column 41, charpos = 57 around = 'unsigned', … BAD:!!!!! static inline int FNAME(is_present_gpte)(unsigned long pte) … NB total files = 1; perfect = 0; pbs = 1; timeout = 0; =========> 0% nb good = 1, nb passed = 1 =========> 10.00% passed nb good = 1, nb bad = 8 =========> 20.00% good or passed How would you like to improve the affected software areas? Regards, Markus
On Mon, 30 Mar 2020, Markus Elfring wrote: > >> How will corresponding software development resources evolve? > > > > I don't think I understand the question, or, actually, are you asking > > me or the coccinelle developers ? > > I hope that more development challenges will be picked up. > > The code from a mentioned source file can be reduced to the following > test file. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kvm/mmu/paging_tmpl.h?id=7111951b8d4973bda27ff663f2cf18b663d15b48#n122 > > // deleted part > static inline int FNAME(is_present_gpte)(unsigned long pte) > { > #if PTTYPE != PTTYPE_EPT > return pte & PT_PRESENT_MASK; > #else > return pte & 7; > #endif > } > // deleted part > > > Application of the software “Coccinelle 1.0.8-00029-ga549b9f0” (OCaml 4.10.0) > > elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-c paging_tmpl-excerpt1.h > … > (ONCE) CPP-MACRO: found known macro = FNAME > … > parse error > = File "paging_tmpl-excerpt1.h", line 2, column 41, charpos = 57 > around = 'unsigned', > … > BAD:!!!!! static inline int FNAME(is_present_gpte)(unsigned long pte) > … > NB total files = 1; perfect = 0; pbs = 1; timeout = 0; =========> 0% > nb good = 1, nb passed = 1 =========> 10.00% passed > nb good = 1, nb bad = 8 =========> 20.00% good or passed > > > How would you like to improve the affected software areas? This can be addressed by adding a macro definition to standard.h. But once the change is done, I don't see any reason to bother with this. julia
>> … >> (ONCE) CPP-MACRO: found known macro = FNAME >> … >> parse error … >> How would you like to improve the affected software areas? > > This can be addressed by adding a macro definition to standard.h. A corresponding specification is used already. https://github.com/coccinelle/coccinelle/blob/fdf0b68ddd0a518cc6ca64f063bc74ed54e29a7b/standard.h#L508 > But once the change is done, I don't see any reason to bother with this. How will the support evolve for data processing around the application of similar macros? Regards, Markus
On Mon, 30 Mar 2020, Markus Elfring wrote: > >> … > >> (ONCE) CPP-MACRO: found known macro = FNAME > >> … > >> parse error > … > >> How would you like to improve the affected software areas? > > > > This can be addressed by adding a macro definition to standard.h. > > A corresponding specification is used already. > https://github.com/coccinelle/coccinelle/blob/fdf0b68ddd0a518cc6ca64f063bc74ed54e29a7b/standard.h#L508 > > > > But once the change is done, I don't see any reason to bother with this. > > How will the support evolve for data processing around the application > of similar macros? Not at all. julia
>> How will the support evolve for data processing around the application >> of similar macros? > > Not at all. Will such feedback trigger any collateral evolution? Regards, Markus
On Fri, Mar 27, 2020 at 03:50:57PM -0700, Michel Lespinasse wrote: > Convert the last few remaining mmap_sem rwsem calls to use the new > mmap locking API. These were missed by coccinelle for some reason > (I think coccinelle does not support some of the preprocessor > constructs in these files ?) Adding the wrappers for instrumentation makes sense to me. For patches 1-5, you can add Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com> I'll wait for the next version to review the rest.
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c index 1e8d00793784..c406250d7761 100644 --- a/arch/mips/mm/fault.c +++ b/arch/mips/mm/fault.c @@ -97,7 +97,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, if (user_mode(regs)) flags |= FAULT_FLAG_USER; retry: - down_read(&mm->mmap_sem); + mmap_read_lock(mm); vma = find_vma(mm, address); if (!vma) goto bad_area; @@ -191,7 +191,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, } } - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); return; /* @@ -199,7 +199,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, * Fix it, but check if it's kernel or user first.. */ bad_area: - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); bad_area_nosemaphore: /* User mode accesses just cause a SIGSEGV */ @@ -251,14 +251,14 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, * We ran out of memory, call the OOM killer, and return the userspace * (which will retry the fault, or kill us if we got oom-killed). */ - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); if (!user_mode(regs)) goto no_context; pagefault_out_of_memory(); return; do_sigbus: - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); /* Kernel mode? Handle exceptions or die */ if (!user_mode(regs)) diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h index e4c8a4cbf407..8cbfe6c96d82 100644 --- a/arch/x86/kvm/mmu/paging_tmpl.h +++ b/arch/x86/kvm/mmu/paging_tmpl.h @@ -165,22 +165,22 @@ static int FNAME(cmpxchg_gpte)(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, unsigned long pfn; unsigned long paddr; - down_read(¤t->mm->mmap_sem); + mmap_read_lock(current->mm); vma = find_vma_intersection(current->mm, vaddr, vaddr + PAGE_SIZE); if (!vma || !(vma->vm_flags & VM_PFNMAP)) { - up_read(¤t->mm->mmap_sem); + mmap_read_unlock(current->mm); return -EFAULT; } pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff; paddr = pfn << PAGE_SHIFT; table = memremap(paddr, PAGE_SIZE, MEMREMAP_WB); if (!table) { - up_read(¤t->mm->mmap_sem); + mmap_read_unlock(current->mm); return -EFAULT; } ret = CMPXCHG(&table[index], orig_pte, new_pte); memunmap(table); - up_read(¤t->mm->mmap_sem); + mmap_read_unlock(current->mm); } return (ret != orig_pte); diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 5e063739a3a8..cbdc43ed0f9f 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -932,7 +932,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, mm = alloc->vma_vm_mm; if (!mmget_not_zero(mm)) goto err_mmget; - if (!down_read_trylock(&mm->mmap_sem)) + if (!mmap_read_trylock(mm)) goto err_down_read_mmap_sem_failed; vma = binder_alloc_get_vma(alloc); @@ -946,7 +946,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, trace_binder_unmap_user_end(alloc, index); } - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); mmput(mm); trace_binder_unmap_kernel_start(alloc, index); diff --git a/fs/proc/base.c b/fs/proc/base.c index 479efdba60b9..0dc54b5d75f2 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2280,7 +2280,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx) if (!mm) goto out_put_task; - ret = down_read_killable(&mm->mmap_sem); + ret = mmap_read_lock_killable(mm); if (ret) { mmput(mm); goto out_put_task; @@ -2307,7 +2307,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx) p = genradix_ptr_alloc(&fa, nr_files++, GFP_KERNEL); if (!p) { ret = -ENOMEM; - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); mmput(mm); goto out_put_task; } @@ -2316,7 +2316,7 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx) p->end = vma->vm_end; p->mode = vma->vm_file->f_mode; } - up_read(&mm->mmap_sem); + mmap_read_unlock(mm); mmput(mm); for (i = 0; i < nr_files; i++) {
Convert the last few remaining mmap_sem rwsem calls to use the new mmap locking API. These were missed by coccinelle for some reason (I think coccinelle does not support some of the preprocessor constructs in these files ?) Signed-off-by: Michel Lespinasse <walken@google.com> --- arch/mips/mm/fault.c | 10 +++++----- arch/x86/kvm/mmu/paging_tmpl.h | 8 ++++---- drivers/android/binder_alloc.c | 4 ++-- fs/proc/base.c | 6 +++--- 4 files changed, 14 insertions(+), 14 deletions(-)