lib: add bf_vector and use it in cgen by pzmarzly · Pull Request #487 · facebook/bpfilter

pzmarzly · 2026-03-24T13:30:28Z

I initially planned to use bf_vector in bf_hashset, but I got convinced by @yaakov-stein that it's not a good idea. However, we could use bf_vector in other places, especially where we use bf_dynbuf or where we handroll vector-like behavior.

Commits:

lib: core: add bf_vector - implementation. Vector doubles in size when it hits the limits. Has a helper to remove elements as well, though shrinking is not implemented.
cgen: use bf_vector for img - Instead of managing img, img_size, and img_cap by hand, use bf_vector.

github-actions · 2026-03-24T13:30:40Z

Claude review of PR #487 (`dd1fe11`)

Well-structured PR that introduces bf_vector as a generic dynamically-sized array and uses it to replace the hand-rolled img/img_size/img_cap management in program.c. The migration is clean, error handling is consistent, and the new vector implementation has thorough unit tests (12 test cases covering add, remove, set, reserve, foreach, take, etc.). Bounds checking via bf_vector_get and bf_vector_set is a safety improvement over direct array indexing.

Must fix

Duplicate include in program.h — src/libbpfilter/cgen/program.h:16 — #include <bpfilter/core/list.h> appears twice (lines 11 and 16). The first was correctly added by commit 1 (move bf_list into subfolder), the second was erroneously re-added in commit 3 when inserting vector.h. Remove the duplicate.
Invalid commit component prefix — commit 309f17f — The third commit message daemon: cgen: use bf_vector for img uses daemon: as the component, but per doc/developers/style.rst, valid components are lib, cli, tests, build, tools, doc. Since cgen is part of libbpfilter, use lib: cgen: use bf_vector for img.

Suggestions

bf_vector_reserve guard checks vec->cap instead of cap — src/libbpfilter/core/vector.c:184 — The guard if (vec->cap > _BF_VECTOR_MAX_CAP) checks the current capacity, not the requested cap. Since we already passed the cap > vec->cap early return, a very large cap (e.g. from network-influenced bf_vector_reserve in _bf_recv_in_buff) bypasses this guard and reaches _bf_vector_realloc uncapped. For elem_size == 1 the overflow check doesn't help — realloc is called with a huge size that fails gracefully, but the intent of _BF_VECTOR_MAX_CAP is to prevent this. Fix: if (cap > _BF_VECTOR_MAX_CAP). (Previously flagged in an earlier review round, not yet addressed.)
bf_vector_add_many memcpy aliasing hazard — src/libbpfilter/core/vector.c:171 — If a caller passes a pointer from bf_vector_get() on the same vector as data, and the append triggers a realloc, the source pointer dangles. Even without realloc, overlapping memcpy is UB. Consider memmove or documenting the restriction.
Fixup null check before type filter — src/libbpfilter/cgen/program.c:215 — The bf_vector_get + null check fired before if (type != fixup->type) continue;. Fixed: the type filter is now correctly placed before the lookup.
bf_vector_foreach void arithmetic in public header* — src/libbpfilter/include/bpfilter/core/vector.h:59 — The macro performs pointer arithmetic on void *, which is a GCC extension not valid in standard C. Since this is a public header, downstream consumers compiling with -pedantic -std=c17 will get errors when using the macro. Cast through char * instead.
Initial capacity reduced from 64 to 8 — src/libbpfilter/cgen/program.c:109 — The old _BF_PROGRAM_DEFAULT_IMG_SIZE was 64; the new default _BF_VECTOR_INIT_CAP is 8. BPF programs routinely contain hundreds of instructions, so this adds ~4 extra reallocations per program generation. Consider an initial bf_vector_reserve in bf_program_new.

Nits

Line exceeds 80-char limit in bf_vector_add_many — src/libbpfilter/core/vector.c:161 — 82 characters vs the 80-char ColumnLimit in .clang-format. Can be split by extracting the ternary.
_bf_vector_realloc missing assert(vec) — src/libbpfilter/core/vector.c:99 — Every other comparable static helper asserts its pointer parameter (_bf_list_node_new, _bf_list_node_free). Adding assert(vec) here would be consistent.
Include ordering in program.h — src/libbpfilter/cgen/program.h:17 — #include <bpfilter/core/vector.h> is out of alphabetical order. It should be placed after <bpfilter/core/list.h> and before <bpfilter/dump.h>. make -C build fixstyle should fix this automatically.
bf_vector_add growth fallback — src/libbpfilter/core/vector.c:132 — When cap * 2 > _BF_VECTOR_MAX_CAP, returns -ENOMEM instead of capping at _BF_VECTOR_MAX_CAP. Purely theoretical on 64-bit but inconsistent with bf_vector_add_many's fallback.
Error message "bytes" vs element count — src/libbpfilter/cgen/program.c:798 — Fixed: format now correctly says "insns" matching the element count.

Workflow run

src/libbpfilter/include/bpfilter/core/vector.h

src/bpfilter/cgen/program.c

src/libbpfilter/cgen/program.c

src/libbpfilter/include/bpfilter/request.h

src/libbpfilter/include/bpfilter/core/vector.h

src/libbpfilter/request.c

src/libbpfilter/core/vector.c

src/libbpfilter/include/bpfilter/core/vector.h

tests/unit/libbpfilter/request.c

tests/unit/libbpfilter/response.c

tests/unit/libbpfilter/core/vector.c

src/libbpfilter/io.c

src/libbpfilter/core/vector.c

src/libbpfilter/cgen/program.h

src/libbpfilter/cgen/program.c

src/libbpfilter/core/vector.c

src/libbpfilter/cgen/program.c

github-actions · 2026-03-26T15:39:58Z

src/libbpfilter/include/bpfilter/core/vector.h

+         (elem) && (elem) < (vec)->data + ((vec)->size * (vec)->elem_size);    \
+         (elem) = (elem) + (vec)->elem_size)
+
+/**


Claude: suggestion: bf_vector_foreach performs pointer arithmetic on void * ((elem) + (vec)->elem_size and (vec)->data + ...), which is a GCC extension not valid in standard C.

Since vector.h is a public header under include/bpfilter/core/, any downstream consumer using this macro with -pedantic -std=c17 will get a compile error. The pedantic integration test (tests/integration/pedantic_c.sh) includes all public headers with -pedantic-errors -std=c17 -Werror — the macro definition alone won't trigger an error (unexpanded macros aren't compiled), but any call site under those flags will fail.

For comparison, bf_list_foreach in list.h avoids this by iterating over typed pointers (bf_list_node *). Consider casting through char *:

Suggested change

/**

#define bf_vector_foreach(vec, elem) \

for (void *(elem) = (vec)->data; \

(elem) && (char *)(elem) < (char *)(vec)->data + ((vec)->size * (vec)->elem_size); \

(elem) = (void *)((char *)(elem) + (vec)->elem_size))

github-actions · 2026-03-26T15:39:59Z

src/libbpfilter/cgen/program.c

    _program->flavor = bf_hook_to_flavor(chain->hook);
    _program->runtime.ops = bf_flavor_ops_get(_program->flavor);
    _program->runtime.chain = chain;
+    _program->img = bf_vector_default(sizeof(struct bpf_insn));


Claude: suggestion: The old _BF_PROGRAM_DEFAULT_IMG_SIZE was 64 (1 << 6); the new generic _BF_VECTOR_INIT_CAP is 8. Since BPF instruction images routinely contain hundreds of instructions, starting at capacity 8 means ~4 extra reallocations (8 -> 16 -> 32 -> 64 -> 128) per program generation compared to the old code.

Consider adding an initial reserve in bf_program_new after creating the vector:

_program->img = bf_vector_default(sizeof(struct bpf_insn)); bf_vector_reserve(&_program->img, 64);

Alternatively, the caller-side reserve preserves the generic _BF_VECTOR_INIT_CAP for other use cases while optimizing the hot path.

+1 with Claude, programs are at least 200 instructions, so we can reserve 512 elements.

qdeslandes · 2026-03-26T14:40:07Z

src/libbpfilter/include/bpfilter/core/vector.h

+ * @brief Iterate over every element of a `bf_vector`.
+ *
+ * `elem` is declared as a pointer to the element type and will point to each
+ * element in turn. Do not add or remove elements during iteration.


Do not add

This should not be an issue.

remove elements during iteration

Same here, as we don't shrink the memory buffer.

qdeslandes · 2026-03-26T14:48:40Z

src/libbpfilter/include/bpfilter/core/vector.h

+ *
+ * @param vec Initialised vector. Must be non-NULL.
+ * @param index Index of the element.
+ * @return Pointer to the element, or NULL if @p index is out of bounds.


@p index -> `index`

As a general rule, use backticks for parameters, variables, functions...

qdeslandes · 2026-03-26T14:49:58Z

src/libbpfilter/include/bpfilter/core/vector.h

+void *bf_vector_get(const bf_vector *vec, size_t index);
+
+/**
+ * @brief Overwrite the n-th element.


"Replace the n-th element" would be clearer.

qdeslandes · 2026-03-26T14:55:19Z

src/libbpfilter/include/bpfilter/core/vector.h

+ * @brief Take ownership of the backing buffer.
+ *
+ * Returns the raw data pointer and resets the vector so it will
+ * re-allocate on the next add.


"on the next bf_vector_add"

qdeslandes · 2026-03-26T15:16:36Z

src/libbpfilter/core/vector.c

+
+    assert(vec);
+
+    if (!vec->elem_size)


This seems off as we never check for vec->elem_size. Either we assume the user might modify vec->elem_size (because it has access to bf_vector structure), in which case we need to validate it in each function, or bf_vector should be moved into vector.c (I would suggest this).

qdeslandes · 2026-03-26T17:18:38Z

src/libbpfilter/cgen/jmp.c

+                     ctx->insn_idx);
+        }
+
+        size_t off = ctx->program->img.size - ctx->insn_idx - 1U;


This variable should be defined at the beginning of the scope.

qdeslandes · 2026-03-26T17:20:00Z

src/libbpfilter/cgen/program.c

    _program->flavor = bf_hook_to_flavor(chain->hook);
    _program->runtime.ops = bf_flavor_ops_get(_program->flavor);
    _program->runtime.chain = chain;
+    _program->img = bf_vector_default(sizeof(struct bpf_insn));


+1 with Claude, programs are at least 200 instructions, so we can reserve 512 elements.

qdeslandes · 2026-03-26T17:21:51Z

src/libbpfilter/cgen/program.c

 int bf_program_emit(struct bf_program *program, struct bpf_insn insn)
 {
-    int r;
-
    assert(program);

-    if (program->img_size == program->img_cap) {
-        r = bf_program_grow_img(program);
-        if (r)
-            return r;
-    }
-
-    program->img[program->img_size++] = insn;
-
-    return 0;
+    return bf_vector_add(&program->img, &insn);
 }


static inline in program.h.

qdeslandes · 2026-03-26T17:24:59Z

src/libbpfilter/cgen/program.c

@@ -535,8 +519,8 @@

    /* This call could fail and return an error, in which case it is not
     * properly handled. However, this shouldn't be an issue as we previously
-     * test whether enough room is available in cgen.img, which is currently
-     * the only reason for EMIT() to fail. */
+     * reserved enough room in program->img, which is currently the only
+     * reason for EMIT() to fail. */
    EMIT(program, insn);


The original code is weird in order to ensure the state of bf_program is always valid (i.e. we don't insert a fixup for an instruction that doesn't exist).

We don't need to maintain a valid bf_program state anymore, as it doesn't survive past the generation step. Hence, you can push the fixup to the list, then use bf_program_emit(), and return an error on failure. It will be properly taken care of by bpfilter.

qdeslandes · 2026-03-26T17:27:20Z

src/libbpfilter/cgen/program.c

 int bf_program_emit_fixup_elfstub(struct bf_program *program,
                                  enum bf_elfstub_id id)
 {
    _free_bf_fixup_ struct bf_fixup *fixup = NULL;
    int r;

    assert(program);

-    if (program->img_size == program->img_cap) {
-        r = bf_program_grow_img(program);
-        if (r)
-            return r;
-    }
+    r = bf_vector_reserve(&program->img, program->img.size + 1);
+    if (r)
+        return r;

-    r = bf_fixup_new(&fixup, BF_FIXUP_ELFSTUB_CALL, program->img_size, NULL);
+    r = bf_fixup_new(&fixup, BF_FIXUP_ELFSTUB_CALL, program->img.size, NULL);
    if (r)
        return r;

    fixup->attr.elfstub_id = id;

    r = bf_list_add_tail(&program->fixups, fixup);
    if (r)
        return r;

    TAKE_PTR(fixup);

    EMIT(program, BPF_CALL_REL(0));

    return 0;
 }


Same as bf_program_emit_fixup.

meta-cla bot added the cla signed label Mar 24, 2026

pzmarzly marked this pull request as draft March 24, 2026 13:30