search

foniod / redbpf

Rust library for building and running BPF/eBPF modules
Apache License 2.0
1.71k stars 136 forks source link

Difference in section name interpretation, compared to libbpf #56

Open tilpner opened 4 years ago

tilpner commented 4 years ago

Hi, I modified redbpf to load BPF_PROG_TYPE_CGROUP_SKB programs, but there's a design decision to be made before sending a PR.

libbpf has a mapping of section names to program parameters, and I think it would be good to keep redbpfs interpretation as close as possible, to prevent confusion by e.g. slightly different uses of the second part.

Should redbpf try to mirror the libbpf list, restricted to implemented types, of course?

Specifically, with a section name of cgroup_skb/egress, libbpf will match it to a program type of BPF_PROG_TYPE_CGROUP_SKB, and an expected attachment type of BPF_CGROUP_INET_EGRESS, but redbpf will use the egress part as a name, and thus can't select an expected attachment type. libbpf instead seems to use the symbol name as the program name.

Assigning meaning to the second part might break some existing code, but does reduce future surprises for new users. Consistency with libbpf helps when following documentation meant for libbpf.

alessandrod commented 4 years ago

Hi, I modified redbpf to load BPF_PROG_TYPE_CGROUP_SKB programs, but there's a design decision to be made before sending a PR.

Awesome! Please keep in mind that in the process of adding support for uprobes, I'm currently refactoring redbpf::Program and friends and I'm aiming to send a PR sometime in the next 24hrs, then get it merged early next week. If you're almost ready to send a PR, we probably want to try to get it merged before we merge mine as my changes are pretty invasive (I'm happy to rebase on your work).

libbpf has a mapping of section names to program parameters, and I think it would be good to keep redbpfs interpretation as close as possible, to prevent confusion by e.g. slightly different uses of the second part.

Should redbpf try to mirror the libbpf list, restricted to implemented types, of course?

Yes that sounds like a good idea.

Specifically, with a section name of cgroup_skb/egress, libbpf will match it to a program type of BPF_PROG_TYPE_CGROUP_SKB, and an expected attachment type of BPF_CGROUP_INET_EGRESS, but redbpf will use the egress part as a name, and thus can't select an expected attachment type. libbpf instead seems to use the symbol name as the program name.

I'd be happy with a PR that implements the same logic as libbpf.

tilpner commented 4 years ago

If you're almost ready to send a PR, we probably want to try to get it merged before we merge mine as my changes are pretty invasive (I'm happy to rebase on your work).

No, I'm not quite ready yet. I'll wait on your PR and see what you changed.

I'd be happy with a PR that implements the same logic as libbpf.

Great! I assume this also depends on your PR?

alessandrod commented 4 years ago

I'd be happy with a PR that implements the same logic as libbpf.

Great! I assume this also depends on your PR?

Probably only tangentially as I guess the ELF symbol name stuff will go in Module::parse, which I barely touched, see: https://github.com/redsift/redbpf/pull/57.