Hi Masami,
On 05/04/2018 10:18 AM, Masami Hiramatsu wrote:
>> +void uprobe_down_write_dup_mmap(void)
>> +{
>> + percpu_down_write(&dup_mmap_sem);
>> +}
>> +
>> +void uprobe_up_write_dup_mmap(void)
>> +{
>> + percpu_up_write(&dup_mmap_sem);
>> +}
>> +
> I'm not sure why these hunks are not done in previous patch.
> If you separate "uprobe_map_info" export patch, this also
> should be separated. (Or both merged into this patch)
Sure, I'll add separate patch for dup_mmap_sem.
>> +/*
>> + * Reference counter gate the invocation of probe. If present,
>> + * by default reference counter is 0. One needs to increment
>> + * it before tracing the probe and decrement it when done.
>> + */
>> +static int
>> +sdt_update_ref_ctr(struct mm_struct *mm, unsigned long vaddr, short d)
>> +{
>> + void *kaddr;
>> + struct page *page;
>> + struct vm_area_struct *vma;
>> + int ret = 0;
>> + unsigned short *ptr;
>> +
>> + if (vaddr == 0)
>> + return -EINVAL;
>> +
>> + ret = get_user_pages_remote(NULL, mm, vaddr, 1,
>> + FOLL_FORCE | FOLL_WRITE, &page, &vma, NULL);
>> + if (ret <= 0)
>> + return ret;
> Hmm, get_user_pages_remote() said
>
> ===
> If nr_pages is 0 or negative, returns 0. If no pages were pinned, returns -errno.
> ===
>
> And you've passed 1 for nr_pages, so it must be 1 or -errno.
>
>> +
>> + kaddr = kmap_atomic(page);
>> + ptr = kaddr + (vaddr & ~PAGE_MASK);
>> + *ptr += d;
>> + kunmap_atomic(kaddr);
>> +
>> + put_page(page);
>> + return 0;
> And obviously 0 means "success" for sdt_update_ref_ctr().
> I think if get_user_pages_remote returns 0, this should
> return -EBUSY (*) or something else.
>
> * It seems that if faultin_page() in __get_user_pages()
> returns -EBUSY, get_user_pages_remote() can return 0.
Ah good catch :). Will change it.
>> +}
>> +
>> +static void sdt_increment_ref_ctr(struct trace_uprobe *tu)
>> +{
>> + struct uprobe_map_info *info;
>> +
>> + uprobe_down_write_dup_mmap();
>> + info = uprobe_build_map_info(tu->inode->i_mapping,
>> + tu->ref_ctr_offset, false);
>> + if (IS_ERR(info))
>> + goto out;
>> +
>> + while (info) {
>> + down_write(&info->mm->mmap_sem);
>> +
>> + if (sdt_find_vma(tu, info->mm, info->vaddr))
>> + sdt_update_ref_ctr(info->mm, info->vaddr, 1);
> Don't you have to handle the error to map pages here?
Correct.. I think, I've to feedback error code to probe_event_{enable|disable}
and handler failure there.
Thanks for the review,
Ravi
