diff mbox series

[net-next,v2,2/5] seg6: improve management of behavior attributes

Message ID 20201107153139.3552-3-andrea.mayer@uniroma2.it (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2,1/5] vrf: add mac header for tunneled packets when sniffer is attached | expand

Commit Message

Andrea Mayer Nov. 7, 2020, 3:31 p.m. UTC
Depending on the attribute (i.e.: SEG6_LOCAL_SRH, SEG6_LOCAL_TABLE, etc),
the parse() callback performs some validity checks on the provided input
and updates the tunnel state (slwt) with the result of the parsing
operation. However, an attribute may also need to reserve some additional
resources (i.e.: memory or setting up an eBPF program) in the parse()
callback to complete the parsing operation.

The parse() callbacks are invoked by the parse_nla_action() for each
attribute belonging to a specific behavior. Given a behavior with N
attributes, if the parsing of the i-th attribute fails, the
parse_nla_action() returns immediately with an error. Nonetheless, the
resources acquired during the parsing of the i-1 attributes are not freed
by the parse_nla_action().

Attributes which acquire resources must release them *in an explicit way*
in both the seg6_local_{build/destroy}_state(). However, adding a new
attribute of this type requires changes to
seg6_local_{build/destroy}_state() to release the resources correctly.

The seg6local infrastructure still lacks a simple and structured way to
release the resources acquired in the parse() operations.

We introduced a new callback in the struct seg6_action_param named
destroy(). This callback releases any resource which may have been acquired
in the parse() counterpart. Each attribute may or may not implement the
destroy() callback depending on whether it needs to free some acquired
resources.

The destroy() callback comes with several of advantages:

 1) we can have many attributes as we want for a given behavior with no
    need to explicitly free the taken resources;

 2) As in case of the seg6_local_build_state(), the
    seg6_local_destroy_state() does not need to handle the release of
    resources directly. Indeed, it calls the destroy_attrs() function which
    is in charge of calling the destroy() callback for every set attribute.
    We do not need to patch seg6_local_{build/destroy}_state() anymore as
    we add new attributes;

 3) the code is more readable and better structured. Indeed, all the
    information needed to handle a given attribute are contained in only
    one place;

 4) it facilitates the integration with new features introduced in further
    patches.

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

Comments

Jakub Kicinski Nov. 10, 2020, 10:50 p.m. UTC | #1
On Sat,  7 Nov 2020 16:31:36 +0100 Andrea Mayer wrote:
> Depending on the attribute (i.e.: SEG6_LOCAL_SRH, SEG6_LOCAL_TABLE, etc),
> the parse() callback performs some validity checks on the provided input
> and updates the tunnel state (slwt) with the result of the parsing
> operation. However, an attribute may also need to reserve some additional
> resources (i.e.: memory or setting up an eBPF program) in the parse()
> callback to complete the parsing operation.

Looks good, a few nit picks below.

> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index eba23279912d..63a82e2fdea9 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -710,6 +710,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;

This should never be called twice, right? No need for defensive
programming then.

> +}
> +
>  static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt)
>  {
>  	slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
> @@ -901,16 +907,33 @@ 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);

Same - why check if prog is NULL? That doesn't seem necessary if the
code is correct.

> +	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 which
> +	 * have been previously acquired 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,13 +957,68 @@ 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 available) for each set attribute in
> + * @parsed_attrs, starting from attribute index @start up to @end excluded.
> + */
> +static void __destroy_attrs(unsigned long parsed_attrs, int start, int end,

You always pass 0 as start, no need for that argument.

slwt and max_parsed should be the only args this function needs.

> +			    struct seg6_local_lwt *slwt)
> +{
> +	struct seg6_action_param *param;
> +	int i;
> +
> +	/* Every seg6local attribute is identified by an ID which is encoded as
> +	 * a flag (i.e: 1 << ID) in the @parsed_attrs bitmask; such bitmask
> +	 * keeps track of the attributes parsed so far.
> +
> +	 * We scan the @parsed_attrs bitmask, starting from the attribute
> +	 * identified by @start up to the attribute identified by @end
> +	 * excluded. For each set attribute, we retrieve the corresponding
> +	 * destroy() callback.
> +	 * If the callback is not available, then we skip to the next
> +	 * attribute; otherwise, we call the destroy() callback.
> +	 */
> +	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 may have been acquired during parsing
> + * operations.
> + */
> +static void destroy_attrs(struct seg6_local_lwt *slwt)
> +{
> +	struct seg6_action_desc *desc;
> +	unsigned long attrs;
> +
> +	desc = slwt->desc;
> +	if (!desc) {
> +		WARN_ONCE(1,
> +			  "seg6local: seg6_action_desc* for action %d is NULL",
> +			  slwt->action);
> +		return;
> +	}

Defensive programming?

> +
> +	/* get the attributes for the current behavior instance */
> +	attrs = desc->attrs;
> +
> +	__destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
> +}
> +
>  static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
>  {
>  	struct seg6_action_param *param;
> +	unsigned long parsed_attrs = 0;
>  	struct seg6_action_desc *desc;
>  	int i, err;
>  
> @@ -963,11 +1041,22 @@ 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 parsed correctly */
> +			parsed_attrs |= (1 << i);

Why do you need parsed_attrs, attributes are not optional. Everything
that's sepecified in desc->attrs and lower than i must had been parsed.

>  		}
>  	}
>  
>  	return 0;
> +
> +parse_err:
> +	/* release any resource that may have been acquired during the i-1
> +	 * parse() operations.
> +	 */
> +	__destroy_attrs(parsed_attrs, 0, i, slwt);
> +
> +	return err;
>  }
>  
>  static int seg6_local_build_state(struct net *net, struct nlattr *nla,
Andrea Mayer Nov. 13, 2020, 12:55 a.m. UTC | #2
Hi Jakub,
many thanks for your review. Please see my responses inline:

On Tue, 10 Nov 2020 14:50:21 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Sat,  7 Nov 2020 16:31:36 +0100 Andrea Mayer wrote:
> > Depending on the attribute (i.e.: SEG6_LOCAL_SRH, SEG6_LOCAL_TABLE, etc),
> > the parse() callback performs some validity checks on the provided input
> > and updates the tunnel state (slwt) with the result of the parsing
> > operation. However, an attribute may also need to reserve some additional
> > resources (i.e.: memory or setting up an eBPF program) in the parse()
> > callback to complete the parsing operation.
> 
> Looks good, a few nit picks below.
> 
> > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> > index eba23279912d..63a82e2fdea9 100644
> > --- a/net/ipv6/seg6_local.c
> > +++ b/net/ipv6/seg6_local.c
> > @@ -710,6 +710,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;
> 
> This should never be called twice, right? No need for defensive
> programming then.
>

Yes, the patch that I wrote does not call the function twice.
When I wrote the code my only concern was if someone (in the future) could ever
call the destroy_attr_srh() in a wrong way or in an inappropriate part of the code.
This choice was driven by an excess of paranoia rather than a real issue.

Given that, I will remove it with no problem at all in v3.

> > +}
> > +
> >  static int parse_nla_table(struct nlattr **attrs, struct seg6_local_lwt *slwt)
> >  {
> >  	slwt->table = nla_get_u32(attrs[SEG6_LOCAL_TABLE]);
> > @@ -901,16 +907,33 @@ 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);
> 
> Same - why check if prog is NULL? That doesn't seem necessary if the
> code is correct.
> 

Same as above.

> > +	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 which
> > +	 * have been previously acquired 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,13 +957,68 @@ 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 available) for each set attribute in
> > + * @parsed_attrs, starting from attribute index @start up to @end excluded.
> > + */
> > +static void __destroy_attrs(unsigned long parsed_attrs, int start, int end,
> 
> You always pass 0 as start, no need for that argument.
> 
> slwt and max_parsed should be the only args this function needs.
> 

My initial goal was to explicitly pass the 'parsed_attrs' as an argument so that
we can reuse this function also for further improvements (i.e.: the patch for
optional attributes I am working on).

However, for v3 I will keep the stuff straight forward following what you
suggested to me.

> > +			    struct seg6_local_lwt *slwt)
> > +{
> > +	struct seg6_action_param *param;
> > +	int i;
> > +
> > +	/* Every seg6local attribute is identified by an ID which is encoded as
> > +	 * a flag (i.e: 1 << ID) in the @parsed_attrs bitmask; such bitmask
> > +	 * keeps track of the attributes parsed so far.
> > +
> > +	 * We scan the @parsed_attrs bitmask, starting from the attribute
> > +	 * identified by @start up to the attribute identified by @end
> > +	 * excluded. For each set attribute, we retrieve the corresponding
> > +	 * destroy() callback.
> > +	 * If the callback is not available, then we skip to the next
> > +	 * attribute; otherwise, we call the destroy() callback.
> > +	 */
> > +	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 may have been acquired during parsing
> > + * operations.
> > + */
> > +static void destroy_attrs(struct seg6_local_lwt *slwt)
> > +{
> > +	struct seg6_action_desc *desc;
> > +	unsigned long attrs;
> > +
> > +	desc = slwt->desc;
> > +	if (!desc) {
> > +		WARN_ONCE(1,
> > +			  "seg6local: seg6_action_desc* for action %d is NULL",
> > +			  slwt->action);
> > +		return;
> > +	}
> 
> Defensive programming?
> 

Yes, like above. I will remove the check on the 'desc' and consequently the
WARN_ON in v3.

> > +
> > +	/* get the attributes for the current behavior instance */
> > +	attrs = desc->attrs;
> > +
> > +	__destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
> > +}
> > +
> >  static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
> >  {
> >  	struct seg6_action_param *param;
> > +	unsigned long parsed_attrs = 0;
> >  	struct seg6_action_desc *desc;
> >  	int i, err;
> >  
> > @@ -963,11 +1041,22 @@ 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 parsed correctly */
> > +			parsed_attrs |= (1 << i);
> 
> Why do you need parsed_attrs, attributes are not optional. Everything
> that's sepecified in desc->attrs and lower than i must had been parsed.
> 

Here, all the attributes are required and not optional. So in this patch, the
parsed_attrs can be certainly avoided. I'll remove it in v3.

> >  		}
> >  	}
> >  
> >  	return 0;
> > +
> > +parse_err:
> > +	/* release any resource that may have been acquired during the i-1
> > +	 * parse() operations.
> > +	 */
> > +	__destroy_attrs(parsed_attrs, 0, i, slwt);
> > +
> > +	return err;
> >  }
> >  
> >  static int seg6_local_build_state(struct net *net, struct nlattr *nla,
> 
> 

Thank you,
Andrea
diff mbox series

Patch

diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index eba23279912d..63a82e2fdea9 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -710,6 +710,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 +907,33 @@  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);
+
+	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 which
+	 * have been previously acquired 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,13 +957,68 @@  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 available) for each set attribute 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;
+
+	/* Every seg6local attribute is identified by an ID which is encoded as
+	 * a flag (i.e: 1 << ID) in the @parsed_attrs bitmask; such bitmask
+	 * keeps track of the attributes parsed so far.
+
+	 * We scan the @parsed_attrs bitmask, starting from the attribute
+	 * identified by @start up to the attribute identified by @end
+	 * excluded. For each set attribute, we retrieve the corresponding
+	 * destroy() callback.
+	 * If the callback is not available, then we skip to the next
+	 * attribute; otherwise, we call the destroy() callback.
+	 */
+	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 may have been acquired during parsing
+ * operations.
+ */
+static void destroy_attrs(struct seg6_local_lwt *slwt)
+{
+	struct seg6_action_desc *desc;
+	unsigned long attrs;
+
+	desc = slwt->desc;
+	if (!desc) {
+		WARN_ONCE(1,
+			  "seg6local: seg6_action_desc* for action %d is NULL",
+			  slwt->action);
+		return;
+	}
+
+	/* get the attributes for the current behavior instance */
+	attrs = desc->attrs;
+
+	__destroy_attrs(attrs, 0, SEG6_LOCAL_MAX + 1, slwt);
+}
+
 static int parse_nla_action(struct nlattr **attrs, struct seg6_local_lwt *slwt)
 {
 	struct seg6_action_param *param;
+	unsigned long parsed_attrs = 0;
 	struct seg6_action_desc *desc;
 	int i, err;
 
@@ -963,11 +1041,22 @@  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 parsed correctly */
+			parsed_attrs |= (1 << i);
 		}
 	}
 
 	return 0;
+
+parse_err:
+	/* release any resource that may have been acquired during the i-1
+	 * parse() operations.
+	 */
+	__destroy_attrs(parsed_attrs, 0, i, slwt);
+
+	return err;
 }
 
 static int seg6_local_build_state(struct net *net, struct nlattr *nla,
@@ -1012,7 +1101,6 @@  static int seg6_local_build_state(struct net *net, struct nlattr *nla,
 	return 0;
 
 out_free:
-	kfree(slwt->srh);
 	kfree(newts);
 	return err;
 }
@@ -1021,12 +1109,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);
-	}
+	destroy_attrs(slwt);
 
 	return;
 }