[prashantpcm94]

Summary

After bfcli chain update (or update-set) on a Netfilter chain returns, the old program keeps filtering for a short while, and an update right after fails:

$ bfcli chain set --from-str "chain c BF_HOOK_NF_LOCAL_IN{priorities=101-102} ACCEPT rule ip4.proto icmp counter DROP"
$ bfcli chain update --name c --from-str "chain c BF_HOOK_NF_LOCAL_IN ACCEPT"
$ ping -c 1 -W 0.1 ...      # still dropped
$ bfcli chain update --name c --from-str "chain c BF_HOOK_NF_LOCAL_IN ACCEPT"
error  : failed to create nf_inet BPF link: Device or resource busy

_bf_link_update_nf() attaches the new program at the chain's other priority, then closes the old links. bf_cgen_update() unpinned them just before, so closing them only drops a reference: they stay attached until bpffs releases the pinned inode. Until then the old program still runs, and its priority is still taken, so the next update can't attach there. This detaches the old links before closing them, as bf_handle_detach() does. 414b9b2 did the same for the previous Netfilter update code; 2b1e12d dropped it when the links moved to bf_link.

Related issue

None.

Testing

CMake release build (-DNO_DOCS=1) on CentOS Stream 9, kernel 6.16.1, gcc 11.5, clang 22.1.3, libbpf 1.6.3, then make -C $BUILD test_bin and ctest (without fuzzing.parser.infinite, which runs until stopped):

99% tests passed, 1 tests failed out of 128
The following tests FAILED:
	127 - check.iwyu (Not Run)

check.iwyu didn't run because iwyu_tool.py isn't installed here (it only checks cgen/program.c). The check.astgrep tests aren't registered because ast-grep isn't installed either.

No test is added: on main the old links go away once bpffs frees the unpinned inode, so whether a test catches the bug depends on timing.

Checked by hand, each run in a fresh network and mount namespace, same link.c on main and with this PR, with the chain above: a ping right after chain update is dropped in 10/10 runs on main and passes in 10/10 with this PR, and an immediate second update fails with EBUSY in 9/10 runs on main and 0/10 with this PR.

Notes for the reviewer

• The old links are only detached once both new links are attached, so the hook is never left without a program, and a failure to create the new links still leaves the old ones in place for bf_cgen_update() to re-pin.

AI disclosure

Claude Code found the issue, drafted the change, the commit message and this description under my direction, and ran the tests listed under Testing. I reviewed the change and the results.

Checklist

• I understand every line in this PR and can explain why it is correct
• I built the project and ran the tests covering this change
• make -C $BUILD test_bin test passes (check.iwyu and check.astgrep not run, see Testing) and the code follows the style guide
• Commits are formatted as component: subcomponent: short description
• I have disclosed any AI usage above

[prashantpcm94]

@claude review

[github-actions]

Claude review of PR #613 (863d3c9)

The fix detaches the old links before closing them, as bf_handle_detach() does and as 414b9b2 did before 2b1e12d dropped it. It only runs once both new links are attached, so the hook always has a program. The extended e2e test covers both effects of the regression: the old program still filtering after the update, and its priority still taken. No blocking issues.

Suggestions

• No rollback after a successful link update — src/libbpfilter/cgen/prog/link.c:272 — not introduced here: if bf_handle_pin() or _bf_cgen_persist() fails after bf_link_update() succeeds, the old links are already gone and the new ones are unpinned. Possible follow-up.

Nits

• Log detach failures — src/libbpfilter/cgen/prog/link.c:271 — 414b9b2 warned on a failed detach; with (void), a later EBUSY can't be traced back to it.
• Share the detach sequence with bf_handle_detach() — src/libbpfilter/cgen/prog/link.c:271 — a common bf_link helper would keep a later refactor from dropping it from one caller again.

Workflow run

[github-actions]

Claude: nit: Casting the detach result to (void) matches bf_handle_detach(). On this path, though, a failed detach produces no log, and its only symptom comes later: the old program keeps filtering and the next chain update fails with -EBUSY, with nothing pointing back to the detach. 414b9b2, which this restores, logged the failure with bf_warn_r(r, "failed to detach existing BPF_NETFILTER link"). Should we keep that warning here, for both fd and fd_extra, so the later EBUSY can be traced? The update should still succeed, since the new links are already attached.

[prashantpcm94]

I kept (void) like bf_handle_detach(): a Netfilter link's detach always returns 0 (bpf_nf_link_detach()), and link->fd is valid here, so the warning could never fire.

[github-actions]

Claude: nit: This detach sequence (detach fd, then detach fd_extra if it is >= 0, ignoring errors) is the same as the one in bf_handle_detach() (src/libbpfilter/cgen/handle.c). Could it move into a small bf_link helper, for example a static _bf_link_detach() in link.c with a public wrapper for handle.c? Then a later refactor of the link code can't drop it from one caller and keep it in the other, which is what 2b1e12d did to the logic from 414b9b2. Optional, since there are only two call sites.

[github-actions]

Claude: suggestion: This PR doesn't introduce this, but it affects whether the update path is complete. When bf_link_update() fails, bf_cgen_update() handles it: the half-created new link is closed and the old handle is re-pinned. After bf_link_update() succeeds there is no rollback. If bf_handle_pin() or _bf_cgen_persist() then fails in bf_cgen_update(), the new links stay unpinned and are released when the process exits, while the old links are already gone. With this patch the old links go away synchronously here; before it, they went once bpffs released the inode. Either way the hook ends up with no program, and the persisted context may still list the old priorities. Is that worth a follow-up, for example detaching the old links only after the new handle is pinned and persisted?

[qdeslandes]

Ok for the fix, but small comment about the test.

[qdeslandes]

Triggering the actual issue is out of our control, so the error might not show up even if the code is not fixed. I would not introduce a non deterministic test.

[prashantpcm94]

Agreed, I dropped the test change: the PR now only contains the fix.

[qdeslandes]

LGTM, thanks!