-
Notifications
You must be signed in to change notification settings - Fork 298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement XDP map types #527
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
2f227e7
to
ac441ef
Compare
eee5ffe
to
ba55193
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments so far... and can you please make this a draft PR if you're still working on it? That way I can hold of doing another review until you're done 😆
Whooops sorry. Done for the draft! I always find additionnal stuff to shove in a PR 😅 I’ll apply your suggestions this afternoon. |
f2e03bf
to
f039aa8
Compare
b952c0f
to
a96b1fc
Compare
@dave-tucker I now consider this PR as ready, I shoved everything I could think of as far as maps are concerned. It just needs #532 first to be "canonically" built with the additional bindings generated by bors. The last pain point are the integration tests somehow not passing in the CI while they run fine locally. I'll try to troubleshoot it but I'm not able to reproduce it... |
80ffbcb
to
e552e3d
Compare
set/insert functions can now take an optional bpf program fd to run once the packet has been redirected from the main probe
Values in those map are small enough to return copied values instead of reference to values.
Not having a generic here allows to pass `None` without specifying the actual type you don't care about.
Use our own type that: - is stable as not from bindgen - does not have an union inside
On startup, the kernel is probed for support of chained program ids for CpuMap, DevMap and DevMapHash, and will patch maps at load time to have the proper size. Then, at runtime, the support is checked and will error out if a program id is passed when the kernel does not support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments!
@@ -128,6 +128,35 @@ pub fn sk_msg(attrs: TokenStream, item: TokenStream) -> TokenStream { | |||
} | |||
} | |||
|
|||
/// Marks a function as an eBPF XDP program that can be attached to a network interface. | |||
/// | |||
/// On some NIC drivers, XDP probes are compatible with jumbo frames through the use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so? Maybe it depends on the attach mode? On aws I've definitely
worked with 9001 bytes large frames with regular (no frags) xdp programs
bpf/aya-bpf/src/maps/xdp/cpu_map.rs
Outdated
/// } | ||
/// ``` | ||
#[inline(always)] | ||
pub fn redirect(&self, index: u32, flags: u64) -> u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but that's a C API :P A rusty equivalent would be
MAP.redirect(0, 0).or(Ok(XDP_DROP))
And if you make it return Result<i32, i32> I guess this also works
MAP.redirect(0, XDP_DROP as u64)
and then in the caller
match try_thing(ctx) {
Ok(res) => res,
Err(res) => res,
}
So I don't see a reason not to return Result?
bpf/aya-bpf/src/maps/xdp/dev_map.rs
Outdated
|
||
#[derive(Clone, Copy)] | ||
pub struct DevMapValue { | ||
pub ifindex: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if_index (search ifindex PR-wide :P)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
bpf/aya-bpf/src/maps/xdp/dev_map.rs
Outdated
pub struct DevMapValue { | ||
/// Target interface index to redirect to. | ||
pub if_index: u32, | ||
pub prog_id: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No docs for prog_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh. fixed.
also changed the u32 -> Option<NonZeroU32>
to reflect the userspace API (and the fact that there's not necessarily a chained program)
couple smol nits, then it's ready to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 31 of 31 files at r14, 32 of 32 files at r17, 13 of 13 files at r19, all commit messages.
Reviewable status: all files reviewed, 56 unresolved discussions (waiting on @alessandrod, @astoycos, @dave-tucker, @Tuetuopay, and @vadorovsky)
aya/src/maps/xdp/cpu_map.rs
line 51 at r17 (raw file):
/// # See also /// /// Kernel documentation: <https://docs.kernel.org/next/bpf/map_cpumap.html>
why the angle brackets?
aya/src/maps/xdp/dev_map.rs
line 43 at r17 (raw file):
/// # See also /// /// Kernel documentation: <https://docs.kernel.org/next/bpf/map_devmap.html>
ditto
aya/src/maps/xdp/dev_map_hash.rs
line 43 at r17 (raw file):
/// # See also /// /// Kernel documentation: <https://docs.kernel.org/next/bpf/map_devmap.html>
ditto
aya/src/maps/xdp/mod.rs
line 16 at r17 (raw file):
#[derive(Error, Debug)] /// Errors occuring from working with XDP Maps
period?
aya/src/maps/xdp/xsk_map.rs
line 36 at r17 (raw file):
/// # See also /// /// Kernel documentation: <https://docs.kernel.org/next/bpf/map_xskmap.html>
ditto
bpf/aya-bpf/src/maps/xdp/cpu_map.rs
line 120 at r19 (raw file):
match ret.unsigned_abs() as u32 { XDP_REDIRECT => Ok(XDP_REDIRECT), // Return XDP_REDIRECT on success, or the value of the two lower bits of the flags
this comment should probably on top of the match?
EDIT: there are many instances of this - consider extracting at least some of this logic so it can be documented just once.
bpf/aya-bpf/src/maps/xdp/dev_map.rs
line 145 at r19 (raw file):
match ret.unsigned_abs() as u32 { XDP_REDIRECT => Ok(XDP_REDIRECT), // Return XDP_REDIRECT on success, or the value of the two lower bits of the flags
ditto
bpf/aya-bpf/src/maps/xdp/dev_map_hash.rs
line 147 at r19 (raw file):
match ret.unsigned_abs() as u32 { XDP_REDIRECT => Ok(XDP_REDIRECT), // Return XDP_REDIRECT on success, or the value of the two lower bits of the flags
ditto
bpf/aya-bpf/src/maps/xdp/xsk_map.rs
line 163 at r19 (raw file):
match ret.unsigned_abs() as u32 { XDP_REDIRECT => Ok(XDP_REDIRECT), // Return XDP_REDIRECT on success, or the value of the two lower bits of the flags
one more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 31 of 34 files reviewed, 57 unresolved discussions (waiting on @alessandrod, @astoycos, @dave-tucker, @tamird, and @vadorovsky)
aya/src/maps/xdp/cpu_map.rs
line 51 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
why the angle brackets?
Rustdocs auto-link. <url>
is equivalent to [url](url)
. This was a suggestion from cargo docs
directly.
aya/src/maps/xdp/dev_map.rs
line 43 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
ditto
aya/src/maps/xdp/dev_map_hash.rs
line 43 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
ditto
aya/src/maps/xdp/mod.rs
line 16 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
period?
done. fixed
aya/src/maps/xdp/xsk_map.rs
line 36 at r17 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
ditto
bpf/aya-bpf/src/maps/xdp/cpu_map.rs
line 120 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this comment should probably on top of the match?
EDIT: there are many instances of this - consider extracting at least some of this logic so it can be documented just once.
Done.
Added a try_redirect_map
function that all four maps call.
bpf/aya-bpf/src/maps/xdp/dev_map.rs
line 145 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
done. ditto
bpf/aya-bpf/src/maps/xdp/dev_map_hash.rs
line 147 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
done. ditto
bpf/aya-bpf/src/maps/xdp/xsk_map.rs
line 163 at r19 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
one more
done. ditto
This should be all good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Thanks so much @dave-tucker and @Tuetuopay for the hard work! And to everyone who reviewed ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all!
Reviewed 1 of 3 files at r20, 8 of 8 files at r21, all commit messages.
Dismissed @vadorovsky, and @vadorovsky from 48 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @astoycos, @dave-tucker, and @vadorovsky)
This PR is an update of @dave-tucker’s #292 updated for the current Aya main.
It adds implementations for:
All of those can be used to redirect XDP packets to various notations. Note that the userspace part (umem ans socket) of xskmap (i.e. AF_XDP) is not handled by this PR and requires other libraries (like libbpf, xsk-rs, …). For this, see #507.
All map types have been tested working.
TODO:
add bindings tostruct xdp_sock
to implementaya::maps::XskMap::get
Cancelled: contrary to what the kernel documentation say,
XskMap
does not support element lookup from the userspace (https://elixir.bootlin.com/linux/v6.2/source/net/xdp/xskmap.c#L148).xdp.frags/devmap
et al)Closes: #193 #194 #195 #199
This change is