[iproute PATCH 0/3] tc: pedit fixes

[iproute PATCH 0/3] tc: pedit fixes

[Phil Sutter]

While implementing an implementation example for a pedit man page, I
noticed several issues with the current code. The following patch
series addreses them.

In order to validate my changes, I implemented a simple unit tester.
It requires the following hack:

--------------------------8<-----------------------------------------
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -513,6 +513,12 @@ parse_pedit(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, stru
                }
        }
 
+       fprintf(stderr, "mask=0x%x\n", sel.keys[0].mask);
+       fprintf(stderr, "val=0x%x\n", sel.keys[0].val);
+       fprintf(stderr, "off=0x%x\n", sel.keys[0].off);
+       fprintf(stderr, "at=0x%x\n", sel.keys[0].at);
+       fprintf(stderr, "offmask=0x%x\n", sel.keys[0].offmask);
+       fprintf(stderr, "shift=0x%x\n", sel.keys[0].shift);
        tail = NLMSG_TAIL(n);
        addattr_l(n, MAX_MSG, tca_id, NULL, 0);
        addattr_l(n, MAX_MSG, TCA_PEDIT_PARMS,&sel, sizeof(sel.sel)+sel.sel.nkeys*sizeof(struct tc_pedit_key));
--------------------------8<-----------------------------------------

With this in place, the following simple shell script can be used to
check generated permutation values:

--------------------------8<-----------------------------------------
#!/bin/bash

run_tc() {
	echo "testing $@"
	eval "$(./tc/tc filter add dev bla123 parent root u32 \
		match u32 0 0 action pedit munge $@ 2>&1 | \
		grep -e '^\(mask\|val\|off\|at\|offmask\|shift\)=')"
}

prove() { # (oval, check, ooff)
	local oval=$1
	local check=$2
	local ooff=$3
	[[ $ooff -eq $off ]] || {
		echo "Wrong offset ($off instead of $ooff)"
		#exit 1
	}
	[[ $(((oval & mask) ^ val)) -eq $check ]] || {
		echo "failed for $oval, should be $check but is 0x$(echo "obase = 16; $(((oval & mask) ^ val))" | bc)"
		echo "mask=$mask"
		echo "val=$val"
		echo "off=$off"
		echo "at=$at"
		echo "offmask=$offmask"
		echo "shift=$shift"
		#exit 1
	}
}

run_tc ip df set 0
prove 0xffffffff 0xffbfffff 4
prove 0x00400000 0x00000000 4
prove 0xffbfffff 0xffbfffff 4
prove 0x00000000 0x00000000 4

run_tc ip df set 0x40
prove 0xffffffff 0xffffffff 4
prove 0x00400000 0x00400000 4
prove 0xffbfffff 0xffffffff 4
prove 0x00000000 0x00400000 4

run_tc ip df preserve
prove 0xffffffff 0xffffffff 4
prove 0x00400000 0x00400000 4
prove 0xffbfffff 0xffbfffff 4
prove 0x00000000 0x00000000 4

run_tc ip df invert
prove 0xffffffff 0xffbfffff 4
prove 0x00400000 0x00000000 4
prove 0xffbfffff 0xffffffff 4
prove 0x00000000 0x00400000 4

run_tc ip mf set 0x20
prove 0xfffff2ff 0xfffff2ff 4
prove 0x00203000 0x00203000 4
prove 0xfadfffff 0xfaffffff 4
prove 0x00000000 0x00200000 4

run_tc ip ihl set 0x04
prove 0xffffffff 0xfffffff4 0
prove 0x00000005 0x00000004 0
prove 0xfffffff0 0xfffffff4 0
prove 0x00000000 0x00000004 0

run_tc ip tos set 0
prove 0xffffffff 0xffff00ff 0
prove 0x00002300 0x00000000 0
prove 0xffff00ff 0xffff00ff 0
prove 0x00000000 0x00000000 0

run_tc ip tos set 17
prove 0xffffffff 0xffff11ff 0
prove 0x00002300 0x00001100 0
prove 0xffff00ff 0xffff11ff 0
prove 0x00000000 0x00001100 0

run_tc ip tos invert
prove 0xffffffff 0xffff00ff 0
prove 0x00002300 0x0000DC00 0
prove 0xffff00ff 0xffffffff 0
prove 0x00000000 0x0000ff00 0

run_tc ip tos preserve
prove 0xffffffff 0xffffffff 0
prove 0x00002300 0x00002300 0
prove 0xffff00ff 0xffff00ff 0
prove 0x00000000 0x00000000 0

run_tc ip tos clear
prove 0xffffffff 0xffff00ff 0
prove 0x00002300 0x00000000 0
prove 0xffff00ff 0xffff00ff 0
prove 0x00000000 0x00000000 0

run_tc ip dport set 0
prove 0xffffffff 0x0000ffff 20
prove 0x12340000 0x00000000 20
prove 0x0000ffff 0x0000ffff 20
prove 0x00000000 0x00000000 20

run_tc ip dport set 27374
prove 0xfffff22f 0xee6af22f 20
prove 0x12340010 0xee6a0010 20
prove 0xee6aff0f 0xee6aff0f 20
prove 0xee6a0000 0xee6a0000 20

run_tc ip dport invert
prove 0xffffffff 0x0000ffff 20
prove 0x12340000 0xEDCB0000 20
prove 0x00000000 0xffff0000 20

run_tc ip dport preserve
prove 0xffffffff 0xffffffff 20
prove 0x12340000 0x12340000 20
prove 0xee6affff 0xee6affff 20
prove 0xee6a0000 0xee6a0000 20

run_tc ip dport clear
prove 0xffff3fff 0x00003fff 20
prove 0x12340030 0x00000030 20
prove 0xee6af1ff 0x0000f1ff 20
prove 0x00000000 0x00000000 20


run_tc ip src set 0.0.0.0
prove 0xffff3fff 0x00000000 12
prove 0x12340030 0x00000000 12
prove 0xee6af1ff 0x00000000 12
prove 0x00000000 0x00000000 12

run_tc ip src set 1.2.3.4
prove 0xffff3fff 0x01020304 12
prove 0x12340030 0x01020304 12
prove 0xee6af1ff 0x01020304 12
prove 0x00000000 0x01020304 12

run_tc ip src clear
prove 0xffff3fff 0x00000000 12
prove 0x12340030 0x00000000 12
prove 0xee6af1ff 0x00000000 12
prove 0x00000000 0x00000000 12

run_tc ip src preserve
prove 0xffff3fff 0xffff3fff 12
prove 0x12340030 0x12340030 12
prove 0xee6af1ff 0xee6af1ff 12
prove 0x00000000 0x00000000 12

run_tc ip src invert
prove 0xffff3fff 0x0000c000 12
prove 0x12340030 0xedcbffcf 12
prove 0xee6af1ff 0x11950e00 12
prove 0x00000000 0xffffffff 12
--------------------------8<-----------------------------------------

Phil Sutter (3):
  tc: pedit: Fix layered op parsing
  tc: pedit: Fix parse_cmd()
  tc: pedit: Fix retain value for ihl adjustments

 tc/m_pedit.c | 24 ++++++++----------------
 tc/p_ip.c    |  2 +-
 2 files changed, 9 insertions(+), 17 deletions(-)

-- 
2.7.2

[iproute PATCH 1/3] tc: pedit: Fix layered op parsing

[Phil Sutter]

After lookup of the layered op submodule, pedit would pass argv and argc
including the layered op identifier at first position which confused the
submodule parser. Fix this by calling NEXT_ARG() before calling the
parse_peopt() callback.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_pedit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index 4fdd189d7d9c6..455e4ffd4b2bb 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -421,6 +421,7 @@ parse_munge(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel)
 			p = get_pedit_kind(k);
 			if (NULL == p)
 				goto bad_val;
+			NEXT_ARG();
 			res = p->parse_peopt(&argc, &argv, sel,&tkey);
 			if (res < 0) {
 				fprintf(stderr,"bad pedit parsing\n");
-- 
2.7.2

[iproute PATCH 2/3] tc: pedit: Fix parse_cmd()

[Phil Sutter]

This was horribly broken:
* pack_key8() and pack_key16() ...
  * missed to invert retain value when applying it to the mask,
  * did not sanitize val by ANDing it with retain,
  * and ignored the mask which is necessary for 'invert' command.
* pack_key16() did not convert mask to network byte order.
* Changing the retain value for 'invert' or 'retain' operation seems
  just plain wrong.
* While here, also got rid of unnecessary offset sanitization in
  pack_key32().
* Simplify code a bit by always assigning the local mask variable to
  tkey->mask before calling any of the pack_key*() variants.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_pedit.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index 455e4ffd4b2bb..a314f482cd9c0 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -152,8 +152,6 @@ pack_key32(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
 
 	tkey->val = htonl(tkey->val & retain);
 	tkey->mask = htonl(tkey->mask | ~retain);
-	/* jamal remove this - it is not necessary given the if check above */
-	tkey->off &= ~3;
 	return pack_key(sel,tkey);
 }
 
@@ -176,11 +174,8 @@ pack_key16(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
 	}
 
 	stride = 8 * ind;
-	tkey->val = htons(tkey->val);
-	tkey->val <<= stride;
-	tkey->mask <<= stride;
-	retain <<= stride;
-	tkey->mask = retain|m[ind];
+	tkey->val = htons(tkey->val & retain) << stride;
+	tkey->mask = (htons(tkey->mask | ~retain) << stride) | m[ind];
 
 	tkey->off &= ~3;
 
@@ -204,10 +199,8 @@ pack_key8(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
 	ind = tkey->off & 3;
 
 	stride = 8 * ind;
-	tkey->val <<= stride;
-	tkey->mask <<= stride;
-	retain <<= stride;
-	tkey->mask = retain|m[ind];
+	tkey->val = (tkey->val & retain) << stride;
+	tkey->mask = ((tkey->mask | ~retain) << stride) | m[ind];
 
 	tkey->off &= ~3;
 
@@ -268,13 +261,13 @@ parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type,__u32 retain,struct t
 		o = 0xFFFFFFFF;
 
 	if (matches(*argv, "invert") == 0) {
-		retain = val = mask = o;
+		val = mask = o;
 	} else if (matches(*argv, "set") == 0) {
 		NEXT_ARG();
 		if (parse_val(&argc, &argv, &val, type))
 			return -1;
 	} else if (matches(*argv, "preserve") == 0) {
-		retain = mask = o;
+		retain = 0;
 	} else {
 		if (matches(*argv, "clear") != 0)
 			return -1;
@@ -290,19 +283,17 @@ parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type,__u32 retain,struct t
 	}
 
 	tkey->val = val;
+	tkey->mask = mask;
 
 	if (len == 1) {
-		tkey->mask = 0xFF;
 		res = pack_key8(retain,sel,tkey);
 		goto done;
 	}
 	if (len == 2) {
-		tkey->mask = mask;
 		res = pack_key16(retain,sel,tkey);
 		goto done;
 	}
 	if (len == 4) {
-		tkey->mask = mask;
 		res = pack_key32(retain,sel,tkey);
 		goto done;
 	}
-- 
2.7.2

[iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Phil Sutter]

Since the IP Header Length field is just half a byte, adjust retain to
only match these bits so the Version field is not overwritten by
accident.

The whole concept is actually broken due to dependency on endianness
which pedit ignores.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/p_ip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/p_ip.c b/tc/p_ip.c
index 08fdbaa41fbf4..10e4bebc71d94 100644
--- a/tc/p_ip.c
+++ b/tc/p_ip.c
@@ -58,7 +58,7 @@ parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_ke
 	if (strcmp(*argv, "ihl") == 0) {
 		NEXT_ARG();
 		tkey->off = 0;
-		res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32,0x0f,sel,tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "protocol") == 0) {
-- 
2.7.2

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Stephen Hemminger]

On Wed, 2 Mar 2016 11:20:31 +0000
Phil Sutter <phil@nwl.cc> wrote:

> +		res = parse_cmd(&argc, &argv, 1, TU32,0x0f,sel,tkey);
>  		goto done;
Please add whitespace after ,

[iproute PATCH v2 0/4] tc: pedit fixes

[Phil Sutter]

While implementing an implementation example for a pedit man page, I
noticed several issues with the current code. The following patch
series addreses them.

In order to validate my changes, I implemented a simple unit tester.
It requires the following hack:

--------------------------8<-----------------------------------------
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -513,6 +513,12 @@ parse_pedit(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, stru
                }
        }
 
+       fprintf(stderr, "mask=0x%x\n", sel.keys[0].mask);
+       fprintf(stderr, "val=0x%x\n", sel.keys[0].val);
+       fprintf(stderr, "off=0x%x\n", sel.keys[0].off);
+       fprintf(stderr, "at=0x%x\n", sel.keys[0].at);
+       fprintf(stderr, "offmask=0x%x\n", sel.keys[0].offmask);
+       fprintf(stderr, "shift=0x%x\n", sel.keys[0].shift);
        tail = NLMSG_TAIL(n);
        addattr_l(n, MAX_MSG, tca_id, NULL, 0);
        addattr_l(n, MAX_MSG, TCA_PEDIT_PARMS,&sel, sizeof(sel.sel)+sel.sel.nkeys*sizeof(struct tc_pedit_key));
--------------------------8<-----------------------------------------

With this in place, the following simple shell script can be used to
check generated permutation values:

--------------------------8<-----------------------------------------
#!/bin/bash

run_tc() {
	echo "testing $@"
	eval "$(./tc/tc filter add dev bla123 parent root u32 \
		match u32 0 0 action pedit munge $@ 2>&1 | \
		grep -e '^\(mask\|val\|off\|at\|offmask\|shift\)=')"
}

prove() { # (oval, check, ooff)
	local oval=$1
	local check=$2
	local ooff=$3
	[[ $ooff -eq $off ]] || {
		echo "Wrong offset ($off instead of $ooff)"
		#exit 1
	}
	[[ $(((oval & mask) ^ val)) -eq $check ]] || {
		echo "failed for $oval, should be $check but is 0x$(echo "obase = 16; $(((oval & mask) ^ val))" | bc)"
		echo "mask=$mask"
		echo "val=$val"
		echo "off=$off"
		echo "at=$at"
		echo "offmask=$offmask"
		echo "shift=$shift"
		#exit 1
	}
}

run_tc ip df set 0
prove 0xffffffff 0xffbfffff 4
prove 0x00400000 0x00000000 4
prove 0xffbfffff 0xffbfffff 4
prove 0x00000000 0x00000000 4

run_tc ip df set 0x40
prove 0xffffffff 0xffffffff 4
prove 0x00400000 0x00400000 4
prove 0xffbfffff 0xffffffff 4
prove 0x00000000 0x00400000 4

run_tc ip df preserve
prove 0xffffffff 0xffffffff 4
prove 0x00400000 0x00400000 4
prove 0xffbfffff 0xffbfffff 4
prove 0x00000000 0x00000000 4

run_tc ip df invert
prove 0xffffffff 0xffbfffff 4
prove 0x00400000 0x00000000 4
prove 0xffbfffff 0xffffffff 4
prove 0x00000000 0x00400000 4

run_tc ip mf set 0x20
prove 0xfffff2ff 0xfffff2ff 4
prove 0x00203000 0x00203000 4
prove 0xfadfffff 0xfaffffff 4
prove 0x00000000 0x00200000 4

run_tc ip ihl set 0x04
prove 0xffffffff 0xfffffff4 0
prove 0x00000005 0x00000004 0
prove 0xfffffff0 0xfffffff4 0
prove 0x00000000 0x00000004 0

run_tc ip tos set 0
prove 0xffffffff 0xffff00ff 0
prove 0x00002300 0x00000000 0
prove 0xffff00ff 0xffff00ff 0
prove 0x00000000 0x00000000 0

run_tc ip tos set 17
prove 0xffffffff 0xffff11ff 0
prove 0x00002300 0x00001100 0
prove 0xffff00ff 0xffff11ff 0
prove 0x00000000 0x00001100 0

run_tc ip tos invert
prove 0xffffffff 0xffff00ff 0
prove 0x00002300 0x0000DC00 0
prove 0xffff00ff 0xffffffff 0
prove 0x00000000 0x0000ff00 0

run_tc ip tos preserve
prove 0xffffffff 0xffffffff 0
prove 0x00002300 0x00002300 0
prove 0xffff00ff 0xffff00ff 0
prove 0x00000000 0x00000000 0

run_tc ip tos clear
prove 0xffffffff 0xffff00ff 0
prove 0x00002300 0x00000000 0
prove 0xffff00ff 0xffff00ff 0
prove 0x00000000 0x00000000 0

run_tc ip dport set 0
prove 0xffffffff 0x0000ffff 20
prove 0x12340000 0x00000000 20
prove 0x0000ffff 0x0000ffff 20
prove 0x00000000 0x00000000 20

run_tc ip dport set 27374
prove 0xfffff22f 0xee6af22f 20
prove 0x12340010 0xee6a0010 20
prove 0xee6aff0f 0xee6aff0f 20
prove 0xee6a0000 0xee6a0000 20

run_tc ip dport invert
prove 0xffffffff 0x0000ffff 20
prove 0x12340000 0xEDCB0000 20
prove 0x00000000 0xffff0000 20

run_tc ip dport preserve
prove 0xffffffff 0xffffffff 20
prove 0x12340000 0x12340000 20
prove 0xee6affff 0xee6affff 20
prove 0xee6a0000 0xee6a0000 20

run_tc ip dport clear
prove 0xffff3fff 0x00003fff 20
prove 0x12340030 0x00000030 20
prove 0xee6af1ff 0x0000f1ff 20
prove 0x00000000 0x00000000 20


run_tc ip src set 0.0.0.0
prove 0xffff3fff 0x00000000 12
prove 0x12340030 0x00000000 12
prove 0xee6af1ff 0x00000000 12
prove 0x00000000 0x00000000 12

run_tc ip src set 1.2.3.4
prove 0xffff3fff 0x01020304 12
prove 0x12340030 0x01020304 12
prove 0xee6af1ff 0x01020304 12
prove 0x00000000 0x01020304 12

run_tc ip src clear
prove 0xffff3fff 0x00000000 12
prove 0x12340030 0x00000000 12
prove 0xee6af1ff 0x00000000 12
prove 0x00000000 0x00000000 12

run_tc ip src preserve
prove 0xffff3fff 0xffff3fff 12
prove 0x12340030 0x12340030 12
prove 0xee6af1ff 0xee6af1ff 12
prove 0x00000000 0x00000000 12

run_tc ip src invert
prove 0xffff3fff 0x0000c000 12
prove 0x12340030 0xedcbffcf 12
prove 0xee6af1ff 0x11950e00 12
prove 0x00000000 0xffffffff 12
--------------------------8<-----------------------------------------

Changes since v1:
- rebased onto current master
- added required whitespace after comma in patch 3
- new patch 4 fixing remaining checkpatch.pl issues in tc/p_ip.c

Phil Sutter (4):
  tc: pedit: Fix layered op parsing
  tc: pedit: Fix parse_cmd()
  tc: pedit: Fix retain value for ihl adjustments
  tc/p_ip.c: Lint through checkpatch.pl

 tc/m_pedit.c | 24 ++++++++----------------
 tc/p_ip.c    | 38 ++++++++++++++++++++------------------
 2 files changed, 28 insertions(+), 34 deletions(-)

-- 
2.7.2

[iproute PATCH v2 1/4] tc: pedit: Fix layered op parsing

[Phil Sutter]

After lookup of the layered op submodule, pedit would pass argv and argc
including the layered op identifier at first position which confused the
submodule parser. Fix this by calling NEXT_ARG() before calling the
parse_peopt() callback.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_pedit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index 4fdd189d7d9c6..455e4ffd4b2bb 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -421,6 +421,7 @@ parse_munge(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel)
 			p = get_pedit_kind(k);
 			if (NULL == p)
 				goto bad_val;
+			NEXT_ARG();
 			res = p->parse_peopt(&argc, &argv, sel,&tkey);
 			if (res < 0) {
 				fprintf(stderr,"bad pedit parsing\n");
-- 
2.7.2

[iproute PATCH v2 2/4] tc: pedit: Fix parse_cmd()

[Phil Sutter]

This was horribly broken:
* pack_key8() and pack_key16() ...
  * missed to invert retain value when applying it to the mask,
  * did not sanitize val by ANDing it with retain,
  * and ignored the mask which is necessary for 'invert' command.
* pack_key16() did not convert mask to network byte order.
* Changing the retain value for 'invert' or 'retain' operation seems
  just plain wrong.
* While here, also got rid of unnecessary offset sanitization in
  pack_key32().
* Simplify code a bit by always assigning the local mask variable to
  tkey->mask before calling any of the pack_key*() variants.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_pedit.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index 455e4ffd4b2bb..a314f482cd9c0 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -152,8 +152,6 @@ pack_key32(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
 
 	tkey->val = htonl(tkey->val & retain);
 	tkey->mask = htonl(tkey->mask | ~retain);
-	/* jamal remove this - it is not necessary given the if check above */
-	tkey->off &= ~3;
 	return pack_key(sel,tkey);
 }
 
@@ -176,11 +174,8 @@ pack_key16(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
 	}
 
 	stride = 8 * ind;
-	tkey->val = htons(tkey->val);
-	tkey->val <<= stride;
-	tkey->mask <<= stride;
-	retain <<= stride;
-	tkey->mask = retain|m[ind];
+	tkey->val = htons(tkey->val & retain) << stride;
+	tkey->mask = (htons(tkey->mask | ~retain) << stride) | m[ind];
 
 	tkey->off &= ~3;
 
@@ -204,10 +199,8 @@ pack_key8(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
 	ind = tkey->off & 3;
 
 	stride = 8 * ind;
-	tkey->val <<= stride;
-	tkey->mask <<= stride;
-	retain <<= stride;
-	tkey->mask = retain|m[ind];
+	tkey->val = (tkey->val & retain) << stride;
+	tkey->mask = ((tkey->mask | ~retain) << stride) | m[ind];
 
 	tkey->off &= ~3;
 
@@ -268,13 +261,13 @@ parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type,__u32 retain,struct t
 		o = 0xFFFFFFFF;
 
 	if (matches(*argv, "invert") == 0) {
-		retain = val = mask = o;
+		val = mask = o;
 	} else if (matches(*argv, "set") == 0) {
 		NEXT_ARG();
 		if (parse_val(&argc, &argv, &val, type))
 			return -1;
 	} else if (matches(*argv, "preserve") == 0) {
-		retain = mask = o;
+		retain = 0;
 	} else {
 		if (matches(*argv, "clear") != 0)
 			return -1;
@@ -290,19 +283,17 @@ parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type,__u32 retain,struct t
 	}
 
 	tkey->val = val;
+	tkey->mask = mask;
 
 	if (len == 1) {
-		tkey->mask = 0xFF;
 		res = pack_key8(retain,sel,tkey);
 		goto done;
 	}
 	if (len == 2) {
-		tkey->mask = mask;
 		res = pack_key16(retain,sel,tkey);
 		goto done;
 	}
 	if (len == 4) {
-		tkey->mask = mask;
 		res = pack_key32(retain,sel,tkey);
 		goto done;
 	}
-- 
2.7.2

[iproute PATCH v2 3/4] tc: pedit: Fix retain value for ihl adjustments

[Phil Sutter]

Since the IP Header Length field is just half a byte, adjust retain to
only match these bits so the Version field is not overwritten by
accident.

The whole concept is actually broken due to dependency on endianness
which pedit ignores.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/p_ip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/p_ip.c b/tc/p_ip.c
index 08fdbaa41fbf4..8f0649134566b 100644
--- a/tc/p_ip.c
+++ b/tc/p_ip.c
@@ -58,7 +58,7 @@ parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_ke
 	if (strcmp(*argv, "ihl") == 0) {
 		NEXT_ARG();
 		tkey->off = 0;
-		res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, 0x0f, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "protocol") == 0) {
-- 
2.7.2

[iproute PATCH v2 4/4] tc/p_ip.c: Lint through checkpatch.pl

[Phil Sutter]

This diff was generated by using the fix-inplace option of checkpatch.pl
and breaking the overlong lines of parse_ip() and parse_ip6()
signatures.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/p_ip.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/tc/p_ip.c b/tc/p_ip.c
index 8f0649134566b..9179698bf9f8c 100644
--- a/tc/p_ip.c
+++ b/tc/p_ip.c
@@ -24,7 +24,8 @@
 #include "m_pedit.h"
 
 static int
-parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
+parse_ip(int *argc_p, char ***argv_p,
+	 struct tc_pedit_sel *sel, struct tc_pedit_key *tkey)
 {
 	int res = -1;
 	int argc = *argc_p;
@@ -36,13 +37,13 @@ parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_ke
 	if (strcmp(*argv, "src") == 0) {
 		NEXT_ARG();
 		tkey->off = 12;
-		res = parse_cmd(&argc, &argv, 4, TIPV4,RU32,sel,tkey);
+		res = parse_cmd(&argc, &argv, 4, TIPV4, RU32, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "dst") == 0) {
 		NEXT_ARG();
 		tkey->off = 16;
-		res = parse_cmd(&argc, &argv, 4, TIPV4,RU32,sel,tkey);
+		res = parse_cmd(&argc, &argv, 4, TIPV4, RU32, sel, tkey);
 		goto done;
 	}
 	/* jamal - look at these and make them either old or new
@@ -52,7 +53,7 @@ parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_ke
 	if (strcmp(*argv, "tos") == 0 || matches(*argv, "dsfield") == 0) {
 		NEXT_ARG();
 		tkey->off = 1;
-		res = parse_cmd(&argc, &argv,  1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv,  1, TU32, RU8, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "ihl") == 0) {
@@ -64,82 +65,83 @@ parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_ke
 	if (strcmp(*argv, "protocol") == 0) {
 		NEXT_ARG();
 		tkey->off = 9;
-		res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
 		goto done;
 	}
 	/* jamal - fix this */
 	if (matches(*argv, "precedence") == 0) {
 		NEXT_ARG();
 		tkey->off = 1;
-		res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
 		goto done;
 	}
 	/* jamal - validate this at some point */
 	if (strcmp(*argv, "nofrag") == 0) {
 		NEXT_ARG();
 		tkey->off = 6;
-		res = parse_cmd(&argc, &argv, 1, TU32,0x3F,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, 0x3F, sel, tkey);
 		goto done;
 	}
 	/* jamal - validate this at some point */
 	if (strcmp(*argv, "firstfrag") == 0) {
 		NEXT_ARG();
 		tkey->off = 6;
-		res = parse_cmd(&argc, &argv, 1, TU32,0x1F,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, 0x1F, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "ce") == 0) {
 		NEXT_ARG();
 		tkey->off = 6;
-		res = parse_cmd(&argc, &argv, 1, TU32,0x80,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, 0x80, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "df") == 0) {
 		NEXT_ARG();
 		tkey->off = 6;
-		res = parse_cmd(&argc, &argv, 1, TU32,0x40,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, 0x40, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "mf") == 0) {
 		NEXT_ARG();
 		tkey->off = 6;
-		res = parse_cmd(&argc, &argv, 1, TU32,0x20,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, 0x20, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "dport") == 0) {
 		NEXT_ARG();
 		tkey->off = 22;
-		res = parse_cmd(&argc, &argv, 2, TU32,RU16,sel,tkey);
+		res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "sport") == 0) {
 		NEXT_ARG();
 		tkey->off = 20;
-		res = parse_cmd(&argc, &argv, 2, TU32,RU16,sel,tkey);
+		res = parse_cmd(&argc, &argv, 2, TU32, RU16, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "icmp_type") == 0) {
 		NEXT_ARG();
 		tkey->off = 20;
-		res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
 		goto done;
 	}
 	if (strcmp(*argv, "icmp_code") == 0) {
 		NEXT_ARG();
 		tkey->off = 20;
-		res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey);
+		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
 		goto done;
 	}
 	return -1;
 
-      done:
+done:
 	*argc_p = argc;
 	*argv_p = argv;
 	return res;
 }
 
 static int
-parse_ip6(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey)
+parse_ip6(int *argc_p, char ***argv_p,
+	  struct tc_pedit_sel *sel, struct tc_pedit_key *tkey)
 {
 	int res = -1;
 	return res;
-- 
2.7.2

Re: [iproute PATCH v2 0/4] tc: pedit fixes

[Stephen Hemminger]

On Wed,  2 Mar 2016 19:45:39 +0100
Phil Sutter <phil@nwl.cc> wrote:

> While implementing an implementation example for a pedit man page, I
> noticed several issues with the current code. The following patch
> series addreses them.

Jamal wrote pedit, does he have any comments on this?

Re: [iproute PATCH v2 0/4] tc: pedit fixes

[Phil Sutter]

On Wed, Mar 02, 2016 at 11:05:33AM -0800, Stephen Hemminger wrote:
> On Wed,  2 Mar 2016 19:45:39 +0100
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > While implementing an implementation example for a pedit man page, I
> > noticed several issues with the current code. The following patch
> > series addreses them.
> 
> Jamal wrote pedit, does he have any comments on this?

I didn't consult him in private, but appreciate his review of course.

Re: [iproute PATCH v2 4/4] tc/p_ip.c: Lint through checkpatch.pl

[Stephen Hemminger]

On Wed, 2 Mar 2016 18:45:43 +0000
Phil Sutter <phil@nwl.cc> wrote:

> This diff was generated by using the fix-inplace option of checkpatch.pl
> and breaking the overlong lines of parse_ip() and parse_ip6()
> signatures.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

You need to resubmit this series, my other cleanups made much of this moot.

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Jamal Hadi Salim]

On 16-03-02 12:54 PM, Stephen Hemminger wrote:
> On Wed, 2 Mar 2016 11:20:31 +0000
> Phil Sutter <phil@nwl.cc> wrote:
>
>> +		res = parse_cmd(&argc, &argv, 1, TU32,0x0f,sel,tkey);
>>   		goto done;
> Please add whitespace after ,
>

There are a few whitespace issues in that code that would be nice to fix
given the cleanup opportunity.
The patches look good to me. Phil, maybe get rid of that comment at the
top which was worrying about endianness. I think you fixed it.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Your tests in patch 0 are nice and could go in tests/ directory.
I can send you some of the basic tests i run via the kernel; they look
something of the form:

#
#RFC
#dst MAC starts at -14
#src MAC at -8
#ethertype at -2
#
#
tc filter add dev eth0 parent ffff: protocol ip prio 10 u32 \
match ip src 192.168.1.10/32 flowid 1:2 \
action pedit munge offset -14 u16 set 0x0000 \
munge offset -12 u32 set 0x00010100 \
munge offset -8 u32 set 0x0aaf0100 \
munge offset -4 u32 set 0x0008ec06 pipe \
action mirred egress redirect dev eth1
#
#

These would of course require more of a larger setup to vet
and running tcpdump to check the correct bytes are being
modified.

cheers,
jamal

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Jamal Hadi Salim]

Phil, there is one thing i noticed in your patch - dont think
it is a big deal but you are doing htons on an int
(instead of u16).

cheers,
jamal

On 16-03-03 09:21 AM, Jamal Hadi Salim wrote:
> On 16-03-02 12:54 PM, Stephen Hemminger wrote:
>> On Wed, 2 Mar 2016 11:20:31 +0000
>> Phil Sutter <phil@nwl.cc> wrote:
>>
>>> +        res = parse_cmd(&argc, &argv, 1, TU32,0x0f,sel,tkey);
>>>           goto done;
>> Please add whitespace after ,
>>
>
> There are a few whitespace issues in that code that would be nice to fix
> given the cleanup opportunity.
> The patches look good to me. Phil, maybe get rid of that comment at the
> top which was worrying about endianness. I think you fixed it.
>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Your tests in patch 0 are nice and could go in tests/ directory.
> I can send you some of the basic tests i run via the kernel; they look
> something of the form:
>
> #
> #RFC
> #dst MAC starts at -14
> #src MAC at -8
> #ethertype at -2
> #
> #
> tc filter add dev eth0 parent ffff: protocol ip prio 10 u32 \
> match ip src 192.168.1.10/32 flowid 1:2 \
> action pedit munge offset -14 u16 set 0x0000 \
> munge offset -12 u32 set 0x00010100 \
> munge offset -8 u32 set 0x0aaf0100 \
> munge offset -4 u32 set 0x0008ec06 pipe \
> action mirred egress redirect dev eth1
> #
> #
>
> These would of course require more of a larger setup to vet
> and running tcpdump to check the correct bytes are being
> modified.
>
> cheers,
> jamal

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Phil Sutter]

On Thu, Mar 03, 2016 at 09:28:27AM -0500, Jamal Hadi Salim wrote:
> Phil, there is one thing i noticed in your patch - dont think
> it is a big deal but you are doing htons on an int
> (instead of u16).

Indeed, I totally overlooked that. Does not look like there's an easy
fix though, as all tc_pedit_key fields are u32. OTOH htons() will cast
to u16 first, therefore cutting off any higher bits. Looks like the code
does the right thing anyway, and the test results fortify that.

Do you see a cleaner way to implement this? Otherwise I would vote for
leaving it as a little surprise to future readers. ;)

Cheers, Phil

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Phil Sutter]

Hi,

On Thu, Mar 03, 2016 at 09:21:23AM -0500, Jamal Hadi Salim wrote:
> On 16-03-02 12:54 PM, Stephen Hemminger wrote:
> > On Wed, 2 Mar 2016 11:20:31 +0000
> > Phil Sutter <phil@nwl.cc> wrote:
> >
> >> +		res = parse_cmd(&argc, &argv, 1, TU32,0x0f,sel,tkey);
> >>   		goto done;
> > Please add whitespace after ,
> >
> 
> There are a few whitespace issues in that code that would be nice to fix
> given the cleanup opportunity.

In v2, I added a separate patch which makes that file conform to
checkpatch.pl (used the --fix-inplace option and tweaked the output a
bit). So that should be fine already.

> The patches look good to me. Phil, maybe get rid of that comment at the
> top which was worrying about endianness. I think you fixed it.

I'm not so sure. The kernel explicitly takes care to get the bit
ordering right:

| struct iphdr
|   {
| #if __BYTE_ORDER == __LITTLE_ENDIAN
|     unsigned int ihl:4;
|     unsigned int version:4;
| #elif __BYTE_ORDER == __BIG_ENDIAN
|     unsigned int version:4;
|     unsigned int ihl:4;
| #else
| # error "Please fix <bits/endian.h>"
| #endif

act_pedit though just mangles the whole byte as-is, and if that was
correct, we would not have to go that extra mile in struct iphdr, or do
we?

> Your tests in patch 0 are nice and could go in tests/ directory.
> I can send you some of the basic tests i run via the kernel; they look
> something of the form:
> 
> #
> #RFC
> #dst MAC starts at -14
> #src MAC at -8
> #ethertype at -2
> #
> #
> tc filter add dev eth0 parent ffff: protocol ip prio 10 u32 \
> match ip src 192.168.1.10/32 flowid 1:2 \
> action pedit munge offset -14 u16 set 0x0000 \
> munge offset -12 u32 set 0x00010100 \
> munge offset -8 u32 set 0x0aaf0100 \
> munge offset -4 u32 set 0x0008ec06 pipe \
> action mirred egress redirect dev eth1
> #
> #
> 
> These would of course require more of a larger setup to vet
> and running tcpdump to check the correct bytes are being
> modified.

Since I am lazy, I wanted to have as much automation as possible while
testing. Therefore I just assumed that act_pedit does the right thing
all the time, and iproute just has to feed it correct values. Given the
scope of this patch, this is also completely sufficient. Of course, the
tests/ directory would benefit more from a full test. But since
automation then becomes tricky, I'm not sure it makes much sense to
deliberately write code for that.

Thanks for the review,

Phil

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Jamal Hadi Salim]

On 16-03-03 09:32 AM, Phil Sutter wrote:
> Hi,
>

>
>> The patches look good to me. Phil, maybe get rid of that comment at the
>> top which was worrying about endianness. I think you fixed it.
>
> I'm not so sure. The kernel explicitly takes care to get the bit
> ordering right:
>
[..]
> act_pedit though just mangles the whole byte as-is, and if that was
> correct, we would not have to go that extra mile in struct iphdr, or do
> we?
>

I meant in general - the note to say that there are endianes issues
should go.


>> These would of course require more of a larger setup to vet
>> and running tcpdump to check the correct bytes are being
>> modified.
>

Indeed - That is how i normally would test. It is more complex.
Your scheme is good - but will not catch a kernel bug.

> Since I am lazy, I wanted to have as much automation as possible while
> testing. Therefore I just assumed that act_pedit does the right thing
> all the time,

famous last words ;->

> and iproute just has to feed it correct values. Given the
> scope of this patch, this is also completely sufficient. Of course, the
> tests/ directory would benefit more from a full test. But since
> automation then becomes tricky, I'm not sure it makes much sense to
> deliberately write code for that.
>

Your test is still useful and i think should go into the tests dir.

cheers,
jamal

> Thanks for the review,
>
> Phil
>

Re: [iproute PATCH 3/3] tc: pedit: Fix retain value for ihl adjustments

[Phil Sutter]

Hi,

On Mon, Mar 07, 2016 at 06:21:17AM -0500, Jamal Hadi Salim wrote:
> On 16-03-03 09:32 AM, Phil Sutter wrote:
> >> The patches look good to me. Phil, maybe get rid of that comment at the
> >> top which was worrying about endianness. I think you fixed it.
> >
> > I'm not so sure. The kernel explicitly takes care to get the bit
> > ordering right:
> >
> [..]
> > act_pedit though just mangles the whole byte as-is, and if that was
> > correct, we would not have to go that extra mile in struct iphdr, or do
> > we?
> >
> 
> I meant in general - the note to say that there are endianes issues
> should go.

Sorry, I didn't get that yet: To me it looks as if on a big-endian
system, the code will actually change the Version field instead of IHL.
Probably the proof of the pudding is in the eating, so I'll try to get
access to a big-endian system for testing.

> >> These would of course require more of a larger setup to vet
> >> and running tcpdump to check the correct bytes are being
> >> modified.
> >
> 
> Indeed - That is how i normally would test. It is more complex.
> Your scheme is good - but will not catch a kernel bug.

Sure. OTOH the algorithm in act_pedit is not overly complex. Plus,
fixing one side only prevents accidental workarounds of other side's
bugs.

> > Since I am lazy, I wanted to have as much automation as possible while
> > testing. Therefore I just assumed that act_pedit does the right thing
> > all the time,
> 
> famous last words ;->

:)

> > and iproute just has to feed it correct values. Given the
> > scope of this patch, this is also completely sufficient. Of course, the
> > tests/ directory would benefit more from a full test. But since
> > automation then becomes tricky, I'm not sure it makes much sense to
> > deliberately write code for that.
> >
> 
> Your test is still useful and i think should go into the tests dir.

OK, I'll give it a thought.

Thanks, Phil