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,
