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

From: Song Liu
Date: Thu Jul 14 2022 - 22:04:45 EST

> On Jul 14, 2022, at 5:48 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Fri, 15 Jul 2022 00:13:51 +0000
> Song Liu <songliubraving@xxxxxx> wrote:
>> I think there is one more problem here. If we force all direct trampoline
>> set IPMODIFY, and remove the SHARE_IPMODIFY flag. It may cause confusion
>> and/or extra work here (__ftrace_hash_update_ipmodify).
> I'm saying that the caller (BPF) does not need to set IPMODIFY. Just
> change the ftrace.c to treat IPMODIFY the same as direct. In fact, the
> two should be mutually exclusive (from a ftrace point of view). That
> is, if DIRECT is set, then IPMODIFY must not be.
> Again, ftrace will take care of the accounting of if a rec has both
> IPMODIFY and DIRECT on it.
>> Say __ftrace_hash_update_ipmodify() tries to attach an ops with IPMODIFY,
>> and found the rec already has IPMODIFY. At this point, we have to iterate
>> all ftrace ops (do_for_each_ftrace_op) to confirm whether the IPMODIFY is
>> from
> 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).

> Like
> I mentioned before, ftrace doesn't care if the direct trampoline
> modifies IP or not. All ftrace will do is ask the owner of the direct
> ops if it is safe to have an IP modify attached to it or not. And at
> the same time will tell the direct ops owner that it is adding an
> IPMODIFY ops such that it can take the appropriate actions.
> In other words, IPMODIFY on the rec means that it can not attach
> anything else with an IPMODIFY on it.
> The direct ops should only set the DIRECT flag. If an IPMODIFY ops is
> being added, if it already has an IPMODIFY then it will fail right there.
> If direct is set, then a loop for the direct ops will be done and the
> callback is made. If the direct ops is OK with the IPMODIFY then it
> will adjust itself to work with the IPMODIFY and the IPMODIFY will
> continue to be added (and then set the rec IPMODIFY flag).
>> 1) a direct ops that can share IPMODIFY, or
>> 2) a direct ops that cannot share IPMODIFY, or
>> 3) another non-direct ops with IPMODIFY.
>> For the 1), this attach works, while for 2) and 3), the attach doesn't work.
>> OTOH, with current version (v2), we trust the direct ops to set or clear
>> IPMODIFY. rec with IPMODIFY makes it clear that it cannot share with another
>> ops with IPMODIFY. Then we don't have to iterate over all ftrace ops here.
> The only time an iterate should be done is if rec->flags is DIRECT and !IPMODIFY.

Yeah, this makes sense. And this shouldn't be too expensive.

>> Does this make sense? Did I miss some better solutions?
> Did I fill in the holes? ;-)

You sure did. :)