Add VLAN ID information to metadata output #470
Conversation
Signed-off-by: Andreas Karis <[email protected]>
There was a problem hiding this comment.
Thanks for the PR!
Since the TCI is reported as part of the "meta" section, the question of retrieving it from the payload arises. But IMO it makes sense as this improves UX (I would just add a data source flag, see one of the comments).
There's also the question of double-tagging, but this is probably out of scope here. Any idea on how we should proceed (as a follow-up)?
The modifications already looks quite good, so I included nits comments as well.
| @@ -70,7 +70,7 @@ On Fedora, one can run: | |||
|
|
|||
| ```none | |||
| $ dnf -y install git cargo clang elfutils-libelf-devel python3-devel \ | |||
| jq libpcap-devel llvm make pkgconf-pkg-config | |||
| python3-clang jq libpcap-devel llvm make pkgconf-pkg-config | |||
There was a problem hiding this comment.
This should be in a dedicated patch.
There was a problem hiding this comment.
This is already part of:
Line 130 in bdec9e9
Normally, python3-clang should only be required for the gen-bindings and not for installing from sources (generated code is committed).
I'm not sure adding it here is what we want.
| @@ -244,6 +244,9 @@ impl EventFmt for SkbEvent { | |||
| if meta.data_len != 0 { | |||
| write!(f, "data_len {} ", meta.data_len)?; | |||
| } | |||
| if meta.vlan_tci != 0 { | |||
| write!(f, "vlan_tci {:#x} vlan_id {} ", meta.vlan_tci, meta.vlan_tci & 0x0fff)?; | |||
There was a problem hiding this comment.
Since TCI includes all other sub-fields I would prefer the above to be something like vlan (id {} prio {} [drop]) with drop being a flag and set if applicable?
In addition it would be nice to add another flag describing the source of the data, eg. accel (vlan (id {} prio {} [drop] [accel]) ). This would require an extra field in the raw and final events. WDYT?
| @@ -463,6 +466,7 @@ pub struct SkbMetaEvent { | |||
| pub csum_level: u8, | |||
| /// QoS priority. | |||
| pub priority: u32, | |||
| pub vlan_tci: u16, | |||
There was a problem hiding this comment.
IMO this would be better in an Option. The != 0 check can be moved from the display logic to the event creation one.
nit: missing field comment.
| if (!eth_type_vlan(h_vlan_proto)) | ||
| return -ENODATA; | ||
|
|
||
| __be16 h_vlan_TCI; |
There was a problem hiding this comment.
nit: Indentation.
nits (if you want): you can group definitions at the start of the block and also use u16 directly in that code block to stay close to the destination.
| @@ -282,6 +284,7 @@ static __always_inline int process_skb(struct retis_raw_event *event, | |||
| e->csum = BPF_CORE_READ(skb, csum); | |||
| e->csum_level = (u8)BPF_CORE_READ_BITFIELD_PROBED(skb, csum_level); | |||
| e->priority = BPF_CORE_READ(skb, priority); | |||
| vlan_get_tag(skb, dev, &(e->vlan_tci)); | |||
| @@ -0,0 +1,107 @@ | |||
| #ifndef __MODULE_IF_VLAN__ | |||
There was a problem hiding this comment.
nit: the rest of the code follow a path-like pattern, here this would be __MODULE_SKB_BPF_IF_VLAN__.
| netdev_features_t features; | ||
| bpf_probe_read_kernel(&features, sizeof(netdev_features_t), &dev->features); | ||
| if (features & NETIF_F_HW_VLAN_CTAG_TX) { | ||
| return __vlan_hwaccel_get_tag(skb, vlan_tci); | ||
| } else { | ||
| return __vlan_get_tag(skb, vlan_tci); | ||
| } |
There was a problem hiding this comment.
Retis can capture packets from many places of the stack and because of this it is not guaranteed that the device information will be linked to the skb. I'm wondering if we could instead not rely on that information and directly do:
diff --git a/retis/src/module/skb/bpf/include/if_vlan.h b/retis/src/module/skb/bpf/include/if_vlan.h
index 0f0233d5ffd3..5c3c04d996d0 100644
--- a/retis/src/module/skb/bpf/include/if_vlan.h
+++ b/retis/src/module/skb/bpf/include/if_vlan.h
@@ -93,15 +93,12 @@ static inline int __vlan_get_tag(const struct sk_buff *skb, u16 *vlan_tci)
*
* Returns error if the skb is not VLAN tagged
*/
-static inline int vlan_get_tag(const struct sk_buff *skb, struct net_device *dev, u16 *vlan_tci)
+static inline void vlan_get_tag(const struct sk_buff *skb, u16 *vlan_tci)
{
- netdev_features_t features;
- bpf_probe_read_kernel(&features, sizeof(netdev_features_t), &dev->features);
- if (features & NETIF_F_HW_VLAN_CTAG_TX) {
- return __vlan_hwaccel_get_tag(skb, vlan_tci);
- } else {
- return __vlan_get_tag(skb, vlan_tci);
- }
+ if (!__vlan_hwaccel_get_tag(skb, vlan_tci))
+ return;
+
+ __vlan_get_tag(skb, vlan_tci);
}
-#endif /* __MODULE_IF_VLAN__ */
\ No newline at end of file
+#endif /* __MODULE_IF_VLAN__ */
diff --git a/retis/src/module/skb/bpf/skb_hook.bpf.c b/retis/src/module/skb/bpf/skb_hook.bpf.c
index 4ee8b81b8c23..0e5efbe990ef 100644
--- a/retis/src/module/skb/bpf/skb_hook.bpf.c
+++ b/retis/src/module/skb/bpf/skb_hook.bpf.c
@@ -284,7 +284,7 @@ skip_netns:
e->csum = BPF_CORE_READ(skb, csum);
e->csum_level = (u8)BPF_CORE_READ_BITFIELD_PROBED(skb, csum_level);
e->priority = BPF_CORE_READ(skb, priority);
- vlan_get_tag(skb, dev, &(e->vlan_tci));
+ vlan_get_tag(skb, &(e->vlan_tci));
}
if (cfg->sections & BIT(SECTION_DATA_REF)) {| @@ -1,6 +1,7 @@ | |||
| #include <vmlinux.h> | |||
| #include <bpf/bpf_core_read.h> | |||
| #include <bpf/bpf_endian.h> | |||
| #include <if_vlan.h> | |||
There was a problem hiding this comment.
nit: please add this below the inclusion of "common.h", this block is for external ones.
Signed-off-by: Andreas Karis <[email protected]>
Add VLAN ID information to metadata output