Re: [PATCH bpf-next v2 3/6] bpf: support attaching freplace programs to multiple attach points

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Jul 20, 2020 at 08:48:04PM -0700, Andrii Nakryiko wrote:
> 
> Right, I wanted to avoid taking a refcnt on aux->linked_prog during
> PROG_LOAD. The reason for that was (and still is) that I don't get who
> and when has to bpf_prog_put() original aux->linked_prog to allow the
> prog X to be freed. I.e., after you re-attach to prog Y, how prog X is
> released (assuming no active bpf_link is keeping it from being freed)?
> That's my biggest confusion right now.
> 
> I also didn't like the idea of half-creating bpf_tracing_link on
> PROG_LOAD and then turning it into a real link with bpf_link_settle on
> attach. That sounded like a hack to me.

The link is kinda already created during prog_load of EXT type.
Typically prog_load needs expected_attach_type that points to something
that is not going to disappear. In case of EXT progs the situation is different,
since the target can be unloaded. So the prog load cmd not only validates the
program extension but links target and ext prog together at the same time.
The target prog will be held until EXT prog is unloaded.
I think it's important to preserve this semantics to the users that the target prog
is frozen at load time and no races are going to happen later.
Otherwise it leads to double validation at attach time and races.

What raw_tp_open is doing right now is a hack. It allocates bpf_tracing_link,
registers it into link_idr and activates trampoline, but in reality that link is already there.
I think we can clean it up by creating bpf_tracing_link at prog load time.
Whether to register it at that time into link_idr is up to discussion.
(I think probably not).
Then raw_tp_open will activate that allocated bpf_tracing_link via trampoline,
_remove_ it from aux->linked_tracing_link (old linked_prog) and
return FD to the user.
So this partially created link at load_time will become complete link and
close of the link will detach EXT from the target and the target can be unloaded.
(Currently the target cannot be unloaded until EXT is loaded which is not great).
The EXT_prog->aux->linked_tracing_link (old linked_prog) will exist only during
the time between prog_load and raw_tp_open without args.
I think that would be a good clean up.
Then multi attach of EXT progs is clean too.
New raw_tp_open with tgt_prog_fd/tgt_btf_id will validate EXT against the new target,
link them via new bpf_tracing_link, activate it via trampoline and return FD.
No link list anywhere.
Note that this second validation of EXT against new target is light weight comparing
to the load. The first load goes through all EXT instructions with verifier ctx of
the target prog. The second validation needs to compare BTF proto tgr_prog_fd+tgt_btf_id
with EXT's btf_id only (and check tgt_prog_fd->type/expected_attach_type).
Since EXT was loaded earlier it has valid insns.
So if you're thinking "cannot we validate insns at load time, but then remember
tgt stuff instead of creating a partial link, and double validate BTF at raw_tp_open
when it's called without tgt_prog_fd?"
The answer is "yes, we can", but double validation of BTF I think is just a waste of cycles,
when tgt prog could have been held a bit between load and attach.
And it's race free. Whereas if we remember target prog_id at load then raw_tp_open is
shooting in the dark. Unlikely, but that prog_id could have been reused.