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 <a...@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;
> > +}
> > +
>
>
