Re: [PATCH v2 bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY

[Song Liu <songliubraving@fb.com>]

> On Jun 6, 2022, at 1:20 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> On Thu, Jun 02, 2022 at 12:37:04PM -0700, Song Liu wrote:
>> live patch and BPF trampoline (kfunc/kretfunc in bpftrace) are important
>> features for modern systems. Currently, it is not possible to use live
>> patch and BPF trampoline on the same kernel function at the same time.
>> This is because of the resitriction that only one ftrace_ops with flag
>> FTRACE_OPS_FL_IPMODIFY on the same kernel function.
> 
> is it hard to make live patch test? would be great to have
> selftest for this, or at least sample module that does that,
> there are already sample modules for direct interface

It is possible, but a little tricky. I can add some when selftests or
samples in later version. 

> 
>> 
>> BPF trampoline uses direct ftrace_ops, which assumes IPMODIFY. However,
>> not all direct ftrace_ops would overwrite the actual function. This means
>> it is possible to have a non-IPMODIFY direct ftrace_ops to share the same
>> kernel function with an IPMODIFY ftrace_ops.
>> 
>> Introduce FTRACE_OPS_FL_SHARE_IPMODIFY, which allows the direct ftrace_ops
>> to share with IPMODIFY ftrace_ops. With FTRACE_OPS_FL_SHARE_IPMODIFY flag
>> set, the direct ftrace_ops would call the target function picked by the
>> IPMODIFY ftrace_ops.
>> 
>> Comment "IPMODIFY, DIRECT, and SHARE_IPMODIFY" in include/linux/ftrace.h
>> contains more information about how SHARE_IPMODIFY interacts with IPMODIFY
>> and DIRECT flags.
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> 

[...]

>> +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) ||
>> +	    (ops->flags & FTRACE_OPS_FL_DIRECT))
>> +		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) {
> 
> would it be better to iterate direct_functions hash instead?
> all the registered direct functions should be there
> 
> hm maybe you would not have the 'op' then..

Yeah, we need ftrace_ops here. 

> 
>> +				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
>> +					continue;
>> +				if (op->flags & FTRACE_OPS_FL_SHARE_IPMODIFY)
>> +					break;
>> +				if (ops_references_ip(op, ip)) {
>> +					found_op = true;
>> +					break;
>> +				}
>> +			} while_for_each_ftrace_op(op);
>> +			mutex_unlock(&ftrace_lock);
> 
> so the 'op' can't go away because it's direct and we hold direct_mutex
> even though we unlocked ftrace_lock, right?

Yep, we need to hold direct_mutex here. 

> 
>> +
>> +			if (found_op) {
>> +				if (!op->ops_func) {
>> +					ret = -EBUSY;
>> +					goto err_out;
>> +				}
>> +				ret = op->ops_func(op, FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY);
> 
> I did not find call with FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY flag

We don't have it yet, and I think we probably don't really need it. 
AFAICT, unloading live patch is not a common operation. So not 
recovering the performance of !SHARE_IPMODIFY should be acceptable
in those cases. That said, I can add that path if we think it is
important. 

Thanks,
Song

[...]