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
