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(&params->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