feature: rate limit

[SkohTV]

Fix #215

Hi, sorry for the long hiatus,
A bunch of stuff came up, and I didn't have the time to do the big rebase+refactor.

Changes:

• Did the big rebase.
• Changed ratelimit from an action to a matcher, supports for eq with an uint32
• Kept the map, because some persistence is required for rate limiting.
• Also kept and slightly changed the elfstub.

	10 first	All after
chain my_chain BF_HOOK_XDP{ifindex=2} DROP rule meta.ratelimit eq 10 ACCEPT	ACCEPT	DROP
chain my_chain BF_HOOK_XDP{ifindex=2} ACCEPT rule meta.ratelimit eq 10 DROP	DROP	ACCEPT
chain my_chain BF_HOOK_XDP{ifindex=2} DROP rule meta.ratelimit not eq 10 ACCEPT	DROP	ACCEPT
chain my_chain BF_HOOK_XDP{ifindex=2} ACCEPT rule meta.ratelimit not eq 10 DROP	ACCEPT	DROP

If that implementation seems good, I'll go ahead and add QoL / the documentation

I'd really like to get this PR over the finish line at some point :)

[qdeslandes]

I went for an early review, mostly for the parsing part. On the BPF side, the map will contain the runtime values for a given rule's rate limit (current burst/allowance, limit, last update time...).

[github-actions]

This PR has had no activity for 14 days. It will be closed in 14 more days unless it is updated. Comment or push to keep it open.

[qdeslandes]

This PR has had no activity for 14 days. It will be closed in 14 more days unless it is updated. Comment or push to keep it open.

That's on me, looking at this today :)

[qdeslandes]

Changing the rate limit to a matcher is the good call, it will provide more flexibility.

The implementation has been simplified a lot, so much that so of the functionalities we would deem necessary have been removed.

You will need to think about two things now:

• How do you handle the unit? For now, supporting only s (e.g. meta.limit 10/s) would be enough, but allowing a unit to be passed will allow us to expand support for rate limiting later on.
• How to handle access to the bf_ratelimit (or whatever the name is) structure from a rule? Rules have IDs, but nothing prevents a rule from having multiple rate-limiting matchers. An BPF array map is a good pick, but you need to ensure a meta.limit matcher is mapped to the correct bf_ratelimit structure in the map.

Also, you'll have to rebase on main :)

[SkohTV]

I think I figured it out
I've kept it as 2 commits for now, to simplify the review, I'll squash them afterward

[qdeslandes]

Some comments, but it's on the right track.

[qdeslandes]

The payload of the meta.limit matcher should not be a string, but an actual value.

[qdeslandes]

You need to reject trailing characters (*endptr != \0\) and prevent integer overflow.

[qdeslandes]

What are you trying to do here?

[SkohTV]

Since I switched from bf_matcher_new_from_raw(...) to bf_matcher_new(...), _bf_parse_limit(...) and _bf_print_limit(...) are not used anymore.
There's no char * to uint32_t conversion anymore.

But that means BF_MATCHER_META_LIMIT in _bf_matcher_metas is just

[BF_MATCHER_META_LIMIT] =
	{
		.layer = BF_MATCHER_NO_LAYER,
	},

Which feels wrong.

[qdeslandes]

@claude review

[github-actions]

Claude review of PR #346 (20c5847)

Must fix

• bf_limit_free is a no-op (memory leak) — src/libbpfilter/limit.c:72 — the function never frees the calloc'd struct, breaking cleanup-attribute and list-destructor paths
• bf_chain_free missing limits cleanup — src/libbpfilter/chain.c:407 — cleans sets and rules but not the new limits list
• Out-of-bounds read: uint64_t from uint32_t payload — src/libbpfilter/cgen/matcher/meta.c:124 — reads 8 bytes from a 4-byte matcher payload buffer
• NULL dereference from bf_list_get_at — src/libbpfilter/cgen/matcher/meta.c:126 — return value is dereferenced without a NULL check
• strtoul validation ineffective — src/libbpfilter/limit.c:50 — overflow undetected due to truncation to uint32_t, dead range checks, wrong errno (-ENOMEM instead of -EINVAL)

Suggestions

• Unconditional limit map loading — src/libbpfilter/cgen/program.c:1270 — creates a BPF map for every chain even when no rules use meta.limit
• Unused _raw_payload strdup — src/libbpfilter/limit.c:57 — allocated and never read, dead code from set-parsing copy
• Unused pstr variable — src/libbpfilter/cgen/program.c:1062 — leftover from copy-paste of printer map loader

Nits

• Misleading field comment — src/libbpfilter/cgen/runtime.h:194 — says "Current timestamp" but field is a packet counter
• Unrelated comment regression — src/libbpfilter/matcher.c:51 — generic matcher docstring changed to "bf_matcher_limit" by accident
• Unnecessary includes in limit.h — src/libbpfilter/include/bpfilter/limit.h:12 — hashset.h, dump.h, matcher.h, stdbool.h, stddef.h are unused

Workflow run

[github-actions]

Claude: must-fix: bf_limit_free is a no-op — it casts away the parameter and never frees the calloc'd struct. This means:

1. Every _free_bf_limit_-annotated local that goes out of scope on an error path leaks.
2. bf_list_clean on a chain's limits list (which uses bf_limit_free as the free callback) leaks every node's data.

Every other _free function in the codebase follows the established contract (see bf_set_free in set.c, bf_rule_free in rule.c). Fix:

void bf_limit_free(struct bf_ratelimit **ratelimit)
{
    assert(ratelimit);

    if (!*ratelimit)
        return;

    BF_FREEP(ratelimit);
}

[github-actions]

Claude: must-fix: Out-of-bounds read: the payload was stored as uint32_t (4 bytes) in the parser (sizeof(limit_id)), but here it is read through a uint64_t *, reading 8 bytes from a 4-byte buffer. This is undefined behavior.

Every other meta matcher in this file casts to the type that was stored (e.g., *(uint32_t *)bf_matcher_payload(matcher) for iface at line 40). Fix:

uint32_t key = *(uint32_t *)bf_matcher_payload(matcher);

[github-actions]

Claude: must-fix: bf_list_get_at returns NULL when the index is out of bounds (see core/list.c:222), but node is dereferenced on line 128 without a NULL check. The existing pattern in chain.c:55-58 guards against this:

set = bf_list_get_at(&chain->sets, set_index);
if (!set)
    return bf_err_r(-EINVAL, "rule %u references non-existent set", ...);

Add a similar guard here and return an appropriate error.

[github-actions]

Claude: must-fix: The strtoul result validation has several issues:

1. limit is uint32_t but strtoul returns unsigned long (64-bit on LP64). The return value is silently truncated before the range check, so overflow is undetected.
2. limit <= 0 on an unsigned type is equivalent to limit == 0 — the < part is dead.
3. limit > UINT32_MAX is always false for a uint32_t.
4. errno is never checked for ERANGE.
5. No check for trailing characters after 's' (e.g., "10/sfoo" would pass).
6. Line 54 returns -ENOMEM but this is a validation error — should be -EINVAL.

The existing pattern in matcher.c uses errno = 0 before the call and checks errno == ERANGE afterward. Use an unsigned long intermediate and validate the range before narrowing:

errno = 0;
unsigned long parsed = strtoul(raw_payload, &endptr, BF_BASE_10);
if (errno == ERANGE || parsed == 0 || parsed > UINT32_MAX ||
    endptr[0] != '/' || endptr[1] != 's' || endptr[2] != '\0')
    return bf_err_r(-EINVAL, "limit is not valid '%s'", raw_payload);
limit = (uint32_t)parsed;

[qdeslandes]

Check how we parse integers for matchers, that logic could be reused.

[github-actions]

Claude: suggestion: _bf_program_load_limit_map is called unconditionally for every program, even when no rules use meta.limit. This creates an unnecessary BPF array map and wastes a file descriptor for every chain.

Other optional maps guard on chain state before creating (e.g., _bf_program_load_log_map returns early when BF_CHAIN_LOG is unset). Consider returning 0 early when the chain has no limits:

if (bf_list_is_empty(&program->runtime.chain->limits))
    return 0;

[github-actions]

Claude: suggestion: _raw_payload is strdup'd but never read — the parsed limit value is used directly. The allocation is cleaned up by _cleanup_free_ so there is no leak, but the strdup and its error path are dead code. This appears copied from bf_set_new_from_raw where the duplicate is actually tokenized. Remove lines 40 and 57-60.

[github-actions]

Claude: suggestion: _cleanup_free_ void *pstr = NULL; is declared but never used. This was likely copied from _bf_program_load_printer_map where pstr holds the assembled printer output. Remove it.

[github-actions]

Claude: nit: The Doxygen comment says "Current timestamp (at the last call of the BPF)" but the field is a packet counter (ratelimit->current++ in limit.bpf.c), not a timestamp. Consider: "Number of packets seen in the current rate-limit window."

[github-actions]

Claude: nit: Unrelated comment change: "A set of matcher defines" was changed to "A set of bf_matcher_limit defines". This Doxygen block documents the general bf_matcher concept, not rate limiting. Revert to the original wording.

[github-actions]

Claude: nit: Several includes are unused in this header: <bpfilter/core/hashset.h>, <bpfilter/dump.h>, <bpfilter/matcher.h>, <stdbool.h>, and <stddef.h>. Only <stdint.h> (for uint32_t) and <bpfilter/pack.h> (for bf_rpack_node_t, bf_wpack_t) are needed.

[qdeslandes]

The approach here is sound, but there are some minor fixes to do:

• Fix review comments and Claude comments
• Create atomic commits
• Add tests

[qdeslandes]

Group it with other variables.

[qdeslandes]

bf_max(1, program->runtime.chain->limits.len) I assume this it to prevent errors when the limits map is empty. If so, the map should not be created.

[qdeslandes]

Group variables definition.

[qdeslandes]

Check how we parse integers for matchers, that logic could be reused.