bpf: prevent cluster ID from being incorrectly retrieved from mark when aliased#43258
Merged
giorio94 merged 2 commits intocilium:mainfrom Dec 11, 2025
Merged
bpf: prevent cluster ID from being incorrectly retrieved from mark when aliased#43258giorio94 merged 2 commits intocilium:mainfrom
giorio94 merged 2 commits intocilium:mainfrom
Conversation
The `ctx_get_cluster_id_mark` helper function allows to retrieve the cluster ID stored inside of `skb->mark`, and it does so after having checked the magic part, to prevent reading it from unrelated data. However, the check incorrectly uses `MARK_MAGIC_CLUSTER_ID` as mask, hence potentially misinterpreting other partially overlapping magic values (e.g., `MARK_MAGIC_IDENTITY`) as matching as well. Let's get this fixed by checking that the magic part exactly matches `MARK_MAGIC_CLUSTER_ID`, and update the corresponding setter function to override the entire mark content, similarly to what `set_identity_mark` also does. Fixes: 9ef6388 ("bpf: Add helper functions to transfer ClusterID with mark") Fixes: 5313b0d ("bpf: clear mark content before storing the cluster ID") Suggested-by: Julian Wiedmann <jwi@isovalent.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
12a1ce5 to
f86ecc1
Compare
Member
Author
|
/test |
julianwiedmann
approved these changes
Dec 11, 2025
Member
julianwiedmann
left a comment
There was a problem hiding this comment.
The changes make sense to me, thank you! Just that one little nit to address.
I feel like set_identity_mark() and ctx_set_cluster_id_mark() are converging more & more. But the bit-shift forest is too deep for me to have confidence in how it might be unified.
f86ecc1 to
9b0405a
Compare
Member
Author
Agree, that's unfortunately fairly convoluted and difficult to reason about. And yep, there's probably room for unifying the logic constructing the cluster ID related part of the mark, which matches in both cases. |
9b0405a to
8c37ac9
Compare
Currently, the `ctx_get_cluster_id_mark` helper function partially resets the content of `skb->mark` after having retrieved the cluster ID value. However, this is potentially misleading, and does not match the behavior of the other getter functions. Hence, let's remove the explicit cleanup step, and let subsequent setter calls (e.g., of `set_identity_mark`) to take care of resetting it. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
8c37ac9 to
149626c
Compare
Member
Author
|
/test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please refer to the individual commits for additional details.
Set the backport/affect labels coherently with #42551 and #43159.