From f666277229c00ce365d77f57a30bdcf8dcf0c993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20M=C3=BCller?= Date: Mon, 20 Nov 2023 11:49:54 -0800 Subject: [PATCH 1/5] Use non-exclusive receiver for immutable datasec getters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we create a skeleton with a getter for an immutable map, we should be able to use a non-exclusive receiver (&self) instead of an exclusive one (&mut self). Using the latter unconditionally can be the cause of unnecessary borrow conflicts. Refs: #606 Signed-off-by: Daniel Müller --- libbpf-cargo/src/gen/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libbpf-cargo/src/gen/mod.rs b/libbpf-cargo/src/gen/mod.rs index 3267ef50..5ff832fa 100644 --- a/libbpf-cargo/src/gen/mod.rs +++ b/libbpf-cargo/src/gen/mod.rs @@ -524,9 +524,9 @@ fn gen_skel_datasec_getters( write!( skel, r#" - pub fn {name}(&mut self) -> &'_ {mutability} {struct_name} {{ + pub fn {name}(&{mutability} self) -> &{mutability} {struct_name} {{ unsafe {{ - std::mem::transmute::<*mut std::ffi::c_void, &'_ {mutability} {struct_name}>( + std::mem::transmute::<*mut std::ffi::c_void, &{mutability} {struct_name}>( self.skel_config.map_mmap_ptr({idx}).unwrap() ) }} From f3e23fed11517532d5601313e0007c4ea2944316 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20M=C3=BCller?= Date: Mon, 20 Nov 2023 11:55:26 -0800 Subject: [PATCH 2/5] Remove 'mutable' argument to gen_skel_prog_getter() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is not too much of a point in passing in the desired mutability to gen_skel_prog_getter() when all call sites invoke it with both true and false. Move the logic for dealing with mutability into the function itself to shield callers from the need to care. Signed-off-by: Daniel Müller --- libbpf-cargo/src/gen/mod.rs | 59 +++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/libbpf-cargo/src/gen/mod.rs b/libbpf-cargo/src/gen/mod.rs index 5ff832fa..166ab58c 100644 --- a/libbpf-cargo/src/gen/mod.rs +++ b/libbpf-cargo/src/gen/mod.rs @@ -462,40 +462,45 @@ fn gen_skel_map_getter( Ok(()) } -fn gen_skel_prog_getter( +fn gen_skel_prog_getters( skel: &mut String, object: &mut BpfObj, obj_name: &str, open: bool, - mutable: bool, ) -> Result<()> { - if ProgIter::new(object.as_mut_ptr()).next().is_none() { - return Ok(()); - } + let mut gen = |mutable| -> Result<()> { + if ProgIter::new(object.as_mut_ptr()).next().is_none() { + return Ok(()); + } - let (struct_suffix, mut_prefix, prog_fn) = if mutable { - ("Mut", "mut ", "progs_mut") - } else { - ("", "", "progs") - }; + let (struct_suffix, mut_prefix, prog_fn) = if mutable { + ("Mut", "mut ", "progs_mut") + } else { + ("", "", "progs") + }; - let return_ty = if open { - format!("Open{obj_name}Progs{struct_suffix}") - } else { - format!("{obj_name}Progs{struct_suffix}") - }; + let return_ty = if open { + format!("Open{obj_name}Progs{struct_suffix}") + } else { + format!("{obj_name}Progs{struct_suffix}") + }; - write!( - skel, - r#" - pub fn {prog_fn}(&{mut_prefix}self) -> {return_ty}<'_> {{ - {return_ty} {{ - inner: &{mut_prefix}self.obj, + write!( + skel, + r#" + pub fn {prog_fn}(&{mut_prefix}self) -> {return_ty}<'_> {{ + {return_ty} {{ + inner: &{mut_prefix}self.obj, + }} }} - }} - "#, - )?; + "#, + )?; + + Ok(()) + }; + let () = gen(true)?; + let () = gen(false)?; Ok(()) } @@ -784,8 +789,7 @@ fn gen_skel_contents(_debug: bool, raw_obj_name: &str, obj_file_path: &Path) -> writeln!(skel, "}}")?; writeln!(skel, "impl Open{name}Skel<'_> {{", name = &obj_name)?; - gen_skel_prog_getter(&mut skel, &mut object, &obj_name, true, false)?; - gen_skel_prog_getter(&mut skel, &mut object, &obj_name, true, true)?; + gen_skel_prog_getters(&mut skel, &mut object, &obj_name, true)?; gen_skel_map_getter(&mut skel, &mut object, &obj_name, true, false)?; gen_skel_map_getter(&mut skel, &mut object, &obj_name, true, true)?; gen_skel_datasec_getters(&mut skel, &mut object, raw_obj_name, false)?; @@ -830,8 +834,7 @@ fn gen_skel_contents(_debug: bool, raw_obj_name: &str, obj_file_path: &Path) -> writeln!(skel, "}}")?; write!(skel, "impl {name}Skel<'_> {{", name = &obj_name)?; - gen_skel_prog_getter(&mut skel, &mut object, &obj_name, false, false)?; - gen_skel_prog_getter(&mut skel, &mut object, &obj_name, false, true)?; + gen_skel_prog_getters(&mut skel, &mut object, &obj_name, false)?; gen_skel_map_getter(&mut skel, &mut object, &obj_name, false, false)?; gen_skel_map_getter(&mut skel, &mut object, &obj_name, false, true)?; gen_skel_datasec_getters(&mut skel, &mut object, raw_obj_name, true)?; From d5e70ebf7c0d6d5f82200fef197d3ecf4843f465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20M=C3=BCller?= Date: Mon, 20 Nov 2023 11:56:26 -0800 Subject: [PATCH 3/5] Remove 'mutable' argument to gen_skel_map_getters() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is not too much of a point in passing in the desired mutability to gen_skel_map_getters() when all call sites invoke it with both true and false. Move the logic for dealing with mutability into the function itself to shield callers from the need to care. Signed-off-by: Daniel Müller --- libbpf-cargo/src/gen/mod.rs | 59 +++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/libbpf-cargo/src/gen/mod.rs b/libbpf-cargo/src/gen/mod.rs index 166ab58c..349075aa 100644 --- a/libbpf-cargo/src/gen/mod.rs +++ b/libbpf-cargo/src/gen/mod.rs @@ -425,40 +425,45 @@ fn gen_skel_datasec_defs(skel: &mut String, obj_name: &str, object: &[u8]) -> Re Ok(()) } -fn gen_skel_map_getter( +fn gen_skel_map_getters( skel: &mut String, object: &mut BpfObj, obj_name: &str, open: bool, - mutable: bool, ) -> Result<()> { - if MapIter::new(object.as_mut_ptr()).next().is_none() { - return Ok(()); - } + let mut gen = |mutable| -> Result<()> { + if MapIter::new(object.as_mut_ptr()).next().is_none() { + return Ok(()); + } - let (struct_suffix, mut_prefix, map_fn) = if mutable { - ("Mut", "mut ", "maps_mut") - } else { - ("", "", "maps") - }; + let (struct_suffix, mut_prefix, map_fn) = if mutable { + ("Mut", "mut ", "maps_mut") + } else { + ("", "", "maps") + }; - let return_ty = if open { - format!("Open{obj_name}Maps{struct_suffix}") - } else { - format!("{obj_name}Maps{struct_suffix}") - }; + let return_ty = if open { + format!("Open{obj_name}Maps{struct_suffix}") + } else { + format!("{obj_name}Maps{struct_suffix}") + }; - write!( - skel, - r#" - pub fn {map_fn}(&{mut_prefix}self) -> {return_ty}<'_> {{ - {return_ty} {{ - inner: &{mut_prefix}self.obj, + write!( + skel, + r#" + pub fn {map_fn}(&{mut_prefix}self) -> {return_ty}<'_> {{ + {return_ty} {{ + inner: &{mut_prefix}self.obj, + }} }} - }} - "#, - )?; + "#, + )?; + Ok(()) + }; + + let () = gen(true)?; + let () = gen(false)?; Ok(()) } @@ -790,8 +795,7 @@ fn gen_skel_contents(_debug: bool, raw_obj_name: &str, obj_file_path: &Path) -> writeln!(skel, "impl Open{name}Skel<'_> {{", name = &obj_name)?; gen_skel_prog_getters(&mut skel, &mut object, &obj_name, true)?; - gen_skel_map_getter(&mut skel, &mut object, &obj_name, true, false)?; - gen_skel_map_getter(&mut skel, &mut object, &obj_name, true, true)?; + gen_skel_map_getters(&mut skel, &mut object, &obj_name, true)?; gen_skel_datasec_getters(&mut skel, &mut object, raw_obj_name, false)?; writeln!(skel, "}}")?; @@ -835,8 +839,7 @@ fn gen_skel_contents(_debug: bool, raw_obj_name: &str, obj_file_path: &Path) -> write!(skel, "impl {name}Skel<'_> {{", name = &obj_name)?; gen_skel_prog_getters(&mut skel, &mut object, &obj_name, false)?; - gen_skel_map_getter(&mut skel, &mut object, &obj_name, false, false)?; - gen_skel_map_getter(&mut skel, &mut object, &obj_name, false, true)?; + gen_skel_map_getters(&mut skel, &mut object, &obj_name, false)?; gen_skel_datasec_getters(&mut skel, &mut object, raw_obj_name, true)?; writeln!(skel, "}}")?; From 1f13d4ec35ab6f9e4cd1a0fed2c358822db8fba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20M=C3=BCller?= Date: Mon, 20 Nov 2023 11:59:59 -0800 Subject: [PATCH 4/5] Remove 'mutable' argument to gen_skel_map_defs() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is not too much of a point in passing in the desired mutability to gen_skel_map_defs() when all call sites invoke it with both true and false. Move the logic for dealing with mutability into the function itself to shield callers from the need to care. Signed-off-by: Daniel Müller --- libbpf-cargo/src/gen/mod.rs | 105 ++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/libbpf-cargo/src/gen/mod.rs b/libbpf-cargo/src/gen/mod.rs index 349075aa..f6bbd52d 100644 --- a/libbpf-cargo/src/gen/mod.rs +++ b/libbpf-cargo/src/gen/mod.rs @@ -267,66 +267,71 @@ fn gen_skel_map_defs( object: &mut BpfObj, obj_name: &str, open: bool, - mutable: bool, ) -> Result<()> { - if MapIter::new(object.as_mut_ptr()).next().is_none() { - return Ok(()); - } - - let (struct_suffix, mut_prefix, map_fn) = if mutable { - ("Mut", "mut ", "map_mut") - } else { - ("", "", "map") - }; - - let (struct_name, inner_ty, return_ty) = if open { - ( - format!("Open{obj_name}Maps{struct_suffix}"), - "libbpf_rs::OpenObject", - "libbpf_rs::OpenMap", - ) - } else { - ( - format!("{obj_name}Maps{struct_suffix}"), - "libbpf_rs::Object", - "libbpf_rs::Map", - ) - }; - - write!( - skel, - r#" - pub struct {struct_name}<'a> {{ - inner: &'a {mut_prefix}{inner_ty}, - }} + let mut gen = |mutable| -> Result<()> { + if MapIter::new(object.as_mut_ptr()).next().is_none() { + return Ok(()); + } - impl {struct_name}<'_> {{ - "#, - )?; + let (struct_suffix, mut_prefix, map_fn) = if mutable { + ("Mut", "mut ", "map_mut") + } else { + ("", "", "map") + }; - for map in MapIter::new(object.as_mut_ptr()) { - let map_name = match get_map_name(map)? { - Some(n) => n, - None => continue, + let (struct_name, inner_ty, return_ty) = if open { + ( + format!("Open{obj_name}Maps{struct_suffix}"), + "libbpf_rs::OpenObject", + "libbpf_rs::OpenMap", + ) + } else { + ( + format!("{obj_name}Maps{struct_suffix}"), + "libbpf_rs::Object", + "libbpf_rs::Map", + ) }; write!( skel, r#" - pub fn {map_name}(&{mut_prefix}self) -> &{mut_prefix}{return_ty} {{ - self.inner.{map_fn}("{raw_map_name}").unwrap() + pub struct {struct_name}<'a> {{ + inner: &'a {mut_prefix}{inner_ty}, }} + + impl {struct_name}<'_> {{ "#, - map_name = map_name, - raw_map_name = get_raw_map_name(map)?, - return_ty = return_ty, - mut_prefix = mut_prefix, - map_fn = map_fn )?; - } - writeln!(skel, "}}")?; + for map in MapIter::new(object.as_mut_ptr()) { + let map_name = match get_map_name(map)? { + Some(n) => n, + None => continue, + }; + + write!( + skel, + r#" + pub fn {map_name}(&{mut_prefix}self) -> &{mut_prefix}{return_ty} {{ + self.inner.{map_fn}("{raw_map_name}").unwrap() + }} + "#, + map_name = map_name, + raw_map_name = get_raw_map_name(map)?, + return_ty = return_ty, + mut_prefix = mut_prefix, + map_fn = map_fn + )?; + } + writeln!(skel, "}}")?; + + Ok(()) + }; + + let () = gen(true)?; + let () = gen(false)?; Ok(()) } @@ -745,8 +750,7 @@ fn gen_skel_contents(_debug: bool, raw_obj_name: &str, obj_file_path: &Path) -> name = obj_name )?; - gen_skel_map_defs(&mut skel, &mut object, &obj_name, true, false)?; - gen_skel_map_defs(&mut skel, &mut object, &obj_name, true, true)?; + gen_skel_map_defs(&mut skel, &mut object, &obj_name, true)?; gen_skel_prog_defs(&mut skel, &mut object, &obj_name, true, false)?; gen_skel_prog_defs(&mut skel, &mut object, &obj_name, true, true)?; gen_skel_datasec_defs(&mut skel, raw_obj_name, &mmap)?; @@ -799,8 +803,7 @@ fn gen_skel_contents(_debug: bool, raw_obj_name: &str, obj_file_path: &Path) -> gen_skel_datasec_getters(&mut skel, &mut object, raw_obj_name, false)?; writeln!(skel, "}}")?; - gen_skel_map_defs(&mut skel, &mut object, &obj_name, false, false)?; - gen_skel_map_defs(&mut skel, &mut object, &obj_name, false, true)?; + gen_skel_map_defs(&mut skel, &mut object, &obj_name, false)?; gen_skel_prog_defs(&mut skel, &mut object, &obj_name, false, false)?; gen_skel_prog_defs(&mut skel, &mut object, &obj_name, false, true)?; gen_skel_link_defs(&mut skel, &mut object, &obj_name)?; From 6f483c24831542748883cf418d5a892358db4caa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20M=C3=BCller?= Date: Mon, 20 Nov 2023 12:15:03 -0800 Subject: [PATCH 5/5] Generate mutable and shared datasec accessors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's not particularly user friendly to only have exclusive shared datasec accessor variants available, as that can lead to unnecessary borrow conflicts. With this change we revamp the generation logic to generate shared and exclusive accessors when possible. Refs: #606 Signed-off-by: Daniel Müller --- examples/capable/src/main.rs | 6 +++--- examples/runqslower/src/main.rs | 6 +++--- examples/tproxy/src/main.rs | 6 +++--- libbpf-cargo/CHANGELOG.md | 2 ++ libbpf-cargo/src/gen/mod.rs | 37 ++++++++++++++++++++------------- libbpf-cargo/src/test.rs | 8 +++---- libbpf-rs/src/skeleton.rs | 22 +++++++++++++++----- 7 files changed, 55 insertions(+), 32 deletions(-) diff --git a/examples/capable/src/main.rs b/examples/capable/src/main.rs index e68ad5de..835398a9 100644 --- a/examples/capable/src/main.rs +++ b/examples/capable/src/main.rs @@ -186,9 +186,9 @@ fn main() -> Result<()> { let mut open_skel = skel_builder.open()?; //Pass configuration to BPF - open_skel.rodata().tool_config.tgid = opts.pid; //tgid in kernel is pid in userland - open_skel.rodata().tool_config.verbose = opts.verbose; - open_skel.rodata().tool_config.unique_type = opts.unique_type; + open_skel.rodata_mut().tool_config.tgid = opts.pid; //tgid in kernel is pid in userland + open_skel.rodata_mut().tool_config.verbose = opts.verbose; + open_skel.rodata_mut().tool_config.unique_type = opts.unique_type; let mut skel = open_skel.load()?; skel.attach()?; diff --git a/examples/runqslower/src/main.rs b/examples/runqslower/src/main.rs index 1ab237af..a1862f39 100644 --- a/examples/runqslower/src/main.rs +++ b/examples/runqslower/src/main.rs @@ -90,9 +90,9 @@ fn main() -> Result<()> { let mut open_skel = skel_builder.open()?; // Write arguments into prog - open_skel.rodata().min_us = opts.latency; - open_skel.rodata().targ_pid = opts.pid; - open_skel.rodata().targ_tgid = opts.tid; + open_skel.rodata_mut().min_us = opts.latency; + open_skel.rodata_mut().targ_pid = opts.pid; + open_skel.rodata_mut().targ_tgid = opts.tid; // Begin tracing let mut skel = open_skel.load()?; diff --git a/examples/tproxy/src/main.rs b/examples/tproxy/src/main.rs index 71b5ef7e..f1916da8 100644 --- a/examples/tproxy/src/main.rs +++ b/examples/tproxy/src/main.rs @@ -64,9 +64,9 @@ fn main() -> Result<()> { // Set constants let mut open_skel = skel_builder.open()?; - open_skel.rodata().target_port = opts.port.to_be(); - open_skel.rodata().proxy_addr = proxy_addr.to_be(); - open_skel.rodata().proxy_port = opts.proxy_port.to_be(); + open_skel.rodata_mut().target_port = opts.port.to_be(); + open_skel.rodata_mut().proxy_addr = proxy_addr.to_be(); + open_skel.rodata_mut().proxy_port = opts.proxy_port.to_be(); // Load into kernel let skel = open_skel.load()?; diff --git a/libbpf-cargo/CHANGELOG.md b/libbpf-cargo/CHANGELOG.md index afcf160e..cd782b2c 100644 --- a/libbpf-cargo/CHANGELOG.md +++ b/libbpf-cargo/CHANGELOG.md @@ -1,5 +1,7 @@ Unreleased ---------- +- Adjusted skeleton creation logic to generate shared and exclusive datasec + accessor functions - Removed `Error` enum in favor of `anyhow::Error` - Bumped minimum Rust version to `1.65` diff --git a/libbpf-cargo/src/gen/mod.rs b/libbpf-cargo/src/gen/mod.rs index f6bbd52d..d41fd3eb 100644 --- a/libbpf-cargo/src/gen/mod.rs +++ b/libbpf-cargo/src/gen/mod.rs @@ -530,24 +530,33 @@ fn gen_skel_datasec_getters( None => continue, }; let struct_name = format!("{obj_name}_{name}_types::{name}"); - let mutability = if loaded && map_is_readonly(map) { - "" + let immutable = loaded && map_is_readonly(map); + let mutabilities = if immutable { + [false].as_ref() } else { - "mut" + [false, true].as_ref() }; - write!( - skel, - r#" - pub fn {name}(&{mutability} self) -> &{mutability} {struct_name} {{ - unsafe {{ - std::mem::transmute::<*mut std::ffi::c_void, &{mutability} {struct_name}>( - self.skel_config.map_mmap_ptr({idx}).unwrap() - ) + for mutable in mutabilities { + let (ref_suffix, ptr_suffix, fn_suffix) = if *mutable { + ("mut", "mut", "_mut") + } else { + ("", "const", "") + }; + + write!( + skel, + r#" + pub fn {name}{fn_suffix}(&{ref_suffix} self) -> &{ref_suffix} {struct_name} {{ + unsafe {{ + std::mem::transmute::<*{ptr_suffix} std::ffi::c_void, &{ref_suffix} {struct_name}>( + self.skel_config.map_mmap_ptr{fn_suffix}({idx}).unwrap() + ) + }} }} - }} - "# - )?; + "# + )?; + } } Ok(()) diff --git a/libbpf-cargo/src/test.rs b/libbpf-cargo/src/test.rs index eb54eda0..bdf4dc90 100644 --- a/libbpf-cargo/src/test.rs +++ b/libbpf-cargo/src/test.rs @@ -653,17 +653,17 @@ fn test_skeleton_datasec() { .expect("failed to open skel"); // Check that we set rodata vars before load - open_skel.rodata().myconst = std::ptr::null_mut(); + open_skel.rodata_mut().myconst = std::ptr::null_mut(); // We can always set bss vars - open_skel.bss().myglobal = 42; + open_skel.bss_mut().myglobal = 42; let mut skel = open_skel .load() .expect("failed to load skel"); // We can always set bss vars - skel.bss().myglobal = 24; + skel.bss_mut().myglobal = 24; // Read only for rodata after load let _rodata: &prog_rodata_types::rodata = skel.rodata(); @@ -924,7 +924,7 @@ fn test_skeleton_builder_arrays_ptrs() { fn main() {{ let builder = ProgSkelBuilder::default(); - let mut open_skel = builder + let open_skel = builder .open() .expect("failed to open skel"); diff --git a/libbpf-rs/src/skeleton.rs b/libbpf-rs/src/skeleton.rs index 80d94608..9b89ea1f 100644 --- a/libbpf-rs/src/skeleton.rs +++ b/libbpf-rs/src/skeleton.rs @@ -18,6 +18,7 @@ use libbpf_sys::bpf_object_skeleton; use libbpf_sys::bpf_prog_skeleton; use libbpf_sys::bpf_program; +use crate::error::IntoError as _; use crate::libbpf_sys; use crate::util; use crate::Error; @@ -263,17 +264,28 @@ impl ObjectSkeletonConfig<'_> { /// `ObjectSkeletonConfigBuilder::map`. Index starts at 0. /// /// Warning: the returned pointer is only valid while the `ObjectSkeletonConfig` is alive. - pub fn map_mmap_ptr(&mut self, index: usize) -> Result<*mut c_void> { + pub fn map_mmap_ptr(&self, index: usize) -> Result<*const c_void> { if index >= self.maps.len() { return Err(Error::with_invalid_data(format!( "Invalid map index: {index}" ))); } - self.maps[index].mmaped.as_ref().map_or_else( - || Err(Error::with_invalid_data("Map does not have mmaped ptr")), - |p| Ok(**p), - ) + let p = self.maps[index] + .mmaped + .as_ref() + .ok_or_invalid_data(|| "Map does not have mmaped ptr")?; + Ok(**p) + } + + /// Returns the `mmaped` pointer for a map at the specified `index`. + /// + /// The index is determined by the order in which the map was passed to + /// `ObjectSkeletonConfigBuilder::map`. Index starts at 0. + /// + /// Warning: the returned pointer is only valid while the `ObjectSkeletonConfig` is alive. + pub fn map_mmap_ptr_mut(&mut self, index: usize) -> Result<*mut c_void> { + self.map_mmap_ptr(index).map(|p| p.cast_mut()) } /// Returns the link pointer for a prog at the specified `index`.