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,