| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJL1qvG54C5V-GzRqOKVhXjL7d1OUj5GuQvyx2vsKpQDdf_0Xg@mail.gmail.com>
Date: Thu, 1 Sep 2016 14:59:28 +0300
From: Hadar Hen Zion <hadarh@....mellanox.co.il>
To: Shmulik Ladkani <shmulik.ladkani@...il.com>
Cc: Hadar Hen Zion <hadarh@...lanox.com>,
"David S. Miller" <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...lanox.com>,
Jiri Benc <jbenc@...hat.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Tom Herbert <tom@...bertland.com>,
Eric Dumazet <edumazet@...gle.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Or Gerlitz <ogerlitz@...lanox.com>,
Amir Vadai <amirva@...lanox.com>, Amir Vadai <amir@...ai.me>
Subject: Re: [PATCH net-next V4 4/4] net/sched: Introduce act_tunnel_key
On Wed, Aug 31, 2016 at 8:44 PM, Shmulik Ladkani
<shmulik.ladkani@...il.com> wrote:
> Hi,
>
> On Wed, 31 Aug 2016 15:46:24 +0300 Hadar Hen Zion <hadarh@...lanox.com> wrote:
>> +static int tunnel_key_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, tunnel_key_net_id);
>> + struct nlattr *tb[TCA_TUNNEL_KEY_MAX + 1];
>> + struct metadata_dst *metadata = NULL;
>> + struct tc_tunnel_key *parm;
>> + struct tcf_tunnel_key *t;
>> + struct tcf_tunnel_key_params *params_old;
>> + struct tcf_tunnel_key_params *params_new;
>> + __be64 key_id;
>> + bool exists = false;
>> + int ret = 0;
>> + int err;
>> +
>> + if (!nla)
>> + return -EINVAL;
>> +
>> + err = nla_parse_nested(tb, TCA_TUNNEL_KEY_MAX, nla, tunnel_key_policy);
>> + if (err < 0)
>> + return err;
>> +
>> + if (!tb[TCA_TUNNEL_KEY_PARMS])
>> + return -EINVAL;
>> +
>> + parm = nla_data(tb[TCA_TUNNEL_KEY_PARMS]);
>> + exists = tcf_hash_check(tn, parm->index, a, bind);
>> + if (exists && bind)
>> + return 0;
>> +
>> + switch (parm->t_action) {
>> + case TCA_TUNNEL_KEY_ACT_RELEASE:
>> + break;
>> + case TCA_TUNNEL_KEY_ACT_SET:
>> + if (!tb[TCA_TUNNEL_KEY_ENC_KEY_ID]) {
>> + ret = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + key_id = key32_to_tunnel_id(nla_get_be32(tb[TCA_TUNNEL_KEY_ENC_KEY_ID]));
>> +
>> + if (tb[TCA_TUNNEL_KEY_ENC_IPV4_SRC] &&
>> + tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]) {
>> + __be32 saddr;
>> + __be32 daddr;
>> +
>> + saddr = nla_get_in_addr(tb[TCA_TUNNEL_KEY_ENC_IPV4_SRC]);
>> + daddr = nla_get_in_addr(tb[TCA_TUNNEL_KEY_ENC_IPV4_DST]);
>> +
>> + metadata = __ip_tun_set_dst(saddr, daddr, 0, 0,
>> + TUNNEL_KEY, key_id, 0);
>> + } else if (tb[TCA_TUNNEL_KEY_ENC_IPV6_SRC] &&
>> + tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]) {
>> + struct in6_addr saddr;
>> + struct in6_addr daddr;
>> +
>> + saddr = nla_get_in6_addr(tb[TCA_TUNNEL_KEY_ENC_IPV6_SRC]);
>> + daddr = nla_get_in6_addr(tb[TCA_TUNNEL_KEY_ENC_IPV6_DST]);
>> +
>> + metadata = __ipv6_tun_set_dst(&saddr, &daddr, 0, 0, 0,
>> + TUNNEL_KEY, key_id, 0);
>> + }
>> +
>> + if (!metadata) {
>> + ret = -EINVAL;
>> + goto err_out;
>> + }
>> +
>> + metadata->u.tun_info.mode |= IP_TUNNEL_INFO_TX;
>> + break;
>> + default:
>> + goto err_out;
>> + }
>> +
>> + if (!exists) {
>> + ret = tcf_hash_create(tn, parm->index, est, a,
>> + &act_tunnel_key_ops, bind, true);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ACT_P_CREATED;
>> + } else {
>> + tcf_hash_release(*a, bind);
>> + if (!ovr)
>> + return -EEXIST;
>> + }
>> +
>> + t = to_tunnel_key(*a);
>> +
>> + ASSERT_RTNL();
>> + params_new = kzalloc(sizeof(*params_new),
>> + GFP_KERNEL);
>
> nit: Fits oneline. Fix if patch needs other amendments.
Sure, will do.
>
>> + if (unlikely(!params_new)) {
>> + if (ovr)
>> + tcf_hash_release(*a, bind);
>> + return -ENOMEM;
>
> Seems we need to call tcf_hash_release regardless 'ovr':
> In case (!exist), we've created a new hash few lines above.
> Therefore in failure, don't we need a tcf_hash_release()?
> Am I missing something?
You are right, "if (ovr)" line should be removed.
>
>> + }
>> +
>> + params_old = rtnl_dereference(t->params);
>> +
>> + t->tcf_action = parm->action;
>> + params_new->tcft_action = parm->t_action;
>> + params_new->tcft_enc_metadata = metadata;
>> +
>> + rcu_assign_pointer(t->params, params_new);
>> +
>> + if (params_old)
>> + kfree_rcu(params_old, rcu);
>> +
>> + if (ret == ACT_P_CREATED)
>> + tcf_hash_insert(tn, *a);
>> +
>> + return ret;
>> +
>> +err_out:
>> + if (exists)
>> + tcf_hash_release(*a, bind);
>> + return ret;
>> +}
>> +
>> +static void tunnel_key_release(struct tc_action *a, int bind)
>> +{
>> + struct tcf_tunnel_key *t = to_tunnel_key(a);
>> + struct tcf_tunnel_key_params *params;
>> +
>> + rcu_read_lock();
>> + params = rcu_dereference(t->params);
>> +
>> + if (params->tcft_action == TCA_TUNNEL_KEY_ACT_SET)
>> + dst_release(¶ms->tcft_enc_metadata->dst);
>> +
>> + rcu_read_unlock();
>
> Not an RCU expert, maybe I'm off...
> This alters params in some way (dst_release), so shouldn't it be
> considered an UPDATE, involving 'params' replacement?
> Current code declares it as an rcu read section.
>
dst_release function is using call_rcu to release the dst, so i think
we are safe here.
> Thanks,
> Shmulik
Powered by blists - more mailing lists