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

On Thu, 27 May 2021 17:16:49 +0200 Justin Iurman wrote:

Implement support for processing the IOAM Pre-allocated Trace with IPv6,
see [1] and [2]. Introduce a new IPv6 Hop-by-Hop TLV option, see IANA [3].

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.

Is 0 a valid interface ID? If not why not use id != 0 instead of
having a separate enabled field?

Documentation is provided at the end of this patchset.

Two relativistic hash tables: one for IOAM namespaces, the other for
IOAM schemas. A namespace can only have a single active schema and a
schema can only be attached to a single namespace (1:1 relationship).

  [1] https://tools.ietf.org/html/draft-ietf-ippm-ioam-ipv6-options
  [2] https://tools.ietf.org/html/draft-ietf-ippm-ioam-data
  [3] https://www.iana.org/assignments/ipv6-parameters/ipv6-parameters.xhtml#ipv6-parameters-2

Signed-off-by: Justin Iurman <redacted>

+extern struct ioam6_namespace *ioam6_namespace(struct net *net, __be16 id);
+extern void ioam6_fill_trace_data(struct sk_buff *skb,
+				  struct ioam6_namespace *ns,
+				  struct ioam6_trace_hdr *trace);
+
+extern int ioam6_init(void);
+extern void ioam6_exit(void);

no need for externs in new headers

Perhaps move it after seg6_flowlabel, better chance next person adding
a 1 byte type will not create a hole.

 };
 
 struct netns_ipv6 {

This one should be constrained to 0/1, right?
proc_dou8vec_minmax? no need for full u32.

+	},
+	{
+		.procname	= "ioam6_id",
+		.data		= &ipv6_devconf.ioam6_id,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,

uint?

This is out of order, ioam6_fail should now jump to rpl_exit()
and igmp6_late_err should point at ioam6_exit().

 	seg6_exit();
 seg6_fail:

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

+		/* Ignore if the IOAM namespace is unknown */
+		if (!ns)
+			goto ignore;
+
+		if (!skb_valid_dst(skb))
+			ip6_route_input(skb);
+
+		ioam6_fill_trace_data(skb, ns, trace);
+		break;
+	default:
+		break;
+	}
+
+ignore:
+	return true;
+
+drop:
+	kfree_skb(skb);
+	return false;
+}
+

+void ioam6_fill_trace_data(struct sk_buff *skb,
+			   struct ioam6_namespace *ns,
+			   struct ioam6_trace_hdr *trace)
+{
+	u8 sclen = 0;
+
+	/* Skip if Overflow flag is set OR
+	 * if an unknown type (bit 12-21) is set
+	 */
+	if (trace->overflow ||
+	    (trace->type.bit12 | trace->type.bit13 | trace->type.bit14 |
+	     trace->type.bit15 | trace->type.bit16 | trace->type.bit17 |
+	     trace->type.bit18 | trace->type.bit19 | trace->type.bit20 |
+	     trace->type.bit21)) {
+		return;
+	}

braces unnecessary

+
+	/* NodeLen does not include Opaque State Snapshot length. We need to
+	 * take it into account if the corresponding bit is set (bit 22) and
+	 * if the current IOAM namespace has an active schema attached to it
+	 */
+	if (trace->type.bit22) {
+		sclen = sizeof_field(struct ioam6_schema, hdr) / 4;
+
+		if (ns->schema)
+			sclen += ns->schema->len / 4;
+	}
+
+	/* If there is no space remaining, we set the Overflow flag and we
+	 * skip without filling the trace
+	 */
+	if (!trace->remlen || trace->remlen < (trace->nodelen + sclen)) {

brackets around sum unnecessary

+		trace->overflow = 1;
+		return;
+	}
+
+	__ioam6_fill_trace_data(skb, ns, trace, sclen);
+	trace->remlen -= trace->nodelen + sclen;
+}

uint?

+	},
 	{ }
 };