[v3,05/10] mmap locking API: convert mmap_sem call sites missed by coccinelle
diff mbox series

Message ID 20200327225102.25061-6-walken@google.com
State New
Headers show
Series
  • Add a new mmap locking API wrapping mmap_sem calls
Related show

Commit Message

Michel Lespinasse March 27, 2020, 10:50 p.m. UTC
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(-)

Comments

Markus Elfring March 28, 2020, 7:36 a.m. UTC | #1
> 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
Michel Lespinasse March 28, 2020, 7:47 a.m. UTC | #2
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.
Markus Elfring March 28, 2020, 8:39 a.m. UTC | #3
> 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
Michel Lespinasse March 28, 2020, 8:53 a.m. UTC | #4
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 ?
Markus Elfring March 28, 2020, 9:01 a.m. UTC | #5
>>> 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
Markus Elfring March 28, 2020, 1:20 p.m. UTC | #6
>> 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
Julia Lawall March 28, 2020, 1:42 p.m. UTC | #7
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
>
Markus Elfring March 28, 2020, 1:52 p.m. UTC | #8
>> 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
Julia Lawall March 28, 2020, 1:53 p.m. UTC | #9
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
>
Julia Lawall March 28, 2020, 2 p.m. UTC | #10
> // 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
Markus Elfring March 28, 2020, 2:16 p.m. UTC | #11
> 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
Markus Elfring March 30, 2020, 6:10 a.m. UTC | #12
>> 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
Julia Lawall March 30, 2020, 8:47 a.m. UTC | #13
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
Markus Elfring March 30, 2020, 10:15 a.m. UTC | #14
>> …
>> (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
Julia Lawall March 30, 2020, 10:20 a.m. UTC | #15
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
Markus Elfring March 30, 2020, 10:42 a.m. UTC | #16
>> 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
Daniel Jordan April 13, 2020, 5:06 p.m. UTC | #17
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.

Patch
diff mbox series

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(&current->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(&current->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(&current->mm->mmap_sem);
+			mmap_read_unlock(current->mm);
 			return -EFAULT;
 		}
 		ret = CMPXCHG(&table[index], orig_pte, new_pte);
 		memunmap(table);
-		up_read(&current->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++) {