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

[Jakub Kicinski]

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,