fix(link): add missing BPF_F_REPLACE flag for RawAttachProgram#1995
Conversation
39d5f8a to
321a33e
Compare
michalskrivanek
left a comment
There was a problem hiding this comment.
Thank you for the quick fix!
I can confirm the patch fixes the problem.
|
@michalskrivanek Good catch — you're right, that comment was misleading. Updated it to clarify that the kernel requires BPF_F_REPLACE in attach_flags for the replace operation to take effect, not just on old kernels. |
|
@michalskrivanek @mmat11 Heads up on the CI — the Build and Lint failures (3 govet inline warnings in and ) are pre-existing and unrelated to my change in . They appear to be triggered by a new inline check in the latest golangci-lint version. Happy to help fix those if needed, or we can handle them separately. |
|
would it be possible to add a test that exercises this code path? |
|
@mmat11 Done both:
All changes compile cleanly and |
e77ae9b to
4130ac2
Compare
There was a problem hiding this comment.
Thanks! This needs a rebase to pass CI, and detach errors shouldn't be ignored. Please squash this into a single commit.
Also, for awareness, our GenAI policy is about to be finalized and will go into effect soon: cilium/community#408. I'm calling this out because your replies feel very generated, and this won't be allowed under the new policy. Please don't be afraid of making mistakes! Most of us aren't native speakers either. We'd rather talk to a real human, we have agents too, just like everyone else. :)
9dbaea2 to
e56b7a7
Compare
|
done, rebased and squashed. moved the detach into t.Cleanup with assert |
e56b7a7 to
75e0db4
Compare
When using ReplaceProgram anchor with RawAttachProgram, the BPF_F_REPLACE flag was not being set in AttachFlags. The Linux kernel requires this flag to actually perform the replacement operation - without it, replace_bpf_fd is ignored and the call is treated as a regular append. Set AttachFlags unconditionally before the if/else, which now only routes fdOrID to ReplaceBpfFd or RelativeFdOrId. Add TestRawAttachProgramReplace that verifies the ReplaceProgram anchor correctly replaces the attached program (only one program remains after replace, not two). Fixes cilium#1994 Signed-off-by: wucm667 <stevenwucongmin@gmail.com>
75e0db4 to
3d3ba3b
Compare
Fixes #1994
Summary
When using
RawAttachProgramwithAnchor: link.ReplaceProgram(oldProg), theBPF_F_REPLACEflag was not being set inAttachFlags. This caused the kernel to silently ignore the replacement and treat the call as a regular append, resulting in two programs being attached instead of one being replaced.Root Cause
In
link/program.go, whenflags == sys.BPF_F_REPLACE:attr.ReplaceBpfFdwas set correctlyattr.AttachFlagswas not updated withsys.BPF_F_REPLACEPer the Linux kernel (commit 7dd68b3), the kernel requires
BPF_F_REPLACEinattach_flagsto perform the replacement — without it,replace_bpf_fdis ignored.Fix
Add
attr.AttachFlags |= sys.BPF_F_REPLACEin theBPF_F_REPLACEbranch ofRawAttachProgram, ensuring the kernel actually performs the replacement operation.