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

Hi Jakub,

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.

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

Mainly for semantic reasons. Indeed, I'd prefer to keep a specific "enable" flag per interface as it sounds more intuitive. But, also because 0 could very well be a "valid" interface id (more like a default value).

quoted

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>

quoted

+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

ACK.

quoted

+#endif /* _NET_IOAM6_H */

diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index bde0b7adb4a3..a0d61a8fcfe1 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h

@@ -53,6 +53,7 @@ struct netns_sysctl_ipv6 {
 	int seg6_flowlabel;
 	bool skip_notify_on_dev_down;
 	u8 fib_notify_on_flag_change;
+	unsigned int ioam6_id;

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

+1.

quoted

 };
 
 struct netns_ipv6 {

quoted

@@ -6932,6 +6938,20 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "ioam6_enabled",
+		.data		= &ipv6_devconf.ioam6_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,

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

Indeed, +1.

quoted

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

uint?

+1.

quoted

+	},
 	{
 		/* sentinel */
 	}

diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 2389ff702f51..aec9664ec909 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c

@@ -62,6 +62,7 @@
 #include <net/rpl.h>
 #include <net/compat.h>
 #include <net/xfrm.h>
+#include <net/ioam6.h>
 
 #include <linux/uaccess.h>
 #include <linux/mroute6.h>

@@ -1191,6 +1192,10 @@ static int __init inet6_init(void)
 	if (err)
 		goto rpl_fail;
 
+	err = ioam6_init();
+	if (err)
+		goto ioam6_fail;
+
 	err = igmp6_late_init();
 	if (err)
 		goto igmp6_late_err;

@@ -1214,6 +1219,8 @@ static int __init inet6_init(void)
 #endif
 igmp6_late_err:
 	rpl_exit();
+ioam6_fail:
+	ioam6_exit();
 rpl_fail:

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

Good catch, I mixed it up *facepalm*.

quoted

 	seg6_exit();
 seg6_fail:

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).

quoted

+		/* 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;
+}
+

quoted

+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

ACK.

quoted

+
+	/* 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

ACK.

quoted

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

quoted

diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index d7cf26f730d7..b97aad7b6aca 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c

@@ -196,6 +196,13 @@ static struct ctl_table ipv6_table_template[] = {
 		.extra1         = SYSCTL_ZERO,
 		.extra2         = &two,
 	},
+	{
+		.procname	= "ioam6_id",
+		.data		= &init_net.ipv6.sysctl.ioam6_id,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec

uint?

+1.