aya-ebpf: Add BPF_F_ADJ_ROOM_ENCAP_L2 "macro"

[aureliar8]

This function is needed to properly add a L2 header when using bpf_skb_adjust_room 1. As it is originally a C macro, it isn't automatically generated in the bindings crate

---

This change is 

[netlify]

❌ Deploy Preview for aya-rs-docs failed.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	14e7f68
🔍 Latest deploy log	https://app.netlify.com/projects/aya-rs-docs/deploys/691603bb255cc600080fe2b5

[aureliar8]

While looking at the generated bindings, I was also very surprised to see that BPF_F_ADJ_ROOM_xxx are c_uint / u32

aya/ebpf/aya-ebpf-bindings/src/x86_64/bindings.rs

Lines 1554 to 1563 in 0c2cc1f

	pub const BPF_F_ADJ_ROOM_FIXED_GSO: _bindgen_ty_17 = 1;
	pub const BPF_F_ADJ_ROOM_ENCAP_L3_IPV4: _bindgen_ty_17 = 2;
	pub const BPF_F_ADJ_ROOM_ENCAP_L3_IPV6: _bindgen_ty_17 = 4;
	pub const BPF_F_ADJ_ROOM_ENCAP_L4_GRE: _bindgen_ty_17 = 8;
	pub const BPF_F_ADJ_ROOM_ENCAP_L4_UDP: _bindgen_ty_17 = 16;
	pub const BPF_F_ADJ_ROOM_NO_CSUM_RESET: _bindgen_ty_17 = 32;
	pub const BPF_F_ADJ_ROOM_ENCAP_L2_ETH: _bindgen_ty_17 = 64;
	pub const BPF_F_ADJ_ROOM_DECAP_L3_IPV4: _bindgen_ty_17 = 128;
	pub const BPF_F_ADJ_ROOM_DECAP_L3_IPV6: _bindgen_ty_17 = 256;
	pub type _bindgen_ty_17 = ::aya_ebpf_cty::c_uint;

Even though the kernel definitions for them is a ULL https://elixir.bootlin.com/linux/v6.16.4/source/include/uapi/linux/bpf.h#L6132

Shouldn't they be mapped to u64 ?

[tamird]

The usual workaround is to add a function (in C) that returns the macro value. As for the other "constants": feel free to try to changing the bindgen options.

[aureliar8]

The usual workaround is to add a function (in C) that returns the macro value

I'm not sure what you mean by that, as I see no C file that I could modify in this repo ? Unless you mean adding this function in the kernel source code ?

I'll have a look at the bindgen options

[tamird]

Yes currently we point bindgen at libbpf headers. You'd have to introduce a new headset that #includes the libbpf headers and defines a function that returns this macro, then point bindgen at it.

[tamird]

Wait, that's not right. That won't work without actual ffi. Maybe ask upstream to figure out why this macro can't generate a rust definition?

[tamird]

@vadorovsky want to take a look?

[vadorovsky]

Thanks! I have a couple of nits.

[vadorovsky]

I would actually prefer to stick to the Rust naming conventions and just call it bpf_d_adj_room_encap_l2. I think we should limit C style names only to bindings (and also - we should try to wrap as many bindings as possible and limit their direct usage in actual programs).

We could still keep a doc alias though:

#[doc(alias = "BPF_F_ADJ_ROOM_ENCAP_L2")]

So when someone types the C name, rust-analyzer's autocomplete still points to this function.

[aureliar8]

Are you sure about following the Rust naming convention ? It's going to look a bit odd to have this function in lowercase while other bindings are in upper case.
For example the code to perform geneve encapsulation would be the following with the current casing

let additional_len = EthHdr::LEN + Ipv4Hdr::LEN + UdpHdr::LEN + GeneveHdr::LEN;
ctx.adjust_room(
  additional_len as i32,
  BPF_ADJ_ROOM_MAC, 
  | BPF_F_ADJ_ROOM_ENCAP_L2_ETH as u64 
  | BPF_F_ADJ_ROOM_ENCAP_L2(EthHdr::LEN) 
  | BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 as u64 
  | BPF_F_ADJ_ROOM_ENCAP_L4_UDP as u64, 
)

With your casing proposal it'd look like that:

let additional_len = EthHdr::LEN + Ipv4Hdr::LEN + UdpHdr::LEN + GeneveHdr::LEN;
ctx.adjust_room(
  additional_len as i32,
  BPF_ADJ_ROOM_MAC, 
  | BPF_F_ADJ_ROOM_ENCAP_L2_ETH as u64 
  | bpf_f_adj_room_encap_l2(EthHdr::LEN) 
  | BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 as u64 
  | BPF_F_ADJ_ROOM_ENCAP_L4_UDP as u64, 
)

And actually I'm also wondering is the definition of BPF_F_ADJ_ROOM_ENCAP_L2 shouldn't live in the aya-ebpf-bindings crate. Because it seems weird thatBPF_F_ADJ_ROOM_ENCAP_L2 lives in a different crate than
BPF_F_ADJ_ROOM_ENCAP_L2_ETH, BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 etc...

[vadorovsky]

Yeah, I think it looks alright. Constants are constants, function calls are function calls. That's how you would write it in idiomatic Rust if there was no bias given by the C API.

You could always keep the function call as the last element of the | chain to group the constants together:

ctx.adjust_room(
  additional_len as i32,
  BPF_ADJ_ROOM_MAC, 
  | BPF_F_ADJ_ROOM_ENCAP_L2_ETH as u64,
  | BPF_F_ADJ_ROOM_ENCAP_L3_IPV4 as u64 
  | BPF_F_ADJ_ROOM_ENCAP_L4_UDP as u64,
  | bpf_f_adj_room_encap_l2(EthHdr::LEN)
)

[aureliar8]

Done :)

The netifly CI seems to have a problem. Should I rebase or do something or is it fine to keep it like that ?

[tamird]

never hurts to rebase. please squash the commits as well

[tamird]

@tamird reviewed 2 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aureliar8 and @vadorovsky)

---

ebpf/aya-ebpf/src/lib.rs line 123 at r10 (raw file):

/// in the Linux user-space API.
///
/// [uapi-bpf-adj-room-encap-l2]: https://elixir.bootlin.com/linux/v6.16.4/source/include/uapi/linux/bpf.h#L6149

would you mind making this a github link instead of elixir?

[tamird]

never hurts to rebase. please squash the commits as well

[aureliar8]

would you mind making this a github link instead of elixir?

Done :)

[tamird]

@tamird reviewed 2 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aureliar8 and @vadorovsky)

---

-- commits line 8 at r10:
can you restore this? we do not retain the PR description, so if you care about this being in git log it must be here in the commit

Code quote:

  This macro is needed to properly add a L2 header when using
  bpf_skb_adjust_room [1]. As it is originally a C macro, it isn't
  automatically generated in the `bindings` crate

  [1]: https://docs.ebpf.io/linux/helper-function/bpf_skb_adjust_room/