| 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: <152739558.34126899.1622373504535.JavaMail.zimbra@uliege.be>
Date: Sun, 30 May 2021 13:18:24 +0200 (CEST)
From: Justin Iurman <justin.iurman@...ege.be>
To: Jakub Kicinski <kuba@...nel.org>
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.
ACK, will do. The reason for it is that I didn't want to increase the stack of ioam6_genl_addns unnecessarily. But I'm OK with that.
>
>> + }
>
> braces unnecessary
ACK.
>
>> + 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().
I thought that, in this specific case, the mutex would be enough. Note that rcu is used everywhere else for both of them (see patch #2). Could you explain your reasoning? So I guess your comment also applies to other functions here (add/del, etc), where either ns or sc is accessed, right?
>
>> + 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()?
Indeed, an oversight. I'll move data after the ns pointer at the end of the sc struct and allocate it that way.
>
>> + 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.
ACK.
>
>> + 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.
Is it already implemented that way somewhere, just to give me an example?
Powered by blists - more mailing lists