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,