Re: [PATCH net-next v4 3/5] ipv6: ioam: IOAM Generic Netlink API

[Justin Iurman <justin.iurman@uliege.be>]

> 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@uliege.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?