Re: [PATCH net-next V7 4/4] net/sched: Introduce act_tunnel_key

[John Fastabend]

On 16-09-09 06:19 AM, Eric Dumazet wrote:
> On Thu, 2016-09-08 at 22:30 -0700, Cong Wang wrote:
>> On Thu, Sep 8, 2016 at 9:15 AM, John Fastabend  
>> wrote:
>>>
>>> This should be rtnl_derefence(t->params) and drop the read_lock/unlock
>>> pair. This is always called with RTNL lock unless you have a path I'm
>>> not seeing.
>>
>> You missed the previous discussion on V6, John.
>>
>> BTW, you really should follow the whole discussion instead of
>> jumping in the middle, like what you did for my patchset.
>> I understand you are eager to comment, but please don't waste
>> others' time in this way Please.
> 
> But John is right, and he definitely is welcome to give his feedback
> even at V13 if he wants to.
> 
> tunnel_key_dump() is called with RTNL being held.
> 
> Take a deep breath, vacations, and come back when you are relaxed.
> 
> Thanks.
> 
> 

Also v6 discussion was around cleanup() call back I see nothing about
the dump() callbacks. And if there was it wasn't fixed so it should
be resolved.

Anyways Dave/Hadar feel free to submit a follow up patch or v8 it
doesn't much matter to me as noted in the original post.

.John

Re: [PATCH net-next V7 4/4] net/sched: Introduce act_tunnel_key

[Eric Dumazet]

On Thu, 2016-09-08 at 22:30 -0700, Cong Wang wrote:
> On Thu, Sep 8, 2016 at 9:15 AM, John Fastabend  
> wrote:
> >
> > This should be rtnl_derefence(t->params) and drop the read_lock/unlock
> > pair. This is always called with RTNL lock unless you have a path I'm
> > not seeing.
> 
> You missed the previous discussion on V6, John.
> 
> BTW, you really should follow the whole discussion instead of
> jumping in the middle, like what you did for my patchset.
> I understand you are eager to comment, but please don't waste
> others' time in this way Please.

But John is right, and he definitely is welcome to give his feedback
even at V13 if he wants to.

tunnel_key_dump() is called with RTNL being held.

Take a deep breath, vacations, and come back when you are relaxed.

Thanks.

Re: [PATCH net-next V7 4/4] net/sched: Introduce act_tunnel_key

[Cong Wang]

On Thu, Sep 8, 2016 at 9:15 AM, John Fastabend  wrote:
>
> This should be rtnl_derefence(t->params) and drop the read_lock/unlock
> pair. This is always called with RTNL lock unless you have a path I'm
> not seeing.

You missed the previous discussion on V6, John.

BTW, you really should follow the whole discussion instead of
jumping in the middle, like what you did for my patchset.
I understand you are eager to comment, but please don't waste
others' time in this way Please.

Re: [PATCH net-next V7 4/4] net/sched: Introduce act_tunnel_key

[John Fastabend]

On 16-09-08 06:23 AM, Hadar Hen Zion wrote:
> From: Amir Vadai 
> 
> This action could be used before redirecting packets to a shared tunnel
> device, or when redirecting packets arriving from a such a device.
> 
> The action will release the metadata created by the tunnel device
> (decap), or set the metadata with the specified values for encap
> operation.
> 
> For example, the following flower filter will forward all ICMP packets
> destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
> redirecting, a metadata for the vxlan tunnel is created using the
> tunnel_key action and it's arguments:
> 
> $ tc filter add dev net0 protocol ip parent : \
> flower \
>   ip_proto 1 \
>   dst_ip 11.11.11.2 \
> action tunnel_key set \
>   src_ip 11.11.0.1 \
>   dst_ip 11.11.0.2 \
>   id 11 \
> action mirred egress redirect dev vxlan0
> 
> Signed-off-by: Amir Vadai 
> Signed-off-by: Hadar Hen Zion 
> Reviewed-by: Shmulik Ladkani 
> Acked-by: Jamal Hadi Salim 
> ---

[...]

> +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(>tcft_enc_metadata->dst);
> +
> + kfree_rcu(params, rcu);
> +
> + rcu_read_unlock();
> +}
> +

Same comment as Eric, you better own the action or else this could
race.

> +
> +static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
> +int bind, int ref)
> +{
> + unsigned char *b = skb_tail_pointer(skb);
> + struct tcf_tunnel_key *t = to_tunnel_key(a);
> + struct tcf_tunnel_key_params *params;
> + struct tc_tunnel_key opt = {
> + .index= t->tcf_index,
> + .refcnt   = t->tcf_refcnt - ref,
> + .bindcnt  = t->tcf_bindcnt - bind,
> + };
> + struct tcf_t tm;
> + int ret = -1;
> +
> + rcu_read_lock();
> + params = rcu_dereference(t->params);

This should be rtnl_derefence(t->params) and drop the read_lock/unlock
pair. This is always called with RTNL lock unless you have a path I'm
not seeing.


> +
> + opt.t_action = params->tcft_action;
> + opt.action = params->action;
> +
> + if (nla_put(skb, TCA_TUNNEL_KEY_PARMS, sizeof(opt), ))
> + goto nla_put_failure;
> +
> + if (params->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
> + struct ip_tunnel_key *key =
> + >tcft_enc_metadata->u.tun_info.key;
> + __be32 key_id = tunnel_id_to_key32(key->tun_id);
> +
> + if (nla_put_be32(skb, TCA_TUNNEL_KEY_ENC_KEY_ID, key_id) ||
> + tunnel_key_dump_addresses(skb,
> +   
> >tcft_enc_metadata->u.tun_info))
> + goto nla_put_failure;
> + }
> +
> + tcf_tm_dump(, >tcf_tm);
> + if (nla_put_64bit(skb, TCA_TUNNEL_KEY_TM, sizeof(tm),
> +   , TCA_TUNNEL_KEY_PAD))
> + goto nla_put_failure;
> +
> + ret = skb->len;
> + goto out;
> +
> +nla_put_failure:
> + nlmsg_trim(skb, b);
> +out:
> + rcu_read_unlock();
> +
> + return ret;
> +}
> +

I don't really care if you roll the above two rcu cleanups on top of
the patch as a follow up or roll a v8. But I think we should get the
annotation right here so its clear later.

.John

Re: [PATCH net-next V7 4/4] net/sched: Introduce act_tunnel_key

[Eric Dumazet]

On Thu, 2016-09-08 at 16:23 +0300, Hadar Hen Zion wrote:
> From: Amir Vadai 
> 
> This action could be used before redirecting packets to a shared tunnel
> device, or when redirecting packets arriving from a such a device.


> +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(>tcft_enc_metadata->dst);
> +
> + kfree_rcu(params, rcu);
> +
> + rcu_read_unlock();
> +}

Note that you own the action here, so technically speaking no writer
could possibly modify t->params while this function is running.

So you could use 

params = rcu_dereference_protected(t->params, 1)

(I could not find a way to express the 'I own this action and am the
last user' for LOCKDEP sake so I used 1)

instead of

rcu_read_lock();
params = rcu_dereference(t->params);
rcu_read_unlock();


But this is a very minor detail, and this patch looks fine to me, thanks
a lot for your patience Hadar .

Acked-by: Eric Dumazet 

[PATCH net-next V7 4/4] net/sched: Introduce act_tunnel_key

[Hadar Hen Zion]

From: Amir Vadai 

This action could be used before redirecting packets to a shared tunnel
device, or when redirecting packets arriving from a such a device.

The action will release the metadata created by the tunnel device
(decap), or set the metadata with the specified values for encap
operation.

For example, the following flower filter will forward all ICMP packets
destined to 11.11.11.2 through the shared vxlan device 'vxlan0'. Before
redirecting, a metadata for the vxlan tunnel is created using the
tunnel_key action and it's arguments:

$ tc filter add dev net0 protocol ip parent : \
flower \
  ip_proto 1 \
  dst_ip 11.11.11.2 \
action tunnel_key set \
  src_ip 11.11.0.1 \
  dst_ip 11.11.0.2 \
  id 11 \
action mirred egress redirect dev vxlan0

Signed-off-by: Amir Vadai 
Signed-off-by: Hadar Hen Zion 
Reviewed-by: Shmulik Ladkani 
Acked-by: Jamal Hadi Salim 
---
 include/net/tc_act/tc_tunnel_key.h|  31 +++
 include/uapi/linux/tc_act/tc_tunnel_key.h |  42 
 net/sched/Kconfig |  11 +
 net/sched/Makefile|   1 +
 net/sched/act_tunnel_key.c| 351 ++
 5 files changed, 436 insertions(+)
 create mode 100644 include/net/tc_act/tc_tunnel_key.h
 create mode 100644 include/uapi/linux/tc_act/tc_tunnel_key.h
 create mode 100644 net/sched/act_tunnel_key.c

diff --git a/include/net/tc_act/tc_tunnel_key.h 
b/include/net/tc_act/tc_tunnel_key.h
new file mode 100644
index 000..6fd2255
--- /dev/null
+++ b/include/net/tc_act/tc_tunnel_key.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2016, Amir Vadai 
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __NET_TC_TUNNEL_KEY_H
+#define __NET_TC_TUNNEL_KEY_H
+
+#include 
+
+struct tcf_tunnel_key_params {
+   struct rcu_head rcu;
+   int tcft_action;
+   int action;
+   struct metadata_dst *tcft_enc_metadata;
+};
+
+struct tcf_tunnel_key {
+   struct tc_action  common;
+   struct tcf_tunnel_key_params __rcu *params;
+};
+
+#define to_tunnel_key(a) ((struct tcf_tunnel_key *)a)
+
+#endif /* __NET_TC_TUNNEL_KEY_H */
+
diff --git a/include/uapi/linux/tc_act/tc_tunnel_key.h 
b/include/uapi/linux/tc_act/tc_tunnel_key.h
new file mode 100644
index 000..f9ddf53
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_tunnel_key.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2016, Amir Vadai 
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_TUNNEL_KEY_H
+#define __LINUX_TC_TUNNEL_KEY_H
+
+#include 
+
+#define TCA_ACT_TUNNEL_KEY 17
+
+#define TCA_TUNNEL_KEY_ACT_SET 1
+#define TCA_TUNNEL_KEY_ACT_RELEASE  2
+
+struct tc_tunnel_key {
+   tc_gen;
+   int t_action;
+};
+
+enum {
+   TCA_TUNNEL_KEY_UNSPEC,
+   TCA_TUNNEL_KEY_TM,
+   TCA_TUNNEL_KEY_PARMS,
+   TCA_TUNNEL_KEY_ENC_IPV4_SRC,/* be32 */
+   TCA_TUNNEL_KEY_ENC_IPV4_DST,/* be32 */
+   TCA_TUNNEL_KEY_ENC_IPV6_SRC,/* struct in6_addr */
+   TCA_TUNNEL_KEY_ENC_IPV6_DST,/* struct in6_addr */
+   TCA_TUNNEL_KEY_ENC_KEY_ID,  /* be64 */
+   TCA_TUNNEL_KEY_PAD,
+   __TCA_TUNNEL_KEY_MAX,
+};
+
+#define TCA_TUNNEL_KEY_MAX (__TCA_TUNNEL_KEY_MAX - 1)
+
+#endif
+
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index ccf931b..72e3426 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -761,6 +761,17 @@ config NET_ACT_IFE
  To compile this code as a module, choose M here: the
  module will be called act_ife.
 
+config NET_ACT_TUNNEL_KEY
+tristate "IP tunnel metadata manipulation"
+depends on NET_CLS_ACT
+---help---
+ Say Y here to set/release ip tunnel metadata.
+
+ If unsure, say N.
+
+ To compile this code as a module, choose M here: the
+ module will be called act_tunnel_key.
+
 config NET_IFE_SKBMARK
 tristate "Support to encoding decoding skb mark on IFE action"
 depends on NET_ACT_IFE
diff --git a/net/sched/Makefile b/net/sched/Makefile
index ae088a5..b9d046b 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_NET_ACT_CONNMARK)+= act_connmark.o
 obj-$(CONFIG_NET_ACT_IFE)  += act_ife.o