On Fri, Mar 09, 2018 at 12:53:17PM +0100, Jiri Benc wrote:
> 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?
Sure, that is fine by me.
>
> > + 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?
True.
> > +
> > + 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.
Sure, will do.
> [...]
> > @@ -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.
Thanks, sorry for missing that.
> > +
> > 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.
Thanks, I'm not sure why I thought otherwise.
> > + 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.
Agreed.
> > +
> > + 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 for pointing that out. Its a pretty fundamental oversight.
