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

[Jakub Kicinski <kuba@kernel.org>]

On Sun, 30 May 2021 12:36:38 +0200 (CEST) Justin Iurman wrote:
> >> 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.

> >> @@ -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?