-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add the option to support multiple and overrideable programs per cgroup #985
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. |
aya/src/programs/lirc_mode2.rs
Outdated
@@ -68,7 +68,8 @@ impl LircMode2 { | |||
let prog_fd = prog_fd.try_clone()?; | |||
let lircdev_fd = lircdev.as_fd().try_clone_to_owned()?; | |||
|
|||
bpf_prog_attach(prog_fd.as_fd(), lircdev_fd.as_fd(), BPF_LIRC_MODE2)?; | |||
const NO_FLAGS : u32 = 0; | |||
bpf_prog_attach(prog_fd.as_fd(), lircdev_fd.as_fd(), BPF_LIRC_MODE2, NO_FLAGS)?; |
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.
Per comment below this would become CgroupAttachFlags::default()
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've updated the fn signature to allow flags to be passed in, and updated doc tests to pass in CgroupAttachFlags::empty()
. Let me know if that's what you had in mind.
aya/src/programs/sock_ops.rs
Outdated
bitflags::bitflags! { | ||
/// Flags passed to [`SockOps::attach()`]. | ||
#[derive(Clone, Copy, Debug, Default)] | ||
pub struct SockOpsFlags: 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.
These aren't just SockOps flags though.
See: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/bpf.h#L1153
This should probably live in links.rs
and be called CgroupAttachFlags
.
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.
Good call. Done.
aya/src/programs/links.rs
Outdated
Self::attach_with_flags(prog_fd, target_fd, attach_type, 0) | ||
} | ||
|
||
pub(crate) fn attach_with_flags( |
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'd prefer changing the API here to add flags: u32
instead of adding a new funciton.
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.
Sure thing, done.
aya/src/programs/sock_ops.rs
Outdated
/// Attaches the program to the given cgroup. | ||
/// | ||
/// The returned value can be used to detach, see [SockOps::detach]. | ||
pub fn attach_with_flags<T: AsFd>(&mut self, cgroup: T, flags: SockOpsFlags) -> Result<SockOpsLinkId, ProgramError> { |
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 think I'd rather break the API here and add flags: CgroupAttachFlags
.
This makes it explicit which flags are being used.
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.
Sounds good. I wasn't sure how much of an expanding change you'd prefer. I've modified the attach()
fn signature here and for other program types.
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.
This makes it explicit which flags are being used.
I'm in two minds about this. For the Cgroup*
and SockOps
programs, I think it makes sense to have to pass CgroupAttachFlags
to attach()
- you're explicitly working with cgroups, so it makes sense to have to think about what's going on with other cgroups.
But for SkMsg
SkSkb
and LircMode2
, I think we should default to no flags like before, and have a separate attach_with_flags()
for the cases in which you do care about cgroups.
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.
That's a keen observation. I've updated this change following your advice to extend attach()
for specific program types, and adding attach_with_flags()
where the developer does not necessarily need to think about cgroups.
aya/src/programs/sock_ops.rs
Outdated
let prog_fd = self.fd()?; | ||
|
||
let link = ProgAttachLink::attach(prog_fd.as_fd(), cgroup.as_fd(), BPF_CGROUP_SOCK_OPS)?; | ||
let link = ProgAttachLink::attach_with_flags(prog_fd.as_fd(), cgroup.as_fd(), BPF_CGROUP_SOCK_OPS, flags.bits())?; |
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.
Aside: Given all the other Cgroup programs can be attached with bpf_link - see:
aya/aya/src/programs/cgroup_device.rs
Line 69 in d5414bf
if KernelVersion::current().unwrap() >= KernelVersion::new(5, 7, 0) { |
I'd wager that this is likely the case for sock_ops programs too. I'll open an issue for that.
#987
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.
👍
Hello, I'm just following up on the status of this one. |
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.
Almost there. See notes on the other generic flags you included in the CgroupAttachFlags.
I'm fine to have them there if you've tested that they work as expected...
I'd be genuinely surprised if all of the flags are honoured by every program type that attaches to cgroups.
I would recommend dropping them for the sake of simplicity 😆
Otherwise, the generic flags might be better off as struct GenericAttachFlags
it's been a while since I looked at bitflags but it would be nice to express that the flags could be either CgroupAttachFlags
or GenericAttachFlags
aya/src/programs/links.rs
Outdated
@@ -28,6 +31,25 @@ pub trait Link: std::fmt::Debug + 'static { | |||
fn detach(self) -> Result<(), ProgramError>; | |||
} | |||
|
|||
bitflags::bitflags! { | |||
/// Flags passed to [`SockOps::attach()`]. |
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.
This doc comment is now inaccurate
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.
Ah, true. I've switched it to the more general comment of "Program attachment flags." Let me know if you'd like a more descriptive comment.
aya/src/programs/links.rs
Outdated
/// Allows multiple eBPF programs to be attached. | ||
const ALLOW_MULTI = BPF_F_ALLOW_MULTI; | ||
/// Replaces an existing attachment. | ||
const REPLACE = BPF_F_REPLACE; |
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.
Have you tested this works as expected?
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.
Please see the Testing comment I've added below. This covers the two flags that have been kept in this PR.
aya/src/programs/links.rs
Outdated
/// Replaces an existing attachment. | ||
const REPLACE = BPF_F_REPLACE; | ||
/// Attaches the program so as to be executed first for the cgroup. | ||
const BEFORE = BPF_F_BEFORE; |
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.
And this one
aya/src/programs/links.rs
Outdated
/// Attaches the program so as to be executed first for the cgroup. | ||
const BEFORE = BPF_F_BEFORE; | ||
/// Attaches the program so as to be executed last for the cgroup. | ||
const AFTER = BPF_F_AFTER; |
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.
And this one too
aya/src/programs/links.rs
Outdated
/// Attaches the program so as to be executed last for the cgroup. | ||
const AFTER = BPF_F_AFTER; | ||
/// Indicates the eBPF program is referred to by its ID. | ||
const ID = BPF_F_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.
This wouldn't make sense to include anyway since we use FD, not 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.
Understood. I've removed these additional flags, since ALLOW_MULTI
and ALLOW_OVERRIDE
were the focus of my change.
Ha, so what's funny is on the fork of Aya I started with internally, I added only the |
TestingI tested with the application I'm developing by attaching the program twice to a cgroup and kicking off some network chatter. This was performed under three different conditions: 1) before this change (i.e. using the |
@reyzell, this pull request is now in conflict and requires a rebase. |
Updates: I've rebased from |
@dave-tucker I see the lint error due to changes to the public API [a]. Are there any actions I should take here? [a] https://github.com/aya-rs/aya/actions/runs/10159730726/job/28109127559?pr=985 |
It looks like you'll need to run |
Thanks @tyrone-wu, done. Blessed the public API changes. |
Hey @alessandrod, this pull request changes the Aya Public API and requires your review. |
Hello, @alessandrod and @dave-tucker, I'm checking in on the status of this one. Please let me know if I can help in any way. |
aya/src/programs/links.rs
Outdated
@@ -28,6 +28,17 @@ pub trait Link: std::fmt::Debug + 'static { | |||
fn detach(self) -> Result<(), ProgramError>; | |||
} | |||
|
|||
bitflags::bitflags! { |
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'm not sure that this should be a bitflag vs a regular enum.
The kernel API is weird and uses bitflags, but say we wanted to add support for
BPF_F_REPLACE to attach, we'd have to provide the old program somehow, so it's
unlikely that we'd use attach()
to do 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.
I see where you're going. I took a look, and I believe the use of bitflags is the more usable interface in this case. This allows the caller to easily set a combination of flags if they wish. The bitflags API frees the developer from having to do bit manipulation themselves.
If we later choose to add support for more flags, such as BPF_F_REPLACE
, BPF_F_BEFORE
, and BPF_F_AFTER
, then we would add the ability to supply the bpf_attr
anonymous struct's three additional fields of replace_bpf_fd
, relative_fd
, relative_id
– those would be separate arguments. We would also add a way to set the BPF_F_ID
flag to distinguish between the latter two.
I think the use of bitflags now is inline with where we would want 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.
This allows the caller to easily set a combination of flags if they wish. The bitflags API frees the developer from having to do bit manipulation themselves.
My point is that I suspect we won't ever expose the bitflag to the aya API.
If we later choose to add support for more flags, such as BPF_F_REPLACE, BPF_F_BEFORE, and BPF_F_AFTER, then we would add the ability to supply the bpf_attr anonymous struct's three additional fields of replace_bpf_fd, relative_fd, relative_id – those would be separate arguments.
Exactly, similar to https://docs.aya-rs.dev/aya/programs/xdp/struct.xdp#method.attach_to_link where we use a REPLACE bit, but don't expose it to the API, see https://docs.aya-rs.dev/src/aya/programs/xdp.rs#233
Or in other words: you have to pass (flag, fd_or_id) and it's unlikely that we'll have a single method to do that, but more realistically we'll have attach_replace(old) attach_relative(AttachFlags::Before, fd) etc, where the bitflags become an implementation detail but are not exposed
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.
These are good points. From what I understand, you can also attach a program with both the ALLOW_MULTI
and ALLOW_OVERRIDE
flags set. A bitflags argument is useful there to allow the caller to specify empty, either flag, or set both flags. However, to keep the Aya API simple, we could go with your idea of an enum, and perhaps with a third value that represents both (ALLOW_MULTI_AND_OVERRIDE
...?). If we want to avoid an EMPTY
value on the enum, we could include both an attach
and attach_with_flags
function.
An alternative is to implement the flags argument as a struct with two booleans:
struct CgroupAttachFlags {
allow_multi: bool,
allow_override: bool,
}
A caller could then set either flag, both flags, or none. That's probably my favorite option, as it seems clean.
Do any of these options resonate with you?
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.
* BPF_F_ALLOW_OVERRIDE: If a sub-cgroup installs some bpf program,
* the program in this cgroup yields to sub-cgroup program.
*
* BPF_F_ALLOW_MULTI: If a sub-cgroup installs some bpf program,
* that cgroup program gets run in addition to the program in this cgroup.
so it seems like they're exclusive.
Given this and the docs of the other flags, I'm not convinced that at the aya level there's any need for bitflags, and bitflags are less idiomatic than regular enums so we should avoid them when possible.
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.
You're right, good instincts. I've updated this change to use an enum, and included a function scoped to the crate to translate the enum to its BPF value.
aya/src/programs/sock_ops.rs
Outdated
/// Attaches the program to the given cgroup. | ||
/// | ||
/// The returned value can be used to detach, see [SockOps::detach]. | ||
pub fn attach_with_flags<T: AsFd>(&mut self, cgroup: T, flags: SockOpsFlags) -> Result<SockOpsLinkId, ProgramError> { |
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.
This makes it explicit which flags are being used.
I'm in two minds about this. For the Cgroup*
and SockOps
programs, I think it makes sense to have to pass CgroupAttachFlags
to attach()
- you're explicitly working with cgroups, so it makes sense to have to think about what's going on with other cgroups.
But for SkMsg
SkSkb
and LircMode2
, I think we should default to no flags like before, and have a separate attach_with_flags()
for the cases in which you do care about cgroups.
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 for doing this work!
Looks great, as per comment I suspect CgroupAttachFlags should be a regular enum, and some programs should have an explicit attach_with_flags, see #985 (comment)
I think I encountered this issue before. Try updating nightly and blessing public-api again. |
Good call. The public API had changed after the last update of this PR, but before the workflow approval jobs had run. Thanks ;) |
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.
This looks good now, just not sure on the naming
aya/src/programs/links.rs
Outdated
@@ -28,6 +28,30 @@ pub trait Link: std::fmt::Debug + 'static { | |||
fn detach(self) -> Result<(), ProgramError>; | |||
} | |||
|
|||
/// Program attachment options. | |||
#[derive(Clone, Copy, Debug, Default)] | |||
pub enum CgroupAttachFlag { |
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.
nit: call this CgroupAttachMode?
aya/src/programs/lirc_mode2.rs
Outdated
/// Attaches the program to the given lirc device. | ||
/// | ||
/// The returned value can be used to detach, see [LircMode2::detach]. | ||
pub fn attach_with_flag<T: AsFd>( |
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.
Here and everywhere else: attach_with_cgroup_mode?
I'm not 100% convinced on mode, do you have a better name?
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.
Ha, now onto the hard part, naming :)
So far I think you've picked a good name. Others I thought of are CgroupAttach- Strategy or Policy or Cardinality or Rule
, they each have pros and cons. ("Strategy" is a bit generic, "Policy" is good but feels more security focused, "Cardinality" is accurate but a bit long, "Rule" I kind of like.)
If we go with any of these, I'm thinking of changing the name of the default enum value, as a mode of None
sounds odd. Do these value names work?
pub enum CgroupAttachMode {
Single,
AllowMultiple,
AllowOverride,
}
CI is failing because it needs rebase to pick up #1016 |
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.
Almost there!
aya/src/programs/cgroup_skb.rs
Outdated
@@ -87,6 +88,7 @@ impl CgroupSkb { | |||
&mut self, | |||
cgroup: T, | |||
attach_type: CgroupSkbAttachType, | |||
flag: CgroupAttachMode, |
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.
here and everywhere else: please rename the argument name from flag to mode
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.
Right! Done.
aya/src/programs/links.rs
Outdated
AllowMultiple, | ||
} | ||
|
||
impl CgroupAttachMode { |
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 think this should be impl From<CgroupAttachMode> for 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.
Done
6abc084
to
17a4c32
Compare
I just checked in the kernel and these are the cgroup-aware programs:
For LIRC_MODE2 SK_MSG and SK_SKB you can't pass any of those flags, so we should remove attach_with_cgroup_mode from them. |
0fe6e21
to
d169919
Compare
Ahh, thanks for catching that! I should have realized that. Fixed. |
Applied two changes:
The following build steps pass:
|
Can you update your nightly and re-run |
Hmm, I made sure to do so before my last push (locally I'm up to date with the latest commit of 15eb935). I've done so again now and don't see anything that has changed. Something isn't clear to me about what's failing. I'll try to dig in... |
Hmm, interesting. What's your output for |
I have the following:
|
It looks you'll need to update nightly again 😅. The latest is currently |
This change allows multiple BPF programs to attach to a cgroup (via the option `CgroupAttachMode::AllowMultiple`), and allows a program to specify that it can be overridden by one in a sub-cgroup (via the option `CgroupAttachMode::AllowOverride`).
You're right Tyron. Updated! |
Add the option to support multiple and overrideable programs per cgroup
This change allows multiple BPF programs to attach to a cgroup (via the option
CgroupAttachMode::AllowMultiple
), and allows a program to specify that it can beoverridden by one in a sub-cgroup (via the option
CgroupAttachMode::AllowOverride
).