feat(aya-ebpf): Implement memmove

[wdullaer]

The compiler will emit this function for certain operations, but aya currently does not provide an implementation.
This leads to ebpf loading failures as the kernel can't find the symbol when loading the program.

The implementation is based on https://github.com/rust-lang/compiler-builtins/blob/master/src/mem/mod.rs#L29-L40 and https://github.com/rust-lang/compiler-builtins/blob/master/src/mem/impls.rs#L128-L135 Only the simplest case has been implemented, none of the word optimizations, since memcpy also doesn't seem to have them.

[netlify]

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	fad75de
🔍 Latest deploy log	https://app.netlify.com/sites/aya-rs-docs/deploys/668300b0adfc860008c212f7
😎 Deploy Preview	https://deploy-preview-971--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[vadorovsky]

I'm actually quite surprised that rust compiler-builtins don't optimize this implementation somehow (by loading bigger chunks and reducing iterations). I think in eBPF we could load with u64 chunks. But anyway, that's not related this PR. 😃

[vadorovsky]

Thanks a lot for the change! 🙏

Could you write a small integration tests which uses memmove? Scrolling back in Discord, I see that your motivation behind this is being able to do

unsafe { (*ip_hdr).dst_addr.in6_u.u6_addr8 = new_ip };

As it was resulting in a verifier error. I think it would be nice to have a test for that.

[wdullaer]

It wasn't immediately clear to me where to add tests for this, which is why they're absent.

I'll take a stab at writing an integration test.
It might be hard to guarantee that we're using memmove though, since people reported on discord that using clone at the right place allowed them to avoid it.

[vadorovsky]

I think if you are able to write a small program which triggers the error on your kernel, it's almost certainly going to be a reliable reproduces. Just avoid clone() in your example and make sure it fails with the current Aya codebase (without your changes). :)

Happy to help with the integration test.

The first step would be adding an .rs file with a regular aya-ebpf based program here:

https://github.com/aya-rs/aya/tree/main/test/integration-ebpf/src

Then including it here as a [[ bin ]] in Cargo.toml:

https://github.com/aya-rs/aya/blob/main/test/integration-ebpf/Cargo.toml

And writing a test on the user-space side. I think in your case just loading the program would be totally sufficient, no need to do any other actions.

https://github.com/aya-rs/aya/blob/main/test/integration-test/src/tests/load.rs#L285

I would literally just try something like:

#[test]
fn memmove() {
    let mut bpf = Ebpf::load(crate::TEST).unwrap();
    let prog: &mut SchedClassifier = bpf
        .program_mut("test_memmove")
        .unwrap()
        .try_into()
        .unwrap();

    prog.load().unwrap();
    assert_loaded("test_memmove");

I assumed SchedClassifier as a program type, since the case with overriding the packet data seems the easiest and TC has less limitations than XDP, at least for testing.

[wdullaer]

I have added an integration test, and simplified the implementation of memmove.
The integration test will fail when the memmove code is commented out, it passes when memmove is present.
The verifier error produced by the integration tests is slightly different from what I saw previously, but I think it may be due to the lack of BTF info here.

I have copy/pasted some struct definitions from network_types to produce the test.
If you prefer I can also take a dependency on the library.

[vadorovsky]

I have added an integration test, and simplified the implementation of memmove. The integration test will fail when the memmove code is commented out, it passes when memmove is present.

Awesome, that's exactly what I had in mind. 🙂

The verifier error produced by the integration tests is slightly different from what I saw previously, but I think it may be due to the lack of BTF info here.

What's exactly the error?

I have copy/pasted some struct definitions from network_types to produce the test. If you prefer I can also take a dependency on the library.

I think I would be fine with adding a dependency on the library, it doesn't have any dependencies by default.

[vadorovsky]

The lint job is failing, because of public API changes.

@wdullaer Can you run cargo xtask public-api --bless and commit the result (preferably as a separate commit)? Also, before doing so, make sure you rebase.

I also have a slight preference towards importing the network-types crate instead of copying the types. I don't think it's going to make the build significantly slower. @alessandrod do you agree?

Otherwise, it looks very good, thanks for working on this! 🙏

[wdullaer]

The verifier error produced by the integration tests is slightly different from what I saw previously, but I think it may be due to the lack of BTF info here.

What's exactly the error?

<snip>
regs=8 stack=0 before 34: (bf) r6 = r3
regs=8 stack=0 before 33: (b7) r5 = 16
regs=8 stack=0 before 32: (0f) r4 += r3
regs=8 stack=0 before 31: (bf) r4 = r0
regs=8 stack=0 before 30: (0f) r7 += r3
regs=8 stack=0 before 29: (bf) r7 = r2
regs=8 stack=0 before 28: (3d) if r1 >= r3 goto pc+67
regs=a stack=0 before 27: (1f) r1 -= r2
regs=e stack=0 before 26: (bf) r0 = r1
regs=e stack=0 before 21: (85) call pc+4
36: (bf) r5 = r4
37: (57) r5 &= -8
R5 bitwise operator &= on pointer prohibited

I checked the assembly of the version with memmove implemented and I can clearly see the function implementation in the assembly. So I'm certain the test exercises the code.

I added the network-types crate as a dependency and ran the bless task (this target does not work on macos btw).

One final note: memmove and memcpy have their src attribute marked as *mut u8. However these are only ever read from, so they can be safely converted into *const u8 I think.
I didn't do it to keep the signature the same between the two and because this constitutes a technical public-api change.

[vadorovsky]

Amazing stuff, looks good!

I'm fine with keeping *mut u8 (for consistency with the compiler-builtins impl and so).

[tamird]

You'll need to rebase to fix the integration test failures.

[vadorovsky]

@wdullaer Lint errors look legit. The integration test errors should be gone after a rebase.

[wdullaer]

I'm sorry about the lint failures.
For some reason I can't get the lsp to work when in the integration test module. My dev/test cycle is not very efficient for this project at the moment.
Anyway, it should all be good now.
The integration tests pass on my VM now, apart from 2 tests which also fail on main.