Skip to content

bpf:trace: refactor L2/L3 packet check into classifiers #38723

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

Jump to bottom
Merged
merged 2 commits into from
May 20, 2025
Merged

bpf:trace: refactor L2/L3 packet check into classifiers #38723

merged 2 commits into from
May 20, 2025

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Apr 3, 2025 •
edited
Loading

  • Refactor current identical check in {trace,drop}.h for packets from L2/L3 devices into a new classifiers.h file.
  • Moving also from using bits in struct {trace,drop}_notify to a new __u8 flags fields. This will help us in subsequent patches where we introduce additional classifiers.

Please refer to commit messages for further details.

@smagnani96 smagnani96 added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/hubble Impacts hubble server or relay labels Apr 3, 2025
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch 2 times, most recently from 88d55b4 to 79465ae Compare April 3, 2025 16:38
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch 3 times, most recently from 0ff6fc2 to 44f00c3 Compare April 11, 2025 22:01
@smagnani96 smagnani96 changed the title bpf:trace:hubble: trace reason refactoring into trace flags bpf:hubble: introduce packet classifiers for enhancing tracing Apr 11, 2025
@smagnani96
Copy link
Contributor Author

/test

@kaworu kaworu self-requested a review April 14, 2025 09:30
smagnani96 added a commit that referenced this pull request Apr 16, 2025
@smagnani96
monitor: Decode Cilium encapsulated traffic in Monitor
9708211 
We introduced in #38723 the initial support to signal from the datapath
trace/drop notify events related to overlay packets.
This patch allows our Monitor tool to decode overlay packets and output
additional information regarding the encap'd packet.

Prior to these changes:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK
```

After applying the patch:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK [tunnel 172.18.0.4:47375 -> 172.18.0.2:8472 vxlan]
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK [tunnel 172.18.0.2:46960 -> 172.18.0.4:8472 vxlan]
```

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch from 44f00c3 to fd4b5e3 Compare April 16, 2025 09:26
smagnani96 added a commit that referenced this pull request Apr 16, 2025
@smagnani96
monitor: Decode Cilium encapsulated traffic in Monitor
7e24d18 
We introduced in #38723 the initial support to signal from the datapath
trace/drop notify events related to overlay packets.
This patch allows our Monitor tool to decode overlay packets and output
additional information regarding the encap'd packet.

Prior to these changes:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK
```

After applying the patch:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK [tunnel 172.18.0.4:47375 -> 172.18.0.2:8472 vxlan]
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK [tunnel 172.18.0.2:46960 -> 172.18.0.4:8472 vxlan]
```

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 mentioned this pull request Apr 16, 2025
Closed
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch from fd4b5e3 to 9afcb39 Compare April 17, 2025 10:41
kaworu
kaworu approved these changes Apr 17, 2025
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Awesome work @smagnani96 patch LGTM 🎉 Before merging could you please open an issue to handle the ipsec/wg flag in Hubble so we can expose it in flows?

@smagnani96 smagnani96 mentioned this pull request Apr 18, 2025
Open
smagnani96 added a commit that referenced this pull request Apr 18, 2025
@smagnani96
monitor: Decode Cilium encapsulated traffic in Monitor
1a3e906 
We introduced in #38723 the initial support to signal from the datapath
trace/drop notify events related to overlay packets.
This patch allows our Monitor tool to decode overlay packets and output
additional information regarding the encap'd packet.

Prior to these changes:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK
```

After applying the patch:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK [tunnel 172.18.0.4:47375 -> 172.18.0.2:8472 vxlan]
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK [tunnel 172.18.0.2:46960 -> 172.18.0.4:8472 vxlan]
```

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 mentioned this pull request Apr 18, 2025
Closed
@smagnani96 smagnani96 changed the base branch from main to pr/smagnani96/cil_from_wireguard April 22, 2025 22:44
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch 2 times, most recently from edb3299 to 8dc765c Compare April 22, 2025 23:07
@smagnani96 smagnani96 force-pushed the pr/smagnani96/cil_from_wireguard branch from 0be5d0f to 16589b2 Compare April 23, 2025 08:09
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch from 8dc765c to cb4156e Compare April 23, 2025 09:36
smagnani96 added a commit that referenced this pull request Apr 23, 2025
@smagnani96
monitor: Decode Cilium encapsulated traffic in Monitor
2a0de51 
We introduced in #38723 the initial support to signal from the datapath
trace/drop notify events related to overlay packets.
This patch allows our Monitor tool to decode overlay packets and output
additional information regarding the encap'd packet.

Prior to these changes:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK
```

After applying the patch:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK [tunnel 172.18.0.4:47375 -> 172.18.0.2:8472 vxlan]
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK [tunnel 172.18.0.2:46960 -> 172.18.0.4:8472 vxlan]
```

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/cil_from_wireguard branch 2 times, most recently from 06c292c to fc7ce5d Compare April 24, 2025 08:54
Base automatically changed from pr/smagnani96/cil_from_wireguard to main April 24, 2025 10:52
@smagnani96 smagnani96 added the area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. label May 8, 2025
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch 2 times, most recently from 69f2a6c to 5a3eafb Compare May 8, 2025 13:45
@smagnani96
Copy link
Contributor Author

Few updates.
I've rebased to solve conflicts wrt main.
I've renamed for clarity such classifiers functions (+ added comments):

  1. ctx_classify_by_eth_hlen
  2. ctx_classify_by_pkt_mark
  3. ctx_classify_by_pkt_hdr

I've pushed a new commit bpf: extend IPSec trace/drop classifiers to give love to IPSec also: for most of the traces, we already used the CLS_FLAG_IPSEC. In all the other cases, we simply trace the packet as plain while it is not. Therefore:

  1. ingress -> IP protocol == ESP
  2. egress -> mark is one of the MARK_MAGIC_ENCRYPTED_OVERLAY, MARK_MAGIC_ENCRYPT, MARK_MAGIC_DECRYPT.

I think I'm happy with this iteration, new improvements I've found (to monitor/hubble) will be pushed in a different PR, so we don't block sig/datapath.

@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 mentioned this pull request May 8, 2025
Closed
smagnani96 added a commit to smagnani96/cilium that referenced this pull request May 16, 2025
@smagnani96
monitor: Decode Cilium encapsulated traffic in Monitor
19d7e97 
We introduced in cilium#38723 the initial support to signal from the datapath
trace/drop notify events related to overlay packets.
This patch allows our Monitor tool to decode overlay packets and output
additional information regarding the encap'd packet.

Prior to these changes:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK
```

After applying the patch:

```bash
-> network flow 0x4e41ca37 , identity host->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.2.63:4240 -> 10.244.1.6:41068 tcp ACK [tunnel 172.18.0.4:47375 -> 172.18.0.2:8472 vxlan]
<- network flow 0xb55b35e9 , identity remote-node->unknown state unknown ifindex eth0 orig-ip 0.0.0.0: 10.244.1.6:41068 -> 10.244.2.63:4240 tcp ACK [tunnel 172.18.0.2:46960 -> 172.18.0.4:8472 vxlan]
```

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch from 5a3eafb to 7ca5fcc Compare May 19, 2025 12:28
@smagnani96 smagnani96 removed request for rolinh and a team May 19, 2025 12:28
@smagnani96 smagnani96 changed the title bpf:hubble: introduce packet classifiers for enhancing tracing bpf:trace: refactor L2/L3 packet check into classifiers May 19, 2025
smagnani96 added 2 commits May 19, 2025 15:22
@smagnani96
bpf: refactor L2/L3 device check for tracing into classifiers
7bba6ac 
This commit refactors the current logic to signal packets coming from a
L3 device and IPv6 packets into a unique file `bpf/lib/classifiers.h` to
be used from within `bpf/lib/{trace,drop}.h`. Right now these two files
share the logic to detect such packets and set the apposite bit into their
message structures. With this commit we move from individual bits to a
unique `__u8 flags` field in the struct trace/drop. This helps with
upcoming patches where we'll add additional classifiers to also tag
Encrypted/Decrypted WireGuard/IPSec packets and overlay. This also aligns
to the userspace Hubble/Monitor, where we have a unique Flag field.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96
bpf: enable ctx_classify for bpf_host used in L3 devices
4fca3c4 
This commit extends the current ctx_classify logic introduced in the
previous commit to additionally account for the use case in which we
attach bpf_host to a L3 device, where ETH_HLEN is equal to zero.
We do not introduce though an additional bpf test, as this is already
covered by the WireGuard use case (classifiers_l2_dev.c).

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 smagnani96 force-pushed the pr/smagnani96/tracing-struct-refactoring branch from 7ca5fcc to 4fca3c4 Compare May 19, 2025 13:23
@smagnani96 smagnani96 requested a review from a team as a code owner May 19, 2025 13:23
@smagnani96 smagnani96 requested a review from aanm May 19, 2025 13:23
@smagnani96
Copy link
Contributor Author

/test

@smagnani96 smagnani96 requested a review from brb May 19, 2025 15:20
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 20, 2025
@joestringer joestringer added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels May 20, 2025
@joestringer
Copy link
Member

Thanks for the PR! I've labelled this with release-note/misc rather than release-note/minor, because this is unlikely to impact end users.

@joestringer joestringer added this pull request to the merge queue May 20, 2025
Merged via the queue into main with commit 017b8a9 May 20, 2025
301 of 307 checks passed
@joestringer joestringer deleted the pr/smagnani96/tracing-struct-refactoring branch May 20, 2025 16:30
@smagnani96
Copy link
Contributor Author

Thanks for the PR! I've labelled this with release-note/misc rather than release-note/minor, because this is unlikely to impact end users.

Many thanks Joe! Changing PR commits on-the-fly sure doesn't help with labeling, as I forgot to update them 😅 TIL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@brb brb brb approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/hubble Impacts hubble server or relay area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. feature/ipsec Relates to Cilium's IPsec feature feature/wireguard Relates to Cilium's Wireguard feature ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smagnani96 @joestringer @kaworu @brb @aanm