Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

[Song Liu <songliubraving@fb.com>]

> On Jul 19, 2022, at 11:28 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Sun, 17 Jul 2022 22:54:47 -0700
> Song Liu <song@kernel.org> 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

[...]