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

From: Song Liu
Date: Fri Jul 15 2022 - 13:43:07 EST

Hi Steven,

> On Jul 14, 2022, at 7:50 PM, Song Liu <songliubraving@xxxxxx> wrote:
>> On Jul 14, 2022, at 7:46 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>> On Fri, 15 Jul 2022 02:04:33 +0000
>> Song Liu <songliubraving@xxxxxx> wrote:
>>>> What I'm suggesting is that a DIRECT ops will never set IPMODIFY.
>>> Aha, this the point I misunderstood. I thought DIRECT ops would always
>>> set IPMODIFY (as it does now).
>> My fault. I was probably not being clear when I was suggesting that
>> DIRECT should *act* like an IPMODIFY, but never explicitly stated that
>> it should not set the IPMODIFY flag.
>> The only reason it does today was to make it easy to act like an
>> IPMODIFY (because it set the flag). But I'm now suggesting to get rid
>> of that and just make DIRECT act like an IPMDOFIY as there can only be
>> one of them on a function, but now we have some cases where DIRECT can
>> work with IPMODIFY via the callbacks.
> Thanks for the clarification. I think we are finally on the same page on
> this. :)

A quick update and ask for feedback/clarification.

Based on my understanding, you recommended calling ops_func() from
__ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline
may make changes to the trampoline. Did I get this right?

I am going towards this direction, but hit some issue. Specifically, in
__ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the
direct trampoline cannot easily make changes with
modify_ftrace_direct_multi(), which locks both direct_mutex and

One solution would be have no-lock version of all the functions called
by modify_ftrace_direct_multi(), but that's a lot of functions and the
code will be pretty ugly. The alternative would be the logic in v2:
__ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to
the direct trampoline in other places:

1) if DIRECT ops attached first, the trampoline is updated in
prepare_direct_functions_for_ipmodify(), see 3/5 of v2;

2) if IPMODIFY ops attached first, the trampoline is updated in
bpf_trampoline_update(), see "goto again" path in 5/5 of v2.

Overall, I think this way is still cleaner. What do you think about this?