Re: [PATCH v5 bpf-next 4/4] bpf: Support bpf_trampoline on functions with IPMODIFY (e.g. livepatch)

[Song Liu]

> On Jul 22, 2022, at 12:31 AM, Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> 
> On Tue, Jul 19, 2022 at 05:21:26PM -0700, Song Liu wrote:
> 
> SNIP
> 
>> +		tr->flags |= BPF_TRAMP_F_SHARE_IPMODIFY;
>> +
>> +		if ((tr->flags & BPF_TRAMP_F_CALL_ORIG) &&
>> +		    !(tr->flags & BPF_TRAMP_F_ORIG_STACK))
>> +			ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
>> +		break;
>> +	case FTRACE_OPS_CMD_DISABLE_SHARE_IPMODIFY_PEER:
>> +		tr->flags &= ~BPF_TRAMP_F_SHARE_IPMODIFY;
>> +
>> +		if (tr->flags & BPF_TRAMP_F_ORIG_STACK)
>> +			ret = bpf_trampoline_update(tr, false /* lock_direct_mutex */);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	};
>> +
>> +	mutex_unlock(&tr->mutex);
>> +	return ret;
>> +}
>> +#endif
>> +
>> bool bpf_prog_has_trampoline(const struct bpf_prog *prog)
>> {
>> 	enum bpf_attach_type eatype = prog->expected_attach_type;
>> @@ -89,6 +165,16 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key)
>> 	tr = kzalloc(sizeof(*tr), GFP_KERNEL);
>> 	if (!tr)
>> 		goto out;
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>> +	tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
>> +	if (!tr->fops) {
>> +		kfree(tr);
>> +		tr = NULL;
>> +		goto out;
>> +	}
> 
> would it be easier to put ftrace_ops directly to bpf_trampoline,
> not just pointer.. it's allocated and freed at the same point
> 
> I recall there were some include issues when I tried that long
> time ago [1], but could make the change bit simpler
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git/commit/?h=bpf/batch&id=52a1d4acdf55df41e99ca2cea51865e6821036ce

I was not aware the CC_USING_FENTRY issue. However, I think we
would like to avoid including ftrace.h in bpf.h, which will 
slow down the build. 

Thanks,
Song