On Thu, Oct 03, 2019 at 02:58:03PM -0700, Tom Herbert wrote:
> From: Tom Herbert <t...@quantonium.net>
>
> Add a netlink interface to manage the TX TLV parameters. Managed
> parameters include those for validating and sending TLVs being sent
> such as alignment, TLV ordering, length limits, etc.
>
> Signed-off-by: Tom Herbert <t...@herbertland.com>
Hi Tom,
I am wondering if you considered including extack support in this code.
...
> diff --git a/include/uapi/linux/ipeh.h b/include/uapi/linux/ipeh.h
> index dbf0728..bac36a7 100644
> --- a/include/uapi/linux/ipeh.h
> +++ b/include/uapi/linux/ipeh.h
> @@ -21,4 +21,33 @@ enum {
> IPEH_TLV_PERM_MAX = IPEH_TLV_PERM_NO_CHECK
> };
>
> +/* NETLINK_GENERIC related info for IP TLVs */
> +
> +enum {
> + IPEH_TLV_ATTR_UNSPEC,
> + IPEH_TLV_ATTR_TYPE, /* u8, > 1 */
> + IPEH_TLV_ATTR_ORDER, /* u16 */
> + IPEH_TLV_ATTR_ADMIN_PERM, /* u8, perm value */
> + IPEH_TLV_ATTR_USER_PERM, /* u8, perm value */
My reading of struct tlv_tx_params is that admin_perm and user_perm are
2-bit entities whose valid values are currently 0, 1 and 2. Perhaps that
would be worth noting here in keeping with restrictions noted for other
attributes.
> + IPEH_TLV_ATTR_CLASS, /* u8, 3 bit flags */
> + IPEH_TLV_ATTR_ALIGN_MULT, /* u8, 1 to 16 */
> + IPEH_TLV_ATTR_ALIGN_OFF, /* u8, 0 to 15 */
> + IPEH_TLV_ATTR_MIN_DATA_LEN, /* u8 (option data length) */
> + IPEH_TLV_ATTR_MAX_DATA_LEN, /* u8 (option data length) */
> + IPEH_TLV_ATTR_DATA_LEN_MULT, /* u8, 1 to 16 */
> + IPEH_TLV_ATTR_DATA_LEN_OFF, /* u8, 0 to 15 */
> +
> + __IPEH_TLV_ATTR_MAX,
> +};
> +
> +#define IPEH_TLV_ATTR_MAX (__IPEH_TLV_ATTR_MAX - 1)
...
> diff --git a/net/ipv6/exthdrs_common.c b/net/ipv6/exthdrs_common.c
> index feaa4a6..e3c1f33 100644
> --- a/net/ipv6/exthdrs_common.c
> +++ b/net/ipv6/exthdrs_common.c
> @@ -454,6 +454,244 @@ int __ipeh_tlv_unset(struct tlv_param_table
> *tlv_param_table,
...
> +int ipeh_tlv_nl_cmd_set(struct tlv_param_table *tlv_param_table,
> + struct genl_family *tlv_nl_family,
> + struct sk_buff *skb, struct genl_info *info)
> +{
> + struct tlv_params new_params;
> + struct tlv_proc *tproc;
> + unsigned char type;
> + unsigned int v;
> + int retv = -EINVAL;
> +
> + if (!info->attrs[IPEH_TLV_ATTR_TYPE])
> + return -EINVAL;
> +
> + type = nla_get_u8(info->attrs[IPEH_TLV_ATTR_TYPE]);
> + if (type < 2)
> + return -EINVAL;
> +
> + rcu_read_lock();
> +
> + /* Base new parameters on existing ones */
> + tproc = ipeh_tlv_get_proc_by_type(tlv_param_table, type);
> + new_params = tproc->params;
> +
> + if (info->attrs[IPEH_TLV_ATTR_ORDER]) {
> + v = nla_get_u16(info->attrs[IPEH_TLV_ATTR_ORDER]);
> + new_params.t.preferred_order = v;
> + }
> +
> + if (info->attrs[IPEH_TLV_ATTR_ADMIN_PERM]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_ADMIN_PERM]);
> + if (v > IPEH_TLV_PERM_MAX)
> + goto out;
> + new_params.t.admin_perm = v;
> + }
> +
> + if (info->attrs[IPEH_TLV_ATTR_USER_PERM]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_USER_PERM]);
> + if (v > IPEH_TLV_PERM_MAX)
> + goto out;
> + new_params.t.user_perm = v;
> + }
> +
> + if (info->attrs[IPEH_TLV_ATTR_CLASS]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_CLASS]);
> + if (!v || (v & ~IPEH_TLV_CLASS_FLAG_MASK))
> + goto out;
> + new_params.t.class = v;
> + }
> +
> + if (info->attrs[IPEH_TLV_ATTR_ALIGN_MULT]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_ALIGN_MULT]);
> + if (v > 16 || v < 1)
> + goto out;
> + new_params.t.align_mult = v - 1;
> + }
> +
> + if (info->attrs[IPEH_TLV_ATTR_ALIGN_OFF]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_ALIGN_OFF]);
> + if (v > 15)
> + goto out;
> + new_params.t.align_off = v;
> + }
> +
> + if (info->attrs[IPEH_TLV_ATTR_MAX_DATA_LEN])
> + new_params.t.max_data_len =
> + nla_get_u8(info->attrs[IPEH_TLV_ATTR_MAX_DATA_LEN]);
> +
> + if (info->attrs[IPEH_TLV_ATTR_MIN_DATA_LEN])
> + new_params.t.min_data_len =
> + nla_get_u8(info->attrs[IPEH_TLV_ATTR_MIN_DATA_LEN]);
> +
> + if (info->attrs[IPEH_TLV_ATTR_DATA_LEN_MULT]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_DATA_LEN_MULT]);
> + if (v > 16 || v < 1)
> + goto out;
> + new_params.t.data_len_mult = v - 1;
> + }
Is some sanity checking warranted for the min/max data len values.
f.e. that min <= max ?
> +
> + if (info->attrs[IPEH_TLV_ATTR_DATA_LEN_OFF]) {
> + v = nla_get_u8(info->attrs[IPEH_TLV_ATTR_DATA_LEN_OFF]);
> + if (v > 15)
> + goto out;
> + new_params.t.data_len_off = v;
> + }
> +
> + retv = ipeh_tlv_set_params(tlv_param_table, type, &new_params);
> +
> +out:
> + rcu_read_unlock();
> + return retv;
> +}
> +EXPORT_SYMBOL(ipeh_tlv_nl_cmd_set);
...
