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

[Andrea Mayer]

Hi Jakub,
many thanks for your review. Please see my responses inline:

On Tue, 10 Nov 2020 14:50:21 -0800
Jakub Kicinski <kuba@xxxxxxxxxx> 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