[net-next PATCH v2 0/6] net sched: Fix broken late binding of actions

[net-next PATCH v2 0/6] net sched: Fix broken late binding of actions

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Some actions were broken in allowing for late binding of actions.
Late binding workflow is as follows:
a) create an action and provide all necessary parameters for it
Optionally provide an index or let the kernel give you one.
Example:
sudo tc actions add action police rate 1kbit burst 90k drop index 1

b) later on bind to the pre-created action from a filter definition
by merely specifying the index.
Example:
sudo tc filter add dev lo parent ffff: protocol ip prio 8 \
u32 match ip src 127.0.0.8/32 flowid 1:8 action police index 1


Jamal Hadi Salim (6):
  tc vlan action fix late binding
  tc ipt action fix late binding
  tc mirred action fix late binding
  tc simple action fix late binding
  tc skbedit action fix late binding
  tc ife action fix late binding

 net/sched/act_ife.c     | 16 +++++++++++-----
 net/sched/act_ipt.c     | 19 ++++++++++++-------
 net/sched/act_mirred.c  | 22 +++++++++++++++-------
 net/sched/act_simple.c  | 18 ++++++++++++------
 net/sched/act_skbedit.c | 22 +++++++++++++++-------
 net/sched/act_vlan.c    | 22 ++++++++++++++++------
 6 files changed, 81 insertions(+), 38 deletions(-)

-- 
1.9.1

[net-next PATCH v2 1/6] net sched: vlan action fix late binding

[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

[net-next PATCH v2 2/6] net sched: ipt action fix late binding

[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

[net-next PATCH v2 3/6] net sched: mirred action fix late binding

[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

[net-next PATCH v2 4/6] net sched: simple action fix late binding

[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

[net-next PATCH v2 5/6] net sched: skbedit action fix late binding

[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

[net-next PATCH v2 6/6] net sched: ife action fix late binding

[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

Re: [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions

[Jamal Hadi Salim]

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

Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding

[Cong Wang]

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.

Re: [net-next PATCH v2 3/6] net sched: mirred action fix late binding

[Cong Wang]

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.

Re: [net-next PATCH v2 5/6] net sched: skbedit action fix late binding

[Cong Wang]

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
>

Re: [net-next PATCH v2 6/6] net sched: ife action fix late binding

[Cong Wang]

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.

Re: [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sun, 8 May 2016 13:29:05 -0400

> 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.

Then don't target them at 'net-next'.  If it's good enough for -stable
it's by definition good enough for 'net'.

Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding

[Jamal Hadi Salim]

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

Re: [net-next PATCH v2 3/6] net sched: mirred action fix late binding

[Jamal Hadi Salim]

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

Re: [net-next PATCH v2 5/6] net sched: skbedit action fix late binding

[Jamal Hadi Salim]

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

Re: [net-next PATCH v2 6/6] net sched: ife action fix late binding

[Jamal Hadi Salim]

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

Re: [net-next PATCH v2 0/6] net sched: Fix broken late binding of actions

[Jamal Hadi Salim]

On 16-05-09 12:30 AM, David Miller wrote:

>> Dave, these deserve to go into -stable as well.
>
> Then don't target them at 'net-next'.  If it's good enough for -stable
> it's by definition good enough for 'net'.
>

will resend after processing comments.

cheers,
jamal

Re: [net-next PATCH v2 3/6] net sched: mirred action fix late binding

[Cong Wang]

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... ;)

Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding

[Jamal Hadi Salim]

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