From 72bc770f51677e7a106e40dc0a61687fd40d0a91 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Mon, 9 Mar 2026 14:47:53 +0000 Subject: [PATCH 1/6] ROX-33471: do not attach progs when no paths configured --- fact-ebpf/src/bpf/file.h | 5 --- fact-ebpf/src/bpf/maps.h | 21 ----------- fact/src/bpf/mod.rs | 57 +++++++++++++++++++++++------- tests/test_config_hotreload.py | 64 ++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 39 deletions(-) diff --git a/fact-ebpf/src/bpf/file.h b/fact-ebpf/src/bpf/file.h index 4e3d6369..bb131f80 100644 --- a/fact-ebpf/src/bpf/file.h +++ b/fact-ebpf/src/bpf/file.h @@ -13,11 +13,6 @@ // clang-format on __always_inline static bool path_is_monitored(struct bound_path_t* path) { - if (!filter_by_prefix()) { - // no path configured, allow all - return true; - } - // Backup bytes length and restore it before exiting unsigned int len = path->len; diff --git a/fact-ebpf/src/bpf/maps.h b/fact-ebpf/src/bpf/maps.h index 048e3934..2a2a97b6 100644 --- a/fact-ebpf/src/bpf/maps.h +++ b/fact-ebpf/src/bpf/maps.h @@ -27,27 +27,6 @@ __always_inline static struct helper_t* get_helper() { return bpf_map_lookup_elem(&helper_map, &zero); } -/** - * A map with a single entry, determining whether prefix filtering - * should be done based on the `path_prefix` map. - */ -struct { - __uint(type, BPF_MAP_TYPE_ARRAY); - __type(key, __u32); - __type(value, char); - __uint(max_entries, 1); -} filter_by_prefix_map SEC(".maps"); - -/// Whether we should filter by path prefix or not. -__always_inline static bool filter_by_prefix() { - unsigned int zero = 0; - char* res = bpf_map_lookup_elem(&filter_by_prefix_map, &zero); - - // The NULL check is simply here to satisfy some verifiers, the result - // will never actually be NULL. - return res == NULL || *res != 0; -} - struct { __uint(type, BPF_MAP_TYPE_LPM_TRIE); __type(key, struct path_prefix_t); diff --git a/fact/src/bpf/mod.rs b/fact/src/bpf/mod.rs index 5604b821..c8f47cb3 100644 --- a/fact/src/bpf/mod.rs +++ b/fact/src/bpf/mod.rs @@ -1,9 +1,9 @@ -use std::{io, path::PathBuf}; +use std::{collections, io, path::PathBuf}; use anyhow::{bail, Context}; use aya::{ - maps::{Array, HashMap, LpmTrie, MapData, PerCpuArray, RingBuf}, - programs::Program, + maps::{HashMap, LpmTrie, MapData, PerCpuArray, RingBuf}, + programs::{lsm::LsmLinkId, Program}, Btf, Ebpf, }; use checks::Checks; @@ -33,6 +33,10 @@ pub struct Bpf { paths_config: watch::Receiver>, paths_globset: GlobSet, + + link_ids: collections::HashMap, + + progs_loaded: bool, } impl Bpf { @@ -65,10 +69,13 @@ impl Bpf { paths, paths_config, paths_globset: GlobSet::empty(), + link_ids: collections::HashMap::new(), + progs_loaded: false, }; bpf.load_paths()?; bpf.load_progs(&btf)?; + bpf.progs_loaded = true; Ok(bpf) } @@ -116,12 +123,18 @@ impl Bpf { } fn load_paths(&mut self) -> anyhow::Result<()> { - let paths_config = self.paths_config.borrow(); - let Some(filter_by_prefix) = self.obj.map_mut("filter_by_prefix_map") else { - bail!("filter_by_prefix_map map not found"); - }; - let mut filter_by_prefix: Array<&mut MapData, c_char> = Array::try_from(filter_by_prefix)?; - filter_by_prefix.set(0, !paths_config.is_empty() as c_char, 0)?; + if self.paths_config.borrow().is_empty() { + if self.progs_loaded { + self.detach_progs()?; + } + self.paths.clear(); + self.paths_globset = GlobSet::empty(); + return Ok(()); + } + + if self.progs_loaded && self.link_ids.is_empty() { + self.attach_progs()?; + } let Some(path_prefix) = self.obj.map_mut("path_prefix") else { bail!("path_prefix map not found"); @@ -130,6 +143,7 @@ impl Bpf { LpmTrie::try_from(path_prefix)?; // Add the new prefixes + let paths_config = self.paths_config.borrow(); let mut new_paths = Vec::with_capacity(paths_config.len()); let mut builder = GlobSetBuilder::new(); for p in paths_config.iter() { @@ -176,11 +190,25 @@ impl Bpf { } fn attach_progs(&mut self) -> anyhow::Result<()> { - for (_, prog) in self.obj.programs_mut() { - match prog { + for (name, prog) in self.obj.programs_mut() { + let name = name.to_string(); + let link_id = match prog { Program::Lsm(prog) => prog.attach()?, u => unimplemented!("{u:?}"), }; + self.link_ids.insert(name, link_id); + } + Ok(()) + } + + fn detach_progs(&mut self) -> anyhow::Result<()> { + for (name, prog) in self.obj.programs_mut() { + if let Some(link_id) = self.link_ids.remove(name) { + match prog { + Program::Lsm(prog) => prog.detach(link_id)?, + u => unimplemented!("{u:?}"), + }; + } } Ok(()) } @@ -194,8 +222,10 @@ impl Bpf { info!("Starting BPF worker..."); tokio::spawn(async move { - self.attach_progs() - .context("Failed to attach ebpf programs")?; + if !self.paths.is_empty() { + self.attach_progs() + .context("Failed to attach ebpf programs")?; + } let rb = self.take_ringbuffer()?; let mut fd = AsyncFd::new(rb)?; @@ -379,4 +409,5 @@ mod bpf_tests { run_tx.send(false).unwrap(); } + } diff --git a/tests/test_config_hotreload.py b/tests/test_config_hotreload.py index e23fdde1..c8bbc3da 100644 --- a/tests/test_config_hotreload.py +++ b/tests/test_config_hotreload.py @@ -154,6 +154,70 @@ def test_paths(fact, fact_config, monitored_dir, ignored_dir, server): server.wait_events([e]) +def test_no_paths_then_add(fact, fact_config, monitored_dir, server): + """ + Start with no paths configured, verify no events are produced, + then add paths via hot-reload and verify events appear. + """ + p = Process.from_proc() + + # Remove all paths + config, config_file = fact_config + config['paths'] = [] + reload_config(fact, config, config_file, delay=0.5) + + # Write to a file — should NOT produce events + fut = os.path.join(monitored_dir, 'test2.txt') + with open(fut, 'w') as f: + f.write('This should be ignored') + sleep(1) + + assert server.is_empty(), 'Should not receive events with no paths configured' + + # Add paths back + config['paths'] = [f'{monitored_dir}/**/*'] + reload_config(fact, config, config_file, delay=0.5) + + # Write to a file — should produce events + with open(fut, 'w') as f: + f.write('This should be detected') + + e = Event(process=p, event_type=EventType.OPEN, + file=fut, host_path=fut) + + server.wait_events([e]) + + +def test_paths_then_remove(fact, fact_config, monitored_dir, server): + """ + Start with paths configured, verify events are produced, + then remove all paths via hot-reload and verify events stop. + """ + p = Process.from_proc() + + # Write to a file — should produce events + fut = os.path.join(monitored_dir, 'test2.txt') + with open(fut, 'w') as f: + f.write('This is a test') + + e = Event(process=p, event_type=EventType.CREATION, + file=fut, host_path='') + + server.wait_events([e]) + + # Remove all paths + config, config_file = fact_config + config['paths'] = [] + reload_config(fact, config, config_file, delay=0.5) + + # Write to a file — should NOT produce events + with open(fut, 'w') as f: + f.write('This should be ignored') + sleep(1) + + assert server.is_empty(), 'Should not receive events after removing paths' + + def test_paths_addition(fact, fact_config, monitored_dir, ignored_dir, server): p = Process.from_proc() From fb954e6da294dede87c9f47025742be1d6dc9c16 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Mon, 9 Mar 2026 15:00:07 +0000 Subject: [PATCH 2/6] Fmt; really must fix my nvim config --- fact/src/bpf/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/fact/src/bpf/mod.rs b/fact/src/bpf/mod.rs index c8f47cb3..daa551b7 100644 --- a/fact/src/bpf/mod.rs +++ b/fact/src/bpf/mod.rs @@ -409,5 +409,4 @@ mod bpf_tests { run_tx.send(false).unwrap(); } - } From 2f99ec2b60a26bc561a3d3095b80f6e7ef556d24 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Tue, 10 Mar 2026 13:26:25 +0000 Subject: [PATCH 3/6] simplifies the link storage and dropping --- fact/src/bpf/mod.rs | 53 +++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 33 deletions(-) diff --git a/fact/src/bpf/mod.rs b/fact/src/bpf/mod.rs index daa551b7..7f654cd9 100644 --- a/fact/src/bpf/mod.rs +++ b/fact/src/bpf/mod.rs @@ -1,9 +1,9 @@ -use std::{collections, io, path::PathBuf}; +use std::{io, path::PathBuf}; use anyhow::{bail, Context}; use aya::{ maps::{HashMap, LpmTrie, MapData, PerCpuArray, RingBuf}, - programs::{lsm::LsmLinkId, Program}, + programs::{lsm::LsmLink, Program}, Btf, Ebpf, }; use checks::Checks; @@ -34,9 +34,7 @@ pub struct Bpf { paths_globset: GlobSet, - link_ids: collections::HashMap, - - progs_loaded: bool, + links: Vec, } impl Bpf { @@ -69,13 +67,11 @@ impl Bpf { paths, paths_config, paths_globset: GlobSet::empty(), - link_ids: collections::HashMap::new(), - progs_loaded: false, + links: Vec::new(), }; - bpf.load_paths()?; bpf.load_progs(&btf)?; - bpf.progs_loaded = true; + bpf.load_paths()?; Ok(bpf) } @@ -124,15 +120,13 @@ impl Bpf { fn load_paths(&mut self) -> anyhow::Result<()> { if self.paths_config.borrow().is_empty() { - if self.progs_loaded { - self.detach_progs()?; - } + self.detach_progs(); self.paths.clear(); self.paths_globset = GlobSet::empty(); return Ok(()); } - if self.progs_loaded && self.link_ids.is_empty() { + if self.links.is_empty() { self.attach_progs()?; } @@ -189,28 +183,26 @@ impl Bpf { Ok(()) } + /// Attaches all BPF programs. If any attach fails, all previously + /// attached programs are automatically detached via drop. fn attach_progs(&mut self) -> anyhow::Result<()> { - for (name, prog) in self.obj.programs_mut() { - let name = name.to_string(); - let link_id = match prog { - Program::Lsm(prog) => prog.attach()?, + let mut links = Vec::new(); + for (_name, prog) in self.obj.programs_mut() { + match prog { + Program::Lsm(prog) => { + let link_id = prog.attach()?; + links.push(prog.take_link(link_id)?); + } u => unimplemented!("{u:?}"), }; - self.link_ids.insert(name, link_id); } + self.links = links; Ok(()) } - fn detach_progs(&mut self) -> anyhow::Result<()> { - for (name, prog) in self.obj.programs_mut() { - if let Some(link_id) = self.link_ids.remove(name) { - match prog { - Program::Lsm(prog) => prog.detach(link_id)?, - u => unimplemented!("{u:?}"), - }; - } - } - Ok(()) + /// Detaches all BPF programs by dropping owned links. + fn detach_progs(&mut self) { + self.links.clear(); } // Gather events from the ring buffer and print them out. @@ -222,11 +214,6 @@ impl Bpf { info!("Starting BPF worker..."); tokio::spawn(async move { - if !self.paths.is_empty() { - self.attach_progs() - .context("Failed to attach ebpf programs")?; - } - let rb = self.take_ringbuffer()?; let mut fd = AsyncFd::new(rb)?; From e3444b2687cd07343909967a90deb0e4899b110a Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Wed, 11 Mar 2026 12:18:33 +0000 Subject: [PATCH 4/6] Small tweaks: - test cleanup - iterators --- Co-authored-by: Mauro Ezequiel Moltrasio --- fact/src/bpf/mod.rs | 14 +++++++------- tests/test_config_hotreload.py | 24 +++++++++++++----------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/fact/src/bpf/mod.rs b/fact/src/bpf/mod.rs index 7f654cd9..44d832ad 100644 --- a/fact/src/bpf/mod.rs +++ b/fact/src/bpf/mod.rs @@ -186,17 +186,17 @@ impl Bpf { /// Attaches all BPF programs. If any attach fails, all previously /// attached programs are automatically detached via drop. fn attach_progs(&mut self) -> anyhow::Result<()> { - let mut links = Vec::new(); - for (_name, prog) in self.obj.programs_mut() { - match prog { + self.links = self + .obj + .programs_mut() + .map(|(_, prog)| match prog { Program::Lsm(prog) => { let link_id = prog.attach()?; - links.push(prog.take_link(link_id)?); + prog.take_link(link_id) } u => unimplemented!("{u:?}"), - }; - } - self.links = links; + }) + .collect::>()?; Ok(()) } diff --git a/tests/test_config_hotreload.py b/tests/test_config_hotreload.py index c8bbc3da..3dfa445e 100644 --- a/tests/test_config_hotreload.py +++ b/tests/test_config_hotreload.py @@ -17,7 +17,7 @@ def assert_endpoint(endpoint, status_code=200): assert resp.status_code == status_code -def reload_config(fact, config, file, delay=0.1): +def reload_config(fact, config, file, delay=0.5): with open(file, 'w') as f: yaml.dump(config, f) fact.kill('SIGHUP') @@ -137,7 +137,7 @@ def test_paths(fact, fact_config, monitored_dir, ignored_dir, server): config, config_file = fact_config config['paths'] = [f'{ignored_dir}/**/*'] - reload_config(fact, config, config_file, delay=0.5) + reload_config(fact, config, config_file) # At this point, the event in the ignored directory should show up # and the event on the monitored directory should be ignored @@ -164,7 +164,7 @@ def test_no_paths_then_add(fact, fact_config, monitored_dir, server): # Remove all paths config, config_file = fact_config config['paths'] = [] - reload_config(fact, config, config_file, delay=0.5) + reload_config(fact, config, config_file) # Write to a file — should NOT produce events fut = os.path.join(monitored_dir, 'test2.txt') @@ -172,19 +172,20 @@ def test_no_paths_then_add(fact, fact_config, monitored_dir, server): f.write('This should be ignored') sleep(1) - assert server.is_empty(), 'Should not receive events with no paths configured' + e = Event(process=p, event_type=EventType.OPEN, + file=fut, host_path=fut) + + with pytest.raises(TimeoutError): + server.wait_events([e]) # Add paths back config['paths'] = [f'{monitored_dir}/**/*'] - reload_config(fact, config, config_file, delay=0.5) + reload_config(fact, config, config_file) # Write to a file — should produce events with open(fut, 'w') as f: f.write('This should be detected') - e = Event(process=p, event_type=EventType.OPEN, - file=fut, host_path=fut) - server.wait_events([e]) @@ -208,14 +209,15 @@ def test_paths_then_remove(fact, fact_config, monitored_dir, server): # Remove all paths config, config_file = fact_config config['paths'] = [] - reload_config(fact, config, config_file, delay=0.5) + reload_config(fact, config, config_file) # Write to a file — should NOT produce events with open(fut, 'w') as f: f.write('This should be ignored') sleep(1) - assert server.is_empty(), 'Should not receive events after removing paths' + with pytest.raises(TimeoutError): + server.wait_events([e]) def test_paths_addition(fact, fact_config, monitored_dir, ignored_dir, server): @@ -238,7 +240,7 @@ def test_paths_addition(fact, fact_config, monitored_dir, ignored_dir, server): config, config_file = fact_config config['paths'] = [f'{monitored_dir}/**/*', f'{ignored_dir}/**/*'] - reload_config(fact, config, config_file, delay=0.5) + reload_config(fact, config, config_file) # At this point, the event in the ignored directory should show up # alongside the regular event From 2db2d44d10c5d98e2c34d6f9c57f5c70bbb68eee Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Wed, 11 Mar 2026 16:02:56 +0000 Subject: [PATCH 5/6] Fix stale thread issue with server --- tests/server.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tests/server.py b/tests/server.py index 004fa8ef..69cb1c33 100644 --- a/tests/server.py +++ b/tests/server.py @@ -1,7 +1,7 @@ from concurrent import futures from collections import deque import json -from threading import Event +from threading import Event as ThreadingEvent from time import sleep import grpc @@ -24,7 +24,7 @@ def __init__(self): sfa_iservice_pb2_grpc.FileActivityService.__init__(self) self.server = grpc.server(futures.ThreadPoolExecutor(max_workers=2)) self.queue = deque() - self.running = Event() + self.running = ThreadingEvent() self.executor = futures.ThreadPoolExecutor(max_workers=2) def Communicate(self, request_iterator, context): @@ -81,8 +81,8 @@ def is_running(self): """ return self.running.is_set() - def _wait_events(self, events: list[Event], strict: bool): - while self.is_running(): + def _wait_events(self, events: list['Event'], strict: bool, cancel: ThreadingEvent): + while self.is_running() and not cancel.is_set(): msg = self.get_next() if msg is None: sleep(0.5) @@ -99,19 +99,23 @@ def _wait_events(self, events: list[Event], strict: bool): elif strict: raise ValueError(json.dumps(diff, indent=4)) - def wait_events(self, events: list[Event], strict: bool = True): + def wait_events(self, events: list['Event'], strict: bool = True): """ Continuously checks the server for incoming events until the specified events are found. Args: - server: The server instance to retrieve events from. - event (list[Event]): The events to search for. + events (list['Event']): The events to search for. strict (bool): Fail if an unexpected event is detected. Raises: TimeoutError: If the required events are not found in 5 seconds. """ print('Waiting for events:', *events, sep='\n') - fs = self.executor.submit(self._wait_events, events, strict) - fs.result(timeout=5) + cancel = ThreadingEvent() + fs = self.executor.submit(self._wait_events, events, strict, cancel) + try: + fs.result(timeout=5) + except TimeoutError: + cancel.set() + raise From ffdf46c90a53abf9e01782e399f109d141536617 Mon Sep 17 00:00:00 2001 From: Giles Hutton Date: Wed, 11 Mar 2026 16:07:11 +0000 Subject: [PATCH 6/6] Always clean up the thread --- tests/server.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/server.py b/tests/server.py index 69cb1c33..c17447d0 100644 --- a/tests/server.py +++ b/tests/server.py @@ -117,5 +117,6 @@ def wait_events(self, events: list['Event'], strict: bool = True): try: fs.result(timeout=5) except TimeoutError: - cancel.set() raise + finally: + cancel.set()