Skip to content
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.

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

Collect and output skb->cb when --filter-trace-tc #461

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Dec 4, 2024

According to kernel verifier implementation[1], __sk_buff->cb will be mapped to ((struct qdisc_skb_cb*)&sk_buff->cb)->data, let's collect 20 bytes from there and output cb as u32[5] when --filter-trace-tc is turned on.

[1] https://elixir.bootlin.com/linux/v6.8/source/net/core/filter.c#L9593

Fixes: #295

@jschwinger233 jschwinger233 marked this pull request as ready for review December 4, 2024 18:18
@jschwinger233 jschwinger233 requested a review from a team as a code owner December 4, 2024 18:18
@jschwinger233 jschwinger233 requested review from brb and removed request for a team December 4, 2024 18:18
Copy link
Contributor

@Asphaltt Asphaltt left a comment

Choose a reason for hiding this comment

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

LGTM, I’ve left two comments for further discussion.

@@ -443,6 +444,10 @@ set_output(void *ctx, struct sk_buff *skb, struct event_t *event) {
if (cfg->output_stack) {
event->print_stack_id = bpf_get_stackid(ctx, &print_stack_map, BPF_F_FAST_STACK_CMP);
}

if (cfg->output_cb) {
bpf_probe_read_kernel(&event->meta.cb, sizeof(event->meta.cb), (void *)&skb->cb[8]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to understand skb->cb[8] here. It's better to convert skb->cb as struct qdisc_skb_cb *, and then read data from cb->data like:

struct qdisc_skb_cb *cb = (typeof(cb))(void *) skb->cb;
bpf_probe_read_kernel(&event->meta.cb, sizeof(event->meta.cb), (void *) &cb->data);

@@ -54,7 +54,7 @@ struct skb_meta {
u32 len;
u32 mtu;
u16 protocol;
u16 pad;
u8 cb[20];
Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK to define as u32 cb[5] like cb in struct __sk_buff, I think.

According to kernel verifier implementation[1], __sk_buff->cb will be
mapped to ((struct qdisc_skb_cb*)&sk_buff->cb)->data, let's collect 20
bytes from there and output cb as u32[5] when --filter-trace-tc is
turned on.

[1] https://elixir.bootlin.com/linux/v6.8/source/net/core/filter.c#L9593

Signed-off-by: gray <[email protected]>
Copy link
Contributor

@Asphaltt Asphaltt left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Good find, thanks!

@brb brb merged commit f9bd25f into cilium:main Dec 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cb[] (array of values) is not printed correctly when dumping the full skb
3 participants