> On Jul 19, 2022, at 11:28 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Sun, 17 Jul 2022 22:54:47 -0700
> Song Liu <song@xxxxxxxxxx> wrote:
>
> Again, make the subject:
>
> ftrace: Allow IPMODIFY and DIRECT ops on the same function
>
Will fix.
> [...]
>> +
>> +/*
>> + * For most ftrace_ops_cmd,
>> + * Returns:
>> + * 0 - Success.
>> + * -EBUSY - The operation cannot process
>> + * -EAGAIN - The operation cannot process tempoorarily.
>
> Just state:
>
> Returns:
> 0 - Success
> Negative on failure. The return value is dependent
> on the callback.
>
> Let's not bind policy of the callback with ftrace.
Will fix.
>
>> + */
>> +typedef int (*ftrace_ops_func_t)(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
>> +
>> #ifdef CONFIG_DYNAMIC_FTRACE
>> /* The hash used to know what functions callbacks trace */
>>
[...]
>>
>> - if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
>> + is_ipmodify = ops->flags & FTRACE_OPS_FL_IPMODIFY;
>> + is_direct = ops->flags & FTRACE_OPS_FL_DIRECT;
>> +
>> + /* either IPMODIFY nor DIRECT, skip */
>> + if (!is_ipmodify && !is_direct)
>> return 0;
>
> I wonder if we should also add:
>
> if (WARN_ON_ONCE(is_ipmodify && is_direct))
> return 0;
>
> As a direct should never have an ipmodify.
Right, I will also remove IPMODIFY from direct_ops:
@ -2487,8 +2490,7 @@ static void call_direct_funcs(unsigned long ip, unsigned long pip,
struct ftrace_ops direct_ops = {
.func = call_direct_funcs,
- .flags = FTRACE_OPS_FL_IPMODIFY
- | FTRACE_OPS_FL_SAVE_REGS
+ .flags = FTRACE_OPS_FL_SAVE_REGS
| FTRACE_OPS_FL_PERMANENT,
/*
* By declaring the main trampoline as this trampoline
>
>>
>> /*
>> - * Since the IPMODIFY is a very address sensitive action, we do not
>> - * allow ftrace_ops to set all functions to new hash.
>> + * Since the IPMODIFY and DIRECT are very address sensitive
>> + * actions, we do not allow ftrace_ops to set all functions to new
>> + * hash.
[...]
>
> Again, these are ops_func() specific and has nothing to do with the logic
> in this file. Just state:
>
> * Returns:
> * 0 - @ops does not have IPMODIFY or @ops itself is DIRECT, no
> * change needed;
> * 1 - @ops has IPMODIFY, hold direct_mutex;
> * Negative on error.
>
> And if we move the logic that this does not keep hold of the direct_mutex,
> we could just let the callback return any non-zero on error.
>
>> + */
>> +static int prepare_direct_functions_for_ipmodify(struct ftrace_ops *ops)
>> + __acquires(&direct_mutex)
>> +{
>> + struct ftrace_func_entry *entry;
>> + struct ftrace_hash *hash;
>> + struct ftrace_ops *op;
>> + int size, i, ret;
>> +
>> + if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
>> + return 0;
>> +
>> + mutex_lock(&direct_mutex);
>> +
>> + hash = ops->func_hash->filter_hash;
>> + size = 1 << hash->size_bits;
>> + for (i = 0; i < size; i++) {
>> + hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
>> + unsigned long ip = entry->ip;
>> + bool found_op = false;
>> +
>> + mutex_lock(&ftrace_lock);
>> + do_for_each_ftrace_op(op, ftrace_ops_list) {
>> + if (!(op->flags & FTRACE_OPS_FL_DIRECT))
>> + continue;
>> + if (ops_references_ip(op, ip)) {
>> + found_op = true;
>> + break;
>
> I think you want a goto here. The macros "do_for_each_ftrace_op() { .. }
> while_for_each_ftrace_op()" is a double loop. The break just moves to the
> next set of pages and does not break out of the outer loop.
Hmmm... really? I didn't see it ...
#define do_for_each_ftrace_op(op, list) \
op = rcu_dereference_raw_check(list); \
do
#define while_for_each_ftrace_op(op) \
while (likely(op = rcu_dereference_raw_check((op)->next)) && \
unlikely((op) != &ftrace_list_end))
Did I miss something...?
>
> goto out_loop;
>
>> + }
>> + } while_for_each_ftrace_op(op);
[...]
>
>> mutex_lock(&ftrace_lock);
>>
>> ret = ftrace_startup(ops, 0);
>>
>> mutex_unlock(&ftrace_lock);
>>
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>> + if (direct_mutex_locked)
>> + mutex_unlock(&direct_mutex);
>> +#endif
>
> Change this to:
>
> out_unlock:
> mutex_unlock(&direct_mutex);
>
We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as
direct_mutex is not defined without that config.
Thanks,
Song
[...]
