* [net-next PATCH v2 1/6] net sched: vlan action fix late binding
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
@ 2016-05-08 17:26 ` Jamal Hadi Salim
2016-05-09 3:08 ` Cong Wang
2016-05-08 17:26 ` [net-next PATCH v2 2/6] net sched: ipt " Jamal Hadi Salim
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
Late binding was broken and is fixed with this patch.
//add a vlan action to pop and give it an instance id of 1
sudo tc actions add action vlan pop index 1
//create filter which binds to vlan action id 1
sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32 match ip dst 17.0.0.1/32 flowid 1:1 action vlan index 1
current message(before bug fix) was:
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_vlan.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index c1682ab..352653f 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -77,7 +77,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
int action;
__be16 push_vid = 0;
__be16 push_proto = 0;
- int ret = 0;
+ int ret = 0, aexists = 0;
int err;
if (!nla)
@@ -90,15 +90,25 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
if (!tb[TCA_VLAN_PARMS])
return -EINVAL;
parm = nla_data(tb[TCA_VLAN_PARMS]);
+ aexists = tcf_hash_check(tn, parm->index, a, bind);
+ if (aexists && bind)
+ return 0;
+
switch (parm->v_action) {
case TCA_VLAN_ACT_POP:
break;
case TCA_VLAN_ACT_PUSH:
- if (!tb[TCA_VLAN_PUSH_VLAN_ID])
+ if (!tb[TCA_VLAN_PUSH_VLAN_ID]) {
+ if (aexists)
+ tcf_hash_release(a, bind);
return -EINVAL;
+ }
push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
- if (push_vid >= VLAN_VID_MASK)
+ if (push_vid >= VLAN_VID_MASK) {
+ if (aexists)
+ tcf_hash_release(a, bind);
return -ERANGE;
+ }
if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
push_proto = nla_get_be16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]);
@@ -114,11 +124,13 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
}
break;
default:
+ if (aexists)
+ tcf_hash_release(a, bind);
return -EINVAL;
}
action = parm->v_action;
- if (!tcf_hash_check(tn, parm->index, a, bind)) {
+ if (!aexists) {
ret = tcf_hash_create(tn, parm->index, est, a,
sizeof(*v), bind, false);
if (ret)
@@ -126,8 +138,6 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
ret = ACT_P_CREATED;
} else {
- if (bind)
- return 0;
tcf_hash_release(a, bind);
if (!ovr)
return -EEXIST;
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding
2016-05-08 17:26 ` [net-next PATCH v2 1/6] net sched: vlan action fix late binding Jamal Hadi Salim
@ 2016-05-09 3:08 ` Cong Wang
2016-05-09 11:50 ` Jamal Hadi Salim
2016-05-10 20:52 ` Jamal Hadi Salim
0 siblings, 2 replies; 20+ messages in thread
From: Cong Wang @ 2016-05-09 3:08 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: David Miller, Linux Kernel Network Developers
On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index c1682ab..352653f 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -77,7 +77,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> int action;
> __be16 push_vid = 0;
> __be16 push_proto = 0;
> - int ret = 0;
> + int ret = 0, aexists = 0;
> int err;
>
> if (!nla)
> @@ -90,15 +90,25 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> if (!tb[TCA_VLAN_PARMS])
> return -EINVAL;
> parm = nla_data(tb[TCA_VLAN_PARMS]);
> + aexists = tcf_hash_check(tn, parm->index, a, bind);
I think 'exists' is a better name than 'aexists', shorter and clear.
> + if (aexists && bind)
> + return 0;
> +
> switch (parm->v_action) {
> case TCA_VLAN_ACT_POP:
> break;
> case TCA_VLAN_ACT_PUSH:
> - if (!tb[TCA_VLAN_PUSH_VLAN_ID])
> + if (!tb[TCA_VLAN_PUSH_VLAN_ID]) {
> + if (aexists)
> + tcf_hash_release(a, bind);
> return -EINVAL;
> + }
> push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
> - if (push_vid >= VLAN_VID_MASK)
> + if (push_vid >= VLAN_VID_MASK) {
> + if (aexists)
> + tcf_hash_release(a, bind);
> return -ERANGE;
> + }
>
> if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
> push_proto = nla_get_be16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]);
> @@ -114,11 +124,13 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> }
> break;
> default:
> + if (aexists)
> + tcf_hash_release(a, bind);
Introduce a goto to reduce duplicated cleanup code?
> return -EINVAL;
> }
> action = parm->v_action;
>
> - if (!tcf_hash_check(tn, parm->index, a, bind)) {
> + if (!aexists) {
> ret = tcf_hash_create(tn, parm->index, est, a,
> sizeof(*v), bind, false);
> if (ret)
> @@ -126,8 +138,6 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>
> ret = ACT_P_CREATED;
> } else {
> - if (bind)
> - return 0;
> tcf_hash_release(a, bind);
> if (!ovr)
> return -EEXIST;
The rest looks good to me.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding
2016-05-09 3:08 ` Cong Wang
@ 2016-05-09 11:50 ` Jamal Hadi Salim
2016-05-10 20:52 ` Jamal Hadi Salim
1 sibling, 0 replies; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-09 11:50 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers
On 16-05-08 11:08 PM, Cong Wang wrote:
> On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
[..]
>> + aexists = tcf_hash_check(tn, parm->index, a, bind);
>
>
> I think 'exists' is a better name than 'aexists', shorter and clear.
>
aexists is more specific (doesnt quiet apply to this case but has better
grep-ability).
exists looked too generic - initially I was going to have act_exists.
Yes, it is a tiny but ocd detail. Let me know if you feel strongly
and i will change it in next update.
>> + if (aexists)
>> + tcf_hash_release(a, bind);
>
>
> Introduce a goto to reduce duplicated cleanup code?
>
Will do.
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding
2016-05-09 3:08 ` Cong Wang
2016-05-09 11:50 ` Jamal Hadi Salim
@ 2016-05-10 20:52 ` Jamal Hadi Salim
1 sibling, 0 replies; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-10 20:52 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers
On 16-05-08 11:08 PM, Cong Wang wrote:
>> + if (aexists)
>> + tcf_hash_release(a, bind);
>
>
> Introduce a goto to reduce duplicated cleanup code?
>
I addressed all your comments except for above goto. There are different
return codes for all failures - and the cleverness of a goto seems
unecessary.
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread
* [net-next PATCH v2 2/6] net sched: ipt action fix late binding
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
2016-05-08 17:26 ` [net-next PATCH v2 1/6] net sched: vlan action fix late binding Jamal Hadi Salim
@ 2016-05-08 17:26 ` Jamal Hadi Salim
2016-05-08 17:26 ` [net-next PATCH v2 3/6] net sched: mirred " Jamal Hadi Salim
` (4 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
This was broken and is fixed with this patch.
//add an ipt action and give it an instance id of 1
sudo tc actions add action ipt -j mark --set-mark 2 index 1
//create a filter which binds to ipt action id 1
sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
match ip dst 17.0.0.1/32 flowid 1:10 action ipt index 1
Message before bug fix was:
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_ipt.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 1464f6a..722b5d8 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -96,7 +96,7 @@ static int __tcf_ipt_init(struct tc_action_net *tn, struct nlattr *nla,
struct tcf_ipt *ipt;
struct xt_entry_target *td, *t;
char *tname;
- int ret = 0, err;
+ int ret = 0, err, aexists = 0;
u32 hook = 0;
u32 index = 0;
@@ -107,18 +107,23 @@ static int __tcf_ipt_init(struct tc_action_net *tn, struct nlattr *nla,
if (err < 0)
return err;
- if (tb[TCA_IPT_HOOK] == NULL)
- return -EINVAL;
- if (tb[TCA_IPT_TARG] == NULL)
+ if (tb[TCA_IPT_INDEX] != NULL)
+ index = nla_get_u32(tb[TCA_IPT_INDEX]);
+
+ aexists = tcf_hash_check(tn, index, a, bind);
+ if (aexists && bind)
+ return 0;
+
+ if (tb[TCA_IPT_HOOK] == NULL || tb[TCA_IPT_TARG] == NULL) {
+ if (aexists)
+ tcf_hash_release(a, bind);
return -EINVAL;
+ }
td = (struct xt_entry_target *)nla_data(tb[TCA_IPT_TARG]);
if (nla_len(tb[TCA_IPT_TARG]) < td->u.target_size)
return -EINVAL;
- if (tb[TCA_IPT_INDEX] != NULL)
- index = nla_get_u32(tb[TCA_IPT_INDEX]);
-
if (!tcf_hash_check(tn, index, a, bind)) {
ret = tcf_hash_create(tn, index, est, a, sizeof(*ipt), bind,
false);
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [net-next PATCH v2 3/6] net sched: mirred action fix late binding
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
2016-05-08 17:26 ` [net-next PATCH v2 1/6] net sched: vlan action fix late binding Jamal Hadi Salim
2016-05-08 17:26 ` [net-next PATCH v2 2/6] net sched: ipt " Jamal Hadi Salim
@ 2016-05-08 17:26 ` Jamal Hadi Salim
2016-05-09 3:19 ` Cong Wang
2016-05-08 17:26 ` [net-next PATCH v2 4/6] net sched: simple " Jamal Hadi Salim
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
The process below was broken and is fixed with this patch.
//add an mirred action and give it an instance id of 1
sudo tc actions add action mirred egress mirror dev $MDEV index 1
//create a filter which binds to mirred action id 1
sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
match ip dst 17.0.0.1/32 flowid 1:10 action mirred index 1
Message before bug fix was:
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_mirred.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index dea57c1..9e682c8 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -61,7 +61,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
struct tc_mirred *parm;
struct tcf_mirred *m;
struct net_device *dev;
- int ret, ok_push = 0;
+ int ret, ok_push = 0, aexists = 0;
if (nla == NULL)
return -EINVAL;
@@ -71,17 +71,27 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
if (tb[TCA_MIRRED_PARMS] == NULL)
return -EINVAL;
parm = nla_data(tb[TCA_MIRRED_PARMS]);
+
+ aexists = tcf_hash_check(tn, parm->index, a, bind);
+ if (aexists && bind)
+ return 0;
+
switch (parm->eaction) {
case TCA_EGRESS_MIRROR:
case TCA_EGRESS_REDIR:
break;
default:
+ if (aexists)
+ tcf_hash_release(a, bind);
return -EINVAL;
}
if (parm->ifindex) {
dev = __dev_get_by_index(net, parm->ifindex);
- if (dev == NULL)
+ if (dev == NULL) {
+ if (aexists)
+ tcf_hash_release(a, bind);
return -ENODEV;
+ }
switch (dev->type) {
case ARPHRD_TUNNEL:
case ARPHRD_TUNNEL6:
@@ -99,7 +109,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
dev = NULL;
}
- if (!tcf_hash_check(tn, parm->index, a, bind)) {
+ if (!aexists) {
if (dev == NULL)
return -EINVAL;
ret = tcf_hash_create(tn, parm->index, est, a,
@@ -108,9 +118,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
return ret;
ret = ACT_P_CREATED;
} else {
- if (bind)
- return 0;
-
tcf_hash_release(a, bind);
if (!ovr)
return -EEXIST;
@@ -195,7 +202,8 @@ out:
return retval;
}
-static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
+ int ref)
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_mirred *m = a->priv;
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 3/6] net sched: mirred action fix late binding
2016-05-08 17:26 ` [net-next PATCH v2 3/6] net sched: mirred " Jamal Hadi Salim
@ 2016-05-09 3:19 ` Cong Wang
2016-05-09 11:51 ` Jamal Hadi Salim
0 siblings, 1 reply; 20+ messages in thread
From: Cong Wang @ 2016-05-09 3:19 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: David Miller, Linux Kernel Network Developers
On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> -static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
> +static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
> + int ref)
> {
> unsigned char *b = skb_tail_pointer(skb);
> struct tcf_mirred *m = a->priv;
Nit: this is irrelevant to the bug you fix.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 3/6] net sched: mirred action fix late binding
2016-05-09 3:19 ` Cong Wang
@ 2016-05-09 11:51 ` Jamal Hadi Salim
2016-05-10 4:37 ` Cong Wang
0 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-09 11:51 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers
On 16-05-08 11:19 PM, Cong Wang wrote:
> On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> -static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>> +static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
>> + int ref)
>> {
>> unsigned char *b = skb_tail_pointer(skb);
>> struct tcf_mirred *m = a->priv;
>
> Nit: this is irrelevant to the bug you fix.
>
It is (80 char) indentation fix. Dont think it deserves its own patch
but it was annoying enough to include.
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 3/6] net sched: mirred action fix late binding
2016-05-09 11:51 ` Jamal Hadi Salim
@ 2016-05-10 4:37 ` Cong Wang
0 siblings, 0 replies; 20+ messages in thread
From: Cong Wang @ 2016-05-10 4:37 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: David Miller, Linux Kernel Network Developers
On Mon, May 9, 2016 at 4:51 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 16-05-08 11:19 PM, Cong Wang wrote:
>>
>> On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>> wrote:
>>>
>>> -static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int
>>> bind, int ref)
>>> +static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int
>>> bind,
>>> + int ref)
>>> {
>>> unsigned char *b = skb_tail_pointer(skb);
>>> struct tcf_mirred *m = a->priv;
>>
>>
>> Nit: this is irrelevant to the bug you fix.
>>
>
> It is (80 char) indentation fix. Dont think it deserves its own patch
> but it was annoying enough to include.
If you target this to net-next, I would not even mention it, but you
are targeting it for -stable... ;)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [net-next PATCH v2 4/6] net sched: simple action fix late binding
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
` (2 preceding siblings ...)
2016-05-08 17:26 ` [net-next PATCH v2 3/6] net sched: mirred " Jamal Hadi Salim
@ 2016-05-08 17:26 ` Jamal Hadi Salim
2016-05-08 17:26 ` [net-next PATCH v2 5/6] net sched: skbedit " Jamal Hadi Salim
` (2 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
The process below was broken and is fixed with this patch.
//add a simple action and give it an instance id of 1
sudo tc actions add action simple sdata "foobar" index 1
//create a filter which binds to simple action id 1
sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
match ip dst 17.0.0.1/32 flowid 1:10 action simple index 1
Message before fix was:
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_simple.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 2057fd56..d9f44be 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -87,7 +87,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
struct tc_defact *parm;
struct tcf_defact *d;
char *defdata;
- int ret = 0, err;
+ int ret = 0, err, aexists = 0;
if (nla == NULL)
return -EINVAL;
@@ -99,13 +99,21 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
if (tb[TCA_DEF_PARMS] == NULL)
return -EINVAL;
- if (tb[TCA_DEF_DATA] == NULL)
- return -EINVAL;
parm = nla_data(tb[TCA_DEF_PARMS]);
+ aexists = tcf_hash_check(tn, parm->index, a, bind);
+ if (aexists && bind)
+ return 0;
+
+ if (tb[TCA_DEF_DATA] == NULL) {
+ if (aexists)
+ tcf_hash_release(a, bind);
+ return -EINVAL;
+ }
+
defdata = nla_data(tb[TCA_DEF_DATA]);
- if (!tcf_hash_check(tn, parm->index, a, bind)) {
+ if (!aexists) {
ret = tcf_hash_create(tn, parm->index, est, a,
sizeof(*d), bind, false);
if (ret)
@@ -122,8 +130,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
} else {
d = to_defact(a);
- if (bind)
- return 0;
tcf_hash_release(a, bind);
if (!ovr)
return -EEXIST;
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [net-next PATCH v2 5/6] net sched: skbedit action fix late binding
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
` (3 preceding siblings ...)
2016-05-08 17:26 ` [net-next PATCH v2 4/6] net sched: simple " Jamal Hadi Salim
@ 2016-05-08 17:26 ` Jamal Hadi Salim
2016-05-09 3:28 ` Cong Wang
2016-05-08 17:26 ` [net-next PATCH v2 6/6] net sched: ife " Jamal Hadi Salim
2016-05-08 17:29 ` [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
6 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
The process below was broken and is fixed with this patch.
//add a skbedit action and give it an instance id of 1
sudo tc actions add action skbedit mark 10 index 1
//create a filter which binds to skbedit action id 1
sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
match ip dst 17.0.0.1/32 flowid 1:10 action skbedit index 1
Message before fix was:
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_skbedit.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 51b2499..784b478 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -69,7 +69,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
struct tcf_skbedit *d;
u32 flags = 0, *priority = NULL, *mark = NULL;
u16 *queue_mapping = NULL;
- int ret = 0, err;
+ int ret = 0, err, aexists = 0;
if (nla == NULL)
return -EINVAL;
@@ -96,12 +96,22 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
mark = nla_data(tb[TCA_SKBEDIT_MARK]);
}
- if (!flags)
- return -EINVAL;
-
parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
- if (!tcf_hash_check(tn, parm->index, a, bind)) {
+ aexists = tcf_hash_check(tn, parm->index, a, bind);
+
+ /* if action exists and this is a late filter bind, no need
+ * to continue processing
+ */
+ if (aexists && bind)
+ return 0;
+
+ if (!flags) {
+ tcf_hash_release(a, bind);
+ return -EINVAL;
+ }
+
+ if (!aexists) {
ret = tcf_hash_create(tn, parm->index, est, a,
sizeof(*d), bind, false);
if (ret)
@@ -111,8 +121,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
ret = ACT_P_CREATED;
} else {
d = to_skbedit(a);
- if (bind)
- return 0;
tcf_hash_release(a, bind);
if (!ovr)
return -EEXIST;
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 5/6] net sched: skbedit action fix late binding
2016-05-08 17:26 ` [net-next PATCH v2 5/6] net sched: skbedit " Jamal Hadi Salim
@ 2016-05-09 3:28 ` Cong Wang
2016-05-09 11:53 ` Jamal Hadi Salim
0 siblings, 1 reply; 20+ messages in thread
From: Cong Wang @ 2016-05-09 3:28 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: David Miller, Linux Kernel Network Developers
On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> The process below was broken and is fixed with this patch.
>
> //add a skbedit action and give it an instance id of 1
> sudo tc actions add action skbedit mark 10 index 1
> //create a filter which binds to skbedit action id 1
> sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
> match ip dst 17.0.0.1/32 flowid 1:10 action skbedit index 1
>
> Message before fix was:
> RTNETLINK answers: Invalid argument
> We have an error talking to the kernel
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
> net/sched/act_skbedit.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 51b2499..784b478 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -69,7 +69,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> struct tcf_skbedit *d;
> u32 flags = 0, *priority = NULL, *mark = NULL;
> u16 *queue_mapping = NULL;
> - int ret = 0, err;
> + int ret = 0, err, aexists = 0;
>
> if (nla == NULL)
> return -EINVAL;
> @@ -96,12 +96,22 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> mark = nla_data(tb[TCA_SKBEDIT_MARK]);
> }
>
> - if (!flags)
> - return -EINVAL;
> -
> parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
>
> - if (!tcf_hash_check(tn, parm->index, a, bind)) {
> + aexists = tcf_hash_check(tn, parm->index, a, bind);
> +
> + /* if action exists and this is a late filter bind, no need
> + * to continue processing
> + */
This comment looks useless, at least for me, because the code
is already clear.
> + if (aexists && bind)
> + return 0;
One extra tab?
> +
> + if (!flags) {
> + tcf_hash_release(a, bind);
> + return -EINVAL;
> + }
> +
> + if (!aexists) {
> ret = tcf_hash_create(tn, parm->index, est, a,
> sizeof(*d), bind, false);
> if (ret)
> @@ -111,8 +121,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> ret = ACT_P_CREATED;
> } else {
> d = to_skbedit(a);
> - if (bind)
> - return 0;
> tcf_hash_release(a, bind);
> if (!ovr)
> return -EEXIST;
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 5/6] net sched: skbedit action fix late binding
2016-05-09 3:28 ` Cong Wang
@ 2016-05-09 11:53 ` Jamal Hadi Salim
0 siblings, 0 replies; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-09 11:53 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers
On 16-05-08 11:28 PM, Cong Wang wrote:
>> + /* if action exists and this is a late filter bind, no need
>> + * to continue processing
>> + */
>
> This comment looks useless, at least for me, because the code
> is already clear.
>
I will get rid of it. Note: I added it to one patch only incase
the feature of late binding was foreign to some people.
>
>> + if (aexists && bind)
>> + return 0;
>
>
> One extra tab?
>
thanks for catching it.
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread
* [net-next PATCH v2 6/6] net sched: ife action fix late binding
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
` (4 preceding siblings ...)
2016-05-08 17:26 ` [net-next PATCH v2 5/6] net sched: skbedit " Jamal Hadi Salim
@ 2016-05-08 17:26 ` Jamal Hadi Salim
2016-05-09 3:32 ` Cong Wang
2016-05-08 17:29 ` [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
6 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:26 UTC (permalink / raw)
To: davem; +Cc: netdev, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
The process below was broken and is fixed with this patch.
//add an ife action and give it an instance id of 1
sudo tc actions add action ife encode \
type 0xDEAD allow mark dst 02:15:15:15:15:15 index 1
//create a filter which binds to skbedit action id 1
sudo tc filter add dev $DEV parent ffff: protocol ip prio 1 u32\
match ip dst 17.0.0.1/32 flowid 1:11 action ife index 1
Message before fix was:
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/act_ife.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 556f44c..17520ed 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -423,7 +423,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
u16 ife_type = 0;
u8 *daddr = NULL;
u8 *saddr = NULL;
- int ret = 0;
+ int ret = 0, aexists = 0;
int err;
err = nla_parse_nested(tb, TCA_IFE_MAX, nla, ife_policy);
@@ -435,25 +435,29 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
parm = nla_data(tb[TCA_IFE_PARMS]);
+ aexists = tcf_hash_check(tn, parm->index, a, bind);
+ if (aexists && bind)
+ return 0;
+
if (parm->flags & IFE_ENCODE) {
/* Until we get issued the ethertype, we cant have
* a default..
**/
if (!tb[TCA_IFE_TYPE]) {
+ if (aexists)
+ tcf_hash_release(a, bind);
pr_info("You MUST pass etherype for encoding\n");
return -EINVAL;
}
}
- if (!tcf_hash_check(tn, parm->index, a, bind)) {
+ if (!aexists) {
ret = tcf_hash_create(tn, parm->index, est, a, sizeof(*ife),
bind, false);
if (ret)
return ret;
ret = ACT_P_CREATED;
} else {
- if (bind) /* dont override defaults */
- return 0;
tcf_hash_release(a, bind);
if (!ovr)
return -EEXIST;
@@ -495,6 +499,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
NULL);
if (err) {
metadata_parse_err:
+ if (aexists)
+ tcf_hash_release(a, bind);
if (ret == ACT_P_CREATED)
_tcf_ife_cleanup(a, bind);
@@ -689,7 +695,7 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
/*
OUTERHDR:TOTMETALEN:{TLVHDR:Metadatum:TLVHDR..}:ORIGDATA
where ORIGDATA = original ethernet header ...
- */
+ */
u16 metalen = ife_get_sz(skb, ife);
int hdrm = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
unsigned int skboff = skb->dev->hard_header_len;
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 6/6] net sched: ife action fix late binding
2016-05-08 17:26 ` [net-next PATCH v2 6/6] net sched: ife " Jamal Hadi Salim
@ 2016-05-09 3:32 ` Cong Wang
2016-05-09 11:54 ` Jamal Hadi Salim
0 siblings, 1 reply; 20+ messages in thread
From: Cong Wang @ 2016-05-09 3:32 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: David Miller, Linux Kernel Network Developers
On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> @@ -689,7 +695,7 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
> /*
> OUTERHDR:TOTMETALEN:{TLVHDR:Metadatum:TLVHDR..}:ORIGDATA
> where ORIGDATA = original ethernet header ...
> - */
> + */
Irrelevant to the bug fix.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [net-next PATCH v2 6/6] net sched: ife action fix late binding
2016-05-09 3:32 ` Cong Wang
@ 2016-05-09 11:54 ` Jamal Hadi Salim
0 siblings, 0 replies; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-09 11:54 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Linux Kernel Network Developers
On 16-05-08 11:32 PM, Cong Wang wrote:
> On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> @@ -689,7 +695,7 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
>> /*
>> OUTERHDR:TOTMETALEN:{TLVHDR:Metadatum:TLVHDR..}:ORIGDATA
>> where ORIGDATA = original ethernet header ...
>> - */
>> + */
>
> Irrelevant to the bug fix.
>
Same as other response. Dont think it is worth sending a patch just to
indent.
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions
2016-05-08 17:26 [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions Jamal Hadi Salim
` (5 preceding siblings ...)
2016-05-08 17:26 ` [net-next PATCH v2 6/6] net sched: ife " Jamal Hadi Salim
@ 2016-05-08 17:29 ` Jamal Hadi Salim
2016-05-09 4:30 ` David Miller
6 siblings, 1 reply; 20+ messages in thread
From: Jamal Hadi Salim @ 2016-05-08 17:29 UTC (permalink / raw)
To: davem; +Cc: netdev
On 16-05-08 01:26 PM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Some actions were broken in allowing for late binding of actions.
Dave, these deserve to go into -stable as well.
cheers,
jamal
^ permalink raw reply [flat|nested] 20+ messages in thread