| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201110145021.761f5ee7@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date: Tue, 10 Nov 2020 14:50:21 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Andrea Mayer <andrea.mayer@...roma2.it>
Cc: "David S. Miller" <davem@...emloft.net>,
David Ahern <dsahern@...nel.org>,
Alexey Kuznetsov <kuznet@....inr.ac.ru>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
Shuah Khan <shuah@...nel.org>,
Shrijeet Mukherjee <shrijeet@...il.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...omium.org>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
Stefano Salsano <stefano.salsano@...roma2.it>,
Paolo Lungaroni <paolo.lungaroni@...t.it>,
Ahmed Abdelsalam <ahabdels.dev@...il.com>
Subject: Re: [net-next,v2,2/5] seg6: improve management of behavior
attributes
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,
Powered by blists - more mailing lists