Skip to content

lib: cgen: detach the old Netfilter links on chain update #613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
qdeslandes merged 1 commit into from
Oct 29, 2026
Merged

lib: cgen: detach the old Netfilter links on chain update #613

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/libbpfilter/cgen/prog/link.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,14 @@ static int _bf_link_update_nf(struct bf_link *link, int prog_fd)
return bf_err_r(r, "failed to create nf_inet6 BPF link");
new_inet6_fd = r;

// Detach old links - safe now that new ones are active
/* Detach old links - safe now that new ones are active. They were
* unpinned, so closing them only drops a reference: they would stay
* attached until bpffs releases the pinned inode, the old program would
* keep filtering, and its priority would stay busy. */
(void)bf_bpf_link_detach(link->fd);

@github-actions github-actions Bot Oct 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 prashantpcm94 Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 github-actions Bot Oct 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

closep(&link->fd);

@github-actions github-actions Bot Oct 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

if (link->fd_extra >= 0)
(void)bf_bpf_link_detach(link->fd_extra);
closep(&link->fd_extra);

// Update link with new file descriptors
Expand Down
Loading