linux-kernel - Re: [PATCH v3 6/9] trace_uprobe: Support SDT markers having reference count (semaphore)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <206e4a16-ae21-7da3-f752-853dc2f51947@linux.ibm.com>
Date:   Fri, 4 May 2018 19:51:32 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>, oleg@...hat.com,
        peterz@...radead.org, srikar@...ux.vnet.ibm.com,
        rostedt@...dmis.org, acme@...nel.org, ananth@...ux.vnet.ibm.com,
        akpm@...ux-foundation.org, alexander.shishkin@...ux.intel.com,
        alexis.berlemont@...il.com, corbet@....net,
        dan.j.williams@...el.com, jolsa@...hat.com, kan.liang@...el.com,
        kjlx@...pleofstupid.com, kstewart@...uxfoundation.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, milian.wolff@...b.com, mingo@...hat.com,
        namhyung@...nel.org, naveen.n.rao@...ux.vnet.ibm.com,
        pc@...ibm.com, tglx@...utronix.de, yao.jin@...ux.intel.com,
        fengguang.wu@...el.com, jglisse@...hat.com
Subject: Re: [PATCH v3 6/9] trace_uprobe: Support SDT markers having reference
 count (semaphore)

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