Thread (34 messages) 34 messages, 3 authors, 2020-09-25

Re: [PATCH bpf-next v8 04/11] bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach

From: Toke Høiland-Jørgensen <hidden>
Date: 2020-09-24 22:00:03
Also in: bpf

Alexei Starovoitov [off-list ref] writes:
quoted
+	struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
+	struct bpf_prog *tgt_prog;
+	struct bpf_trampoline *tgt_trampoline;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
...
quoted
 struct bpf_tracing_link {
 	struct bpf_link link;
 	enum bpf_attach_type attach_type;
+	struct bpf_trampoline *trampoline;
+	struct bpf_prog *tgt_prog;
imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
I started changing this as you suggested, but I think it actually makes
the code weirder. We'll end up with a lot of 'tgt_prog =
prog->aux->dest_prog' assignments in the verifier, unless we also rename
all of the local variables, which I think is just code churn for very
little gain (the existing 'target' meaning is quite clear, I think).

I also think it's quite natural that the target moves; I mean, it's
literally the same pointer being re-assigned from prog->aux to the link.
We could rename the link member to 'attached_tgt_prog' or something like
that, but I'm not sure it helps (and I don't see much of a problem in
the first place).

WDYT?

-Toke
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help