Prevent serialization of `bf_program` object by qdeslandes · Pull Request #419 · facebook/bpfilter

qdeslandes · 2026-02-14T09:23:53Z

bf_program is used to generate the BPF bytecode for every chain: when a chain needs to be created, a new bf_program object is created and is used to contain the generation context. Once the chain is loaded, this object becomes irrelevant.

This change moves the references to the BPF objects into a new bf_handle object: when the program is loaded, all the maps and the program FD are stored in bf_program.handle, bf_cgen can then steal the pointer to bf_handle and discard the bf_program object.

bf_program creates the bf_link object in bf_program_new(), for every new program. If the program is not attached, then program->link->hookopts is NULL, and that's how we know. This approach is not ideal as we might create a bf_link object we don't need. Additionally, it leaves bf_link in a temporary state as long as we don't attach the program to the kernel: the bf_link is in the bf_program object and serialized, but not materialized. This commit only create bf_link when the BPF link object is needed. Calling bf_link_new() will create the BPF link, and fail on error. Doing so, we know that any bf_link object refers to a program attached to the kernel.

bf_program creates the necessary maps objects by default (in bf_program_new()), then update them with the proper size and number of elements. This commit improves this behaviour by following bf_link's example: bf_map are not created until they are required by the program. Calling bf_map_new() will create the BPF map object, which makes the behaviour consistent with bf_link.

Create bf_handle to store references to all the BPF objects defined for a chain, instead of centralizing them in bf_program. The benefit is that bf_cgen can take the ownership of bf_handle and discard the bf_program object that becomes irrelevant once the bytecode is loaded.

Replace bf_program in bf_cgen with bf_handle. The bytecode generation context (bf_program) don't need to be stored anymore, as we can keep a reference to the BPF objects.

Copilot

Pull request overview

This pull request refactors BPF program management by introducing a bf_handle object that separates BPF object references (program FD, maps, link) from the bytecode generation context (bf_program). The goal is to prevent serialization of the transient bf_program object by allowing it to be discarded after the program is loaded, while keeping only the handle with the BPF object references.

Changes:

Introduced bf_handle to manage BPF object references separately from bytecode generation context
Moved BPF map creation from a two-phase approach (allocate then create) to single-phase (create immediately)
Integrated link attachment into bf_link_new, removing separate attach/detach functions
Updated bf_cgen to use bf_handle instead of bf_program for managing loaded programs

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/bpfilter/cgen/handle.h	New header defining bf_handle structure and API
src/bpfilter/cgen/handle.c	New implementation of bf_handle management
src/bpfilter/cgen/program.h	Removed BPF object fields, added handle pointer, simplified API
src/bpfilter/cgen/program.c	Refactored to create maps/handle during load, transfer handle ownership
src/bpfilter/cgen/prog/map.h	Removed map mutation APIs, simplified to create-once approach
src/bpfilter/cgen/prog/map.c	Maps now created immediately with BTF data
src/bpfilter/cgen/prog/link.h	Integrated attachment into link creation
src/bpfilter/cgen/prog/link.c	Moved attachment logic into bf_link_new
src/bpfilter/cgen/cgen.h	Changed from bf_program pointer to bf_handle pointer
src/bpfilter/cgen/cgen.c	Updated to use bf_handle for program management
src/bpfilter/xlate/cli.c	Updated to access BPF objects through handle
src/libbpfilter/include/bpfilter/bpf.h	Added btf_data_len parameter to bf_bpf_btf_load
src/libbpfilter/bpf.c	Updated bf_bpf_btf_load to use btf_data_len
src/bpfilter/CMakeLists.txt	Added handle.c/handle.h to build
tests/unit/libbpfilter/bpf.c	Updated test calls to bf_bpf_btf_load
tests/e2e/daemon/restore_with_sets.sh	New E2E test for restoring chains with sets
tests/e2e/CMakeLists.txt	Added restore_with_sets.sh test
Multiple files	Code formatting improvements

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-14T09:33:36Z

src/bpfilter/cgen/handle.c

+    if (handle->lmap) {
+        DUMP(prefix, "lmap: struct bf_map *");
+        bf_dump_prefix_push(prefix);
+        bf_map_dump(handle->lmap, bf_dump_prefix_last(prefix));
+        bf_dump_prefix_pop(prefix);
+    } else {
+        DUMP(prefix, "lmap: struct bf_map * (NULL)");
+    }
+


The lmap field is being dumped twice in bf_handle_dump. The second dump block (lines 229-236) is a duplicate of lines 220-227 and should be removed.

Suggested change

if (handle->lmap) {

DUMP(prefix, "lmap: struct bf_map *");

bf_dump_prefix_push(prefix);

bf_map_dump(handle->lmap, bf_dump_prefix_last(prefix));

bf_dump_prefix_pop(prefix);

} else {

DUMP(prefix, "lmap: struct bf_map * (NULL)");

}

Copilot · 2026-02-14T09:33:36Z

src/bpfilter/cgen/prog/map.c

+    if (type != BF_MAP_TYPE_LOG && key_size == 0)
+        return bf_err_r(-EINVAL, "bf_map %s: key size can't be 0", name);
+    if (type == BF_MAP_TYPE_LOG && key_size != 0)
+        return bf_err_r(-EINVAL, "bf_map %s: key size must be", name);


The error message for log maps is incomplete. It says "key size must be" but doesn't say what the key size must be. The message should be "key size must be 0" to match the validation logic.

Suggested change

return bf_err_r(-EINVAL, "bf_map %s: key size must be", name);

return bf_err_r(-EINVAL, "bf_map %s: key size must be 0", name);

Copilot · 2026-02-14T09:33:37Z

src/bpfilter/cgen/program.c

@@ -1028,28 +857,19 @@ static int _bf_program_load_sets_maps(struct bf_program *new_prog)
        }

        r = bf_bpf_map_update_batch(map->fd, keys, values, nelems, BPF_ANY);
-        if (r < 0) {
-            bf_err_r(r, "failed to add set elements to the map");
-            goto err_destroy_maps;
-        }
+        if (r)
+            return bf_err_r(r, "failed to add set elements to the map");

-        set_node = bf_list_node_next(set_node);
-        map_node = bf_list_node_next(map_node);
-    }
+        r = bf_list_push(&new_prog->handle->sets, (void **)&map);
+        if (r)
+            return r;
+    };


This code breaks the index mapping for sets. When an empty set is encountered, the code skips adding it to handle->sets but continues without maintaining the index correspondence. The BPF fixups reference sets by their index in chain->sets, which includes empty sets, but handle->sets will only contain non-empty sets after this change.

To fix this, either:

Add NULL entries for empty sets (like the old code did), or

Track a mapping between chain set indices and handle set indices

The old code maintained a 1:1 mapping by adding NULL entries, which was the correct approach.

+1 to this copilot comment

Copilot · 2026-02-14T09:33:37Z

src/bpfilter/cgen/handle.c

+        _free_bf_map_ struct bf_map *map = NULL;
+
+        r = bf_list_emplace(&_handle->sets, bf_map_new_from_pack, map, dir_fd,
+                            array_node);
+        if (r)
+            return bf_err_r(r, "failed to unpack bf_map into bf_handle.sets");


The deserialization code doesn't handle NULL entries in the sets array. The old code in bf_program_new_from_pack would check if the array node is nil and add a NULL entry to maintain the index correspondence. This new code always tries to deserialize a map, which will fail if a nil was serialized for an empty set. Add a check: if (!bf_rpack_is_nil(array_node)) { ... } else { bf_list_add_tail(&_handle->sets, NULL); }

Suggested change

_free_bf_map_ struct bf_map *map = NULL;

r = bf_list_emplace(&_handle->sets, bf_map_new_from_pack, map, dir_fd,

array_node);

if (r)

return bf_err_r(r, "failed to unpack bf_map into bf_handle.sets");

if (!bf_rpack_is_nil(array_node)) {

_free_bf_map_ struct bf_map *map = NULL;

r = bf_list_emplace(&_handle->sets, bf_map_new_from_pack, map,

dir_fd, array_node);

if (r)

return bf_err_r(r,

"failed to unpack bf_map into bf_handle.sets");

} else {

bf_list_add_tail(&_handle->sets, NULL);

}

Copilot · 2026-02-14T09:33:37Z

src/bpfilter/cgen/handle.c

+    bf_list_foreach (&handle->sets, set_node) {
+        r = bf_map_pin(bf_list_node_get_data(set_node), dir_fd);
+        if (r) {
+            bf_err_r(r, "failed to pin BPF set map");
+            goto err_unpin_all;
+        }
+    }


This code will crash if there are NULL entries in the sets list. After fixing the set index mapping bug (where NULL entries should be added for empty sets), this code needs to check if the map is NULL before attempting to pin it. Add: struct bf_map *map = bf_list_node_get_data(set_node); if (!map) continue;

Copilot · 2026-02-14T09:33:37Z

src/bpfilter/cgen/handle.c

+    bf_list_foreach (&handle->sets, set_node)
+        bf_map_unpin(bf_list_node_get_data(set_node), dir_fd);
+


This code will crash if there are NULL entries in the sets list. After fixing the set index mapping bug (where NULL entries should be added for empty sets), this code needs to check if the map is NULL before attempting to unpin it. Add: struct bf_map *map = bf_list_node_get_data(set_node); if (map) bf_map_unpin(map, dir_fd);

Suggested change

bf_list_foreach (&handle->sets, set_node)

bf_map_unpin(bf_list_node_get_data(set_node), dir_fd);

bf_list_foreach (&handle->sets, set_node) {

struct bf_map *map = bf_list_node_get_data(set_node);

if (map)

bf_map_unpin(map, dir_fd);

}

Copilot · 2026-02-14T09:33:38Z

src/bpfilter/cgen/handle.c

+    bf_list_foreach (&handle->sets, map_node) {
+        struct bf_map *map = bf_list_node_get_data(map_node);
+
+        if (bf_list_is_tail(&handle->sets, map_node))
+            bf_dump_prefix_last(prefix);
+
+        bf_map_dump(map, prefix);
+    }


The dump code will crash if there are NULL entries in the sets list. After fixing the set index mapping bug (where NULL entries should be added for empty sets), this code needs to check if the map is NULL before attempting to dump it. Add a check: if (!map) { DUMP(prefix, "<empty set - no map>"); continue; }

yaakov-stein

Only reviewed 51bdadb so far, can review the rest soon if that'd be helpful. Re 51bdadb: Mostly a few nits - I had honestly been a bit confused until now about the need for separate steps (new + attach), so this seems much cleaner.

yaakov-stein · 2026-02-15T04:05:43Z

src/bpfilter/cgen/prog/link.c

-    _link = malloc(sizeof(*_link));
+    if (!*hookopts) {
+        return bf_err_r(-EINVAL,
+                        "hookopts are required when created a new bf_link");


nit: "when creating"

yaakov-stein · 2026-02-15T04:21:56Z

src/bpfilter/cgen/prog/link.h

+    /** Hook to which the link is attached. */
+    enum bf_hook hook;

    /** Hook options used for the link. Only valid if the link is materialized */


nit: I believe this comment should be updated now that hookopts should always be valid

yaakov-stein · 2026-02-15T04:22:00Z

src/bpfilter/cgen/prog/link.h

 * @param link `bf_link` object to allocate and initialize. On failure,
 *        this parameter is unchanged. Can't be NULL.
- * @param name Name of the link. Can't be empty or NULL.
+ * @param hook Hook to attach the link to.


Not sure how to let github let me comment on unchanged lines above, but I believe line 40/41:

@note This function won't create a new BPF link, but a bpfilter-specific object used to keep track of a BPF link on the system.

is no longer accurate (this does now create the new BPF link as well as the bpfilter-specific link).

yaakov-stein · 2026-02-15T04:22:07Z

src/bpfilter/cgen/prog/link.c

-        r = bf_bpf_obj_get("bf_link", dir_fd, &_link->fd);
-        if (r)
-            return bf_err_r(r, "failed to open pinned BPF link 'bf_link'");
+    r = bf_bpf_obj_get("bf_link", dir_fd, &_link->fd);


nit: this removes the dir_fd != -1 check (which make sense given the changes) but doesn't update the method's docs (link.h 59/60)

yaakov-stein · 2026-02-15T04:27:41Z

src/bpfilter/cgen/prog/link.c

 #include <bpfilter/pack.h>

-int bf_link_new(struct bf_link **link, const char *name)
+static int _bf_link_try_attach_xdp(int prog_fd, int ifindex, enum bf_hook hook);


Would it be simpler and less duplicate code if we just move this entire method here?

yaakov-stein

Review for fc16f93 -> 6b9fd02. To ensure we don't need commits like fc16f93 going forward, I added #421, which should enforce formatting. (Also, should we split out the fc16f93 commit, given that it's not too relevant to the rest of the PR?)

Overall this seems much cleaner and easier to follow than before, similar to the bf_link changes.

yaakov-stein · 2026-02-15T18:55:45Z

tests/e2e/daemon/restore_with_sets.sh

+
+start_bpfilter
+    # Verify chain with sets is restored properly
+    ${FROM_NS} bfcli chain get --name test


Should we verify the set is still there and with the same elements? It seems that all we're verifying is that the chain still exists. (ie we can add a ping and ensure it's dropped or something like that.)

yaakov-stein · 2026-02-15T19:13:47Z

tests/e2e/daemon/restore_with_sets.sh

+#!/usr/bin/env bash
+
+set -eux
+set -o pipefail


I believe these are no longer needed as of #417

yaakov-stein · 2026-02-15T19:26:05Z

src/bpfilter/cgen/prog/map.c


+#define _free_bf_btf_ __attribute__((__cleanup__(_bf_btf_free)))
+
+static void _bf_btf_free(struct bf_btf **btf);


Similar to the forward declaration comment in bf_link - is the forward declaration beneficial as opposed to just moving the method here?

yaakov-stein · 2026-02-15T19:28:32Z

src/bpfilter/cgen/prog/map.c

-        }
-    } else {
-        _map->fd = -1;
+    r = bf_bpf_obj_get(_map->name, dir_fd, &_map->fd);


Same comment regarding stale docs as dir_fd == -1 in bf_link (bf_map_new_from_pack docs in bf_map.h L84/85 still say dir_fd can equal -1).

yaakov-stein · 2026-02-15T19:29:09Z

src/bpfilter/cgen/prog/map.c

-    } else {
-        _map->fd = -1;
+    r = bf_bpf_obj_get(_map->name, dir_fd, &_map->fd);
+    if (r < 0) {


nit: unnecessary brackets

yaakov-stein · 2026-02-15T20:16:30Z

src/bpfilter/cgen/program.c

    r = _bf_program_load_counters_map(prog);
    if (r)
-        return r;
+        return bf_err_r(r, "failed to load the counter map");


nit: Now that we add these bf_err_r, we're logging the error both in bf_program_load as well as in the _bf_program_load_*_map methods. The error logging granularity could be helpful, but I could also see that being unnecessary. Just wanted to flag, don't have an opinion either way.

yaakov-stein · 2026-02-15T20:18:06Z

src/bpfilter/cgen/program.c


 void bf_program_unload(struct bf_program *prog)
 {
+    _clean_bf_list_ bf_list list = bf_list_default_from(prog->sets);


prog->sets accesses prog before assert(prog) (L1091)

yaakov-stein · 2026-02-15T20:50:38Z

src/bpfilter/cgen/program.c

@@ -1028,28 +857,19 @@ static int _bf_program_load_sets_maps(struct bf_program *new_prog)
        }

        r = bf_bpf_map_update_batch(map->fd, keys, values, nelems, BPF_ANY);
-        if (r < 0) {
-            bf_err_r(r, "failed to add set elements to the map");
-            goto err_destroy_maps;
-        }
+        if (r)
+            return bf_err_r(r, "failed to add set elements to the map");

-        set_node = bf_list_node_next(set_node);
-        map_node = bf_list_node_next(map_node);
-    }
+        r = bf_list_push(&new_prog->handle->sets, (void **)&map);
+        if (r)
+            return r;
+    };


+1 to this copilot comment

yaakov-stein · 2026-02-15T21:02:14Z

src/bpfilter/cgen/program.c

-    bf_wpack_open_object(pack, "cmap");
-    bf_map_pack(program->cmap, pack);
-    bf_wpack_close_object(pack);
+    if (program->cmap) {


For L256, L264, L311, L320, L1140, L1148, L1196, L1198 - while an optimization was added for the log map and it is possible it may not be created, it appears that the counter map and printer map are always created. Do we need these null checks?

yaakov-stein · 2026-02-15T21:06:28Z

src/bpfilter/cgen/program.c

+    bf_map_free(&prog->cmap);
+    bf_map_free(&prog->pmap);
+    bf_map_free(&prog->lmap);
+    bf_swap(list, prog->sets);


This is a pretty cool trick!

yaakov-stein

Review for the rest of the commits. It seems like copilot caught the important thing about the sets - the rest is mostly nits.

yaakov-stein · 2026-02-16T21:16:31Z

src/bpfilter/cgen/program.c

    if (!_program)
        return -ENOMEM;

-    (void)snprintf(_program->prog_name, BPF_OBJ_NAME_LEN, "%s", _BF_PROG_NAME);


We keep prog_name in the bf_program struct but remove the initialization. program->prog_name is still used (for example in bf_program_dump on L217 and in bf_program_load on 910) and is UB.

yaakov-stein · 2026-02-16T21:19:03Z

src/bpfilter/cgen/handle.c

+{
+    _clean_bf_list_ bf_list list = bf_list_default_from(handle->sets);
+
+    assert(handle);


asset(handle) after using handle on L374

yaakov-stein · 2026-02-16T21:19:47Z

src/bpfilter/cgen/handle.c

+}
+
+void bf_handle_free(struct bf_handle **handle)
+{


Add an assert(handle) here?

yaakov-stein · 2026-02-16T21:21:19Z

src/bpfilter/cgen/program.c

-     * used, then the file descriptors are already closed (as
-     * bf_program_unload() has been called). Otherwise, bf_program_unload()
-     * won't be called, but the programs are pinned, so they can be closed
-     * safely. */


Should we keep this comment but move it to bf_handle_free? (IIUC the behavior is still the same)

yaakov-stein · 2026-02-16T21:22:07Z

src/bpfilter/cgen/handle.h

+void bf_handle_free(struct bf_handle **handle);
+
+/**
+ * Serialize a bf_handle.


yaakov-stein · 2026-02-16T21:35:28Z

src/bpfilter/cgen/handle.c

+        bf_wpack_kv_nil(pack, "link");
+    }
+
+    if (handle->cmap) {


Similar to a comment from the previous review - handle->cmap and handle->pmap are guaranteed to be non-null IIUC. Should we remove the checks on L152, L160, L202, L211, L265, L273, L316, L318?

yaakov-stein · 2026-02-16T21:55:48Z

src/bpfilter/cgen/cgen.c

    bf_cgen_dump(cgen, EMPTY_PREFIX);

-    cgen->program = TAKE_PTR(prog);
+    cgen->handle = TAKE_PTR(handle);


Because we're only taking the pointer here, the bf_cgen_dump on L302 will have stale/null data.

qdeslandes added 7 commits February 14, 2026 08:48

cli,lib: fix style

fc16f93

tests: e2e: add test to validate sets are restored on daemon restart

0671059

daemon: cgen: remove bf_program in favor of bf_handle

0a6b768

Replace bf_program in bf_cgen with bf_handle. The bytecode generation context (bf_program) don't need to be stored anymore, as we can keep a reference to the BPF objects.

lib: bpf: require BTF data len on bf_bpf_btf_load()

86884e5

Copilot AI review requested due to automatic review settings February 14, 2026 09:23

meta-cla bot added the cla signed label Feb 14, 2026

Copilot started reviewing on behalf of qdeslandes February 14, 2026 09:24 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

qdeslandes mentioned this pull request Feb 14, 2026

Prevent bf_map and bf_link serialization #420

Draft

yaakov-stein reviewed Feb 15, 2026

View reviewed changes

qdeslandes marked this pull request as draft February 16, 2026 17:29

yaakov-stein reviewed Feb 16, 2026

View reviewed changes

	return bf_err_r(-EINVAL, "bf_map %s: key size must be", name);
	return bf_err_r(-EINVAL, "bf_map %s: key size must be 0", name);

		bf_list_foreach (&handle->sets, set_node)
		bf_map_unpin(bf_list_node_get_data(set_node), dir_fd);


		#define _free_bf_btf_ __attribute__((__cleanup__(_bf_btf_free)))

		static void _bf_btf_free(struct bf_btf **btf);

Conversation

qdeslandes commented Feb 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

yaakov-stein Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

yaakov-stein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakov-stein Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakov-stein left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakov-stein Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakov-stein Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakov-stein Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakov-stein left a comment

Choose a reason for hiding this comment

yaakov-stein Feb 15, 2026 •

edited

Loading

yaakov-stein Feb 15, 2026 •

edited

Loading

yaakov-stein left a comment •

edited

Loading

yaakov-stein Feb 15, 2026 •

edited

Loading

yaakov-stein Feb 15, 2026 •

edited

Loading

yaakov-stein Feb 15, 2026 •

edited

Loading