linux-kernel - Re: [PATCH 0/7] Uprobes: 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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 8 Jun 2018 07:59:38 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     oleg@...hat.com, srikar@...ux.vnet.ibm.com, rostedt@...dmis.org,
        peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        alexander.shishkin@...ux.intel.com, jolsa@...hat.com,
        namhyung@...nel.org, linux-kernel@...r.kernel.org, corbet@....net,
        linux-doc@...r.kernel.org, ananth@...ux.vnet.ibm.com,
        alexis.berlemont@...il.com, naveen.n.rao@...ux.vnet.ibm.com,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH 0/7] Uprobes: Support SDT markers having reference count
 (semaphore)

Hi Masami,

On 06/08/2018 06:40 AM, Masami Hiramatsu wrote:
> On Wed,  6 Jun 2018 14:03:37 +0530
> Ravi Bangoria <ravi.bangoria@...ux.ibm.com> wrote:
> 
>> Why RFC again:
>>
>> This series is different from earlier versions[1]. Earlier series
>> implemented this feature in trace_uprobe while this has implemented
>> the logic in core uprobe. Few reasons for this:
>>  1. One of the major reason was the deadlock between uprobe_lock and
>>  mm->mmap inside trace_uprobe_mmap(). That deadlock was not easy to fix
>>  because mm->mmap is not in control of trace_uprobe_mmap() and it has
>>  to take uprobe_lock to loop over trace_uprobe list. More details can
>>  be found at[2]. With this new approach, there are no deadlocks found
>>  so far.
>>  2. Many of the core uprobe function and data-structures needs to be
>>  exported to make earlier implementation simple. With this new approach,
>>  reference counter logic is been implemented in core uprobe and thus
>>  no need to export anything.
> 
> I agree with you. Moreover, since uprobe_register/unregister() are
> exported to modules, this enablement would better be implemented
> inside uprobe so that all uprobe users benefit from this.


Sorry, I think you got me wrong. I meant, I don't need to expose all core
uprobe _static_ functions to tarce_uprobe.

Now, about kernel modules, basically uprobe_register() takes three parameters:
    inode, offset and consumer.
There is no scope for the reference counter there. So I've created one more
function: uprobe_register_refctr(). But this function is not exported as ABI
to kernel module. i.e. kernel modules still does not have a way to create
uprobe with reference counter. So for kernel modules,

is it fine to change current ABI from
    uprobe_register(inode, offset, consumer)
to
    uprobe_register(inode, offset, ref_ctr_offset, consumer)

Or I should introduce new function for this:
    uprobe_register_refctr(inode, offset, ref_ctr_offset, consumer)
and export it to kernel module?

What's your suggestion?

[...]

>> 
>>  - This patches still has one issue. If there are multiple instances of
>>    same application running and user wants to trace any particular
>>    instance, trace_uprobe is updating reference counter in all instances.
>>    This is not a problem on user side because instruction is not replaced
>>    with trap/int3 and thus user will only see samples from his interested
>>    process. But still this is more of a correctness issue. I'm working on
>>    a fix for this.
> 
> Hmm, it sounds like not a correctness issue, but there maybe a performace
> tradeoff. Tracing one particulear instance, other instances also will get
> a performance loss


Right, but it's temporary. I mean, putting everything in to this series was making
it complex. So this is the initial one and I'll send followup patches which will
optimize the reference counter update.


> (Only if the parameter preparation block is heavy,
> because the heaviest part of probing - trap/int3 and recording data - isn't
> executed.)
>> BTW, why this happens? I thought the refcounter part is just a data which
> is not shared among processes...
> 

This happens because we are not calling consumer_filter function. consumer_filter
is the one who decides whether to change the instruction to trap or not in a given
mm. We also need to call it before updating reference counter.

Let me know your thoughts.

Thanks,
Ravi