From 299bfae61435cbeda55b65311562e95fc352dbe4 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 1acf3f9ac32aecb4882ee469eab001a558bbd025 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 7f3dbacf0698155fe43b6849c10ed9ef77bb4371 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 4cf65e999a5cd27cd1a2a36b96d2c67773935971 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 1b0ff3a2db605d1a800f24d1a7b23b8dbe15b2ee 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 9098e111..fdac7f0f 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 - 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`.