Re: [PATCH net-next v4 2/5] ipv6: ioam: Data plane support for Pre-allocated Trace

On Sun, 30 May 2021 12:36:38 +0200 (CEST) Justin Iurman wrote:

quoted

quoted

A per-interface sysctl ioam6_enabled is provided to process/ignore IOAM
headers. Default is ignore (= disabled). Another per-interface sysctl
ioam6_id is provided to define the IOAM (unique) identifier of the
interface. Default is 0. A per-namespace sysctl ioam6_id is provided to
define the IOAM (unique) identifier of the node. Default is 0.  

Last two sentences are repeated.  

One describes net.ipv6.conf.XXX.ioam6_id (per interface) and the
other describes net.ipv6.ioam6_id (per namespace). It allows for
defining an IOAM id to an interface and, also, the node in general.

I see it now, please rephrase.

quoted

quoted

@@ -929,6 +932,50 @@ static bool ipv6_hop_ra(struct sk_buff *skb, int optoff)
 	return false;
 }
 
+/* IOAM */
+
+static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
+{
+	struct ioam6_trace_hdr *trace;
+	struct ioam6_namespace *ns;
+	struct ioam6_hdr *hdr;
+
+	/* Must be 4n-aligned */
+	if (optoff & 3)
+		goto drop;
+
+	/* Ignore if IOAM is not enabled on ingress */
+	if (!__in6_dev_get(skb->dev)->cnf.ioam6_enabled)
+		goto ignore;
+
+	hdr = (struct ioam6_hdr *)(skb_network_header(skb) + optoff);
+
+	switch (hdr->type) {
+	case IOAM6_TYPE_PREALLOC:
+		trace = (struct ioam6_trace_hdr *)((u8 *)hdr + sizeof(*hdr));
+		ns = ioam6_namespace(ipv6_skb_net(skb), trace->namespace_id);  

Shouldn't there be validation that the header is not truncated or
malformed before we start poking into the fields?  

ioam6_fill_trace_data is responsible (right after that) for checking
the header and making sure the whole thing makes sense before
inserting data. But, first, we need to parse the IOAM-Namespace ID to
check if it is a known (defined) one or not, and therefore either
going deeper or ignoring the option. Anyway, maybe I could add a
check on hdr->opt_len and make sure it has at least the length of the
required header (what comes after is data).

Right, don't we also need to check hdr->opt_len vs trace->remlen?

BTW the ASCII art in patch 1 looks like node data is filled in in order
but:

+	data = trace->data + trace->remlen * 4 - trace->nodelen * 4 - sclen * 4;

Looks like we'd start from the last node data?