Re: [PATCH/RFC 3/3] net/sched: add tunnel option support to act_tunnel_key

[Jiri Benc <jbenc@redhat.com>]

On Tue,  6 Mar 2018 18:08:05 +0100, Simon Horman wrote:
> +static int
> +tunnel_key_copy_geneve_opt(const struct nlattr *nla, int dst_len, void *dst,
> +			   struct netlink_ext_ack *extack)
> +{
> +	struct nlattr *tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX + 1];
> +	int err, data_len, opt_len;
> +	u8 *data;
> +
> +	err = nla_parse_nested(tb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX,
> +			       nla, geneve_opt_policy, extack);
> +	if (err < 0)
> +		return err;
> +
> +	if (!tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS] ||
> +	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE] ||
> +	    !tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]) {
> +		NL_SET_ERR_MSG(extack, "Missing tunnel key enc geneve option class, type or data");

I think it's obvious by now that I don't like the "enc" :-)

> +		return -EINVAL;
> +	}
> +
> +	data = nla_data(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
> +	data_len = nla_len(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA]);
> +	if (data_len < 4) {
> +		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is less than 4 bytes long");
> +		return -ERANGE;
> +	}
> +	if (data_len % 4) {
> +		NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data is not a multiple of 4 bytes long");
> +		return -ERANGE;
> +	}
> +
> +	opt_len = sizeof(struct geneve_opt) + data_len;
> +	if (dst) {
> +		struct geneve_opt *opt = dst;
> +		u16 class;
> +
> +		if (dst_len < opt_len) {
> +			NL_SET_ERR_MSG(extack, "Tunnel key enc geneve option data length is longer than the supplied data");

I don't understand this error. What can the user do about it?
Furthermore, the error is not propagated to the user space (see also
below).

Shouldn't this be WARN_ON or something?

> +			return -EINVAL;
> +		}
> +
> +		class = nla_get_u16(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS]);
> +		put_unaligned_be16(class, &opt->opt_class);

How this can be unaligned, given we check that the option length is a
multiple of 4 bytes and the option header is 4 bytes?

> +
> +		opt->type = nla_get_u8(tb[TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE]);
> +		opt->length = data_len / 4; /* length is in units of 4 bytes */
> +		opt->r1 = 0;
> +		opt->r2 = 0;
> +		opt->r3 = 0;
> +
> +		memcpy(opt + 1, data, data_len);
> +	}
> +
> +	return opt_len;
> +}
> +
> +static int tunnel_key_copy_opts(const struct nlattr *nla, u8 *dst,
> +				int dst_len, struct netlink_ext_ack *extack)

Please be consistent with parameter ordering, dst and dst_len are in a
different order here and in tunnel_key_copy_geneve_opt.

[...]
> @@ -157,6 +292,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  			goto err_out;
>  		}
>  
> +		if (opts_len)
> +			tunnel_key_opts_set(tb[TCA_TUNNEL_KEY_ENC_OPTS],
> +					    &metadata->u.tun_info, opts_len,
> +					    extack);

You need to propagate the error here. The previous validation is not
enough as errors while copying to tun_info were not covered.

> +
>  		metadata->u.tun_info.mode |= IP_TUNNEL_INFO_TX;
>  		break;
>  	default:
> @@ -221,6 +361,53 @@ static void tunnel_key_release(struct tc_action *a)
>  	kfree_rcu(params, rcu);
>  }
>  
> +static int tunnel_key_geneve_opts_dump(struct sk_buff *skb,
> +				       const struct ip_tunnel_info *info)
> +{
> +	int len = info->options_len;
> +	u8 *src = (u8 *)(info + 1);
> +
> +	while (len > 0) {
> +		struct geneve_opt *opt = (struct geneve_opt *)src;
> +		u16 class;
> +
> +		class = get_unaligned_be16(&opt->opt_class);

I don't think this can be unaligned.

> +		if (nla_put_u16(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS,
> +				class) ||
> +		    nla_put_u8(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE,
> +			       opt->type) ||
> +		    nla_put(skb, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA,
> +			    opt->length * 4, opt + 1))
> +			return -EMSGSIZE;
> +
> +		len -= sizeof(struct geneve_opt) + opt->length * 4;
> +		src += sizeof(struct geneve_opt) + opt->length * 4;
> +	}

All of this needs to be nested in TCA_TUNNEL_KEY_ENC_OPTS_GENEVE.

> +
> +	return 0;
> +}
> +
> +static int tunnel_key_opts_dump(struct sk_buff *skb,
> +				const struct ip_tunnel_info *info)
> +{
> +	struct nlattr *start;
> +	int err;
> +
> +	if (!info->options_len)
> +		return 0;
> +
> +	start = nla_nest_start(skb, TCA_TUNNEL_KEY_ENC_OPTS);
> +	if (!start)
> +		return -EMSGSIZE;
> +
> +	err = tunnel_key_geneve_opts_dump(skb, info);

How do you know it is geneve opts and not some other (future) tunnel
type options?

This is actually more fundamental problem with this patch. The
metadata_dst options are blindly set to the provided data, yet nowhere
we check whether the tunnel type matches. This must be done somehow. We
probably need to store the options type in metadata_dst.

Thanks,

 Jiri