| 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
| ||
|
Date: Wed, 23 Oct 2019 15:57:49 -0700
From: Divya Indi <divya.indi@...cle.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, Joe Jin <joe.jin@...cle.com>,
Srinivas Eeda <srinivas.eeda@...cle.com>,
Aruna Ramakrishna <aruna.ramakrishna@...cle.com>
Subject: Re: [PATCH 4/5] tracing: Handle the trace array ref counter in new
functions
Hi Steven,
A few clarifications on this discussion on reference counter -
1) We will still need to export trace_array_put() to be used for every
trace_array_get_by_name() OR trace_array_create() + trace_array_get().
How else will we reduce the reference counter [For eg: When multiple modules
lookup the same trace array (say, reference counter = 4)]?
2) tr = trace_array_create("my_tr");
trace_array_get(tr);
Both of these functions will iterate through the list of trace arrays to verify
whether the trace array exists (redundant, but more intuitive? Does this seem
acceptable?)
To avoid iterating twice, we went with increasing ref_ctr in trace_array_create.
This necessitated a trace_array_put() in instance_mkdir (Or as suggested below,
we can do this trace_array_put() in instance_rmdir().)
3) A summary of suggested changes (Let me know if this looks good) -
tr = trace_array_get_by_name("foo-bar"); // ref_ctr++.
if (!tr)
{
// instance_mkdir also causes ref_ctr = 1
tr = trace_array_create("foo-bar"); // ref_ctr = 1
trace_array_get(tr); // ref_ctr++
}
trace_array_printk(.....);
trace_array_set_clr_event(......);
...
...
...
// Done using the trace array.
trace_array_put(tr); // ref_ctr--
...
...
...
// We can now remove the trace array via trace_array_destroy or instance_rmdir()
trace_array_destroy(tr); // ref_ctr > 1 returns -EBUSY.
Thanks,
Divya
On 10/22/19 7:52 PM, Steven Rostedt wrote:
> On Wed, 16 Oct 2019 16:42:02 -0700
> Divya Indi <divya.indi@...cle.com> wrote:
>
>> Hi Steve,
>>
>> Thanks again for taking the time to review and providing feedback. Please find my comments inline.
>>
>> On 10/15/19 4:04 PM, Steven Rostedt wrote:
>>> Sorry for taking so long to getting to these patches.
>>>
>>> On Wed, 14 Aug 2019 10:55:26 -0700
>>> Divya Indi <divya.indi@...cle.com> wrote:
>>>
>>>> For functions returning a trace array Eg: trace_array_create(), we need to
>>>> increment the reference counter associated with the trace array to ensure it
>>>> does not get freed when in use.
>>>>
>>>> Once we are done using the trace array, we need to call
>>>> trace_array_put() to make sure we are not holding a reference to it
>>>> anymore and the instance/trace array can be removed when required.
>>> I think it would be more in line with other parts of the kernel if we
>>> don't need to do the trace_array_put() before calling
>>> trace_array_destroy().
>> The reason we went with this approach is
>>
>> instance_mkdir - ref_ctr = 0 // Does not return a trace array ptr.
>> trace_array_create - ref_ctr = 1 // Since this returns a trace array ptr.
>> trace_array_lookup - ref_ctr = 1 // Since this returns a trace array ptr.
>>
>> if we make trace_array_destroy to expect ref_ctr to be 1, we risk destroying the trace array while in use.
>>
>> We could make it -
>>
>> instance_mkdir - ref_ctr = 1
>> trace_array_create - ref_ctr = 2
>> trace_array_lookup - ref_ctr = 2+ // depending on no of lookups
>>
>> but, we'd still need the trace_array_put() (?)
>>
>> We can also have one function doing create (if does not exist) or lookup (if exists), but that would require
>> some redundant code since instance_mkdir needs to return -EXIST when a trace array already exists.
>>
>> Let me know your thoughts on this.
>>
> Can't we just move the trace_array_put() in the instance_rmdir()?
>
> static int instance_rmdir(const char *name)
> {
> struct trace_array *tr;
> int ret;
>
> mutex_lock(&event_mutex);
> mutex_lock(&trace_types_lock);
>
> ret = -ENODEV;
> list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> if (tr->name && strcmp(tr->name, name) == 0) {
> __trace_array_put(tr);
> ret = __remove_instance(tr);
> if (ret)
> tr->ref++;
> break;
> }
> }
>
> mutex_unlock(&trace_types_lock);
> mutex_unlock(&event_mutex);
>
> return ret;
> }
>
> -- Steve
Powered by blists - more mailing lists