[net-next v1 0/2] seg6: add NEXT-C-SID support for SRv6 End behavior

[net-next v1 0/2] seg6: add NEXT-C-SID support for SRv6 End behavior

[Andrea Mayer]

The Segment Routing (SR) architecture is based on loose source routing.
A list of instructions, called segments, can be added to the packet headers to
influence the forwarding and processing of the packets in an SR enabled network.
In SRv6 (Segment Routing over IPv6 data plane) [1], the segment identifiers
(SIDs) are IPv6 addresses (128 bits) and the segment list (SID List) is carried
in the Segment Routing Header (SRH). A segment may correspond to a "behavior"
that is executed by a node when the packet is received. The Linux kernel
currently supports a subset of behaviors described in [2] (e.g., End, End.X,
End.T and so on).

Some SRv6 scenarios (e.g., traffic-engineering, fast-rerouting, VPN, mobile
network backhaul, etc.) may require a large number of segments (i.e. up to 15).
Therefore, reducing the size of the SID List is useful to minimize the impact on
MTU (Maximum Transfer Unit) and to enable SRv6 on legacy hardware devices with
limited processing power that can suffer from long IPv6 headers.

Draft-ietf-spring-srv6-srh-compression [3] extends the SRv6 architecture by
providing different mechanisms for the efficient representation (i.e.
compression) of the SID List.

The NEXT-C-SID mechanism described in [3] offers the possibility of encoding
several SRv6 segments within a single 128 bit SID address. Such a SID address is
called a Compressed SID Container. In this way, the length of the SID List can
be drastically reduced. In some cases, the SRH can be omitted, as the IPv6
Destination Address can carry the whole Segment List, using its compressed
representation.

The NEXT-C-SID mechanism relies on the "flavors" framework defined in [2]. The
flavors represent additional operations that can modify or extend a subset of
the existing behaviors.

In this patchset we extend the SRv6 Subsystem in order to support the NEXT-C-SID
mechanism.

In details the patchset is made of:
 - patch 1/2: seg6: add netlink_ext_ack support in parsing SRv6 behavior attributes
 - patch 2/2: seg6: add NEXT-C-SID support for SRv6 End behavior

The corresponding iproute2 patch for supporting the NEXT-C-SID in SRv6 End
behavior is provided in a separated patchset.

Comments, improvements and suggestions are always appreciated.

Thank you all,
Andrea

[1] - https://datatracker.ietf.org/doc/html/rfc8754
[2] - https://datatracker.ietf.org/doc/html/rfc8986
[3] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression 

Andrea Mayer (2):
  seg6: add netlink_ext_ack support in parsing SRv6 behavior attributes
  seg6: add NEXT-C-SID support for SRv6 End behavior

 include/uapi/linux/seg6_local.h |  24 +++
 net/ipv6/seg6_local.c           | 355 ++++++++++++++++++++++++++++++--
 2 files changed, 360 insertions(+), 19 deletions(-)

-- 
2.20.1

[net-next v1 1/2] seg6: add netlink_ext_ack support in parsing SRv6 behavior attributes

[Andrea Mayer]

An SRv6 behavior instance can be set up using mandatory and/or optional
attributes.
In the setup phase, each supplied attribute is parsed and processed. If
the parsing operation fails, the creation of the behavior instance stops
and an error number/code is reported to the user. In many cases, it is
challenging for the user to figure out exactly what happened by relying
only on the error code.

For this reason, we add the support for netlink_ext_ack in parsing SRv6
behavior attributes. In this way, when an SRv6 behavior attribute is
parsed and an error occurs, the kernel can send a message to the
userspace describing the error through a meaningful text message in
addition to the classic error code.

Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
---
 net/ipv6/seg6_local.c | 44 +++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 9fbe243a0e81..5ea51ee2ef71 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -1135,7 +1135,8 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = {
 	[SEG6_LOCAL_COUNTERS]	= { .type = NLA_NESTED },
 };
 
-static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			 struct netlink_ext_ack *extack)
 {
 	struct ipv6_sr_hdr *srh;
 	int len;
@@ -1192,7 +1193,8 @@ static void destroy_attr_srh(struct seg6_local_lwt *slwt)
 	kfree(slwt->srh);
 }
 
-static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			   struct netlink_ext_ack *extack)
 {
 	slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
 
@@ -1226,7 +1228,8 @@ seg6_end_dt_info *seg6_possible_end_dt_info(struct seg6_local_lwt *slwt)
 }
 
 static int parse_nla_vrftable(struct nlattr **attrs,
-			      struct seg6_local_lwt *slwt)
+			      struct seg6_local_lwt *slwt,
+			      struct netlink_ext_ack *extack)
 {
 	struct seg6_end_dt_info *info = seg6_possible_end_dt_info(slwt);
 
@@ -1262,7 +1265,8 @@ static int cmp_nla_vrftable(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return 0;
 }
 
-static int parse_nla_nh4(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_nh4(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			 struct netlink_ext_ack *extack)
 {
 	memcpy(&slwt->nh4, nla_data(attrs[SEG6_LOCAL_NH4]),
 	       sizeof(struct in_addr));
@@ -1288,7 +1292,8 @@ static int cmp_nla_nh4(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return memcmp(&a->nh4, &b->nh4, sizeof(struct in_addr));
 }
 
-static int parse_nla_nh6(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_nh6(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			 struct netlink_ext_ack *extack)
 {
 	memcpy(&slwt->nh6, nla_data(attrs[SEG6_LOCAL_NH6]),
 	       sizeof(struct in6_addr));
@@ -1314,7 +1319,8 @@ static int cmp_nla_nh6(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return memcmp(&a->nh6, &b->nh6, sizeof(struct in6_addr));
 }
 
-static int parse_nla_iif(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_iif(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			 struct netlink_ext_ack *extack)
 {
 	slwt->iif = nla_get_u32(attrs[SEG6_LOCAL_IIF]);
 
@@ -1337,7 +1343,8 @@ static int cmp_nla_iif(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return 0;
 }
 
-static int parse_nla_oif(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_oif(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			 struct netlink_ext_ack *extack)
 {
 	slwt->oif = nla_get_u32(attrs[SEG6_LOCAL_OIF]);
 
@@ -1367,7 +1374,8 @@ static const struct nla_policy bpf_prog_policy[SEG6_LOCAL_BPF_PROG_MAX + 1] = {
 				       .len = MAX_PROG_NAME },
 };
 
-static int parse_nla_bpf(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_bpf(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			 struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb[SEG6_LOCAL_BPF_PROG_MAX + 1];
 	struct bpf_prog *p;
@@ -1445,7 +1453,8 @@ nla_policy seg6_local_counters_policy[SEG6_LOCAL_CNT_MAX + 1] = {
 };
 
 static int parse_nla_counters(struct nlattr **attrs,
-			      struct seg6_local_lwt *slwt)
+			      struct seg6_local_lwt *slwt,
+			      struct netlink_ext_ack *extack)
 {
 	struct pcpu_seg6_local_counters __percpu *pcounters;
 	struct nlattr *tb[SEG6_LOCAL_CNT_MAX + 1];
@@ -1544,7 +1553,8 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt)
 }
 
 struct seg6_action_param {
-	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt);
+	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+		     struct netlink_ext_ack *extack);
 	int (*put)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int (*cmp)(struct seg6_local_lwt *a, struct seg6_local_lwt *b);
 
@@ -1637,7 +1647,8 @@ static void destroy_attrs(struct seg6_local_lwt *slwt)
 }
 
 static int parse_nla_optional_attrs(struct nlattr **attrs,
-				    struct seg6_local_lwt *slwt)
+				    struct seg6_local_lwt *slwt,
+				    struct netlink_ext_ack *extack)
 {
 	struct seg6_action_desc *desc = slwt->desc;
 	unsigned long parsed_optattrs = 0;
@@ -1653,7 +1664,7 @@ static int parse_nla_optional_attrs(struct nlattr **attrs,
 		 */
 		param = &seg6_action_params[i];
 
-		err = param->parse(attrs, slwt);
+		err = param->parse(attrs, slwt, extack);
 		if (err < 0)
 			goto parse_optattrs_err;
 
@@ -1706,7 +1717,8 @@ static void seg6_local_lwtunnel_destroy_state(struct seg6_local_lwt *slwt)
 	ops->destroy_state(slwt);
 }
 
-static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
+static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			    struct netlink_ext_ack *extack)
 {
 	struct seg6_action_param *param;
 	struct seg6_action_desc *desc;
@@ -1750,14 +1762,14 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 
 			param = &seg6_action_params[i];
 
-			err = param->parse(attrs, slwt);
+			err = param->parse(attrs, slwt, extack);
 			if (err < 0)
 				goto parse_attrs_err;
 		}
 	}
 
 	/* parse the optional attributes, if any */
-	err = parse_nla_optional_attrs(attrs, slwt);
+	err = parse_nla_optional_attrs(attrs, slwt, extack);
 	if (err < 0)
 		goto parse_attrs_err;
 
@@ -1801,7 +1813,7 @@ static int seg6_local_build_state(struct net *net, struct nlattr *nla,
 	slwt = seg6_local_lwtunnel(newts);
 	slwt->action = nla_get_u32(tb[SEG6_LOCAL_ACTION]);
 
-	err = parse_nla_action(tb, slwt);
+	err = parse_nla_action(tb, slwt, extack);
 	if (err < 0)
 		goto out_free;
 
-- 
2.20.1

[net-next v1 2/2] seg6: add NEXT-C-SID support for SRv6 End behavior

[Andrea Mayer]

The NEXT-C-SID mechanism described in [1] offers the possibility of
encoding several SRv6 segments within a single 128 bit SID address. Such
a SID address is called a Compressed SID (C-SID) container. In this way,
the length of the SID List can be drastically reduced.

A SID instantiated with the NEXT-C-SID flavor considers an IPv6 address
logically structured in three main blocks: i) Locator-Block; ii)
Locator-Node Function; iii) Argument.

                        C-SID container
+------------------------------------------------------------------+
|     Locator-Block      |Loc-Node|            Argument            |
|                        |Function|                                |
+------------------------------------------------------------------+
<--------- B -----------> <- NF -> <------------- A --------------->

   (i) The Locator-Block can be any IPv6 prefix available to the provider;

  (ii) The Locator-Node Function represents the node and the function to
       be triggered when a packet is received on the node;

 (iii) The Argument carries the remaining C-SIDs in the current C-SID
       container.

The NEXT-C-SID mechanism relies on the "flavors" framework defined in
[2]. The flavors represent additional operations that can modify or
extend a subset of the existing behaviors.

This patch introduces the support for flavors in SRv6 End behavior
implementing the NEXT-C-SID one. An SRv6 End behavior with NEXT-C-SID
flavor works as an End behavior but it is capable of processing the
compressed SID List encoded in C-SID containers.

An SRv6 End behavior with NEXT-C-SID flavor can be configured to support
user-provided Locator-Block and Locator-Node Function lengths. In this
implementation, such lengths must be evenly divisible by 8 (i.e. must be
byte-aligned), otherwise the kernel informs the user about invalid
values with a meaningful error code and message through netlink_ext_ack.

If Locator-Block and/or Locator-Node Function lengths are not provided
by the user during configuration of an SRv6 End behavior instance with
NEXT-C-SID flavor, the kernel will choose their default values i.e.,
32-bit Locator-Block and 16-bit Locator-Node Function.

[1] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression
[2] - https://datatracker.ietf.org/doc/html/rfc8986

Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
---
 include/uapi/linux/seg6_local.h |  24 +++
 net/ipv6/seg6_local.c           | 311 +++++++++++++++++++++++++++++++-
 2 files changed, 332 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h
index 332b18f318f8..7919940c84d0 100644
--- a/include/uapi/linux/seg6_local.h
+++ b/include/uapi/linux/seg6_local.h
@@ -28,6 +28,7 @@ enum {
 	SEG6_LOCAL_BPF,
 	SEG6_LOCAL_VRFTABLE,
 	SEG6_LOCAL_COUNTERS,
+	SEG6_LOCAL_FLAVORS,
 	__SEG6_LOCAL_MAX,
 };
 #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1)
@@ -110,4 +111,27 @@ enum {
 
 #define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1)
 
+/* SRv6 End* Flavor attributes */
+enum {
+	SEG6_LOCAL_FLV_UNSPEC,
+	SEG6_LOCAL_FLV_OPERATION,
+	SEG6_LOCAL_FLV_LCBLOCK_LEN,
+	SEG6_LOCAL_FLV_LCNODE_FN_LEN,
+	__SEG6_LOCAL_FLV_MAX,
+};
+
+#define SEG6_LOCAL_FLV_MAX (__SEG6_LOCAL_FLV_MAX - 1)
+
+/* Designed flavor operations for SRv6 End* Behavior */
+enum {
+	SEG6_LOCAL_FLV_OP_UNSPEC,
+	SEG6_LOCAL_FLV_OP_PSP,
+	SEG6_LOCAL_FLV_OP_USP,
+	SEG6_LOCAL_FLV_OP_USD,
+	SEG6_LOCAL_FLV_OP_NEXT_CSID,
+	__SEG6_LOCAL_FLV_OP_MAX
+};
+
+#define SEG6_LOCAL_FLV_OP_MAX (__SEG6_LOCAL_FLV_OP_MAX - 1)
+
 #endif
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 5ea51ee2ef71..eb31c6c838e3 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -73,6 +73,39 @@ struct bpf_lwt_prog {
 	char *name;
 };
 
+/* default length values (expressed in bits) for both Locator-Block and
+ * Locator-Node Function.
+ *
+ * Both SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN *must* be:
+ *    i) greater than 0;
+ *   ii) evenly divisible by 8. In other terms, the lengths of the
+ *	 Locator-Block and Locator-Node Function must be byte-aligned (we can
+ *	 relax this constraint in the future if really needed).
+ *
+ * Moreover, a third condition must hold:
+ *  iii) SEG6_LOCAL_LCBLOCK_DLEN + SEG6_LOCAL_LCNODE_FN_DLEN <= 128.
+ *
+ * The correctness of SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN
+ * values are checked during the kernel compilation. If the compilation stops,
+ * check the value of these parameters to see if they meet conditions (i), (ii)
+ * and (iii).
+ */
+#define SEG6_LOCAL_LCBLOCK_DLEN		32
+#define SEG6_LOCAL_LCNODE_FN_DLEN	16
+
+/* Supported Flavor operations are reported in this bitmask */
+#define SEG6_LOCAL_FLV_SUPP_OPS	(BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID))
+
+struct seg6_flavors_info {
+	/* Flavor operations */
+	__u32 flv_ops;
+
+	/* Locator-Block length, expressed in bits */
+	__u8 lcblock_len;
+	/* Locator-Node Function length, expressed in bits*/
+	__u8 lcnode_func_len;
+};
+
 enum seg6_end_dt_mode {
 	DT_INVALID_MODE	= -EINVAL,
 	DT_LEGACY_MODE	= 0,
@@ -136,6 +169,8 @@ struct seg6_local_lwt {
 #ifdef CONFIG_NET_L3_MASTER_DEV
 	struct seg6_end_dt_info dt_info;
 #endif
+	struct seg6_flavors_info flv_info;
+
 	struct pcpu_seg6_local_counters __percpu *pcpu_counters;
 
 	int headroom;
@@ -270,8 +305,50 @@ int seg6_lookup_nexthop(struct sk_buff *skb,
 	return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false);
 }
 
-/* regular endpoint function */
-static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
+static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo)
+{
+	return finfo->lcblock_len >> 3;
+}
+
+static __u8 seg6_flv_lcnode_func_octects(const struct seg6_flavors_info *finfo)
+{
+	return finfo->lcnode_func_len >> 3;
+}
+
+static bool seg6_next_csid_is_arg_zero(const struct in6_addr *addr,
+				       const struct seg6_flavors_info *finfo)
+{
+	__u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo);
+	__u8 blk_octects = seg6_flv_lcblock_octects(finfo);
+	__u8 arg_octects;
+	int i;
+
+	arg_octects = 16 - blk_octects - fnc_octects;
+	for (i = 0; i < arg_octects; ++i) {
+		if (addr->s6_addr[blk_octects + fnc_octects + i] != 0x00)
+			return false;
+	}
+
+	return true;
+}
+
+/* assume that DA.Argument length > 0 */
+static void seg6_next_csid_advance_arg(struct in6_addr *addr,
+				       const struct seg6_flavors_info *finfo)
+{
+	__u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo);
+	__u8 blk_octects = seg6_flv_lcblock_octects(finfo);
+
+	/* advance DA.Argument */
+	memmove((void *)&addr->s6_addr[blk_octects],
+		(const void *)&addr->s6_addr[blk_octects + fnc_octects],
+		16 - blk_octects - fnc_octects);
+
+	memset((void *)&addr->s6_addr[16 - fnc_octects], 0x00, fnc_octects);
+}
+
+static int input_action_end_core(struct sk_buff *skb,
+				 struct seg6_local_lwt *slwt)
 {
 	struct ipv6_sr_hdr *srh;
 
@@ -290,6 +367,38 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
 	return -EINVAL;
 }
 
+static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt)
+{
+	const struct seg6_flavors_info *finfo = &slwt->flv_info;
+	struct in6_addr *daddr = &ipv6_hdr(skb)->daddr;
+
+	if (seg6_next_csid_is_arg_zero(daddr, finfo))
+		return input_action_end_core(skb, slwt);
+
+	/* update DA */
+	seg6_next_csid_advance_arg(daddr, finfo);
+
+	seg6_lookup_nexthop(skb, NULL, 0);
+
+	return dst_input(skb);
+}
+
+static bool seg6_next_csid_enabled(__u32 fops)
+{
+	return fops & BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID);
+}
+
+/* regular endpoint function */
+static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
+{
+	const struct seg6_flavors_info *finfo = &slwt->flv_info;
+
+	if (seg6_next_csid_enabled(finfo->flv_ops))
+		return end_next_csid_core(skb, slwt);
+
+	return input_action_end_core(skb, slwt);
+}
+
 /* regular endpoint, and forward to specified nexthop */
 static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt)
 {
@@ -952,7 +1061,8 @@ static struct seg6_action_desc seg6_action_table[] = {
 	{
 		.action		= SEG6_LOCAL_ACTION_END,
 		.attrs		= 0,
-		.optattrs	= SEG6_F_LOCAL_COUNTERS,
+		.optattrs	= SEG6_F_LOCAL_COUNTERS |
+				  SEG6_F_ATTR(SEG6_LOCAL_FLAVORS),
 		.input		= input_action_end,
 	},
 	{
@@ -1133,6 +1243,7 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = {
 	[SEG6_LOCAL_OIF]	= { .type = NLA_U32 },
 	[SEG6_LOCAL_BPF]	= { .type = NLA_NESTED },
 	[SEG6_LOCAL_COUNTERS]	= { .type = NLA_NESTED },
+	[SEG6_LOCAL_FLAVORS]	= { .type = NLA_NESTED },
 };
 
 static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt,
@@ -1552,6 +1663,190 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt)
 	free_percpu(slwt->pcpu_counters);
 }
 
+static const
+struct nla_policy seg6_local_flavors_policy[SEG6_LOCAL_FLV_MAX + 1] = {
+	[SEG6_LOCAL_FLV_OPERATION]	= { .type = NLA_U32 },
+	[SEG6_LOCAL_FLV_LCBLOCK_LEN]	= { .type = NLA_U8 },
+	[SEG6_LOCAL_FLV_LCNODE_FN_LEN]	= { .type = NLA_U8 },
+};
+
+/* check whether the lengths of the Locator-Block and Locator-Node Function
+ * are compatible with the dimension of a C-SID container.
+ */
+static int seg6_chk_next_csid_cfg(__u8 block_len, __u8 func_len)
+{
+	/* Locator-Block and Locator-Node Function cannot exceed 128 bits */
+	if (block_len + func_len > 128)
+		return -EINVAL;
+
+	/* Locator-Block length must be greater than zero and evenly divisible
+	 * by 8. There must be room for a Locator-Node Function, at least.
+	 */
+	if (block_len < 8 || block_len > 120 || (block_len & 0x07))
+		return -EINVAL;
+
+	/* Locator-Node Function length must be greater than zero and evenly
+	 * divisible by 8. There must be room for the Locator-Block.
+	 */
+	if (func_len < 8 || func_len > 120 || (func_len & 0x07))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int seg6_parse_nla_next_csid_cfg(struct nlattr **tb,
+					struct seg6_flavors_info *finfo,
+					struct netlink_ext_ack *extack)
+{
+	__u8 func_len = SEG6_LOCAL_LCNODE_FN_DLEN;
+	__u8 block_len = SEG6_LOCAL_LCBLOCK_DLEN;
+	int rc;
+
+	if (tb[SEG6_LOCAL_FLV_LCBLOCK_LEN])
+		block_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]);
+
+	if (tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN])
+		func_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]);
+
+	rc = seg6_chk_next_csid_cfg(block_len, func_len);
+	if (rc < 0) {
+		NL_SET_ERR_MSG(extack,
+			       "Invalid Locator Block/Node Function lengths");
+		return rc;
+	}
+
+	finfo->lcblock_len = block_len;
+	finfo->lcnode_func_len = func_len;
+
+	return 0;
+}
+
+static int parse_nla_flavors(struct nlattr **attrs, struct seg6_local_lwt *slwt,
+			     struct netlink_ext_ack *extack)
+{
+	struct seg6_flavors_info *finfo = &slwt->flv_info;
+	struct nlattr *tb[SEG6_LOCAL_FLV_MAX + 1];
+	unsigned long fops;
+	int rc;
+
+	rc = nla_parse_nested_deprecated(tb, SEG6_LOCAL_FLV_MAX,
+					 attrs[SEG6_LOCAL_FLAVORS],
+					 seg6_local_flavors_policy, NULL);
+	if (rc < 0)
+		return rc;
+
+	/* this attribute MUST always be present since it represents the Flavor
+	 * operation(s) to carry out.
+	 */
+	if (!tb[SEG6_LOCAL_FLV_OPERATION])
+		return -EINVAL;
+
+	fops = nla_get_u32(tb[SEG6_LOCAL_FLV_OPERATION]);
+	if (~SEG6_LOCAL_FLV_SUPP_OPS & fops) {
+		NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)");
+		return -EOPNOTSUPP;
+	}
+
+	finfo->flv_ops = fops;
+
+	if (seg6_next_csid_enabled(fops)) {
+		/* Locator-Block and Locator-Node Function lengths can be
+		 * provided by the user space. If not, default values are going
+		 * to be applied.
+		 */
+		rc = seg6_parse_nla_next_csid_cfg(tb, finfo, extack);
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
+static int seg6_fill_nla_next_csid_cfg(struct sk_buff *skb,
+				       struct seg6_flavors_info *finfo)
+{
+	if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCBLOCK_LEN, finfo->lcblock_len))
+		return -EMSGSIZE;
+
+	if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCNODE_FN_LEN,
+		       finfo->lcnode_func_len))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int put_nla_flavors(struct sk_buff *skb, struct seg6_local_lwt *slwt)
+{
+	struct seg6_flavors_info *finfo = &slwt->flv_info;
+	__u32 fops = finfo->flv_ops;
+	struct nlattr *nest;
+	int rc;
+
+	nest = nla_nest_start(skb, SEG6_LOCAL_FLAVORS);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, SEG6_LOCAL_FLV_OPERATION, fops)) {
+		rc = -EMSGSIZE;
+		goto err;
+	}
+
+	if (seg6_next_csid_enabled(fops)) {
+		rc = seg6_fill_nla_next_csid_cfg(skb, finfo);
+		if (rc < 0)
+			goto err;
+	}
+
+	return nla_nest_end(skb, nest);
+
+err:
+	nla_nest_cancel(skb, nest);
+	return rc;
+}
+
+static int seg6_cmp_nla_next_csid_cfg(struct seg6_flavors_info *finfo_a,
+				      struct seg6_flavors_info *finfo_b)
+{
+	if (finfo_a->lcblock_len != finfo_b->lcblock_len)
+		return 1;
+
+	if (finfo_a->lcnode_func_len != finfo_b->lcnode_func_len)
+		return 1;
+
+	return 0;
+}
+
+static int cmp_nla_flavors(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
+{
+	struct seg6_flavors_info *finfo_a = &a->flv_info;
+	struct seg6_flavors_info *finfo_b = &b->flv_info;
+
+	if (finfo_a->flv_ops != finfo_b->flv_ops)
+		return 1;
+
+	if (seg6_next_csid_enabled(finfo_a->flv_ops)) {
+		if (seg6_cmp_nla_next_csid_cfg(finfo_a, finfo_b))
+			return 1;
+	}
+
+	return 0;
+}
+
+static int encap_size_flavors(struct seg6_local_lwt *slwt)
+{
+	struct seg6_flavors_info *finfo = &slwt->flv_info;
+	int nlsize;
+
+	nlsize = nla_total_size(0) +	/* nest SEG6_LOCAL_FLAVORS */
+		 nla_total_size(4);	/* SEG6_LOCAL_FLV_OPERATION */
+
+	if (seg6_next_csid_enabled(finfo->flv_ops))
+		nlsize += nla_total_size(1) + /* SEG6_LOCAL_FLV_LCBLOCK_LEN */
+			  nla_total_size(1);  /* SEG6_LOCAL_FLV_LCNODE_FN_LEN */
+
+	return nlsize;
+}
+
 struct seg6_action_param {
 	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt,
 		     struct netlink_ext_ack *extack);
@@ -1604,6 +1899,10 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 				    .put = put_nla_counters,
 				    .cmp = cmp_nla_counters,
 				    .destroy = destroy_attr_counters },
+
+	[SEG6_LOCAL_FLAVORS]	= { .parse = parse_nla_flavors,
+				    .put = put_nla_flavors,
+				    .cmp = cmp_nla_flavors },
 };
 
 /* call the destroy() callback (if available) for each set attribute in
@@ -1917,6 +2216,9 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt)
 			  /* SEG6_LOCAL_CNT_ERRORS */
 			  nla_total_size_64bit(sizeof(__u64));
 
+	if (attrs & SEG6_F_ATTR(SEG6_LOCAL_FLAVORS))
+		nlsize += encap_size_flavors(slwt);
+
 	return nlsize;
 }
 
@@ -1972,6 +2274,9 @@ int __init seg6_local_init(void)
 	 */
 	BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > BITS_PER_TYPE(unsigned long));
 
+	BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN,
+					    SEG6_LOCAL_LCNODE_FN_DLEN) != 0);
+
 	return lwtunnel_encap_add_ops(&seg6_local_ops,
 				      LWTUNNEL_ENCAP_SEG6_LOCAL);
 }
-- 
2.20.1

Re: [net-next v1 2/2] seg6: add NEXT-C-SID support for SRv6 End behavior

[kernel test robot]

Hi Andrea,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrea-Mayer/seg6-add-NEXT-C-SID-support-for-SRv6-End-behavior/20220611-185055
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 27f2533bcc6e909b85d3c1b738fa1f203ed8a835
config: csky-buildonly-randconfig-r006-20220613 (https://download.01.org/0day-ci/archive/20220614/202206141513.QKv7c89J-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5d121eb7b2b3ceb14906485b89e9c90dfa16b3c9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Andrea-Mayer/seg6-add-NEXT-C-SID-support-for-SRv6-End-behavior/20220611-185055
        git checkout 5d121eb7b2b3ceb14906485b89e9c90dfa16b3c9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash net/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:
   net/ipv6/seg6_local.c: In function 'seg6_local_init':
>> include/linux/compiler_types.h:352:45: error: call to '__compiletime_assert_593' declared with attribute error: BUILD_BUG_ON failed: seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN, SEG6_LOCAL_LCNODE_FN_DLEN) != 0
     352 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                                             ^
   include/linux/compiler_types.h:333:25: note: in definition of macro '__compiletime_assert'
     333 |                         prefix ## suffix();                             \
         |                         ^~~~~~
   include/linux/compiler_types.h:352:9: note: in expansion of macro '_compiletime_assert'
     352 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |         ^~~~~~~~~~~~~~~~
   net/ipv6/seg6_local.c:2278:9: note: in expansion of macro 'BUILD_BUG_ON'
    2278 |         BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN,
         |         ^~~~~~~~~~~~


vim +/__compiletime_assert_593 +352 include/linux/compiler_types.h

eb5c2d4b45e3d2 Will Deacon 2020-07-21  338  
eb5c2d4b45e3d2 Will Deacon 2020-07-21  339  #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21  340  	__compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21  341  
eb5c2d4b45e3d2 Will Deacon 2020-07-21  342  /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21  343   * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21  344   * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21  345   * @msg:       a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21  346   *
eb5c2d4b45e3d2 Will Deacon 2020-07-21  347   * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21  348   * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21  349   * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21  350   */
eb5c2d4b45e3d2 Will Deacon 2020-07-21  351  #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @352  	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21  353  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [net-next v1 2/2] seg6: add NEXT-C-SID support for SRv6 End behavior

[Paolo Abeni]

On Sat, 2022-06-11 at 12:47 +0200, Andrea Mayer wrote:
> The NEXT-C-SID mechanism described in [1] offers the possibility of
> encoding several SRv6 segments within a single 128 bit SID address. Such
> a SID address is called a Compressed SID (C-SID) container. In this way,
> the length of the SID List can be drastically reduced.
> 
> A SID instantiated with the NEXT-C-SID flavor considers an IPv6 address
> logically structured in three main blocks: i) Locator-Block; ii)
> Locator-Node Function; iii) Argument.
> 
>                         C-SID container
> +------------------------------------------------------------------+
> >     Locator-Block      |Loc-Node|            Argument            |
> >                        |Function|                                |
> +------------------------------------------------------------------+
> <--------- B -----------> <- NF -> <------------- A --------------->
> 
>    (i) The Locator-Block can be any IPv6 prefix available to the provider;
> 
>   (ii) The Locator-Node Function represents the node and the function to
>        be triggered when a packet is received on the node;
> 
>  (iii) The Argument carries the remaining C-SIDs in the current C-SID
>        container.
> 
> The NEXT-C-SID mechanism relies on the "flavors" framework defined in
> [2]. The flavors represent additional operations that can modify or
> extend a subset of the existing behaviors.
> 
> This patch introduces the support for flavors in SRv6 End behavior
> implementing the NEXT-C-SID one. An SRv6 End behavior with NEXT-C-SID
> flavor works as an End behavior but it is capable of processing the
> compressed SID List encoded in C-SID containers.
> 
> An SRv6 End behavior with NEXT-C-SID flavor can be configured to support
> user-provided Locator-Block and Locator-Node Function lengths. In this
> implementation, such lengths must be evenly divisible by 8 (i.e. must be
> byte-aligned), otherwise the kernel informs the user about invalid
> values with a meaningful error code and message through netlink_ext_ack.
> 
> If Locator-Block and/or Locator-Node Function lengths are not provided
> by the user during configuration of an SRv6 End behavior instance with
> NEXT-C-SID flavor, the kernel will choose their default values i.e.,
> 32-bit Locator-Block and 16-bit Locator-Node Function.
> 
> [1] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression
> [2] - https://datatracker.ietf.org/doc/html/rfc8986
> 
> Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
> ---
>  include/uapi/linux/seg6_local.h |  24 +++
>  net/ipv6/seg6_local.c           | 311 +++++++++++++++++++++++++++++++-
>  2 files changed, 332 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h
> index 332b18f318f8..7919940c84d0 100644
> --- a/include/uapi/linux/seg6_local.h
> +++ b/include/uapi/linux/seg6_local.h
> @@ -28,6 +28,7 @@ enum {
>  	SEG6_LOCAL_BPF,
>  	SEG6_LOCAL_VRFTABLE,
>  	SEG6_LOCAL_COUNTERS,
> +	SEG6_LOCAL_FLAVORS,
>  	__SEG6_LOCAL_MAX,
>  };
>  #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1)
> @@ -110,4 +111,27 @@ enum {
>  
>  #define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1)
>  
> +/* SRv6 End* Flavor attributes */
> +enum {
> +	SEG6_LOCAL_FLV_UNSPEC,
> +	SEG6_LOCAL_FLV_OPERATION,
> +	SEG6_LOCAL_FLV_LCBLOCK_LEN,
> +	SEG6_LOCAL_FLV_LCNODE_FN_LEN,
> +	__SEG6_LOCAL_FLV_MAX,
> +};
> +
> +#define SEG6_LOCAL_FLV_MAX (__SEG6_LOCAL_FLV_MAX - 1)
> +
> +/* Designed flavor operations for SRv6 End* Behavior */
> +enum {
> +	SEG6_LOCAL_FLV_OP_UNSPEC,
> +	SEG6_LOCAL_FLV_OP_PSP,
> +	SEG6_LOCAL_FLV_OP_USP,
> +	SEG6_LOCAL_FLV_OP_USD,
> +	SEG6_LOCAL_FLV_OP_NEXT_CSID,
> +	__SEG6_LOCAL_FLV_OP_MAX
> +};
> +
> +#define SEG6_LOCAL_FLV_OP_MAX (__SEG6_LOCAL_FLV_OP_MAX - 1)
> +
>  #endif
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 5ea51ee2ef71..eb31c6c838e3 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -73,6 +73,39 @@ struct bpf_lwt_prog {
>  	char *name;
>  };
>  
> +/* default length values (expressed in bits) for both Locator-Block and
> + * Locator-Node Function.
> + *
> + * Both SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN *must* be:
> + *    i) greater than 0;
> + *   ii) evenly divisible by 8. In other terms, the lengths of the
> + *	 Locator-Block and Locator-Node Function must be byte-aligned (we can
> + *	 relax this constraint in the future if really needed).
> + *
> + * Moreover, a third condition must hold:
> + *  iii) SEG6_LOCAL_LCBLOCK_DLEN + SEG6_LOCAL_LCNODE_FN_DLEN <= 128.
> + *
> + * The correctness of SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN
> + * values are checked during the kernel compilation. If the compilation stops,
> + * check the value of these parameters to see if they meet conditions (i), (ii)
> + * and (iii).
> + */
> +#define SEG6_LOCAL_LCBLOCK_DLEN		32
> +#define SEG6_LOCAL_LCNODE_FN_DLEN	16
> +
> +/* Supported Flavor operations are reported in this bitmask */
> +#define SEG6_LOCAL_FLV_SUPP_OPS	(BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID))
> +
> +struct seg6_flavors_info {
> +	/* Flavor operations */
> +	__u32 flv_ops;
> +
> +	/* Locator-Block length, expressed in bits */
> +	__u8 lcblock_len;
> +	/* Locator-Node Function length, expressed in bits*/
> +	__u8 lcnode_func_len;

IMHO the above names are misleading. I suggest to use a '_bits' suffix
instead.

> +};
> +
>  enum seg6_end_dt_mode {
>  	DT_INVALID_MODE	= -EINVAL,
>  	DT_LEGACY_MODE	= 0,
> @@ -136,6 +169,8 @@ struct seg6_local_lwt {
>  #ifdef CONFIG_NET_L3_MASTER_DEV
>  	struct seg6_end_dt_info dt_info;
>  #endif
> +	struct seg6_flavors_info flv_info;
> +
>  	struct pcpu_seg6_local_counters __percpu *pcpu_counters;
>  
>  	int headroom;
> @@ -270,8 +305,50 @@ int seg6_lookup_nexthop(struct sk_buff *skb,
>  	return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false);
>  }
>  
> -/* regular endpoint function */
> -static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> +static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo)
> +{
> +	return finfo->lcblock_len >> 3;
> +}
> +
> +static __u8 seg6_flv_lcnode_func_octects(const struct seg6_flavors_info *finfo)
> +{
> +	return finfo->lcnode_func_len >> 3;
> +}
> +
> +static bool seg6_next_csid_is_arg_zero(const struct in6_addr *addr,
> +				       const struct seg6_flavors_info *finfo)
> +{
> +	__u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo);
> +	__u8 blk_octects = seg6_flv_lcblock_octects(finfo);
> +	__u8 arg_octects;
> +	int i;
> +
> +	arg_octects = 16 - blk_octects - fnc_octects;
> +	for (i = 0; i < arg_octects; ++i) {
> +		if (addr->s6_addr[blk_octects + fnc_octects + i] != 0x00)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +/* assume that DA.Argument length > 0 */
> +static void seg6_next_csid_advance_arg(struct in6_addr *addr,
> +				       const struct seg6_flavors_info *finfo)
> +{
> +	__u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo);
> +	__u8 blk_octects = seg6_flv_lcblock_octects(finfo);
> +
> +	/* advance DA.Argument */
> +	memmove((void *)&addr->s6_addr[blk_octects],
> +		(const void *)&addr->s6_addr[blk_octects + fnc_octects],
> +		16 - blk_octects - fnc_octects);

The void cast should not be needed

> +
> +	memset((void *)&addr->s6_addr[16 - fnc_octects], 0x00, fnc_octects);

Same here.

> +}
> +
> +static int input_action_end_core(struct sk_buff *skb,
> +				 struct seg6_local_lwt *slwt)
>  {
>  	struct ipv6_sr_hdr *srh;
>  
> @@ -290,6 +367,38 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
>  	return -EINVAL;
>  }
>  
> +static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> +{
> +	const struct seg6_flavors_info *finfo = &slwt->flv_info;
> +	struct in6_addr *daddr = &ipv6_hdr(skb)->daddr;
> +
> +	if (seg6_next_csid_is_arg_zero(daddr, finfo))
> +		return input_action_end_core(skb, slwt);
> +
> +	/* update DA */
> +	seg6_next_csid_advance_arg(daddr, finfo);
> +
> +	seg6_lookup_nexthop(skb, NULL, 0);
> +
> +	return dst_input(skb);
> +}
> +
> +static bool seg6_next_csid_enabled(__u32 fops)
> +{
> +	return fops & BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID);
> +}
> +
> +/* regular endpoint function */
> +static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> +{
> +	const struct seg6_flavors_info *finfo = &slwt->flv_info;
> +
> +	if (seg6_next_csid_enabled(finfo->flv_ops))
> +		return end_next_csid_core(skb, slwt);
> +
> +	return input_action_end_core(skb, slwt);
> +}
> +
>  /* regular endpoint, and forward to specified nexthop */
>  static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt)
>  {
> @@ -952,7 +1061,8 @@ static struct seg6_action_desc seg6_action_table[] = {
>  	{
>  		.action		= SEG6_LOCAL_ACTION_END,
>  		.attrs		= 0,
> -		.optattrs	= SEG6_F_LOCAL_COUNTERS,
> +		.optattrs	= SEG6_F_LOCAL_COUNTERS |
> +				  SEG6_F_ATTR(SEG6_LOCAL_FLAVORS),
>  		.input		= input_action_end,
>  	},
>  	{
> @@ -1133,6 +1243,7 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = {
>  	[SEG6_LOCAL_OIF]	= { .type = NLA_U32 },
>  	[SEG6_LOCAL_BPF]	= { .type = NLA_NESTED },
>  	[SEG6_LOCAL_COUNTERS]	= { .type = NLA_NESTED },
> +	[SEG6_LOCAL_FLAVORS]	= { .type = NLA_NESTED },
>  };
>  
>  static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt,
> @@ -1552,6 +1663,190 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt)
>  	free_percpu(slwt->pcpu_counters);
>  }
>  
> +static const
> +struct nla_policy seg6_local_flavors_policy[SEG6_LOCAL_FLV_MAX + 1] = {
> +	[SEG6_LOCAL_FLV_OPERATION]	= { .type = NLA_U32 },
> +	[SEG6_LOCAL_FLV_LCBLOCK_LEN]	= { .type = NLA_U8 },
> +	[SEG6_LOCAL_FLV_LCNODE_FN_LEN]	= { .type = NLA_U8 },
> +};
> +
> +/* check whether the lengths of the Locator-Block and Locator-Node Function
> + * are compatible with the dimension of a C-SID container.
> + */
> +static int seg6_chk_next_csid_cfg(__u8 block_len, __u8 func_len)
> +{
> +	/* Locator-Block and Locator-Node Function cannot exceed 128 bits */
> +	if (block_len + func_len > 128)
> +		return -EINVAL;
> +
> +	/* Locator-Block length must be greater than zero and evenly divisible
> +	 * by 8. There must be room for a Locator-Node Function, at least.
> +	 */
> +	if (block_len < 8 || block_len > 120 || (block_len & 0x07))

The 'block_len < 8' part is not needed, since you later check the 3
less significant bits can't be set and this is an unsigned number.

> +		return -EINVAL;
> +
> +	/* Locator-Node Function length must be greater than zero and evenly
> +	 * divisible by 8. There must be room for the Locator-Block.
> +	 */
> +	if (func_len < 8 || func_len > 120 || (func_len & 0x07))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int seg6_parse_nla_next_csid_cfg(struct nlattr **tb,
> +					struct seg6_flavors_info *finfo,
> +					struct netlink_ext_ack *extack)
> +{
> +	__u8 func_len = SEG6_LOCAL_LCNODE_FN_DLEN;
> +	__u8 block_len = SEG6_LOCAL_LCBLOCK_DLEN;
> +	int rc;
> +
> +	if (tb[SEG6_LOCAL_FLV_LCBLOCK_LEN])
> +		block_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]);
> +
> +	if (tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN])
> +		func_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]);
> +
> +	rc = seg6_chk_next_csid_cfg(block_len, func_len);
> +	if (rc < 0) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Invalid Locator Block/Node Function lengths");
> +		return rc;
> +	}
> +
> +	finfo->lcblock_len = block_len;
> +	finfo->lcnode_func_len = func_len;
> +
> +	return 0;
> +}
> +
> +static int parse_nla_flavors(struct nlattr **attrs, struct seg6_local_lwt *slwt,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct seg6_flavors_info *finfo = &slwt->flv_info;
> +	struct nlattr *tb[SEG6_LOCAL_FLV_MAX + 1];
> +	unsigned long fops;
> +	int rc;
> +
> +	rc = nla_parse_nested_deprecated(tb, SEG6_LOCAL_FLV_MAX,
> +					 attrs[SEG6_LOCAL_FLAVORS],
> +					 seg6_local_flavors_policy, NULL);
> +	if (rc < 0)
> +		return rc;
> +
> +	/* this attribute MUST always be present since it represents the Flavor
> +	 * operation(s) to carry out.
> +	 */
> +	if (!tb[SEG6_LOCAL_FLV_OPERATION])
> +		return -EINVAL;
> +
> +	fops = nla_get_u32(tb[SEG6_LOCAL_FLV_OPERATION]);
> +	if (~SEG6_LOCAL_FLV_SUPP_OPS & fops) {

Please avoid 'yoda-style' syntax. The compilar warnings will catch the
eventual mistakes this is supposed to avoid, and the conventional
syntax is more readable.

> +		NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	finfo->flv_ops = fops;
> +
> +	if (seg6_next_csid_enabled(fops)) {
> +		/* Locator-Block and Locator-Node Function lengths can be
> +		 * provided by the user space. If not, default values are going
> +		 * to be applied.
> +		 */
> +		rc = seg6_parse_nla_next_csid_cfg(tb, finfo, extack);
> +		if (rc < 0)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static int seg6_fill_nla_next_csid_cfg(struct sk_buff *skb,
> +				       struct seg6_flavors_info *finfo)
> +{
> +	if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCBLOCK_LEN, finfo->lcblock_len))
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCNODE_FN_LEN,
> +		       finfo->lcnode_func_len))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
> +static int put_nla_flavors(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> +{
> +	struct seg6_flavors_info *finfo = &slwt->flv_info;
> +	__u32 fops = finfo->flv_ops;
> +	struct nlattr *nest;
> +	int rc;
> +
> +	nest = nla_nest_start(skb, SEG6_LOCAL_FLAVORS);
> +	if (!nest)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u32(skb, SEG6_LOCAL_FLV_OPERATION, fops)) {
> +		rc = -EMSGSIZE;
> +		goto err;
> +	}
> +
> +	if (seg6_next_csid_enabled(fops)) {
> +		rc = seg6_fill_nla_next_csid_cfg(skb, finfo);
> +		if (rc < 0)
> +			goto err;
> +	}
> +
> +	return nla_nest_end(skb, nest);
> +
> +err:
> +	nla_nest_cancel(skb, nest);
> +	return rc;
> +}
> +
> +static int seg6_cmp_nla_next_csid_cfg(struct seg6_flavors_info *finfo_a,
> +				      struct seg6_flavors_info *finfo_b)
> +{
> +	if (finfo_a->lcblock_len != finfo_b->lcblock_len)
> +		return 1;
> +
> +	if (finfo_a->lcnode_func_len != finfo_b->lcnode_func_len)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int cmp_nla_flavors(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
> +{
> +	struct seg6_flavors_info *finfo_a = &a->flv_info;
> +	struct seg6_flavors_info *finfo_b = &b->flv_info;
> +
> +	if (finfo_a->flv_ops != finfo_b->flv_ops)
> +		return 1;
> +
> +	if (seg6_next_csid_enabled(finfo_a->flv_ops)) {
> +		if (seg6_cmp_nla_next_csid_cfg(finfo_a, finfo_b))
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int encap_size_flavors(struct seg6_local_lwt *slwt)
> +{
> +	struct seg6_flavors_info *finfo = &slwt->flv_info;
> +	int nlsize;
> +
> +	nlsize = nla_total_size(0) +	/* nest SEG6_LOCAL_FLAVORS */
> +		 nla_total_size(4);	/* SEG6_LOCAL_FLV_OPERATION */
> +
> +	if (seg6_next_csid_enabled(finfo->flv_ops))
> +		nlsize += nla_total_size(1) + /* SEG6_LOCAL_FLV_LCBLOCK_LEN */
> +			  nla_total_size(1);  /* SEG6_LOCAL_FLV_LCNODE_FN_LEN */
> +
> +	return nlsize;
> +}
> +
>  struct seg6_action_param {
>  	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt,
>  		     struct netlink_ext_ack *extack);
> @@ -1604,6 +1899,10 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
>  				    .put = put_nla_counters,
>  				    .cmp = cmp_nla_counters,
>  				    .destroy = destroy_attr_counters },
> +
> +	[SEG6_LOCAL_FLAVORS]	= { .parse = parse_nla_flavors,
> +				    .put = put_nla_flavors,
> +				    .cmp = cmp_nla_flavors },
>  };
>  
>  /* call the destroy() callback (if available) for each set attribute in
> @@ -1917,6 +2216,9 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt)
>  			  /* SEG6_LOCAL_CNT_ERRORS */
>  			  nla_total_size_64bit(sizeof(__u64));
>  
> +	if (attrs & SEG6_F_ATTR(SEG6_LOCAL_FLAVORS))
> +		nlsize += encap_size_flavors(slwt);
> +
>  	return nlsize;
>  }
>  
> @@ -1972,6 +2274,9 @@ int __init seg6_local_init(void)
>  	 */
>  	BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > BITS_PER_TYPE(unsigned long));
>  
> +	BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN,
> +					    SEG6_LOCAL_LCNODE_FN_DLEN) != 0);
> +

It looks like you are asking too much to the compiler with the above.
You can possibly resort open code the relevant test here.

It would be great if you could add some self-tests on top of the
iproute2 support.

Thanks!

Paolo

Re: [net-next v1 2/2] seg6: add NEXT-C-SID support for SRv6 End behavior

[Andrea Mayer]

Hi Paolo,
Thanks for your time. Please see my answers inline.

On Tue, 14 Jun 2022 12:52:42 +0200
Paolo Abeni <pabeni@redhat.com> wrote:

> On Sat, 2022-06-11 at 12:47 +0200, Andrea Mayer wrote:
> > The NEXT-C-SID mechanism described in [1] offers the possibility of
> > encoding several SRv6 segments within a single 128 bit SID address. Such
> > a SID address is called a Compressed SID (C-SID) container. In this way,
> > the length of the SID List can be drastically reduced.
> > 
> > A SID instantiated with the NEXT-C-SID flavor considers an IPv6 address
> > logically structured in three main blocks: i) Locator-Block; ii)
> > Locator-Node Function; iii) Argument.
> > 
> >                         C-SID container
> > +------------------------------------------------------------------+
> > >     Locator-Block      |Loc-Node|            Argument            |
> > >                        |Function|                                |
> > +------------------------------------------------------------------+
> > <--------- B -----------> <- NF -> <------------- A --------------->
> > 
> >    (i) The Locator-Block can be any IPv6 prefix available to the provider;
> > 
> >   (ii) The Locator-Node Function represents the node and the function to
> >        be triggered when a packet is received on the node;
> > 
> >  (iii) The Argument carries the remaining C-SIDs in the current C-SID
> >        container.
> > 
> > The NEXT-C-SID mechanism relies on the "flavors" framework defined in
> > [2]. The flavors represent additional operations that can modify or
> > extend a subset of the existing behaviors.
> > 
> > This patch introduces the support for flavors in SRv6 End behavior
> > implementing the NEXT-C-SID one. An SRv6 End behavior with NEXT-C-SID
> > flavor works as an End behavior but it is capable of processing the
> > compressed SID List encoded in C-SID containers.
> > 
> > An SRv6 End behavior with NEXT-C-SID flavor can be configured to support
> > user-provided Locator-Block and Locator-Node Function lengths. In this
> > implementation, such lengths must be evenly divisible by 8 (i.e. must be
> > byte-aligned), otherwise the kernel informs the user about invalid
> > values with a meaningful error code and message through netlink_ext_ack.
> > 
> > If Locator-Block and/or Locator-Node Function lengths are not provided
> > by the user during configuration of an SRv6 End behavior instance with
> > NEXT-C-SID flavor, the kernel will choose their default values i.e.,
> > 32-bit Locator-Block and 16-bit Locator-Node Function.
> > 
> > [1] - https://datatracker.ietf.org/doc/html/draft-ietf-spring-srv6-srh-compression
> > [2] - https://datatracker.ietf.org/doc/html/rfc8986
> > 
> > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
> > ---
> >  include/uapi/linux/seg6_local.h |  24 +++
> >  net/ipv6/seg6_local.c           | 311 +++++++++++++++++++++++++++++++-
> >  2 files changed, 332 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/uapi/linux/seg6_local.h b/include/uapi/linux/seg6_local.h
> > index 332b18f318f8..7919940c84d0 100644
> > --- a/include/uapi/linux/seg6_local.h
> > +++ b/include/uapi/linux/seg6_local.h
> > @@ -28,6 +28,7 @@ enum {
> >  	SEG6_LOCAL_BPF,
> >  	SEG6_LOCAL_VRFTABLE,
> >  	SEG6_LOCAL_COUNTERS,
> > +	SEG6_LOCAL_FLAVORS,
> >  	__SEG6_LOCAL_MAX,
> >  };
> >  #define SEG6_LOCAL_MAX (__SEG6_LOCAL_MAX - 1)
> > @@ -110,4 +111,27 @@ enum {
> >  
> >  #define SEG6_LOCAL_CNT_MAX (__SEG6_LOCAL_CNT_MAX - 1)
> >  
> > +/* SRv6 End* Flavor attributes */
> > +enum {
> > +	SEG6_LOCAL_FLV_UNSPEC,
> > +	SEG6_LOCAL_FLV_OPERATION,
> > +	SEG6_LOCAL_FLV_LCBLOCK_LEN,
> > +	SEG6_LOCAL_FLV_LCNODE_FN_LEN,
> > +	__SEG6_LOCAL_FLV_MAX,
> > +};
> > +
> > +#define SEG6_LOCAL_FLV_MAX (__SEG6_LOCAL_FLV_MAX - 1)
> > +
> > +/* Designed flavor operations for SRv6 End* Behavior */
> > +enum {
> > +	SEG6_LOCAL_FLV_OP_UNSPEC,
> > +	SEG6_LOCAL_FLV_OP_PSP,
> > +	SEG6_LOCAL_FLV_OP_USP,
> > +	SEG6_LOCAL_FLV_OP_USD,
> > +	SEG6_LOCAL_FLV_OP_NEXT_CSID,
> > +	__SEG6_LOCAL_FLV_OP_MAX
> > +};
> > +
> > +#define SEG6_LOCAL_FLV_OP_MAX (__SEG6_LOCAL_FLV_OP_MAX - 1)
> > +
> >  #endif
> > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> > index 5ea51ee2ef71..eb31c6c838e3 100644
> > --- a/net/ipv6/seg6_local.c
> > +++ b/net/ipv6/seg6_local.c
> > @@ -73,6 +73,39 @@ struct bpf_lwt_prog {
> >  	char *name;
> >  };
> >  
> > +/* default length values (expressed in bits) for both Locator-Block and
> > + * Locator-Node Function.
> > + *
> > + * Both SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN *must* be:
> > + *    i) greater than 0;
> > + *   ii) evenly divisible by 8. In other terms, the lengths of the
> > + *	 Locator-Block and Locator-Node Function must be byte-aligned (we can
> > + *	 relax this constraint in the future if really needed).
> > + *
> > + * Moreover, a third condition must hold:
> > + *  iii) SEG6_LOCAL_LCBLOCK_DLEN + SEG6_LOCAL_LCNODE_FN_DLEN <= 128.
> > + *
> > + * The correctness of SEG6_LOCAL_LCBLOCK_DLEN and SEG6_LOCAL_LCNODE_FN_DLEN
> > + * values are checked during the kernel compilation. If the compilation stops,
> > + * check the value of these parameters to see if they meet conditions (i), (ii)
> > + * and (iii).
> > + */
> > +#define SEG6_LOCAL_LCBLOCK_DLEN		32
> > +#define SEG6_LOCAL_LCNODE_FN_DLEN	16
> > +
> > +/* Supported Flavor operations are reported in this bitmask */
> > +#define SEG6_LOCAL_FLV_SUPP_OPS	(BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID))
> > +
> > +struct seg6_flavors_info {
> > +	/* Flavor operations */
> > +	__u32 flv_ops;
> > +
> > +	/* Locator-Block length, expressed in bits */
> > +	__u8 lcblock_len;
> > +	/* Locator-Node Function length, expressed in bits*/
> > +	__u8 lcnode_func_len;
> 
> IMHO the above names are misleading. I suggest to use a '_bits' suffix
> instead.
> 

Ok, that looks nice to me, e.g,: lcblock_len -> lcblock_bits.
I keep the other vars/macros consistent, so for instance:
SEG6_LOCAL_LCBLOCK_DLEN -> SEG6_LOCAL_LCBLOCK_DBITS.

We should also update labels such as SEG6_LOCAL_FLV_LCBLOCK_LEN and
SEG6_LOCAL_FLV_LCNODE_FN_LEN used in netlink messages.

> > +};
> > +
> >  enum seg6_end_dt_mode {
> >  	DT_INVALID_MODE	= -EINVAL,
> >  	DT_LEGACY_MODE	= 0,
> > @@ -136,6 +169,8 @@ struct seg6_local_lwt {
> >  #ifdef CONFIG_NET_L3_MASTER_DEV
> >  	struct seg6_end_dt_info dt_info;
> >  #endif
> > +	struct seg6_flavors_info flv_info;
> > +
> >  	struct pcpu_seg6_local_counters __percpu *pcpu_counters;
> >  
> >  	int headroom;
> > @@ -270,8 +305,50 @@ int seg6_lookup_nexthop(struct sk_buff *skb,
> >  	return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false);
> >  }
> >  
> > -/* regular endpoint function */
> > -static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > +static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo)
> > +{
> > +	return finfo->lcblock_len >> 3;
> > +}
> > +
> > +static __u8 seg6_flv_lcnode_func_octects(const struct seg6_flavors_info *finfo)
> > +{
> > +	return finfo->lcnode_func_len >> 3;
> > +}
> > +
> > +static bool seg6_next_csid_is_arg_zero(const struct in6_addr *addr,
> > +				       const struct seg6_flavors_info *finfo)
> > +{
> > +	__u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo);
> > +	__u8 blk_octects = seg6_flv_lcblock_octects(finfo);
> > +	__u8 arg_octects;
> > +	int i;
> > +
> > +	arg_octects = 16 - blk_octects - fnc_octects;
> > +	for (i = 0; i < arg_octects; ++i) {
> > +		if (addr->s6_addr[blk_octects + fnc_octects + i] != 0x00)
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/* assume that DA.Argument length > 0 */
> > +static void seg6_next_csid_advance_arg(struct in6_addr *addr,
> > +				       const struct seg6_flavors_info *finfo)
> > +{
> > +	__u8 fnc_octects = seg6_flv_lcnode_func_octects(finfo);
> > +	__u8 blk_octects = seg6_flv_lcblock_octects(finfo);
> > +
> > +	/* advance DA.Argument */
> > +	memmove((void *)&addr->s6_addr[blk_octects],
> > +		(const void *)&addr->s6_addr[blk_octects + fnc_octects],
> > +		16 - blk_octects - fnc_octects);
> 
> The void cast should not be needed

yes.

> 
> > +
> > +	memset((void *)&addr->s6_addr[16 - fnc_octects], 0x00, fnc_octects);
> 
> Same here.
> 

yes.

> > +}
> > +
> > +static int input_action_end_core(struct sk_buff *skb,
> > +				 struct seg6_local_lwt *slwt)
> >  {
> >  	struct ipv6_sr_hdr *srh;
> >  
> > @@ -290,6 +367,38 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> >  	return -EINVAL;
> >  }
> >  
> > +static int end_next_csid_core(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > +{
> > +	const struct seg6_flavors_info *finfo = &slwt->flv_info;
> > +	struct in6_addr *daddr = &ipv6_hdr(skb)->daddr;
> > +
> > +	if (seg6_next_csid_is_arg_zero(daddr, finfo))
> > +		return input_action_end_core(skb, slwt);
> > +
> > +	/* update DA */
> > +	seg6_next_csid_advance_arg(daddr, finfo);
> > +
> > +	seg6_lookup_nexthop(skb, NULL, 0);
> > +
> > +	return dst_input(skb);
> > +}
> > +
> > +static bool seg6_next_csid_enabled(__u32 fops)
> > +{
> > +	return fops & BIT(SEG6_LOCAL_FLV_OP_NEXT_CSID);
> > +}
> > +
> > +/* regular endpoint function */
> > +static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > +{
> > +	const struct seg6_flavors_info *finfo = &slwt->flv_info;
> > +
> > +	if (seg6_next_csid_enabled(finfo->flv_ops))
> > +		return end_next_csid_core(skb, slwt);
> > +
> > +	return input_action_end_core(skb, slwt);
> > +}
> > +
> >  /* regular endpoint, and forward to specified nexthop */
> >  static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> >  {
> > @@ -952,7 +1061,8 @@ static struct seg6_action_desc seg6_action_table[] = {
> >  	{
> >  		.action		= SEG6_LOCAL_ACTION_END,
> >  		.attrs		= 0,
> > -		.optattrs	= SEG6_F_LOCAL_COUNTERS,
> > +		.optattrs	= SEG6_F_LOCAL_COUNTERS |
> > +				  SEG6_F_ATTR(SEG6_LOCAL_FLAVORS),
> >  		.input		= input_action_end,
> >  	},
> >  	{
> > @@ -1133,6 +1243,7 @@ static const struct nla_policy seg6_local_policy[SEG6_LOCAL_MAX + 1] = {
> >  	[SEG6_LOCAL_OIF]	= { .type = NLA_U32 },
> >  	[SEG6_LOCAL_BPF]	= { .type = NLA_NESTED },
> >  	[SEG6_LOCAL_COUNTERS]	= { .type = NLA_NESTED },
> > +	[SEG6_LOCAL_FLAVORS]	= { .type = NLA_NESTED },
> >  };
> >  
> >  static int parse_nla_srh(struct nlattr **attrs, struct seg6_local_lwt *slwt,
> > @@ -1552,6 +1663,190 @@ static void destroy_attr_counters(struct seg6_local_lwt *slwt)
> >  	free_percpu(slwt->pcpu_counters);
> >  }
> >  
> > +static const
> > +struct nla_policy seg6_local_flavors_policy[SEG6_LOCAL_FLV_MAX + 1] = {
> > +	[SEG6_LOCAL_FLV_OPERATION]	= { .type = NLA_U32 },
> > +	[SEG6_LOCAL_FLV_LCBLOCK_LEN]	= { .type = NLA_U8 },
> > +	[SEG6_LOCAL_FLV_LCNODE_FN_LEN]	= { .type = NLA_U8 },
> > +};
> > +
> > +/* check whether the lengths of the Locator-Block and Locator-Node Function
> > + * are compatible with the dimension of a C-SID container.
> > + */
> > +static int seg6_chk_next_csid_cfg(__u8 block_len, __u8 func_len)
> > +{
> > +	/* Locator-Block and Locator-Node Function cannot exceed 128 bits */
> > +	if (block_len + func_len > 128)
> > +		return -EINVAL;
> > +
> > +	/* Locator-Block length must be greater than zero and evenly divisible
> > +	 * by 8. There must be room for a Locator-Node Function, at least.
> > +	 */
> > +	if (block_len < 8 || block_len > 120 || (block_len & 0x07))
> 
> The 'block_len < 8' part is not needed, since you later check the 3
> less significant bits can't be set and this is an unsigned number.
> 

Ok, but I need to be sure that block_len must be different from zero. For this
reason, I will replace 'block_len < 8' with 'block_len == 0'.

> > +		return -EINVAL;
> > +
> > +	/* Locator-Node Function length must be greater than zero and evenly
> > +	 * divisible by 8. There must be room for the Locator-Block.
> > +	 */
> > +	if (func_len < 8 || func_len > 120 || (func_len & 0x07))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int seg6_parse_nla_next_csid_cfg(struct nlattr **tb,
> > +					struct seg6_flavors_info *finfo,
> > +					struct netlink_ext_ack *extack)
> > +{
> > +	__u8 func_len = SEG6_LOCAL_LCNODE_FN_DLEN;
> > +	__u8 block_len = SEG6_LOCAL_LCBLOCK_DLEN;
> > +	int rc;
> > +
> > +	if (tb[SEG6_LOCAL_FLV_LCBLOCK_LEN])
> > +		block_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCBLOCK_LEN]);
> > +
> > +	if (tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN])
> > +		func_len = nla_get_u8(tb[SEG6_LOCAL_FLV_LCNODE_FN_LEN]);
> > +
> > +	rc = seg6_chk_next_csid_cfg(block_len, func_len);
> > +	if (rc < 0) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Invalid Locator Block/Node Function lengths");
> > +		return rc;
> > +	}
> > +
> > +	finfo->lcblock_len = block_len;
> > +	finfo->lcnode_func_len = func_len;
> > +
> > +	return 0;
> > +}
> > +
> > +static int parse_nla_flavors(struct nlattr **attrs, struct seg6_local_lwt *slwt,
> > +			     struct netlink_ext_ack *extack)
> > +{
> > +	struct seg6_flavors_info *finfo = &slwt->flv_info;
> > +	struct nlattr *tb[SEG6_LOCAL_FLV_MAX + 1];
> > +	unsigned long fops;
> > +	int rc;
> > +
> > +	rc = nla_parse_nested_deprecated(tb, SEG6_LOCAL_FLV_MAX,
> > +					 attrs[SEG6_LOCAL_FLAVORS],
> > +					 seg6_local_flavors_policy, NULL);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	/* this attribute MUST always be present since it represents the Flavor
> > +	 * operation(s) to carry out.
> > +	 */
> > +	if (!tb[SEG6_LOCAL_FLV_OPERATION])
> > +		return -EINVAL;
> > +
> > +	fops = nla_get_u32(tb[SEG6_LOCAL_FLV_OPERATION]);
> > +	if (~SEG6_LOCAL_FLV_SUPP_OPS & fops) {
> 
> Please avoid 'yoda-style' syntax. The compilar warnings will catch the
> eventual mistakes this is supposed to avoid, and the conventional
> syntax is more readable.
> 

Ok, fine!

> > +		NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	finfo->flv_ops = fops;
> > +
> > +	if (seg6_next_csid_enabled(fops)) {
> > +		/* Locator-Block and Locator-Node Function lengths can be
> > +		 * provided by the user space. If not, default values are going
> > +		 * to be applied.
> > +		 */
> > +		rc = seg6_parse_nla_next_csid_cfg(tb, finfo, extack);
> > +		if (rc < 0)
> > +			return rc;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int seg6_fill_nla_next_csid_cfg(struct sk_buff *skb,
> > +				       struct seg6_flavors_info *finfo)
> > +{
> > +	if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCBLOCK_LEN, finfo->lcblock_len))
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_u8(skb, SEG6_LOCAL_FLV_LCNODE_FN_LEN,
> > +		       finfo->lcnode_func_len))
> > +		return -EMSGSIZE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int put_nla_flavors(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > +{
> > +	struct seg6_flavors_info *finfo = &slwt->flv_info;
> > +	__u32 fops = finfo->flv_ops;
> > +	struct nlattr *nest;
> > +	int rc;
> > +
> > +	nest = nla_nest_start(skb, SEG6_LOCAL_FLAVORS);
> > +	if (!nest)
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_u32(skb, SEG6_LOCAL_FLV_OPERATION, fops)) {
> > +		rc = -EMSGSIZE;
> > +		goto err;
> > +	}
> > +
> > +	if (seg6_next_csid_enabled(fops)) {
> > +		rc = seg6_fill_nla_next_csid_cfg(skb, finfo);
> > +		if (rc < 0)
> > +			goto err;
> > +	}
> > +
> > +	return nla_nest_end(skb, nest);
> > +
> > +err:
> > +	nla_nest_cancel(skb, nest);
> > +	return rc;
> > +}
> > +
> > +static int seg6_cmp_nla_next_csid_cfg(struct seg6_flavors_info *finfo_a,
> > +				      struct seg6_flavors_info *finfo_b)
> > +{
> > +	if (finfo_a->lcblock_len != finfo_b->lcblock_len)
> > +		return 1;
> > +
> > +	if (finfo_a->lcnode_func_len != finfo_b->lcnode_func_len)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmp_nla_flavors(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
> > +{
> > +	struct seg6_flavors_info *finfo_a = &a->flv_info;
> > +	struct seg6_flavors_info *finfo_b = &b->flv_info;
> > +
> > +	if (finfo_a->flv_ops != finfo_b->flv_ops)
> > +		return 1;
> > +
> > +	if (seg6_next_csid_enabled(finfo_a->flv_ops)) {
> > +		if (seg6_cmp_nla_next_csid_cfg(finfo_a, finfo_b))
> > +			return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int encap_size_flavors(struct seg6_local_lwt *slwt)
> > +{
> > +	struct seg6_flavors_info *finfo = &slwt->flv_info;
> > +	int nlsize;
> > +
> > +	nlsize = nla_total_size(0) +	/* nest SEG6_LOCAL_FLAVORS */
> > +		 nla_total_size(4);	/* SEG6_LOCAL_FLV_OPERATION */
> > +
> > +	if (seg6_next_csid_enabled(finfo->flv_ops))
> > +		nlsize += nla_total_size(1) + /* SEG6_LOCAL_FLV_LCBLOCK_LEN */
> > +			  nla_total_size(1);  /* SEG6_LOCAL_FLV_LCNODE_FN_LEN */
> > +
> > +	return nlsize;
> > +}
> > +
> >  struct seg6_action_param {
> >  	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt,
> >  		     struct netlink_ext_ack *extack);
> > @@ -1604,6 +1899,10 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
> >  				    .put = put_nla_counters,
> >  				    .cmp = cmp_nla_counters,
> >  				    .destroy = destroy_attr_counters },
> > +
> > +	[SEG6_LOCAL_FLAVORS]	= { .parse = parse_nla_flavors,
> > +				    .put = put_nla_flavors,
> > +				    .cmp = cmp_nla_flavors },
> >  };
> >  
> >  /* call the destroy() callback (if available) for each set attribute in
> > @@ -1917,6 +2216,9 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt)
> >  			  /* SEG6_LOCAL_CNT_ERRORS */
> >  			  nla_total_size_64bit(sizeof(__u64));
> >  
> > +	if (attrs & SEG6_F_ATTR(SEG6_LOCAL_FLAVORS))
> > +		nlsize += encap_size_flavors(slwt);
> > +
> >  	return nlsize;
> >  }
> >  
> > @@ -1972,6 +2274,9 @@ int __init seg6_local_init(void)
> >  	 */
> >  	BUILD_BUG_ON(SEG6_LOCAL_MAX + 1 > BITS_PER_TYPE(unsigned long));
> >  
> > +	BUILD_BUG_ON(seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN,
> > +					    SEG6_LOCAL_LCNODE_FN_DLEN) != 0);
> > +
> 
> It looks like you are asking too much to the compiler with the above.

Yes, indeed. Definitely, too much :-)

> You can possibly resort open code the relevant test here.
> 

Do you mean something like that?

int __init_seg6_local(void)
{
    [...]
    rc = seg6_chk_next_csid_cfg(SEG6_LOCAL_LCBLOCK_DLEN,
                                SEG6_LOCAL_LCNODE_FN_DLEN);
    if (rc < 0) {
        WARN(1, "seg6: wrong default Locator-Block/Node Function lengths!");
        return rc;
    }

    [...]
}

In this way, the developer who mangles these macros and recompiles the kernel
will not receive any warning at compilation time. Later on, when the kernel is
loaded, the initialization of the inet6 will fail with a log message in the
dmesg. I am not sure if this is a good failure mode.

A variant of this approach is to issue the warning in the dmesg but continue to
load the inet6. In this case if the developer changes the values in a wrong
way, the segment routing next csid will behave in a wrong way (not predictable).

---

Another possibility is to skip any check, since these two macro values are not
supposed to be changed; and if they are changed it is at the risk of the
developer. 

---

There is another possibility that I am trying to implement, which is to create
three small macros to use in seg6_chk_next_csid_cfg(...) and also with
BUILD_BUG_ON(...).

Something like that:

#define __ncsid_chk_bits(blen, flen)	      \
        ((blen) + (flen) > 128)

#define __ncsid_chk_lcblock_bits(blen)        \
        (!(blen) || (blen) > 120 || ((blen) & 0x07))

#define __ncsid_chk_lcnode_fn_bits(flen)      \
        __ncsid_chk_lcblock_bits(flen)

int __init_seg6_local(void)
{
    [...]

    BUILD_BUG_ON(__ncsid_chk_bits(SEG6_LOCAL_LCBLOCK_DLEN,
                                  SEG6_LOCAL_LCNODE_FN_DLEN));
    BUILD_BUG_ON(__ncsid_chk_lcblock_bits(SEG6_LOCAL_LCBLOCK_DLEN));
    BUILD_BUG_ON(__ncsid_chk_lcnode_fn_bits(SEG6_LOCAL_LCNODE_FN_DLEN));

    [...]
}

Since these are only and exclusively macros (and by the way they are very
simple), there should be no problems at compile time.
This approach looks promising to me.

What do you think is the best approach to take in a case like that?

> It would be great if you could add some self-tests on top of the
> iproute2 support.
> 

yes for sure ;-) I will add them to v2.

> Thanks!
> 

you're welcome! and thanks again for your help.

Andrea