lkml.org 
[lkml]   [2022]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 bpf-next 3/5] ftrace: introduce FTRACE_OPS_FL_SHARE_IPMODIFY
Date


> On Jul 14, 2022, at 5:48 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 15 Jul 2022 00:13:51 +0000
> Song Liu <songliubraving@fb.com> 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. :)

Thanks,
Song

\
 
 \ /
  Last update: 2022-07-15 04:05    [W:0.088 / U:1.560 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site