| 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: <20210529140601.1ab9d40e@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date: Sat, 29 May 2021 14:06:01 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Justin Iurman <justin.iurman@...ege.be>
Cc: netdev@...r.kernel.org, davem@...emloft.net, tom@...bertland.com
Subject: Re: [PATCH net-next v4 3/5] ipv6: ioam: IOAM Generic Netlink API
On Thu, 27 May 2021 17:16:50 +0200 Justin Iurman wrote:
> Add Generic Netlink commands to allow userspace to configure IOAM
> namespaces and schemas. The target is iproute2 and the patch is ready.
> It will be posted as soon as this patchset is merged. Here is an overview:
>
> $ ip ioam
> Usage: ip ioam { COMMAND | help }
> ip ioam namespace show
> ip ioam namespace add ID [ DATA ]
> ip ioam namespace del ID
> ip ioam schema show
> ip ioam schema add ID DATA
> ip ioam schema del ID
> ip ioam namespace set ID schema { ID | none }
>
> Signed-off-by: Justin Iurman <justin.iurman@...ege.be>
> +static int ioam6_genl_addns(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct ioam6_pernet_data *nsdata;
> + struct ioam6_namespace *ns;
> + __be16 ns_id;
> + int err;
> +
> + if (!info->attrs[IOAM6_ATTR_NS_ID])
> + return -EINVAL;
> +
> + ns_id = cpu_to_be16(nla_get_u16(info->attrs[IOAM6_ATTR_NS_ID]));
> + nsdata = ioam6_pernet(genl_info_net(info));
> +
> + mutex_lock(&nsdata->lock);
> +
> + ns = rhashtable_lookup_fast(&nsdata->namespaces, &ns_id, rht_ns_params);
> + if (ns) {
> + err = -EEXIST;
> + goto out_unlock;
> + }
> +
> + ns = kzalloc(sizeof(*ns), GFP_KERNEL);
> + if (!ns) {
> + err = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + ns->id = ns_id;
> +
> + if (!info->attrs[IOAM6_ATTR_NS_DATA]) {
> + ns->data = cpu_to_be64(IOAM6_EMPTY_u64);
> + } else {
> + ns->data = cpu_to_be64(
> + nla_get_u64(info->attrs[IOAM6_ATTR_NS_DATA]));
Store data in a temporary variable to avoid this long line.
> + }
braces unnecessary
> + err = rhashtable_lookup_insert_fast(&nsdata->namespaces, &ns->head,
> + rht_ns_params);
> + if (err)
> + kfree(ns);
> +
> +out_unlock:
> + mutex_unlock(&nsdata->lock);
> + return err;
> +}
> +
> +static int ioam6_genl_delns(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct ioam6_pernet_data *nsdata;
> + struct ioam6_namespace *ns;
> + struct ioam6_schema *sc;
> + __be16 ns_id;
> + int err;
> +
> + if (!info->attrs[IOAM6_ATTR_NS_ID])
> + return -EINVAL;
> +
> + ns_id = cpu_to_be16(nla_get_u16(info->attrs[IOAM6_ATTR_NS_ID]));
> + nsdata = ioam6_pernet(genl_info_net(info));
> +
> + mutex_lock(&nsdata->lock);
> +
> + ns = rhashtable_lookup_fast(&nsdata->namespaces, &ns_id, rht_ns_params);
> + if (!ns) {
> + err = -ENOENT;
> + goto out_unlock;
> + }
> +
> + sc = ns->schema;
> + err = rhashtable_remove_fast(&nsdata->namespaces, &ns->head,
> + rht_ns_params);
> + if (err)
> + goto out_unlock;
> +
> + if (sc)
> + sc->ns = NULL;
the sc <> ns pointers should be annotated with __rcu, and appropriate
accessors used. At the very least the need READ/WRITE_ONCE().
> + ioam6_ns_release(ns);
> +
> +out_unlock:
> + mutex_unlock(&nsdata->lock);
> + return err;
> +}
> +
> +static int ioam6_genl_addsc(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct ioam6_pernet_data *nsdata;
> + int len, len_aligned, err;
> + struct ioam6_schema *sc;
> + u32 sc_id;
> +
> + if (!info->attrs[IOAM6_ATTR_SC_ID] || !info->attrs[IOAM6_ATTR_SC_DATA])
> + return -EINVAL;
> +
> + sc_id = nla_get_u32(info->attrs[IOAM6_ATTR_SC_ID]);
> + nsdata = ioam6_pernet(genl_info_net(info));
> +
> + mutex_lock(&nsdata->lock);
> +
> + sc = rhashtable_lookup_fast(&nsdata->schemas, &sc_id, rht_sc_params);
> + if (sc) {
> + err = -EEXIST;
> + goto out_unlock;
> + }
> +
> + sc = kzalloc(sizeof(*sc), GFP_KERNEL);
> + if (!sc) {
> + err = -ENOMEM;
> + goto out_unlock;
> + }
Why not store the data after the sc structure?
u8 data[] + struct_size()?
> + len = nla_len(info->attrs[IOAM6_ATTR_SC_DATA]);
> + len_aligned = ALIGN(len, 4);
> +
> + sc->data = kzalloc(len_aligned, GFP_KERNEL);
> + if (!sc->data) {
> + err = -ENOMEM;
> + goto free_sc;
> + }
> +
> + sc->id = sc_id;
> + sc->len = len_aligned;
> + sc->hdr = cpu_to_be32(sc->id | ((u8)(sc->len / 4) << 24));
> +
> + nla_memcpy(sc->data, info->attrs[IOAM6_ATTR_SC_DATA], len);
> +
> + err = rhashtable_lookup_insert_fast(&nsdata->schemas, &sc->head,
> + rht_sc_params);
> + if (err)
> + goto free_data;
> +
> +out_unlock:
> + mutex_unlock(&nsdata->lock);
> + return err;
> +free_data:
> + kfree(sc->data);
> +free_sc:
> + kfree(sc);
> + goto out_unlock;
> +}
> +
> +static int ioam6_genl_ns_set_schema(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct ioam6_pernet_data *nsdata;
> + struct ioam6_namespace *ns;
> + struct ioam6_schema *sc;
> + __be16 ns_id;
> + int err = 0;
No need to init.
> + u32 sc_id;
> +
> + if (!info->attrs[IOAM6_ATTR_NS_ID] ||
> + (!info->attrs[IOAM6_ATTR_SC_ID] &&
> + !info->attrs[IOAM6_ATTR_SC_NONE]))
> + return -EINVAL;
> +
> + ns_id = cpu_to_be16(nla_get_u16(info->attrs[IOAM6_ATTR_NS_ID]));
> + nsdata = ioam6_pernet(genl_info_net(info));
> +
> + mutex_lock(&nsdata->lock);
> +
> + ns = rhashtable_lookup_fast(&nsdata->namespaces, &ns_id, rht_ns_params);
> + if (!ns) {
> + err = -ENOENT;
> + goto out_unlock;
> + }
> +
> + if (info->attrs[IOAM6_ATTR_SC_NONE]) {
> + sc = NULL;
> + } else {
> + sc_id = nla_get_u32(info->attrs[IOAM6_ATTR_SC_ID]);
> + sc = rhashtable_lookup_fast(&nsdata->schemas, &sc_id,
> + rht_sc_params);
> + if (!sc) {
> + err = -ENOENT;
> + goto out_unlock;
> + }
> + }
> +
> + if (ns->schema)
> + ns->schema->ns = NULL;
> + ns->schema = sc;
> +
> + if (sc) {
> + if (sc->ns)
> + sc->ns->schema = NULL;
> + sc->ns = ns;
> + }
> +
> +out_unlock:
> + mutex_unlock(&nsdata->lock);
> + return err;
> +}
> +
> +static const struct genl_ops ioam6_genl_ops[] = {
> + {
> + .cmd = IOAM6_CMD_ADD_NAMESPACE,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = ioam6_genl_addns,
> + .flags = GENL_ADMIN_PERM,
> + },
> + {
> + .cmd = IOAM6_CMD_DEL_NAMESPACE,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = ioam6_genl_delns,
> + .flags = GENL_ADMIN_PERM,
> + },
> + {
> + .cmd = IOAM6_CMD_DUMP_NAMESPACES,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .start = ioam6_genl_dumpns_start,
> + .dumpit = ioam6_genl_dumpns,
> + .done = ioam6_genl_dumpns_done,
> + .flags = GENL_ADMIN_PERM,
> + },
> + {
> + .cmd = IOAM6_CMD_ADD_SCHEMA,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = ioam6_genl_addsc,
> + .flags = GENL_ADMIN_PERM,
> + },
> + {
> + .cmd = IOAM6_CMD_DEL_SCHEMA,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = ioam6_genl_delsc,
> + .flags = GENL_ADMIN_PERM,
> + },
> + {
> + .cmd = IOAM6_CMD_DUMP_SCHEMAS,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .start = ioam6_genl_dumpsc_start,
> + .dumpit = ioam6_genl_dumpsc,
> + .done = ioam6_genl_dumpsc_done,
> + .flags = GENL_ADMIN_PERM,
> + },
> + {
> + .cmd = IOAM6_CMD_NS_SET_SCHEMA,
> + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> + .doit = ioam6_genl_ns_set_schema,
> + .flags = GENL_ADMIN_PERM,
> + },
> +};
These days I think we should use policy tailored to each op, rather
than a single policy per family. That way we don't ignore any
attributes user may specify but kernel doesn't expect.
Powered by blists - more mailing lists