[net-next] seg6: add support for optional attributes during behavior construction

[net-next] seg6: add support for optional attributes during behavior construction

[Andrea Mayer]

before this patch, each SRv6 behavior specifies a set of required
attributes that must be provided by the userspace application when the
behavior is created. If an attribute is not supplied, the creation
operation fails.
As a workaround, if an attribute is not needed by a behavior, it requires
to be set by the userspace application to a conventional skip-value. The
kernel side, that processes the creation request of a behavior, reads the
supplied attribute values and checks if it has been set to the
conventional skip-value or not. Hence, each optional attribute must have a
conventional skip-value which is known a priori and shared between
userspace applications and kernel.

Messy code and complicated tricks may arise from this approach.
On the other hand, this patch explicitly differentiates the required
mandatory attributes from the optional ones. Now, each behavior can declare
a set of required attributes and a set of optional ones. The behavior
creation fails in case a required attribute is missing, while it goes on
without generating any issue if an optional attribute is not supplied by
the userspace application.

To properly combine the required and optional attributes, a new callback
function called destroy() is used for releasing resources that have been
acquired, during the parse() operation, by a given attribute.
However, the destroy() function is optional and if an attribute does not
require resources that have to be later released, the callback can be
omitted.

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

diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 85a5447a3e8d..480f1ab35221 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -7,6 +7,13 @@
  *  eBPF support: Mathieu Xhonneux <m.xhonneux@gmail.com>
  */
 
+/* Changes:
+ *
+ * Andrea Mayer <andrea.mayer@uniroma2.it>
+ *	add support for optional attributes during behavior construction
+ *
+ */
+
 #include <linux/types.h>
 #include <linux/skbuff.h>
 #include <linux/net.h>
@@ -34,7 +41,21 @@ struct seg6_local_lwt;
 
 struct seg6_action_desc {
 	int action;
-	unsigned long attrs;
+	unsigned long required_attrs;
+
+	/* optional_attrs is used to specify attributes which can be defined
+	 * as optional attributes (also called optional parameters). If one of
+	 * these attributes is not present in the netlink msg during the
+	 * behavior creation, no errors will be returned to the userland (as
+	 * opposed to a missing required_attrs, where indeed a -EINVAL error
+	 * is returned to userland).
+	 *
+	 * Each attribute can be 1) required or 2) optional. Anyway, if the
+	 * attribute is set in both ways then it is considered to be only
+	 * required.
+	 */
+	unsigned long optional_attrs;
+
 	int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int static_headroom;
 };
@@ -56,6 +77,9 @@ struct seg6_local_lwt {
 
 	int headroom;
 	struct seg6_action_desc *desc;
+
+	/* parsed optional attributes */
+	unsigned long parsed_optional_attrs;
 };
 
 static struct seg6_local_lwt *seg6_local_lwtunnel(struct lwtunnel_state *lwt)
@@ -559,53 +583,53 @@ static int input_action_end_bpf(struct sk_buff *skb,
 static struct seg6_action_desc seg6_action_table[] = {
 	{
 		.action		= SEG6_LOCAL_ACTION_END,
-		.attrs		= 0,
+		.required_attrs	= 0,
 		.input		= input_action_end,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_X,
-		.attrs		= (1 << SEG6_LOCAL_NH6),
+		.required_attrs	= (1 << SEG6_LOCAL_NH6),
 		.input		= input_action_end_x,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_T,
-		.attrs		= (1 << SEG6_LOCAL_TABLE),
+		.required_attrs	= (1 << SEG6_LOCAL_TABLE),
 		.input		= input_action_end_t,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX2,
-		.attrs		= (1 << SEG6_LOCAL_OIF),
+		.required_attrs	= (1 << SEG6_LOCAL_OIF),
 		.input		= input_action_end_dx2,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX6,
-		.attrs		= (1 << SEG6_LOCAL_NH6),
+		.required_attrs	= (1 << SEG6_LOCAL_NH6),
 		.input		= input_action_end_dx6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX4,
-		.attrs		= (1 << SEG6_LOCAL_NH4),
+		.required_attrs	= (1 << SEG6_LOCAL_NH4),
 		.input		= input_action_end_dx4,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DT6,
-		.attrs		= (1 << SEG6_LOCAL_TABLE),
+		.required_attrs	= (1 << SEG6_LOCAL_TABLE),
 		.input		= input_action_end_dt6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_B6,
-		.attrs		= (1 << SEG6_LOCAL_SRH),
+		.required_attrs	= (1 << SEG6_LOCAL_SRH),
 		.input		= input_action_end_b6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_B6_ENCAP,
-		.attrs		= (1 << SEG6_LOCAL_SRH),
+		.required_attrs	= (1 << SEG6_LOCAL_SRH),
 		.input		= input_action_end_b6_encap,
 		.static_headroom	= sizeof(struct ipv6hdr),
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_BPF,
-		.attrs		= (1 << SEG6_LOCAL_BPF),
+		.required_attrs	= (1 << SEG6_LOCAL_BPF),
 		.input		= input_action_end_bpf,
 	},
 
@@ -708,6 +732,12 @@ static int cmp_nla_srh(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return memcmp(a->srh, b->srh, len);
 }
 
+static void destroy_attr_srh(struct seg6_local_lwt *slwt)
+{
+	kfree(slwt->srh);
+	slwt->srh = NULL;
+}
+
 static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
 	slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
@@ -899,16 +929,36 @@ static int cmp_nla_bpf(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return strcmp(a->bpf.name, b->bpf.name);
 }
 
+static void destroy_attr_bpf(struct seg6_local_lwt *slwt)
+{
+	kfree(slwt->bpf.name);
+	if (slwt->bpf.prog)
+		bpf_prog_put(slwt->bpf.prog);
+
+	/* avoid to mess up everything if the function is called more
+	 * than once.
+	 */
+	slwt->bpf.name = NULL;
+	slwt->bpf.prog = NULL;
+}
+
 struct seg6_action_param {
 	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt);
 	int (*put)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int (*cmp)(struct seg6_local_lwt *a, struct seg6_local_lwt *b);
+
+	/* optional destroy() callback useful for releasing resources that
+	 * have been previously allocated in the corresponding parse()
+	 * function.
+	 */
+	void (*destroy)(struct seg6_local_lwt *slwt);
 };
 
 static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 	[SEG6_LOCAL_SRH]	= { .parse = parse_nla_srh,
 				    .put = put_nla_srh,
-				    .cmp = cmp_nla_srh },
+				    .cmp = cmp_nla_srh,
+				    .destroy = destroy_attr_srh },
 
 	[SEG6_LOCAL_TABLE]	= { .parse = parse_nla_table,
 				    .put = put_nla_table,
@@ -932,12 +982,96 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 
 	[SEG6_LOCAL_BPF]	= { .parse = parse_nla_bpf,
 				    .put = put_nla_bpf,
-				    .cmp = cmp_nla_bpf },
+				    .cmp = cmp_nla_bpf,
+				    .destroy = destroy_attr_bpf	},
 
 };
 
+/* call the destroy() callback, if any, for each attribute set in
+ * @parsed_attrs, starting from attribute index @start up to @end excluded.
+ */
+static void __destroy_attrs(unsigned long parsed_attrs, int start, int end,
+			    struct seg6_local_lwt *slwt)
+{
+	struct seg6_action_param *param;
+	int i;
+
+	for (i = start; i < end; i++) {
+		if (!(parsed_attrs & (1 << i)))
+			continue;
+
+		param = &seg6_action_params[i];
+
+		if (param->destroy)
+			param->destroy(slwt);
+	}
+}
+
+/* release all the resources that have been possibly taken by attributes
+ * during parsing operations.
+ */
+static void destroy_attrs(struct seg6_local_lwt *slwt)
+{
+	unsigned long attrs;
+
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
+
+	__destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
+}
+
+/* optional attributes differ from the required (mandatory) ones because they
+ * can be or they cannot be present at all. If an attribute is declared but is
+ * not given then it will simply be discarded without generating any error.
+ */
+static int parse_nla_optional_attrs(struct nlattr **attrs,
+				    struct seg6_local_lwt *slwt)
+{
+	unsigned long optional_attrs, parsed_optional_attrs;
+	struct seg6_action_param *param;
+	struct seg6_action_desc *desc;
+	int i, err;
+
+	desc = slwt->desc;
+	parsed_optional_attrs = 0;
+	optional_attrs = desc->optional_attrs;
+
+	if (!optional_attrs)
+		goto out;
+
+	/* we call the parse() function for each optional attribute.
+	 * note: required attributes have already been parsed.
+	 */
+	for (i = 0; i < SEG6_LOCAL_MAX + 1; ++i) {
+		if (!(optional_attrs & (1 << i)) || !attrs[i])
+			continue;
+
+		param = &seg6_action_params[i];
+
+		err = param->parse(attrs, slwt);
+		if (err < 0)
+			goto parse_err;
+
+		/* current attribute has been correctly parsed */
+		parsed_optional_attrs |= (1 << i);
+	}
+
+out:
+	slwt->parsed_optional_attrs = parsed_optional_attrs;
+
+	return 0;
+
+parse_err:
+	/* release any resource that has been possibly allocated during
+	 * successful parse() operations.
+	 */
+	__destroy_attrs(parsed_optional_attrs, 0, i, slwt);
+
+	return err;
+}
+
 static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
+	unsigned long parsed_required_attrs;
 	struct seg6_action_param *param;
 	struct seg6_action_desc *desc;
 	int i, err;
@@ -950,10 +1084,18 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 		return -EOPNOTSUPP;
 
 	slwt->desc = desc;
+	parsed_required_attrs = 0;
 	slwt->headroom += desc->static_headroom;
 
+	/* if an attribute is set both optional and required, then we consider
+	 * it only as a required one. Therefore, we adjust the optional_attrs
+	 * bit mask so that it cannot contain any required attribute when the
+	 * same has already been specified in the required_attrs bit mask.
+	 */
+	desc->optional_attrs &= ~desc->required_attrs;
+
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (desc->attrs & (1 << i)) {
+		if (desc->required_attrs & (1 << i)) {
 			if (!attrs[i])
 				return -EINVAL;
 
@@ -961,11 +1103,27 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 
 			err = param->parse(attrs, slwt);
 			if (err < 0)
-				return err;
+				goto parse_err;
+
+			/* current attribute has been correctly parsed */
+			parsed_required_attrs |= (1 << i);
 		}
 	}
 
+	/* if we support optional attributes, then we parse all of them */
+	err = parse_nla_optional_attrs(attrs, slwt);
+	if (err < 0)
+		goto parse_err;
+
 	return 0;
+
+parse_err:
+	/* release any resource that has been possibly allocated during
+	 * successful parse() operations.
+	 */
+	__destroy_attrs(parsed_required_attrs, 0, i, slwt);
+
+	return err;
 }
 
 static int seg6_local_build_state(struct nlattr *nla, unsigned int family,
@@ -1009,8 +1167,16 @@ static int seg6_local_build_state(struct nlattr *nla, unsigned int family,
 	return 0;
 
 out_free:
-	kfree(slwt->srh);
+	/* parse_nla_action() is in charge of calling destroy_attrs() if,
+	 * during the parsing operation, something went wrong. However, if the
+	 * creation of the behavior fails after the parse_nla_action()
+	 * successfully returned, then destroy_attrs() MUST be called.
+	 *
+	 * Please, keep that in mind if you need to add more logic here after
+	 * the parse_nla_action().
+	 */
 	kfree(newts);
+
 	return err;
 }
 
@@ -1018,14 +1184,7 @@ static void seg6_local_destroy_state(struct lwtunnel_state *lwt)
 {
 	struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
 
-	kfree(slwt->srh);
-
-	if (slwt->desc->attrs & (1 << SEG6_LOCAL_BPF)) {
-		kfree(slwt->bpf.name);
-		bpf_prog_put(slwt->bpf.prog);
-	}
-
-	return;
+	destroy_attrs(slwt);
 }
 
 static int seg6_local_fill_encap(struct sk_buff *skb,
@@ -1033,13 +1192,20 @@ static int seg6_local_fill_encap(struct sk_buff *skb,
 {
 	struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
 	struct seg6_action_param *param;
+	unsigned long attrs;
 	int i, err;
 
 	if (nla_put_u32(skb, SEG6_LOCAL_ACTION, slwt->action))
 		return -EMSGSIZE;
 
+	/* the set of attributes is now made of two parts:
+	 *  1) required_attrs (the default attributes);
+	 *  2) a variable number of parsed optional_attrs.
+	 */
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
+
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (slwt->desc->attrs & (1 << i)) {
+		if (attrs & (1 << i)) {
 			param = &seg6_action_params[i];
 			err = param->put(skb, slwt);
 			if (err < 0)
@@ -1058,7 +1224,7 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt)
 
 	nlsize = nla_total_size(4); /* action */
 
-	attrs = slwt->desc->attrs;
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
 
 	if (attrs & (1 << SEG6_LOCAL_SRH))
 		nlsize += nla_total_size((slwt->srh->hdrlen + 1) << 3);
@@ -1091,6 +1257,7 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a,
 {
 	struct seg6_local_lwt *slwt_a, *slwt_b;
 	struct seg6_action_param *param;
+	unsigned long attrs_a, attrs_b;
 	int i;
 
 	slwt_a = seg6_local_lwtunnel(a);
@@ -1099,11 +1266,14 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a,
 	if (slwt_a->action != slwt_b->action)
 		return 1;
 
-	if (slwt_a->desc->attrs != slwt_b->desc->attrs)
+	attrs_a = slwt_a->desc->required_attrs | slwt_a->parsed_optional_attrs;
+	attrs_b = slwt_b->desc->required_attrs | slwt_b->parsed_optional_attrs;
+
+	if (attrs_a != attrs_b)
 		return 1;
 
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (slwt_a->desc->attrs & (1 << i)) {
+		if (attrs_a & (1 << i)) {
 			param = &seg6_action_params[i];
 			if (param->cmp(slwt_a, slwt_b))
 				return 1;
-- 
2.20.1

Re: [net-next] seg6: add support for optional attributes during behavior construction

[Leon Romanovsky]

On Mon, Feb 03, 2020 at 03:36:58PM +0100, Andrea Mayer wrote:
> before this patch, each SRv6 behavior specifies a set of required
> attributes that must be provided by the userspace application when the
> behavior is created. If an attribute is not supplied, the creation
> operation fails.
> As a workaround, if an attribute is not needed by a behavior, it requires
> to be set by the userspace application to a conventional skip-value. The
> kernel side, that processes the creation request of a behavior, reads the
> supplied attribute values and checks if it has been set to the
> conventional skip-value or not. Hence, each optional attribute must have a
> conventional skip-value which is known a priori and shared between
> userspace applications and kernel.
>
> Messy code and complicated tricks may arise from this approach.
> On the other hand, this patch explicitly differentiates the required
> mandatory attributes from the optional ones. Now, each behavior can declare
> a set of required attributes and a set of optional ones. The behavior
> creation fails in case a required attribute is missing, while it goes on
> without generating any issue if an optional attribute is not supplied by
> the userspace application.
>
> To properly combine the required and optional attributes, a new callback
> function called destroy() is used for releasing resources that have been
> acquired, during the parse() operation, by a given attribute.
> However, the destroy() function is optional and if an attribute does not
> require resources that have to be later released, the callback can be
> omitted.
>
> Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
> ---
>  net/ipv6/seg6_local.c | 226 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 198 insertions(+), 28 deletions(-)
>
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index 85a5447a3e8d..480f1ab35221 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -7,6 +7,13 @@
>   *  eBPF support: Mathieu Xhonneux <m.xhonneux@gmail.com>
>   */
>
> +/* Changes:
> + *
> + * Andrea Mayer <andrea.mayer@uniroma2.it>
> + *	add support for optional attributes during behavior construction
> + *
> + */

The lines above look strange in 2020 when all of us are using git.

Thanks

Re: [net-next] seg6: add support for optional attributes during behavior construction

[Andrea Mayer]

On Mon, 3 Feb 2020 17:08:00 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> On Mon, Feb 03, 2020 at 03:36:58PM +0100, Andrea Mayer wrote:
> > before this patch, each SRv6 behavior specifies a set of required
> > [...]
> >
> > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> > index 85a5447a3e8d..480f1ab35221 100644
> > --- a/net/ipv6/seg6_local.c
> > +++ b/net/ipv6/seg6_local.c
> > @@ -7,6 +7,13 @@
> >   *  eBPF support: Mathieu Xhonneux <m.xhonneux@gmail.com>
> >   */
> >
> > +/* Changes:
> > + *
> > + * Andrea Mayer <andrea.mayer@uniroma2.it>
> > + *	add support for optional attributes during behavior construction
> > + *
> > + */
> 
> The lines above look strange in 2020 when all of us are using git.
> 
> Thanks

Hi Leon,

We forgot to remove it from the patch. I will remove in the next revision. 
Thanks

-- 
Andrea Mayer <andrea.mayer@uniroma2.it>

Re: [net-next] seg6: add support for optional attributes during behavior construction

[Stefano Salsano]

Il 2020-03-31 02:49, David Miller ha scritto:
> From: Stefano Salsano <stefano.salsano@uniroma2.it>
> Date: Tue, 31 Mar 2020 01:23:48 +0200
> 
>> Of course a new application (e.g. iproute2, pyroute) using a new optional
>> parameter will not work on older kernels, but simply because the new parameter
>> is not supported. It will not work even without our proposed patch.
>>
>> On the other hand, we think that the solution in the patch is more backward
>> compatible. Without the patch, if we define new attributes, old applications
>> (e.g. iproute2 scripts) will not work on newer kernels, while with the optional
>> attributes approach proposed in the patch they will work with no issues !
> 
> Translation: You want to add backwards compatibility problems because
> otherwise you'll have to add backwards compatibility problems.

no this is not the correct translation :-) we do not want to add any 
backward compatility problem

we need to add a number of new parameters, if we keep the current 
approach these parameters will be mandatory and we will have backward 
compatibility problems: old applications will not work with new kernels

if we are allowed to add optional parameters, old applications and new 
applications will be able to work with old kernels and new kernels in 
any combination

> Sorry, I'm still not convinced.
> 
> You must find another way to achieve your objective.


-- 
*******************************************************************
Stefano Salsano
Professore Associato
Dipartimento Ingegneria Elettronica
Universita' di Roma Tor Vergata
Viale Politecnico, 1 - 00133 Roma - ITALY

http://netgroup.uniroma2.it/Stefano_Salsano/

E-mail  : stefano.salsano@uniroma2.it
Cell.   : +39 320 4307310
Office  : (Tel.) +39 06 72597770 (Fax.) +39 06 72597435
*******************************************************************

Re: [net-next] seg6: add support for optional attributes during behavior construction

[David Miller]

From: Stefano Salsano <stefano.salsano@uniroma2.it>
Date: Tue, 31 Mar 2020 01:23:48 +0200

> Of course a new application (e.g. iproute2, pyroute) using a new optional
> parameter will not work on older kernels, but simply because the new parameter
> is not supported. It will not work even without our proposed patch.
> 
> On the other hand, we think that the solution in the patch is more backward
> compatible. Without the patch, if we define new attributes, old applications
> (e.g. iproute2 scripts) will not work on newer kernels, while with the optional
> attributes approach proposed in the patch they will work with no issues !

Translation: You want to add backwards compatibility problems because
otherwise you'll have to add backwards compatibility problems.

Sorry, I'm still not convinced.

You must find another way to achieve your objective.

Re: [net-next] seg6: add support for optional attributes during behavior construction

[Stefano Salsano]

On Wed, 25 Mar 2020 19:30:16 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Andrea Mayer <andrea.mayer@uniroma2.it>
> Date: Thu, 19 Mar 2020 19:36:41 +0100
> 
> > Messy code and complicated tricks may arise from this approach.
> 
> People taking advantage of this new flexibility will write
> applications that DO NOT WORK on older kernels.
> 
> I think we are therefore stuck with the current optional attribute
> semantics, sorry.

Dear David,

sorry we have provided this patch without giving enough context. 

We are planning several enhancements to the SRv6 kernel implementation to keep
it aligned with the IETF standardization evolution and to add important
features like monitoring (some examples below). So far, the implemented SRv6
behaviors require few mandatory attributes and the code has been implemented in
a naive way following this requirement. We believe it is important to overcome
the limitations of this implementation considering the requirements coming from
new SRv6 behaviors and features shown in the examples below.

To provide 100% backward compatibility, we are not going to use the proposed
optional attribute semantic on any of the currently defined attributes, so
there is no risk to write applications using the existing attributes that will
not work on older kernels, which is (wisely) your main concern. 

Of course a new application (e.g. iproute2, pyroute) using a new optional
parameter will not work on older kernels, but simply because the new parameter
is not supported. It will not work even without our proposed patch.

On the other hand, we think that the solution in the patch is more backward
compatible. Without the patch, if we define new attributes, old applications
(e.g. iproute2 scripts) will not work on newer kernels, while with the optional
attributes approach proposed in the patch they will work with no issues !

In the light of the above clarification, what is your opinion?

Hereafter we list the SRv6 use cases that benefit from the proposed patch. We
have patches that implement these use cases, do you think that we should submit
one or two of them to show how we use the optional parameters?

Thank you for your attention!
Stefano

4 examples of enhancements to SRv6 that require optional parameters

1) Enhancement to End.DX4 behavior to support explicit indication of outgoing
device: "oif" parameter used as optional in the context of End.DX4 behavior

 ip -6 route add 2001:db8::1 encap seg6local action End.DX4 nh4 1.2.3.4 dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End.DX4 nh4 1.2.3.4 oif eth1 dev eth0

2) Statistics (per behavior counting of packets/bytes/errors) : new "stats"
parameter used as optional in the context of any behavior

 ip -6 route add 2001:db8::1 encap seg6local action End.DT6 table 100 dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End.DT6 table 100 stats dev eth0
 
 ip -6 route add 2001:db8::1 encap seg6local action End.DX4 nh4 1.2.3.4 dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End.DX4 nh4 1.2.3.4 stats dev eth0
 
 ip -6 route add 2001:db8::1 encap seg6local action End.DX4 nh4 1.2.3.4 oif eth1 dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End.DX4 nh4 1.2.3.4 oif eth1 stats dev eth0

3) Flavors (as per sec. 4.16 of draft-ietf-spring-srv6-network-programming-14):
new "flavors" parameter used as optional in the context of End, End.X, End.T

 ip -6 route add 2001:db8::1 encap seg6local action End dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End flavors PSP dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End flavors USP dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End flavors USD dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End flavors PSP,USP,USD dev eth0
 
 ip -6 route add 2001:db8::1 encap seg6local action End.T table 100 dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action End.T table 100 flavors USP dev eth0

4) micro SID (draft-filsfils-spring-net-pgm-extension-srv6-usid-04): in the
context of the new behavior uN, new optional parameters "ubl" and "ul"

 ip -6 route add 2001:db8::1 encap seg6local action uN dev eth0
 ip -6 route add 2001:db8::1 encap seg6local action uN ubl 32 ul 16 dev eth0


-- 
*******************************************************************
Stefano Salsano
Professore Associato
Dipartimento Ingegneria Elettronica
Universita' di Roma Tor Vergata
Viale Politecnico, 1 - 00133 Roma - ITALY

http://netgroup.uniroma2.it/Stefano_Salsano/

E-mail  : stefano.salsano@uniroma2.it
Cell.   : +39 320 4307310
Office  : (Tel.) +39 06 72597770 (Fax.) +39 06 72597435
*******************************************************************

Re: [net-next] seg6: add support for optional attributes during behavior construction

[David Miller]

From: Andrea Mayer <andrea.mayer@uniroma2.it>
Date: Thu, 19 Mar 2020 19:36:41 +0100

> Messy code and complicated tricks may arise from this approach.

People taking advantage of this new flexibility will write
applications that DO NOT WORK on older kernels.

I think we are therefore stuck with the current optional attribute
semantics, sorry.

[net-next] seg6: add support for optional attributes during behavior construction

[Andrea Mayer]

before this patch, each SRv6 behavior specifies a set of required
attributes that must be provided by the userspace application when the
behavior is created. If an attribute is not supplied, the creation
operation fails.
As a workaround, if an attribute is not needed by a behavior, it requires
to be set by the userspace application to a conventional skip-value. The
kernel side, that processes the creation request of a behavior, reads the
supplied attribute values and checks if it has been set to the
conventional skip-value or not. Hence, each optional attribute must have a
conventional skip-value which is known a priori and shared between
userspace applications and kernel.

Messy code and complicated tricks may arise from this approach.
On the other hand, this patch explicitly differentiates the required
mandatory attributes from the optional ones. Now, each behavior can declare
a set of required attributes and a set of optional ones. The behavior
creation fails in case a required attribute is missing, while it goes on
without generating any issue if an optional attribute is not supplied by
the userspace application.

To properly combine the required and optional attributes, a new callback
function called destroy() is used for releasing resources that have been
acquired, during the parse() operation, by a given attribute.
However, the destroy() function is optional and if an attribute does not
require resources that have to be later released, the callback can be
omitted.

Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
Acked-by: Paolo Lungaroni <paolo.lungaroni@cnit.it>
---
 net/ipv6/seg6_local.c | 217 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 189 insertions(+), 28 deletions(-)

diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 7cbc19731997..d165743eeda2 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -35,7 +35,21 @@ struct seg6_local_lwt;
 
 struct seg6_action_desc {
 	int action;
-	unsigned long attrs;
+	unsigned long required_attrs;
+
+	/* optional_attrs is used to specify attributes which can be defined
+	 * as optional attributes (also called optional parameters). If one of
+	 * these attributes is not present in the netlink msg during the
+	 * behavior creation, no errors will be returned to the userland (as
+	 * opposed to a missing required_attrs, where indeed a -EINVAL error
+	 * is returned to userland).
+	 *
+	 * Each attribute can be 1) required or 2) optional. Anyway, if the
+	 * attribute is set in both ways then it is considered to be only
+	 * required.
+	 */
+	unsigned long optional_attrs;
+
 	int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int static_headroom;
 };
@@ -57,6 +71,9 @@ struct seg6_local_lwt {
 
 	int headroom;
 	struct seg6_action_desc *desc;
+
+	/* parsed optional attributes */
+	unsigned long parsed_optional_attrs;
 };
 
 static struct seg6_local_lwt *seg6_local_lwtunnel(struct lwtunnel_state *lwt)
@@ -561,53 +578,53 @@ static int input_action_end_bpf(struct sk_buff *skb,
 static struct seg6_action_desc seg6_action_table[] = {
 	{
 		.action		= SEG6_LOCAL_ACTION_END,
-		.attrs		= 0,
+		.required_attrs	= 0,
 		.input		= input_action_end,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_X,
-		.attrs		= (1 << SEG6_LOCAL_NH6),
+		.required_attrs	= (1 << SEG6_LOCAL_NH6),
 		.input		= input_action_end_x,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_T,
-		.attrs		= (1 << SEG6_LOCAL_TABLE),
+		.required_attrs	= (1 << SEG6_LOCAL_TABLE),
 		.input		= input_action_end_t,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX2,
-		.attrs		= (1 << SEG6_LOCAL_OIF),
+		.required_attrs	= (1 << SEG6_LOCAL_OIF),
 		.input		= input_action_end_dx2,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX6,
-		.attrs		= (1 << SEG6_LOCAL_NH6),
+		.required_attrs	= (1 << SEG6_LOCAL_NH6),
 		.input		= input_action_end_dx6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX4,
-		.attrs		= (1 << SEG6_LOCAL_NH4),
+		.required_attrs	= (1 << SEG6_LOCAL_NH4),
 		.input		= input_action_end_dx4,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DT6,
-		.attrs		= (1 << SEG6_LOCAL_TABLE),
+		.required_attrs	= (1 << SEG6_LOCAL_TABLE),
 		.input		= input_action_end_dt6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_B6,
-		.attrs		= (1 << SEG6_LOCAL_SRH),
+		.required_attrs	= (1 << SEG6_LOCAL_SRH),
 		.input		= input_action_end_b6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_B6_ENCAP,
-		.attrs		= (1 << SEG6_LOCAL_SRH),
+		.required_attrs	= (1 << SEG6_LOCAL_SRH),
 		.input		= input_action_end_b6_encap,
 		.static_headroom	= sizeof(struct ipv6hdr),
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_BPF,
-		.attrs		= (1 << SEG6_LOCAL_BPF),
+		.required_attrs	= (1 << SEG6_LOCAL_BPF),
 		.input		= input_action_end_bpf,
 	},
 
@@ -710,6 +727,12 @@ static int cmp_nla_srh(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return memcmp(a->srh, b->srh, len);
 }
 
+static void destroy_attr_srh(struct seg6_local_lwt *slwt)
+{
+	kfree(slwt->srh);
+	slwt->srh = NULL;
+}
+
 static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
 	slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
@@ -901,16 +924,36 @@ static int cmp_nla_bpf(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return strcmp(a->bpf.name, b->bpf.name);
 }
 
+static void destroy_attr_bpf(struct seg6_local_lwt *slwt)
+{
+	kfree(slwt->bpf.name);
+	if (slwt->bpf.prog)
+		bpf_prog_put(slwt->bpf.prog);
+
+	/* avoid to mess up everything if the function is called more
+	 * than once.
+	 */
+	slwt->bpf.name = NULL;
+	slwt->bpf.prog = NULL;
+}
+
 struct seg6_action_param {
 	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt);
 	int (*put)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int (*cmp)(struct seg6_local_lwt *a, struct seg6_local_lwt *b);
+
+	/* optional destroy() callback useful for releasing resources that
+	 * have been previously allocated in the corresponding parse()
+	 * function.
+	 */
+	void (*destroy)(struct seg6_local_lwt *slwt);
 };
 
 static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 	[SEG6_LOCAL_SRH]	= { .parse = parse_nla_srh,
 				    .put = put_nla_srh,
-				    .cmp = cmp_nla_srh },
+				    .cmp = cmp_nla_srh,
+				    .destroy = destroy_attr_srh },
 
 	[SEG6_LOCAL_TABLE]	= { .parse = parse_nla_table,
 				    .put = put_nla_table,
@@ -934,12 +977,96 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 
 	[SEG6_LOCAL_BPF]	= { .parse = parse_nla_bpf,
 				    .put = put_nla_bpf,
-				    .cmp = cmp_nla_bpf },
+				    .cmp = cmp_nla_bpf,
+				    .destroy = destroy_attr_bpf	},
 
 };
 
+/* call the destroy() callback, if any, for each attribute set in
+ * @parsed_attrs, starting from attribute index @start up to @end excluded.
+ */
+static void __destroy_attrs(unsigned long parsed_attrs, int start, int end,
+			    struct seg6_local_lwt *slwt)
+{
+	struct seg6_action_param *param;
+	int i;
+
+	for (i = start; i < end; i++) {
+		if (!(parsed_attrs & (1 << i)))
+			continue;
+
+		param = &seg6_action_params[i];
+
+		if (param->destroy)
+			param->destroy(slwt);
+	}
+}
+
+/* release all the resources that have been possibly taken by attributes
+ * during parsing operations.
+ */
+static void destroy_attrs(struct seg6_local_lwt *slwt)
+{
+	unsigned long attrs;
+
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
+
+	__destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
+}
+
+/* optional attributes differ from the required (mandatory) ones because they
+ * can be or they cannot be present at all. If an attribute is declared but is
+ * not given then it will simply be discarded without generating any error.
+ */
+static int parse_nla_optional_attrs(struct nlattr **attrs,
+				    struct seg6_local_lwt *slwt)
+{
+	unsigned long optional_attrs, parsed_optional_attrs;
+	struct seg6_action_param *param;
+	struct seg6_action_desc *desc;
+	int i, err;
+
+	desc = slwt->desc;
+	parsed_optional_attrs = 0;
+	optional_attrs = desc->optional_attrs;
+
+	if (!optional_attrs)
+		goto out;
+
+	/* we call the parse() function for each optional attribute.
+	 * note: required attributes have already been parsed.
+	 */
+	for (i = 0; i < SEG6_LOCAL_MAX + 1; ++i) {
+		if (!(optional_attrs & (1 << i)) || !attrs[i])
+			continue;
+
+		param = &seg6_action_params[i];
+
+		err = param->parse(attrs, slwt);
+		if (err < 0)
+			goto parse_err;
+
+		/* current attribute has been correctly parsed */
+		parsed_optional_attrs |= (1 << i);
+	}
+
+out:
+	slwt->parsed_optional_attrs = parsed_optional_attrs;
+
+	return 0;
+
+parse_err:
+	/* release any resource that has been possibly allocated during
+	 * successful parse() operations.
+	 */
+	__destroy_attrs(parsed_optional_attrs, 0, i, slwt);
+
+	return err;
+}
+
 static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
+	unsigned long parsed_required_attrs;
 	struct seg6_action_param *param;
 	struct seg6_action_desc *desc;
 	int i, err;
@@ -952,10 +1079,16 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 		return -EOPNOTSUPP;
 
 	slwt->desc = desc;
+	parsed_required_attrs = 0;
 	slwt->headroom += desc->static_headroom;
 
+	/* if an attribute is set both optional and required, then we consider
+	 * it only as a required one.
+	 */
+	desc->optional_attrs &= ~desc->required_attrs;
+
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (desc->attrs & (1 << i)) {
+		if (desc->required_attrs & (1 << i)) {
 			if (!attrs[i])
 				return -EINVAL;
 
@@ -963,11 +1096,27 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 
 			err = param->parse(attrs, slwt);
 			if (err < 0)
-				return err;
+				goto parse_err;
+
+			/* current attribute has been correctly parsed */
+			parsed_required_attrs |= (1 << i);
 		}
 	}
 
+	/* if we support optional attributes, then we parse all of them */
+	err = parse_nla_optional_attrs(attrs, slwt);
+	if (err < 0)
+		goto parse_err;
+
 	return 0;
+
+parse_err:
+	/* release any resource that has been possibly allocated during
+	 * successful parse() operations.
+	 */
+	__destroy_attrs(parsed_required_attrs, 0, i, slwt);
+
+	return err;
 }
 
 static int seg6_local_build_state(struct nlattr *nla, unsigned int family,
@@ -1011,8 +1160,16 @@ static int seg6_local_build_state(struct nlattr *nla, unsigned int family,
 	return 0;
 
 out_free:
-	kfree(slwt->srh);
+	/* parse_nla_action() is in charge of calling destroy_attrs() if,
+	 * during the parsing operation, something went wrong. However, if the
+	 * creation of the behavior fails after the parse_nla_action()
+	 * successfully returned, then destroy_attrs() MUST be called.
+	 *
+	 * Please, keep that in mind if you need to add more logic here after
+	 * the parse_nla_action().
+	 */
 	kfree(newts);
+
 	return err;
 }
 
@@ -1020,14 +1177,7 @@ static void seg6_local_destroy_state(struct lwtunnel_state *lwt)
 {
 	struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
 
-	kfree(slwt->srh);
-
-	if (slwt->desc->attrs & (1 << SEG6_LOCAL_BPF)) {
-		kfree(slwt->bpf.name);
-		bpf_prog_put(slwt->bpf.prog);
-	}
-
-	return;
+	destroy_attrs(slwt);
 }
 
 static int seg6_local_fill_encap(struct sk_buff *skb,
@@ -1035,13 +1185,20 @@ static int seg6_local_fill_encap(struct sk_buff *skb,
 {
 	struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
 	struct seg6_action_param *param;
+	unsigned long attrs;
 	int i, err;
 
 	if (nla_put_u32(skb, SEG6_LOCAL_ACTION, slwt->action))
 		return -EMSGSIZE;
 
+	/* the set of attributes is now made of two parts:
+	 *  1) required_attrs (the default attributes);
+	 *  2) a variable number of parsed optional_attrs.
+	 */
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
+
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (slwt->desc->attrs & (1 << i)) {
+		if (attrs & (1 << i)) {
 			param = &seg6_action_params[i];
 			err = param->put(skb, slwt);
 			if (err < 0)
@@ -1060,7 +1217,7 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt)
 
 	nlsize = nla_total_size(4); /* action */
 
-	attrs = slwt->desc->attrs;
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
 
 	if (attrs & (1 << SEG6_LOCAL_SRH))
 		nlsize += nla_total_size((slwt->srh->hdrlen + 1) << 3);
@@ -1093,6 +1250,7 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a,
 {
 	struct seg6_local_lwt *slwt_a, *slwt_b;
 	struct seg6_action_param *param;
+	unsigned long attrs_a, attrs_b;
 	int i;
 
 	slwt_a = seg6_local_lwtunnel(a);
@@ -1101,11 +1259,14 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a,
 	if (slwt_a->action != slwt_b->action)
 		return 1;
 
-	if (slwt_a->desc->attrs != slwt_b->desc->attrs)
+	attrs_a = slwt_a->desc->required_attrs | slwt_a->parsed_optional_attrs;
+	attrs_b = slwt_b->desc->required_attrs | slwt_b->parsed_optional_attrs;
+
+	if (attrs_a != attrs_b)
 		return 1;
 
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (slwt_a->desc->attrs & (1 << i)) {
+		if (attrs_a & (1 << i)) {
 			param = &seg6_action_params[i];
 			if (param->cmp(slwt_a, slwt_b))
 				return 1;
-- 
2.20.1

Re: [net-next] seg6: add support for optional attributes during behavior construction

[David Miller]

Please resubmit this when the net-next tree opens back up, thank you.

[net-next] seg6: add support for optional attributes during behavior construction

[Andrea Mayer]

before this patch, each SRv6 behavior specifies a set of required
attributes that must be provided by the userspace application when the
behavior is created. If an attribute is not supplied, the creation
operation fails.
As a workaround, if an attribute is not needed by a behavior, it requires
to be set by the userspace application to a conventional skip-value. The
kernel side, that processes the creation request of a behavior, reads the
supplied attribute values and checks if it has been set to the
conventional skip-value or not. Hence, each optional attribute must have a
conventional skip-value which is known a priori and shared between
userspace applications and kernel.

Messy code and complicated tricks may arise from this approach.
On the other hand, this patch explicitly differentiates the required
mandatory attributes from the optional ones. Now, each behavior can declare
a set of required attributes and a set of optional ones. The behavior
creation fails in case a required attribute is missing, while it goes on
without generating any issue if an optional attribute is not supplied by
the userspace application.

To properly combine the required and optional attributes, a new callback
function called destroy() is used for releasing resources that have been
acquired, during the parse() operation, by a given attribute.
However, the destroy() function is optional and if an attribute does not
require resources that have to be later released, the callback can be
omitted.

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

diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index 85a5447a3e8d..480f1ab35221 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -7,6 +7,13 @@
  *  eBPF support: Mathieu Xhonneux <m.xhonneux@gmail.com>
  */
 
+/* Changes:
+ *
+ * Andrea Mayer <andrea.mayer@uniroma2.it>
+ *	add support for optional attributes during behavior construction
+ *
+ */
+
 #include <linux/types.h>
 #include <linux/skbuff.h>
 #include <linux/net.h>
@@ -34,7 +41,21 @@ struct seg6_local_lwt;
 
 struct seg6_action_desc {
 	int action;
-	unsigned long attrs;
+	unsigned long required_attrs;
+
+	/* optional_attrs is used to specify attributes which can be defined
+	 * as optional attributes (also called optional parameters). If one of
+	 * these attributes is not present in the netlink msg during the
+	 * behavior creation, no errors will be returned to the userland (as
+	 * opposed to a missing required_attrs, where indeed a -EINVAL error
+	 * is returned to userland).
+	 *
+	 * Each attribute can be 1) required or 2) optional. Anyway, if the
+	 * attribute is set in both ways then it is considered to be only
+	 * required.
+	 */
+	unsigned long optional_attrs;
+
 	int (*input)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int static_headroom;
 };
@@ -56,6 +77,9 @@ struct seg6_local_lwt {
 
 	int headroom;
 	struct seg6_action_desc *desc;
+
+	/* parsed optional attributes */
+	unsigned long parsed_optional_attrs;
 };
 
 static struct seg6_local_lwt *seg6_local_lwtunnel(struct lwtunnel_state *lwt)
@@ -559,53 +583,53 @@ static int input_action_end_bpf(struct sk_buff *skb,
 static struct seg6_action_desc seg6_action_table[] = {
 	{
 		.action		= SEG6_LOCAL_ACTION_END,
-		.attrs		= 0,
+		.required_attrs	= 0,
 		.input		= input_action_end,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_X,
-		.attrs		= (1 << SEG6_LOCAL_NH6),
+		.required_attrs	= (1 << SEG6_LOCAL_NH6),
 		.input		= input_action_end_x,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_T,
-		.attrs		= (1 << SEG6_LOCAL_TABLE),
+		.required_attrs	= (1 << SEG6_LOCAL_TABLE),
 		.input		= input_action_end_t,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX2,
-		.attrs		= (1 << SEG6_LOCAL_OIF),
+		.required_attrs	= (1 << SEG6_LOCAL_OIF),
 		.input		= input_action_end_dx2,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX6,
-		.attrs		= (1 << SEG6_LOCAL_NH6),
+		.required_attrs	= (1 << SEG6_LOCAL_NH6),
 		.input		= input_action_end_dx6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DX4,
-		.attrs		= (1 << SEG6_LOCAL_NH4),
+		.required_attrs	= (1 << SEG6_LOCAL_NH4),
 		.input		= input_action_end_dx4,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_DT6,
-		.attrs		= (1 << SEG6_LOCAL_TABLE),
+		.required_attrs	= (1 << SEG6_LOCAL_TABLE),
 		.input		= input_action_end_dt6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_B6,
-		.attrs		= (1 << SEG6_LOCAL_SRH),
+		.required_attrs	= (1 << SEG6_LOCAL_SRH),
 		.input		= input_action_end_b6,
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_B6_ENCAP,
-		.attrs		= (1 << SEG6_LOCAL_SRH),
+		.required_attrs	= (1 << SEG6_LOCAL_SRH),
 		.input		= input_action_end_b6_encap,
 		.static_headroom	= sizeof(struct ipv6hdr),
 	},
 	{
 		.action		= SEG6_LOCAL_ACTION_END_BPF,
-		.attrs		= (1 << SEG6_LOCAL_BPF),
+		.required_attrs	= (1 << SEG6_LOCAL_BPF),
 		.input		= input_action_end_bpf,
 	},
 
@@ -708,6 +732,12 @@ static int cmp_nla_srh(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return memcmp(a->srh, b->srh, len);
 }
 
+static void destroy_attr_srh(struct seg6_local_lwt *slwt)
+{
+	kfree(slwt->srh);
+	slwt->srh = NULL;
+}
+
 static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
 	slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
@@ -899,16 +929,36 @@ static int cmp_nla_bpf(struct seg6_local_lwt *a, struct seg6_local_lwt *b)
 	return strcmp(a->bpf.name, b->bpf.name);
 }
 
+static void destroy_attr_bpf(struct seg6_local_lwt *slwt)
+{
+	kfree(slwt->bpf.name);
+	if (slwt->bpf.prog)
+		bpf_prog_put(slwt->bpf.prog);
+
+	/* avoid to mess up everything if the function is called more
+	 * than once.
+	 */
+	slwt->bpf.name = NULL;
+	slwt->bpf.prog = NULL;
+}
+
 struct seg6_action_param {
 	int (*parse)(struct nlattr **attrs, struct seg6_local_lwt *slwt);
 	int (*put)(struct sk_buff *skb, struct seg6_local_lwt *slwt);
 	int (*cmp)(struct seg6_local_lwt *a, struct seg6_local_lwt *b);
+
+	/* optional destroy() callback useful for releasing resources that
+	 * have been previously allocated in the corresponding parse()
+	 * function.
+	 */
+	void (*destroy)(struct seg6_local_lwt *slwt);
 };
 
 static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 	[SEG6_LOCAL_SRH]	= { .parse = parse_nla_srh,
 				    .put = put_nla_srh,
-				    .cmp = cmp_nla_srh },
+				    .cmp = cmp_nla_srh,
+				    .destroy = destroy_attr_srh },
 
 	[SEG6_LOCAL_TABLE]	= { .parse = parse_nla_table,
 				    .put = put_nla_table,
@@ -932,12 +982,96 @@ static struct seg6_action_param seg6_action_params[SEG6_LOCAL_MAX + 1] = {
 
 	[SEG6_LOCAL_BPF]	= { .parse = parse_nla_bpf,
 				    .put = put_nla_bpf,
-				    .cmp = cmp_nla_bpf },
+				    .cmp = cmp_nla_bpf,
+				    .destroy = destroy_attr_bpf	},
 
 };
 
+/* call the destroy() callback, if any, for each attribute set in
+ * @parsed_attrs, starting from attribute index @start up to @end excluded.
+ */
+static void __destroy_attrs(unsigned long parsed_attrs, int start, int end,
+			    struct seg6_local_lwt *slwt)
+{
+	struct seg6_action_param *param;
+	int i;
+
+	for (i = start; i < end; i++) {
+		if (!(parsed_attrs & (1 << i)))
+			continue;
+
+		param = &seg6_action_params[i];
+
+		if (param->destroy)
+			param->destroy(slwt);
+	}
+}
+
+/* release all the resources that have been possibly taken by attributes
+ * during parsing operations.
+ */
+static void destroy_attrs(struct seg6_local_lwt *slwt)
+{
+	unsigned long attrs;
+
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
+
+	__destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
+}
+
+/* optional attributes differ from the required (mandatory) ones because they
+ * can be or they cannot be present at all. If an attribute is declared but is
+ * not given then it will simply be discarded without generating any error.
+ */
+static int parse_nla_optional_attrs(struct nlattr **attrs,
+				    struct seg6_local_lwt *slwt)
+{
+	unsigned long optional_attrs, parsed_optional_attrs;
+	struct seg6_action_param *param;
+	struct seg6_action_desc *desc;
+	int i, err;
+
+	desc = slwt->desc;
+	parsed_optional_attrs = 0;
+	optional_attrs = desc->optional_attrs;
+
+	if (!optional_attrs)
+		goto out;
+
+	/* we call the parse() function for each optional attribute.
+	 * note: required attributes have already been parsed.
+	 */
+	for (i = 0; i < SEG6_LOCAL_MAX + 1; ++i) {
+		if (!(optional_attrs & (1 << i)) || !attrs[i])
+			continue;
+
+		param = &seg6_action_params[i];
+
+		err = param->parse(attrs, slwt);
+		if (err < 0)
+			goto parse_err;
+
+		/* current attribute has been correctly parsed */
+		parsed_optional_attrs |= (1 << i);
+	}
+
+out:
+	slwt->parsed_optional_attrs = parsed_optional_attrs;
+
+	return 0;
+
+parse_err:
+	/* release any resource that has been possibly allocated during
+	 * successful parse() operations.
+	 */
+	__destroy_attrs(parsed_optional_attrs, 0, i, slwt);
+
+	return err;
+}
+
 static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
+	unsigned long parsed_required_attrs;
 	struct seg6_action_param *param;
 	struct seg6_action_desc *desc;
 	int i, err;
@@ -950,10 +1084,18 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 		return -EOPNOTSUPP;
 
 	slwt->desc = desc;
+	parsed_required_attrs = 0;
 	slwt->headroom += desc->static_headroom;
 
+	/* if an attribute is set both optional and required, then we consider
+	 * it only as a required one. Therefore, we adjust the optional_attrs
+	 * bit mask so that it cannot contain any required attribute when the
+	 * same has already been specified in the required_attrs bit mask.
+	 */
+	desc->optional_attrs &= ~desc->required_attrs;
+
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (desc->attrs & (1 << i)) {
+		if (desc->required_attrs & (1 << i)) {
 			if (!attrs[i])
 				return -EINVAL;
 
@@ -961,11 +1103,27 @@ static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 
 			err = param->parse(attrs, slwt);
 			if (err < 0)
-				return err;
+				goto parse_err;
+
+			/* current attribute has been correctly parsed */
+			parsed_required_attrs |= (1 << i);
 		}
 	}
 
+	/* if we support optional attributes, then we parse all of them */
+	err = parse_nla_optional_attrs(attrs, slwt);
+	if (err < 0)
+		goto parse_err;
+
 	return 0;
+
+parse_err:
+	/* release any resource that has been possibly allocated during
+	 * successful parse() operations.
+	 */
+	__destroy_attrs(parsed_required_attrs, 0, i, slwt);
+
+	return err;
 }
 
 static int seg6_local_build_state(struct nlattr *nla, unsigned int family,
@@ -1009,8 +1167,16 @@ static int seg6_local_build_state(struct nlattr *nla, unsigned int family,
 	return 0;
 
 out_free:
-	kfree(slwt->srh);
+	/* parse_nla_action() is in charge of calling destroy_attrs() if,
+	 * during the parsing operation, something went wrong. However, if the
+	 * creation of the behavior fails after the parse_nla_action()
+	 * successfully returned, then destroy_attrs() MUST be called.
+	 *
+	 * Please, keep that in mind if you need to add more logic here after
+	 * the parse_nla_action().
+	 */
 	kfree(newts);
+
 	return err;
 }
 
@@ -1018,14 +1184,7 @@ static void seg6_local_destroy_state(struct lwtunnel_state *lwt)
 {
 	struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
 
-	kfree(slwt->srh);
-
-	if (slwt->desc->attrs & (1 << SEG6_LOCAL_BPF)) {
-		kfree(slwt->bpf.name);
-		bpf_prog_put(slwt->bpf.prog);
-	}
-
-	return;
+	destroy_attrs(slwt);
 }
 
 static int seg6_local_fill_encap(struct sk_buff *skb,
@@ -1033,13 +1192,20 @@ static int seg6_local_fill_encap(struct sk_buff *skb,
 {
 	struct seg6_local_lwt *slwt = seg6_local_lwtunnel(lwt);
 	struct seg6_action_param *param;
+	unsigned long attrs;
 	int i, err;
 
 	if (nla_put_u32(skb, SEG6_LOCAL_ACTION, slwt->action))
 		return -EMSGSIZE;
 
+	/* the set of attributes is now made of two parts:
+	 *  1) required_attrs (the default attributes);
+	 *  2) a variable number of parsed optional_attrs.
+	 */
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
+
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (slwt->desc->attrs & (1 << i)) {
+		if (attrs & (1 << i)) {
 			param = &seg6_action_params[i];
 			err = param->put(skb, slwt);
 			if (err < 0)
@@ -1058,7 +1224,7 @@ static int seg6_local_get_encap_size(struct lwtunnel_state *lwt)
 
 	nlsize = nla_total_size(4); /* action */
 
-	attrs = slwt->desc->attrs;
+	attrs = slwt->desc->required_attrs | slwt->parsed_optional_attrs;
 
 	if (attrs & (1 << SEG6_LOCAL_SRH))
 		nlsize += nla_total_size((slwt->srh->hdrlen + 1) << 3);
@@ -1091,6 +1257,7 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a,
 {
 	struct seg6_local_lwt *slwt_a, *slwt_b;
 	struct seg6_action_param *param;
+	unsigned long attrs_a, attrs_b;
 	int i;
 
 	slwt_a = seg6_local_lwtunnel(a);
@@ -1099,11 +1266,14 @@ static int seg6_local_cmp_encap(struct lwtunnel_state *a,
 	if (slwt_a->action != slwt_b->action)
 		return 1;
 
-	if (slwt_a->desc->attrs != slwt_b->desc->attrs)
+	attrs_a = slwt_a->desc->required_attrs | slwt_a->parsed_optional_attrs;
+	attrs_b = slwt_b->desc->required_attrs | slwt_b->parsed_optional_attrs;
+
+	if (attrs_a != attrs_b)
 		return 1;
 
 	for (i = 0; i < SEG6_LOCAL_MAX + 1; i++) {
-		if (slwt_a->desc->attrs & (1 << i)) {
+		if (attrs_a & (1 << i)) {
 			param = &seg6_action_params[i];
 			if (param->cmp(slwt_a, slwt_b))
 				return 1;
-- 
2.20.1