> On Jun 6, 2022, at 1:20 AM, Jiri Olsa <olsajiri@xxxxxxxxx> 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@xxxxxxxxxx>
>>
[...]
>> +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
[...]
