Skip to content

daemon: cgen: use float for meta.probability matcher#453

Closed
qdeslandes wants to merge 1 commit intofacebook:mainfrom
qdeslandes:float_proba
Closed

daemon: cgen: use float for meta.probability matcher#453
qdeslandes wants to merge 1 commit intofacebook:mainfrom
qdeslandes:float_proba

Conversation

@qdeslandes
Copy link
Contributor

Unify meta.probability and meta.flow_probability to share the same floating-point parsing and printing logic, replacing the integer-only code path. This allows meta.probability to accept fractional percentages (e.g., "33.33%") and removes duplicate parser/printer functions.

Unify meta.probability and meta.flow_probability to share the same
floating-point parsing and printing logic, replacing the integer-only
code path. This allows meta.probability to accept fractional percentages
(e.g., "33.33%") and removes duplicate parser/printer functions.
@meta-cla meta-cla bot added the cla signed label Mar 3, 2026
@@ -928,7 +898,7 @@ static struct bf_matcher_meta _bf_matcher_metas[_BF_MATCHER_TYPE_MAX] = {
.layer = BF_MATCHER_NO_LAYER,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: The payload size for BF_MATCHER_META_PROBABILITY changes from sizeof(uint8_t) (1 byte) to sizeof(float) (4 bytes). Since bf_matcher_pack serializes the payload as raw binary and bf_matcher_new_from_packbf_matcher_new copies payload bytes without validating against ref_payload_size, any previously persisted matchers with the old 1-byte payload will be deserialized into a matcher struct with only 1 byte of payload. Subsequent *(float *)bf_matcher_payload(matcher) reads in both codegen and print would then read 3 bytes out of bounds.

Is there a version/migration mechanism for serialized state, or is this acceptable because the daemon always re-generates matchers from rules on restart?

.layer = BF_MATCHER_NO_LAYER,

EMIT(program, BPF_EMIT_CALL(BPF_FUNC_get_prandom_u32));
EMIT_FIXUP_JMP_NEXT_RULE(
program,
BPF_JMP_IMM(BPF_JGT, BPF_REG_0,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: _bf_matcher_generate_meta_probability uses BPF_JMP_IMM (64-bit comparison) while the analogous _bf_matcher_generate_meta_flow_probability at line 251 uses BPF_JMP32_IMM (32-bit comparison). Since bpf_get_prandom_u32() returns a 32-bit value, BPF_JMP32_IMM is the more precise match. This is a pre-existing inconsistency, but now that the two matchers share the same parsing/printing logic, aligning the codegen would be a natural follow-up.

BPF_JMP_IMM(BPF_JGT, BPF_REG_0,

@github-actions
Copy link

github-actions bot commented Mar 3, 2026

Claude: review of facebook/bpfilter #453 (47e22e8)

Suggestions

  • ABI break: payload size changes from 1 to 4 bytessrc/libbpfilter/matcher.c:898BF_MATCHER_META_PROBABILITY payload changes from sizeof(uint8_t) to sizeof(float). Previously serialized matchers will have a 1-byte payload that the new code reads as 4 bytes (*(float *)), causing out-of-bounds reads in codegen and print. Is there a version/migration mechanism, or does the daemon always regenerate from rules?
  • BPF_JMP_IMM vs BPF_JMP32_IMM inconsistencysrc/bpfilter/cgen/matcher/meta.c:75_bf_matcher_generate_meta_probability uses BPF_JMP_IMM while the analogous _bf_matcher_generate_meta_flow_probability uses BPF_JMP32_IMM. Since bpf_get_prandom_u32() returns a 32-bit value, BPF_JMP32_IMM is the more precise match. Pre-existing inconsistency, but natural to fix alongside this unification.

Nits

  • strtod accepts scientific notationsrc/libbpfilter/matcher.c — The shared strtod-based parser will accept inputs like "1e2%" (parsed as 100.0%) or "+50%". The lexer prevents these via bfcli, but other translation paths calling bf_matcher_new_from_raw directly could pass such strings. Pre-existing in _bf_parse_flow_probability, now also applies to meta.probability.
  • Commit message component — The title is daemon: cgen: but the primary change is in src/libbpfilter/matcher.c (lib: matcher). The daemon/cgen change is a 2-line side effect. Something like lib: matcher: use float for meta.probability may be more accurate.

@qdeslandes qdeslandes closed this Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant