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

[Amir Vadai <amir@vadai.me>]

On Mon, Aug 22, 2016 at 08:57:06PM +0300, Shmulik Ladkani wrote:
> Hi,
> 
> On Mon, 22 Aug 2016 17:38:34 +0300 Amir Vadai <amir@vadai.me> 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.
ack

> 
> > +
> > +	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.
ack

> 
> > +			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.
good catch. strange that the compiler/sparse didn't catch it

> 
> > +
> > +	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.
another good catch - thanks.

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