[RFC v2 PATCH 0/5] seg6: Segment routing fixes

[RFC v2 PATCH 0/5] seg6: Segment routing fixes

[Tom Herbert]

This patch set includes fixes to bring the segment routing
implementation into conformance with the latest version of the
draft (draft-ietf-6man-segment-routing-header-19). Also, segment
routing receive function calls ip6_parse to properly parse TLVs
in parsing loop.

Changes include:

  - Update TLV macro definitions. Mark obsoleted TLVs as such, add
    definitions for PAD1 and PADN
  - Mark obsoleted flags as such
  - Implement parsing loop in sr_has_hmac to find HMAC TLV without
    relying on obsoleted flag
  - Parameterize ip6_parse_tlv so that it can be used for parsing
    segment routing TLVs
  - Add sysctls for segment routing TLV parsing limits
  - Segment routing receive functions call ip6_parse_tlv

Tom Herbert (5):
  seg6: Fix TLV definitions
  seg6: Obsolete unused SRH flags
  ipv6: Paramterize TLV parsing
  seg6: Add sysctl limits for segment routing header
  seg6: Leverage ip6_parse_tlv

 include/net/ipv6.h         | 31 ++++++++++-------
 include/net/netns/ipv6.h   |  2 ++
 include/net/seg6.h         |  5 +++
 include/net/seg6_hmac.h    |  2 +-
 include/uapi/linux/seg6.h  | 54 +++++++++++++++++++++++------
 net/ipv6/af_inet6.c        |  2 ++
 net/ipv6/exthdrs.c         | 84 +++++++++++++++++++++++++++++++++++++++-------
 net/ipv6/seg6_hmac.c       | 16 ++-------
 net/ipv6/seg6_local.c      | 21 ++++++++----
 net/ipv6/sysctl_net_ipv6.c | 16 +++++++++
 10 files changed, 176 insertions(+), 57 deletions(-)

-- 
2.7.4

[RFC v2 PATCH 1/5] seg6: Fix TLV definitions

[Tom Herbert]

The definitions of TLVs in uapi/linux/seg6.h are out of date and
incomplete. Fix this.

TLV constants are defined for PAD1, PADN, and HMAC (the three defined in
draft-ietf-6man-segment-routing-header-19). The other TLV are unused and
and are marked as obsoleted.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/uapi/linux/seg6.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/seg6.h b/include/uapi/linux/seg6.h
index 286e8d6..3a7d324 100644
--- a/include/uapi/linux/seg6.h
+++ b/include/uapi/linux/seg6.h
@@ -38,10 +38,13 @@ struct ipv6_sr_hdr {
 #define SR6_FLAG1_ALERT		(1 << 4)
 #define SR6_FLAG1_HMAC		(1 << 3)
 
-#define SR6_TLV_INGRESS		1
-#define SR6_TLV_EGRESS		2
-#define SR6_TLV_OPAQUE		3
-#define SR6_TLV_PADDING		4
+
+#define SR6_TLV_INGRESS		1	/* obsoleted */
+#define SR6_TLV_EGRESS		2	/* obsoleted */
+#define SR6_TLV_OPAQUE		3	/* obsoleted */
+
+#define SR6_TLV_PAD1		0
+#define SR6_TLV_PADN		1
 #define SR6_TLV_HMAC		5
 
 #define sr_has_hmac(srh) ((srh)->flags & SR6_FLAG1_HMAC)
-- 
2.7.4

[RFC v2 PATCH 2/5] seg6: Obsolete unused SRH flags

[Tom Herbert]

Currently no flags are defined for segment routing in
draft-ietf-6man-segment-routing-header-19. Mark them as being
obsolete.

The HMAC flag is the only one used by the stack. This needs
additional consideration. Rewrite sr_has_hmac in uapi/linux/seg6.h
to properly parse a segment routing header as opposed to relying
on the now obsolete flag.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/uapi/linux/seg6.h | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/seg6.h b/include/uapi/linux/seg6.h
index 3a7d324..0d19a9c 100644
--- a/include/uapi/linux/seg6.h
+++ b/include/uapi/linux/seg6.h
@@ -33,11 +33,10 @@ struct ipv6_sr_hdr {
 	struct in6_addr segments[0];
 };
 
-#define SR6_FLAG1_PROTECTED	(1 << 6)
-#define SR6_FLAG1_OAM		(1 << 5)
-#define SR6_FLAG1_ALERT		(1 << 4)
-#define SR6_FLAG1_HMAC		(1 << 3)
-
+#define SR6_FLAG1_PROTECTED	(1 << 6)	/* obsoleted */
+#define SR6_FLAG1_OAM		(1 << 5)	/* obsoleted */
+#define SR6_FLAG1_ALERT		(1 << 4)	/* obsoleted */
+#define SR6_FLAG1_HMAC		(1 << 3)	/* obsoleted */
 
 #define SR6_TLV_INGRESS		1	/* obsoleted */
 #define SR6_TLV_EGRESS		2	/* obsoleted */
@@ -47,12 +46,42 @@ struct ipv6_sr_hdr {
 #define SR6_TLV_PADN		1
 #define SR6_TLV_HMAC		5
 
-#define sr_has_hmac(srh) ((srh)->flags & SR6_FLAG1_HMAC)
-
 struct sr6_tlv {
 	__u8 type;
 	__u8 len;
 	__u8 data[0];
 };
 
+static inline int sr_hmac_offset(struct ipv6_sr_hdr *srh)
+{
+	unsigned char *opt = (unsigned char *)srh;
+	unsigned int off = sizeof(*srh) + ((srh->first_segment + 1) << 4);
+	int len = ((srh->hdrlen + 1) << 8) - off;
+	unsigned int optlen;
+
+	while (len > 0) {
+		switch (opt[off]) {
+		case SR6_TLV_PAD1:
+			optlen = 1;
+			break;
+		case SR6_TLV_HMAC:
+			return off;
+		default:
+			if (len < sizeof(struct sr6_tlv))
+				return 0;
+
+			optlen = sizeof(struct sr6_tlv) +
+				 ((struct sr6_tlv *)&opt[off])->len;
+			break;
+		}
+
+		off += optlen;
+		len -= optlen;
+	}
+
+	return 0;
+}
+
+#define sr_has_hmac(srh) (!!sr_hmac_offset(srh))
+
 #endif
-- 
2.7.4

[RFC v2 PATCH 3/5] ipv6: Paramterize TLV parsing

[Tom Herbert]

Add parameters to ip6_parse_tlv that will allow leveraging the function
for parsing segment routing TLVs. The new parameters are offset of
TLVs, length of the TLV block, and a function that is called in the case
of an unrecognized option.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 net/ipv6/exthdrs.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 20291c2..a394d20 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -112,15 +112,26 @@ static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff,
 	return false;
 }
 
-/* Parse tlv encoded option header (hop-by-hop or destination) */
+/* Parse tlv encoded option header (hop-by-hop or destination)
+ *
+ * Arguments:
+ *   procs - TLV proc structure
+ *   skb - skbuff containing TLVs
+ *   max_count - absolute value is maximum nuber of TLVs. If less than zero
+ *		 then unknown TLVs are disallowed regardless of disposition
+ *		 indicated by TLV type
+ *   off - offset of first TLV relative to the first byte of the extension
+ *	   header which is transport header of the skb
+ *   len - length of TLV block
+ *   unknown_opt - function called when unknown option is encountered
+ */
 
 static bool ip6_parse_tlv(const struct tlvtype_proc *procs,
-			  struct sk_buff *skb,
-			  int max_count)
+			  struct sk_buff *skb, int max_count, int off, int len,
+			  bool (*unknown_opt)(struct sk_buff *skb, int optoff,
+					      bool disallow_unknowns))
 {
-	int len = (skb_transport_header(skb)[1] + 1) << 3;
 	const unsigned char *nh = skb_network_header(skb);
-	int off = skb_network_header_len(skb);
 	const struct tlvtype_proc *curr;
 	bool disallow_unknowns = false;
 	int tlv_count = 0;
@@ -131,11 +142,11 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs,
 		max_count = -max_count;
 	}
 
-	if (skb_transport_offset(skb) + len > skb_headlen(skb))
+	if (skb_transport_offset(skb) + off + len > skb_headlen(skb))
 		goto bad;
 
-	off += 2;
-	len -= 2;
+	/* Offset relative to network header for parse loop */
+	off += skb_network_header_len(skb);
 
 	while (len > 0) {
 		int optlen = nh[off + 1] + 2;
@@ -187,7 +198,7 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs,
 				}
 			}
 			if (curr->type < 0 &&
-			    !ip6_tlvopt_unknown(skb, off, disallow_unknowns))
+			    !unknown_opt(skb, off, disallow_unknowns))
 				return false;
 
 			padlen = 0;
@@ -309,7 +320,8 @@ static int ipv6_destopt_rcv(struct sk_buff *skb)
 #endif
 
 	if (ip6_parse_tlv(tlvprocdestopt_lst, skb,
-			  init_net.ipv6.sysctl.max_dst_opts_cnt)) {
+			  init_net.ipv6.sysctl.max_dst_opts_cnt,
+			  2, extlen - 2, ip6_tlvopt_unknown)) {
 		skb->transport_header += extlen;
 		opt = IP6CB(skb);
 #if IS_ENABLED(CONFIG_IPV6_MIP6)
@@ -848,7 +860,8 @@ int ipv6_parse_hopopts(struct sk_buff *skb)
 
 	opt->flags |= IP6SKB_HOPBYHOP;
 	if (ip6_parse_tlv(tlvprochopopt_lst, skb,
-			  init_net.ipv6.sysctl.max_hbh_opts_cnt)) {
+			  init_net.ipv6.sysctl.max_hbh_opts_cnt,
+			  2, extlen - 2, ip6_tlvopt_unknown)) {
 		skb->transport_header += extlen;
 		opt = IP6CB(skb);
 		opt->nhoff = sizeof(struct ipv6hdr);
-- 
2.7.4

[RFC v2 PATCH 4/5] seg6: Add sysctl limits for segment routing header

[Tom Herbert]

These are analoguous to the sysctls that were defined for IPv6
Destination and Hop-by-Hop Options.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/ipv6.h         | 31 ++++++++++++++++++-------------
 include/net/netns/ipv6.h   |  2 ++
 net/ipv6/af_inet6.c        |  2 ++
 net/ipv6/sysctl_net_ipv6.c | 16 ++++++++++++++++
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 0d34f6e..0633e50 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -52,45 +52,50 @@
 #define IPV6_DEFAULT_HOPLIMIT   64
 #define IPV6_DEFAULT_MCASTHOPS	1
 
-/* Limits on Hop-by-Hop and Destination options.
+/* Limits on Hop-by-Hop, Destination, and Segment Routing TLV options.
  *
  * Per RFC8200 there is no limit on the maximum number or lengths of options in
  * Hop-by-Hop or Destination options other then the packet must fit in an MTU.
- * We allow configurable limits in order to mitigate potential denial of
- * service attacks.
+ * Similarly, TLVs in a segment routing header lack a specific limit. We allow
+ * configurable limits in order to mitigate potential denial of service attacks.
  *
  * There are three limits that may be set:
  *   - Limit the number of options in a Hop-by-Hop or Destination options
- *     extension header
+ *     extension header, or the number of TLVs in a Segment Routing Header.
  *   - Limit the byte length of a Hop-by-Hop or Destination options extension
- *     header
- *   - Disallow unknown options
+ *     header, or the length of TLV block in a Segment Routing Header.
+ *   - Disallow unknown options.
  *
  * The limits are expressed in corresponding sysctls:
  *
  * ipv6.sysctl.max_dst_opts_cnt
  * ipv6.sysctl.max_hbh_opts_cnt
+ * ipv6.sysctl.max_srh_opts_cnt
  * ipv6.sysctl.max_dst_opts_len
  * ipv6.sysctl.max_hbh_opts_len
+ * ipv6.sysctl.max_srh_opts_len
  *
  * max_*_opts_cnt is the number of TLVs that are allowed for Destination
- * options or Hop-by-Hop options. If the number is less than zero then unknown
- * TLVs are disallowed and the number of known options that are allowed is the
- * absolute value. Setting the value to INT_MAX indicates no limit.
+ * Options or Hop-by-Hop Options, or the number of TLVs in Segment Routing
+ * TLVs. If the number is less than zero then unknown TLVs are disallowed and
+ * the number of known options that are allowed is the absolute value. Setting
+ * the value to INT_MAX indicates no limit.
  *
- * max_*_opts_len is the length limit in bytes of a Destination or
- * Hop-by-Hop options extension header. Setting the value to INT_MAX
- * indicates no length limit.
+ * max_*_opts_len is the length limit in bytes of a Destination or Hop-by-Hop,
+ * options extension header, or the length of the TLV block in a Segment
+ * Routing Header. Setting the value to INT_MAX indicates no length limit.
  *
  * If a limit is exceeded when processing an extension header the packet is
- * silently discarded.
+ * discarded and an appropriate ICMP error is sent.
  */
 
 /* Default limits for Hop-by-Hop and Destination options */
 #define IP6_DEFAULT_MAX_DST_OPTS_CNT	 8
 #define IP6_DEFAULT_MAX_HBH_OPTS_CNT	 8
+#define IP6_DEFAULT_MAX_SRH_OPTS_CNT	 8
 #define IP6_DEFAULT_MAX_DST_OPTS_LEN	 INT_MAX /* No limit */
 #define IP6_DEFAULT_MAX_HBH_OPTS_LEN	 INT_MAX /* No limit */
+#define IP6_DEFAULT_MAX_SRH_OPTS_LEN	 INT_MAX /* No limit */
 
 /*
  *	Addr type
diff --git a/include/net/netns/ipv6.h b/include/net/netns/ipv6.h
index 022a0fd..2cb53b3 100644
--- a/include/net/netns/ipv6.h
+++ b/include/net/netns/ipv6.h
@@ -47,8 +47,10 @@ struct netns_sysctl_ipv6 {
 	int flowlabel_reflect;
 	int max_dst_opts_cnt;
 	int max_hbh_opts_cnt;
+	int max_srh_opts_cnt;
 	int max_dst_opts_len;
 	int max_hbh_opts_len;
+	int max_srh_opts_len;
 	int seg6_flowlabel;
 	bool skip_notify_on_dev_down;
 };
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index ceab2fe2..d8dc360 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -862,8 +862,10 @@ static int __net_init inet6_net_init(struct net *net)
 	net->ipv6.sysctl.flowlabel_state_ranges = 0;
 	net->ipv6.sysctl.max_dst_opts_cnt = IP6_DEFAULT_MAX_DST_OPTS_CNT;
 	net->ipv6.sysctl.max_hbh_opts_cnt = IP6_DEFAULT_MAX_HBH_OPTS_CNT;
+	net->ipv6.sysctl.max_srh_opts_cnt = IP6_DEFAULT_MAX_SRH_OPTS_CNT;
 	net->ipv6.sysctl.max_dst_opts_len = IP6_DEFAULT_MAX_DST_OPTS_LEN;
 	net->ipv6.sysctl.max_hbh_opts_len = IP6_DEFAULT_MAX_HBH_OPTS_LEN;
+	net->ipv6.sysctl.max_srh_opts_len = IP6_DEFAULT_MAX_SRH_OPTS_LEN;
 	atomic_set(&net->ipv6.fib6_sernum, 1);
 
 	err = ipv6_init_mibs(net);
diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index 6d86fac..5fee576 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -162,6 +162,20 @@ static struct ctl_table ipv6_table_template[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
+	{
+		.procname	= "max_srh_opts_number",
+		.data		= &init_net.ipv6.sysctl.max_srh_opts_cnt,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
+	{
+		.procname	= "max_srh_tlvs_length",
+		.data		= &init_net.ipv6.sysctl.max_srh_opts_len,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
 	{ }
 };
 
@@ -228,6 +242,8 @@ static int __net_init ipv6_sysctl_net_init(struct net *net)
 	ipv6_table[13].data = &net->ipv6.sysctl.max_hbh_opts_len;
 	ipv6_table[14].data = &net->ipv6.sysctl.multipath_hash_policy,
 	ipv6_table[15].data = &net->ipv6.sysctl.seg6_flowlabel;
+	ipv6_table[16].data = &net->ipv6.sysctl.max_srh_opts_cnt;
+	ipv6_table[17].data = &net->ipv6.sysctl.max_srh_opts_len;
 
 	ipv6_route_table = ipv6_route_sysctl_init(net);
 	if (!ipv6_route_table)
-- 
2.7.4

[RFC v2 PATCH 5/5] seg6: Leverage ip6_parse_tlv

[Tom Herbert]

Call ip6_parse_tlv from segment routing receive function to properly
parse TLVs and to leverage to existing implementation that already
parses Destination and Hop-by-Hop Options. This includes applying
the denial of service mitigations and limits to processing segment
routing TLVs.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/seg6.h      |  5 +++++
 include/net/seg6_hmac.h |  2 +-
 net/ipv6/exthdrs.c      | 51 ++++++++++++++++++++++++++++++++++++++++++++++---
 net/ipv6/seg6_hmac.c    | 16 +++-------------
 net/ipv6/seg6_local.c   | 21 ++++++++++++++------
 5 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/include/net/seg6.h b/include/net/seg6.h
index 8b2dc68..b7d8a94 100644
--- a/include/net/seg6.h
+++ b/include/net/seg6.h
@@ -38,6 +38,11 @@ static inline void update_csum_diff16(struct sk_buff *skb, __be32 *from,
 	skb->csum = ~csum_partial((char *)diff, sizeof(diff), ~skb->csum);
 }
 
+static inline unsigned int seg6_tlv_offset(struct ipv6_sr_hdr *srh)
+{
+	return sizeof(*srh) + ((srh->first_segment + 1) << 4);
+}
+
 struct seg6_pernet_data {
 	struct mutex lock;
 	struct in6_addr __rcu *tun_src;
diff --git a/include/net/seg6_hmac.h b/include/net/seg6_hmac.h
index 7fda469..6ad33e2 100644
--- a/include/net/seg6_hmac.h
+++ b/include/net/seg6_hmac.h
@@ -53,7 +53,7 @@ extern int seg6_hmac_info_add(struct net *net, u32 key,
 extern int seg6_hmac_info_del(struct net *net, u32 key);
 extern int seg6_push_hmac(struct net *net, struct in6_addr *saddr,
 			  struct ipv6_sr_hdr *srh);
-extern bool seg6_hmac_validate_skb(struct sk_buff *skb);
+extern bool seg6_hmac_validate_skb(struct sk_buff *skb, int optoff);
 extern int seg6_hmac_init(void);
 extern void seg6_hmac_exit(void);
 extern int seg6_hmac_net_init(struct net *net);
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index a394d20..7d14c0d 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -112,7 +112,8 @@ static bool ip6_tlvopt_unknown(struct sk_buff *skb, int optoff,
 	return false;
 }
 
-/* Parse tlv encoded option header (hop-by-hop or destination)
+/* Parse tlv encoded option header (hop-by-hop, destination, or
+ * segment routing header)
  *
  * Arguments:
  *   procs - TLV proc structure
@@ -365,14 +366,42 @@ static void seg6_update_csum(struct sk_buff *skb)
 			   (__be32 *)addr);
 }
 
+static bool seg6_recv_hmac(struct sk_buff *skb, int optoff)
+{
+	if (!seg6_hmac_validate_skb(skb, optoff)) {
+		kfree_skb(skb);
+		return false;
+	}
+
+	return true;
+}
+
+static const struct tlvtype_proc tlvprocsrhopt_lst[] = {
+#ifdef CONFIG_IPV6_SEG6_HMAC
+	{
+		.type	= SR6_TLV_HMAC,
+		.func	= seg6_recv_hmac,
+	},
+#endif
+	{-1,			NULL}
+};
+
+static bool seg6_srhopt_unknown(struct sk_buff *skb, int optoff,
+				bool disallow_unknowns)
+{
+	/* Unknown segment routing header options are ignored */
+
+	return !disallow_unknowns;
+}
+
 static int ipv6_srh_rcv(struct sk_buff *skb)
 {
 	struct inet6_skb_parm *opt = IP6CB(skb);
 	struct net *net = dev_net(skb->dev);
+	int accept_seg6, tlvoff, tlvlen;
 	struct ipv6_sr_hdr *hdr;
 	struct inet6_dev *idev;
 	struct in6_addr *addr;
-	int accept_seg6;
 
 	hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
 
@@ -387,8 +416,24 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
 		return -1;
 	}
 
+	tlvoff = seg6_tlv_offset(hdr);
+	tlvlen = ipv6_optlen((struct ipv6_opt_hdr *)hdr) - tlvoff;
+
+	if (tlvlen) {
+		if (tlvlen > net->ipv6.sysctl.max_srh_opts_len) {
+			kfree_skb(skb);
+			return -1;
+		}
+
+		if (!ip6_parse_tlv(tlvprocsrhopt_lst, skb,
+				   init_net.ipv6.sysctl.max_srh_opts_cnt,
+				   tlvoff, tlvlen, seg6_srhopt_unknown))
+			return -1;
+	}
+
 #ifdef CONFIG_IPV6_SEG6_HMAC
-	if (!seg6_hmac_validate_skb(skb)) {
+	if (idev->cnf.seg6_require_hmac > 0 && !sr_has_hmac(hdr)) {
+		/* mandatory check but no HMAC tlv */
 		kfree_skb(skb);
 		return -1;
 	}
diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
index 8546f94..18f82f2 100644
--- a/net/ipv6/seg6_hmac.c
+++ b/net/ipv6/seg6_hmac.c
@@ -240,7 +240,7 @@ EXPORT_SYMBOL(seg6_hmac_compute);
  *
  * called with rcu_read_lock()
  */
-bool seg6_hmac_validate_skb(struct sk_buff *skb)
+bool seg6_hmac_validate_skb(struct sk_buff *skb, int optoff)
 {
 	u8 hmac_output[SEG6_HMAC_FIELD_LEN];
 	struct net *net = dev_net(skb->dev);
@@ -251,23 +251,13 @@ bool seg6_hmac_validate_skb(struct sk_buff *skb)
 
 	idev = __in6_dev_get(skb->dev);
 
-	srh = (struct ipv6_sr_hdr *)skb_transport_header(skb);
-
-	tlv = seg6_get_tlv_hmac(srh);
-
-	/* mandatory check but no tlv */
-	if (idev->cnf.seg6_require_hmac > 0 && !tlv)
-		return false;
-
 	/* no check */
 	if (idev->cnf.seg6_require_hmac < 0)
 		return true;
 
-	/* check only if present */
-	if (idev->cnf.seg6_require_hmac == 0 && !tlv)
-		return true;
+	srh = (struct ipv6_sr_hdr *)skb_transport_header(skb);
 
-	/* now, seg6_require_hmac >= 0 && tlv */
+	tlv = (struct sr6_tlv_hmac *)(skb_network_header(skb) + optoff);
 
 	hinfo = seg6_hmac_info_lookup(net, be32_to_cpu(tlv->hmackeyid));
 	if (!hinfo)
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 78155fd..d486ed8 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -92,6 +92,19 @@ static struct ipv6_sr_hdr *get_srh(struct sk_buff *skb)
 	return srh;
 }
 
+static bool seg6_local_hmac_validate_skb(struct sk_buff *skb,
+					 struct ipv6_sr_hdr *srh)
+{
+#ifdef CONFIG_IPV6_SEG6_HMAC
+	int off = sr_hmac_offset(srh);
+
+	return off ? seg6_hmac_validate_skb(skb, off) :
+		     (__in6_dev_get(skb->dev)->cnf.seg6_require_hmac <= 0);
+#else
+	return true;
+#endif
+}
+
 static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
 {
 	struct ipv6_sr_hdr *srh;
@@ -103,10 +116,8 @@ static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
 	if (srh->segments_left == 0)
 		return NULL;
 
-#ifdef CONFIG_IPV6_SEG6_HMAC
-	if (!seg6_hmac_validate_skb(skb))
+	if (!seg6_local_hmac_validate_skb(skb, srh))
 		return NULL;
-#endif
 
 	return srh;
 }
@@ -120,10 +131,8 @@ static bool decap_and_validate(struct sk_buff *skb, int proto)
 	if (srh && srh->segments_left > 0)
 		return false;
 
-#ifdef CONFIG_IPV6_SEG6_HMAC
-	if (srh && !seg6_hmac_validate_skb(skb))
+	if (srh && !seg6_local_hmac_validate_skb(skb, srh))
 		return false;
-#endif
 
 	if (ipv6_find_hdr(skb, &off, proto, NULL, NULL) < 0)
 		return false;
-- 
2.7.4

Re: [RFC v2 PATCH 1/5] seg6: Fix TLV definitions

[David Lebrun]

On 07/06/2019 19:55, Tom Herbert wrote:
> -#define SR6_TLV_PADDING		4

 From a uapi perspective, should we rather keep the definition and mark 
it as obsoleted as for the rest of the TLV types ?

Note that I'm fine with both.

Re: [RFC v2 PATCH 2/5] seg6: Obsolete unused SRH flags

[David Lebrun]

On 07/06/2019 19:55, Tom Herbert wrote:
> +	int len = ((srh->hdrlen + 1) << 8) - off;

You want << 3 instead of << 8.

Re: [RFC v2 PATCH 3/5] ipv6: Paramterize TLV parsing

[David Lebrun]

On 07/06/2019 19:55, Tom Herbert wrote:
> +			  bool (*unknown_opt)(struct sk_buff *skb, int optoff,
> +					      bool disallow_unknowns))

What about changing this boolean argument to 'allow_unknowns' (here, in 
ip6_tlvopt_unknown() and ip6_parse_tlv()) to avoid using negative names ?

Re: [RFC v2 PATCH 4/5] seg6: Add sysctl limits for segment routing header

[David Lebrun]

On 07/06/2019 19:55, Tom Herbert wrote:
> +		.procname	= "max_srh_tlvs_length",

Should this be "max_srh_opts_length" to be consistent with the rest of 
the naming ?

Re: [RFC v2 PATCH 0/5] seg6: Segment routing fixes

[David Lebrun]

On 07/06/2019 19:55, Tom Herbert wrote:
> This patch set includes fixes to bring the segment routing
> implementation into conformance with the latest version of the
> draft (draft-ietf-6man-segment-routing-header-19). Also, segment
> routing receive function calls ip6_parse to properly parse TLVs
> in parsing loop.

Thanks for your patch set !

General comment regarding uapi changes: it might be wise to wait for RFC 
status in case the IESG or IANA decide different type/flags values.

Re: [RFC v2 PATCH 5/5] seg6: Leverage ip6_parse_tlv

[David Lebrun]

On 07/06/2019 19:55, Tom Herbert wrote:
>   

...

> @@ -387,8 +416,24 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
>   		return -1;
>   	}
>   
> +	tlvoff = seg6_tlv_offset(hdr);
> +	tlvlen = ipv6_optlen((struct ipv6_opt_hdr *)hdr) - tlvoff;
> +
> +	if (tlvlen) {
> +		if (tlvlen > net->ipv6.sysctl.max_srh_opts_len) {
> +			kfree_skb(skb);
> +			return -1;
> +		}
> +
> +		if (!ip6_parse_tlv(tlvprocsrhopt_lst, skb,
> +				   init_net.ipv6.sysctl.max_srh_opts_cnt,

Why init_net ? I assume you mean 'net->ipv6.sysctl' instead.

> +				   tlvoff, tlvlen, seg6_srhopt_unknown))
> +			return -1;
> +	}
> +
>   #ifdef CONFIG_IPV6_SEG6_HMAC
> -	if (!seg6_hmac_validate_skb(skb)) {
> +	if (idev->cnf.seg6_require_hmac > 0 && !sr_has_hmac(hdr)) {
> +		/* mandatory check but no HMAC tlv */
>   		kfree_skb(skb);
>   		return -1;
>   	}
> diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
> index 8546f94..18f82f2 100644
> --- a/net/ipv6/seg6_hmac.c
> +++ b/net/ipv6/seg6_hmac.c
> @@ -240,7 +240,7 @@ EXPORT_SYMBOL(seg6_hmac_compute);
>    *
>    * called with rcu_read_lock()
>    */
> -bool seg6_hmac_validate_skb(struct sk_buff *skb)
> +bool seg6_hmac_validate_skb(struct sk_buff *skb, int optoff)
>   {
>   	u8 hmac_output[SEG6_HMAC_FIELD_LEN];
>   	struct net *net = dev_net(skb->dev);
> @@ -251,23 +251,13 @@ bool seg6_hmac_validate_skb(struct sk_buff *skb)
>   
>   	idev = __in6_dev_get(skb->dev);
>   
> -	srh = (struct ipv6_sr_hdr *)skb_transport_header(skb);
> -
> -	tlv = seg6_get_tlv_hmac(srh);
> -
> -	/* mandatory check but no tlv */
> -	if (idev->cnf.seg6_require_hmac > 0 && !tlv)
> -		return false;
> -
>   	/* no check */
>   	if (idev->cnf.seg6_require_hmac < 0)
>   		return true;
>   
> -	/* check only if present */
> -	if (idev->cnf.seg6_require_hmac == 0 && !tlv)
> -		return true;
> +	srh = (struct ipv6_sr_hdr *)skb_transport_header(skb);
>   
> -	/* now, seg6_require_hmac >= 0 && tlv */
> +	tlv = (struct sr6_tlv_hmac *)(skb_network_header(skb) + optoff);
>   
>   	hinfo = seg6_hmac_info_lookup(net, be32_to_cpu(tlv->hmackeyid));
>   	if (!hinfo)
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 78155fd..d486ed8 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -92,6 +92,19 @@ static struct ipv6_sr_hdr *get_srh(struct sk_buff *skb)
>   	return srh;
>   }
>   
> +static bool seg6_local_hmac_validate_skb(struct sk_buff *skb,
> +					 struct ipv6_sr_hdr *srh)
> +{
> +#ifdef CONFIG_IPV6_SEG6_HMAC
> +	int off = sr_hmac_offset(srh);
> +
> +	return off ? seg6_hmac_validate_skb(skb, off) :
> +		     (__in6_dev_get(skb->dev)->cnf.seg6_require_hmac <= 0);

If I read sr_hmac_offset() correctly, it returns an offset relative to 
the start of the SRH, while seg_hmac_validate_skb() now expects an 
offset relative to the network header (and rightly so as it receives 
such an offset from ip6_parse_tlv). A solution might be:

seg6_hmac_validate_skb(skb, skb_network_header_len(skb) + off)

But this also assumes that the SRH is present immediately after the IPv6 
header, which might not be true when HBH or Destination Options are also 
present. So I'd suggest something like:

int nhoff = (unsigned char *)srh - skb_network_header(skb);
int off = sr_hmac_offset(srh);

return off ? seg6_hmac_validate_skb(skb, off + nhoff) ...

> +#else
> +	return true;
> +#endif
> +}
> +
>   static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
>   {
>   	struct ipv6_sr_hdr *srh;
> @@ -103,10 +116,8 @@ static struct ipv6_sr_hdr *get_and_validate_srh(struct sk_buff *skb)
>   	if (srh->segments_left == 0)
>   		return NULL;
>   
> -#ifdef CONFIG_IPV6_SEG6_HMAC
> -	if (!seg6_hmac_validate_skb(skb))
> +	if (!seg6_local_hmac_validate_skb(skb, srh))
>   		return NULL;
> -#endif
>   
>   	return srh;
>   }
> @@ -120,10 +131,8 @@ static bool decap_and_validate(struct sk_buff *skb, int proto)
>   	if (srh && srh->segments_left > 0)
>   		return false;
>   
> -#ifdef CONFIG_IPV6_SEG6_HMAC
> -	if (srh && !seg6_hmac_validate_skb(skb))
> +	if (srh && !seg6_local_hmac_validate_skb(skb, srh))
>   		return false;
> -#endif
>   
>   	if (ipv6_find_hdr(skb, &off, proto, NULL, NULL) < 0)
>   		return false;
> 

Re: [RFC v2 PATCH 1/5] seg6: Fix TLV definitions

[Tom Herbert]

On Sat, Jun 8, 2019 at 2:50 AM David Lebrun <dav.lebrun@gmail.com> wrote:
>
> On 07/06/2019 19:55, Tom Herbert wrote:
> > -#define SR6_TLV_PADDING              4
>
>  From a uapi perspective, should we rather keep the definition and mark
> it as obsoleted as for the rest of the TLV types ?
>
Yes, that is an omission.

> Note that I'm fine with both.

Re: [RFC v2 PATCH 0/5] seg6: Segment routing fixes

[Tom Herbert]

On Sat, Jun 8, 2019 at 3:39 AM David Lebrun <dav.lebrun@gmail.com> wrote:
>
> On 07/06/2019 19:55, Tom Herbert wrote:
> > This patch set includes fixes to bring the segment routing
> > implementation into conformance with the latest version of the
> > draft (draft-ietf-6man-segment-routing-header-19). Also, segment
> > routing receive function calls ip6_parse to properly parse TLVs
> > in parsing loop.
>
> Thanks for your patch set !
>
> General comment regarding uapi changes: it might be wise to wait for RFC
> status in case the IESG or IANA decide different type/flags values.

David,

It's a "chicken and the egg problem". If we wait for publication to
implement the protocol, then we can't implement the protocol which is
necessary for publication. So we have to implement and deploy Internet
Drafts, but the definition of I-Ds expressly makes them "works in
progress" that can change at any time.

In this case, the segment routing draft is in working group last call
so it's unlikely that any major changes will occur. There is one
proposed change in changing a padding value, it has not been accepted
by the WG. IESG and IANA are very unlikely to impose different
protocol parameter values. A potential hangup for publication is that
there is very little evidence that large portions of the draft have
been implemented. Linux is the only draft cited as having implemented
TLVs and HMAC, but as these patches point out it's pretty out of date
(HMAC flag stil used, no padding, no parse loop). These patches should
help the argument that segment routing can move forward.

Tom

Re: [RFC v2 PATCH 0/5] seg6: Segment routing fixes

[Edward Cree]

On 07/06/2019 19:55, Tom Herbert wrote:
> This patch set includes fixes to bring the segment routing
> implementation into conformance with the latest version of the
> draft (draft-ietf-6man-segment-routing-header-19).
AIUI the concept of "conformance" doesn't really belong in the context
 of an Internet-Draft.  See e.g. RFC2026 §2.2, and the I-D preface
    'It is inappropriate to use Internet-Drafts as reference
     material or to cite them other than as "work in progress."'

-Ed

Re: [RFC v2 PATCH 0/5] seg6: Segment routing fixes

[Edward Cree]

On 10/06/2019 15:03, Tom Herbert wrote:
> On Mon, Jun 10, 2019, 5:54 AM Edward Cree <ecree@solarflare.com <mailto:ecree@solarflare.com>> wrote:
>
>     On 07/06/2019 19:55, Tom Herbert wrote:
>     > This patch set includes fixes to bring the segment routing
>     > implementation into conformance with the latest version of the
>     > draft (draft-ietf-6man-segment-routing-header-19).
>     AIUI the concept of "conformance" doesn't really belong in the context
>
>
> They can't be referred to as standards, but if conform is too strong a term, maybe we can say implements draft xyz.
That seems reasonable, yes.