Re: [PATCH 1/3] uprobes: install_breakpoint() should fail if is_swbp_insn() == T

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

> 
> So its the -EEXIST from set_swbp() that I was thinking about. I think I
> was also wrong on the reason that that can happen. register's vma
> iteration is very careful not to have the same vma twice, but it can
> race against mmap() because of the uprobe_hash() vs uprobe_mmap_hash()
> madness, right?


right. 

> 
> Something like so?
> 
> ---
>  kernel/events/uprobes.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 985be4d..b4e749e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -45,6 +45,19 @@ static DEFINE_SPINLOCK(uprobes_treelock);	/* serialize rbtree access */
>  
>  #define UPROBES_HASH_SZ	13
>  
> +/*
> + * We need separate register/unregister and mmap/munmap lock hashes because of
> + * mmap_sem nesting.
> + *
> + * {,un}egister_uprobe() needs to install probes on (potentially) all processes
> + * and thus need to acquire multiple mmap_sems (consecutively, not
> + * concurrently), whereas uprobe_m{,un}map() is called while holding mmap_sem
> + * for the particular process doing the mmap.
> + *
> + * This all means that register_uprobe() can race with uprobe_mmap() and we
> + * can try and install a probe where one is already installed.
> + */

Nit: {,un}egister_uprobe should have been uprobe_{,un}register  at
couple of places.

> +
>  /* serialize (un)register */
>  static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
>  
> @@ -356,6 +369,9 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
>  {
>  	int result;
>  
> +	/*
> +	 * See the comment near uprobes_hash().
> +	 */
>  	result = is_swbp_at_addr(mm, vaddr);
>  	if (result == 1)
>  		return -EEXIST;
>