Re: [PATCH net-next 3/3] net/sched: Introduce act_iptunnel

Hi,

On Mon, 22 Aug 2016 17:38:34 +0300 Amir Vadai [off-list ref] wrote:

+static struct metadata_dst *iptunnel_alloc(struct tcf_iptunnel *t,
+					   __be32 saddr, __be32 daddr,
+					   __be64 key_id)
+{
+	struct ip_tunnel_info *tun_info;
+	struct metadata_dst *metadata;
+
+	metadata = metadata_dst_alloc(0, GFP_KERNEL);
+	if (!metadata)
+		return ERR_PTR(-ENOMEM);
+
+	tun_info = &metadata->u.tun_info;
+	tun_info->mode = IP_TUNNEL_INFO_TX;

+	ip_tunnel_key_init(&tun_info->key, saddr, daddr, 0, 0, 0, 0, 0,
+			   key_id, 0);

Seems key.tun_flags should be armed with TUNNEL_KEY.
This will make things work with GRE as well.
Pass it in the 'tun_flags' parameter.

+
+	return metadata;
+}
+
+static int tcf_iptunnel_init(struct net *net, struct nlattr *nla,
+			     struct nlattr *est, struct tc_action **a,
+			     int ovr, int bind)
+{
+	struct tc_action_net *tn = net_generic(net, iptunnel_net_id);
+	struct nlattr *tb[TCA_IPTUNNEL_MAX + 1];
+	struct metadata_dst *metadata;
+	struct tc_iptunnel *parm;
+	struct tcf_iptunnel *t;
+	__be32 saddr = 0;
+	__be32 daddr = 0;
+	__be64 key_id = 0;
+	int encapdecap;
+	bool exists = false;
+	int ret = -EINVAL;
+	int err;
+
+	if (!nla)
+		return -EINVAL;
+
+	err = nla_parse_nested(tb, TCA_IPTUNNEL_MAX, nla, iptunnel_policy);
+	if (err < 0)
+		return err;
+
+	if (!tb[TCA_IPTUNNEL_PARMS])
+		return -EINVAL;
+	parm = nla_data(tb[TCA_IPTUNNEL_PARMS]);
+	exists = tcf_hash_check(tn, parm->index, a, bind);
+	if (exists && bind)
+		return 0;
+
+	encapdecap = parm->t_action;
+
+	switch (encapdecap) {
+	case TCA_IPTUNNEL_ACT_DECAP:
+		break;
+	case TCA_IPTUNNEL_ACT_ENCAP:
+		if (tb[TCA_IPTUNNEL_ENC_IPV4_SRC])
+			saddr = nla_get_be32(tb[TCA_IPTUNNEL_ENC_IPV4_SRC]);
+		if (tb[TCA_IPTUNNEL_ENC_IPV4_DST])
+			daddr = nla_get_be32(tb[TCA_IPTUNNEL_ENC_IPV4_DST]);
+		if (tb[TCA_IPTUNNEL_ENC_KEY_ID])
+			key_id = key32_to_tunnel_id(nla_get_be32(tb[TCA_IPTUNNEL_ENC_KEY_ID]));
+
+		if (!saddr || !daddr || !key_id) {

A zero tunnel ID is legit.

+			ret = -EINVAL;
+			goto err_out;
+		}
+
+		metadata = iptunnel_alloc(t, saddr, daddr, key_id);
+		if (IS_ERR(metadata)) {
+			ret = PTR_ERR(metadata);
+			goto err_out;
+		}
+
+		break;
+	default:
+		goto err_out;
+	}
+
+	if (!exists) {
+		ret = tcf_hash_create(tn, parm->index, est, a,
+				      &act_iptunnel_ops, bind, false);
+		if (ret)
+			return ret;
+
+		ret = ACT_P_CREATED;
+	} else {
+		tcf_hash_release(*a, bind);
+		if (!ovr)
+			return -EEXIST;
+	}
+
+	t = to_iptunnel(*a);
+
+	spin_lock_bh(&t->tcf_lock);
+
+	t->tcf_action = parm->action;
+
+	t->tcft_action = encapdecap;
+	t->tcft_enc_metadata = metadata;

Although tcft_enc_metadata is not used in TCA_IPTUNNEL_ACT_DECAP, still
prefer to nullify it instead of initializing it to stack junk.

+
+	spin_unlock_bh(&t->tcf_lock);
+
+	if (ret == ACT_P_CREATED)
+		tcf_hash_insert(tn, *a);
+
+	return ret;

In the (exists && ovr) case, 'ret' seems to be left as '-EINVAL' as was
initialized. Initialize 'ret' to zero instead.

+
+err_out:
+	if (exists)
+		tcf_hash_release(*a, bind);
+	return ret;
+}
+