[PATCH iproute2] pedit action: add mask for changing bits.

[PATCH iproute2] pedit action: add mask for changing bits.

[Anton 'EvilMan' Danilov]

This patch adds the ability to set the mask for changing bits. The
mask parameter is optional.

Example of usage:
#rewrite vlan id. cos field in 802.1q header is unchanged.
ip link add link eth0 name eth0.99 type vlan id 99 reorder_hdr off
egress-qos-map 0:7
tc qdisc add dev eth0 root handle 1: prio bands 8
tc filter add dev eth0 parent 1: protocol all pref 1 u32
tc filter add dev eth0 parent 1: protocol all pref 1 handle ::1 u32 \
   match u16 0x8100 0xffff at -6 \
   match u16 0x0063 0x0fff at -4 \
   classid 1:5 \
   action pedit munge offset -4 u16 set 0x000d 0x0fff


diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index 7499846..f081eff 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -44,7 +44,8 @@ explain(void)
		"\t\tNOTE: offval is byte offset, must be multiple of 4\n "
		"\t\tNOTE: maskval is a 32 bit hex number\n "
		"\t\tNOTE: shiftval is a is a shift value\n "
-		"\t\tCMD:= clear | invert | set <setval>| retain\n "
+		"\t\tCMD:= clear [<mask>] | invert [<mask>]\n"
+		" \t\t| set <setval> [<mask>] | retain [<mask>] \n "
		"\t<LAYERED>:= ip <ipdata> | ip6 <ip6data> \n "
		" \t\t| udp <udpdata> | tcp <tcpdata> | icmp <icmpdata> \n"
		"For Example usage look at the examples directory\n");
@@ -152,7 +153,7 @@ 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);
+	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);
@@ -186,12 +187,12 @@ pack_key16(__u32 retain,struct tc_pedit_sel
*sel,struct tc_pedit_key *tkey)

	stride = 8 * ind;
	tkey->val = htons(tkey->val);
+	tkey->mask = htons(tkey->mask);
	if (stride > 0) {
		tkey->val <<= stride;
-		tkey->mask <<= stride;
		retain <<= stride;
	}
-	tkey->mask = retain|m[ind];
+	tkey->mask = retain|m[ind]|~(tkey->mask << stride);

	tkey->off &= ~3;

@@ -216,16 +217,15 @@ pack_key8(__u32 retain,struct tc_pedit_sel
*sel,struct tc_pedit_key *tkey)
	}

	if (tkey->val > 0xFF || tkey->mask > 0xFF) {
-		fprintf(stderr, "pack_key8 bad value (val %x mask %x\n", tkey->val,
tkey->mask);
+		fprintf(stderr, "pack_key8 bad value (val %x mask %x)\n",
tkey->val, tkey->mask);
		return -1;
	}

	ind = tkey->off & 3;
	stride = 8 * ind;
	tkey->val <<= stride;
-	tkey->mask <<= stride;
	retain <<= stride;
-	tkey->mask = retain|m[ind];
+	tkey->mask = retain|m[ind]|~(tkey->mask << stride);
	tkey->off &= ~3;

	if (pedit_debug)
@@ -309,17 +309,26 @@ parse_cmd(int *argc_p, char ***argv_p, __u32
len, int type,__u32 retain,struct t
	tkey->val = val;

	if (len == 1) {
-		tkey->mask = 0xFF;
+		if (!get_u32(&tkey->mask, *argv, 0)) {
+			argc--; argv++;
+		} else
+			tkey->mask = 0xFF;
		res = pack_key8(retain,sel,tkey);
		goto done;
	}
	if (len == 2) {
-		tkey->mask = mask;
+		if (!get_u32(&tkey->mask, *argv, 0)) {
+			argc--; argv++;
+		} else
+			tkey->mask = 0xFFFF;
		res = pack_key16(retain,sel,tkey);
		goto done;
	}
	if (len == 4) {
-		tkey->mask = mask;
+		if (!get_u32(&tkey->mask, *argv, 0)) {
+			argc--; argv++;
+		} else
+			tkey->mask = 0xFFFFFFFF;
		res = pack_key32(retain,sel,tkey);
		goto done;
	}


-- 
Anton.

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[jamal]

Hi Anton,

On Sat, 2012-02-25 at 22:01 +0400, Anton 'EvilMan' Danilov wrote:
> This patch adds the ability to set the mask for changing bits. The
> mask parameter is optional.
> 
> Example of usage:
> #rewrite vlan id. cos field in 802.1q header is unchanged.
> ip link add link eth0 name eth0.99 type vlan id 99 reorder_hdr off
> egress-qos-map 0:7
> tc qdisc add dev eth0 root handle 1: prio bands 8
> tc filter add dev eth0 parent 1: protocol all pref 1 u32
> tc filter add dev eth0 parent 1: protocol all pref 1 handle ::1 u32 \
>    match u16 0x8100 0xffff at -6 \
>    match u16 0x0063 0x0fff at -4 \
>    classid 1:5 \
>    action pedit munge offset -4 u16 set 0x000d 0x0fff

Looks like a reasonable feature to have - but it has to continue working
as before for things that dont need the mask.

> 
> diff --git a/tc/m_pedit.c b/tc/m_pedit.c
> index 7499846..f081eff 100644
> --- a/tc/m_pedit.c
> +++ b/tc/m_pedit.c

> 
> 	tkey->val = htonl(tkey->val & retain);
> -	tkey->mask = htonl(tkey->mask | ~retain);
> +	tkey->mask = htonl(~tkey->mask | ~retain);

A change like the above worries me that this may work for your use case
but will break other things that used to work. I'd like to run some
tests but dont have the cycles for the next few days. Alternatively, I
could explain some tests to you and you could validate and fix whatever
breaks.
Let me know what is best for you.

cheers,
jamal

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[Anton 'EvilMan' Danilov]

Hi, Jamal.

2012/2/27 jamal <hadi@cyberus.ca>:
>
> Looks like a reasonable feature to have - but it has to continue working
> as before for things that dont need the mask.
>
>>
>> diff --git a/tc/m_pedit.c b/tc/m_pedit.c
>> index 7499846..f081eff 100644
>> --- a/tc/m_pedit.c
>> +++ b/tc/m_pedit.c
>
>>
>>       tkey->val = htonl(tkey->val & retain);
>> -     tkey->mask = htonl(tkey->mask | ~retain);
>> +     tkey->mask = htonl(~tkey->mask | ~retain);
>
> A change like the above worries me that this may work for your use case
> but will break other things that used to work. I'd like to run some
> tests but dont have the cycles for the next few days. Alternatively, I
> could explain some tests to you and you could validate and fix whatever
> breaks.
> Let me know what is best for you.
>

Can You explain tests to me? I'll check behavior with my changes and fix breaks


-- 
Anton.

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[Jamal Hadi Salim]

Hi Anton,

Thinking about this some more:
Did you try the "retain" option? it is the xor mask to set operation.

Examples:
----
#At offset 3 retain the first nibble but set the second to 0xA
action pedit munge offset 3 u8 set 0xA0 retain 0xF
#At offset 9, retain the second nibble but set the first to 0xA
action pedit munge offset 9 u8 set 0x0A retain 0xF0
---

So if you were to point to the nibble holding the cos field
in your match, then you can set it to a new value while retaining
bitfields you dont want to change...

I was going to ask you to test the retain in addition to the
extra mask you are adding on different sets - but i am beginning 
to think the mask you are adding is not needed at all.

Here is an example of other tests i was going to ask you to 
run (this time with 16bit field):

#At offset 0, invert/clear/preserve the first 16 bits
action pedit munge offset 0 u16 invert

original data:
45000054 00004000 40013ca7 7f000001
modified data (invert):
baff0054 00004000 40013ca7 7f000001
modified data (clear):
00000054 00004000 40013ca7 7f000001
retain
45000054 00004000 40013ca7 7f000001
set 0x1234
12340054 00004000 40013ca7 7f000001

You can pipe to a mirror action to send to dummy0 and then run tcpdump
on dummy to watch the edited fields. I can describe a simple setup
i use if this still doesnt make sense.

As a side note: I think we need an action to work on VLANs since
they are such an important field. The action would push/pop and edit
VLAN fields. If you are interested in this, I can help provide guidance
(It is one of those things on my todo longtimenow but i cant seem to get
around to it).

cheers,
jamal


cheers,
jamal

On Mon, 2012-02-27 at 17:15 +0300, Anton 'EvilMan' Danilov wrote:
> Hi, Jamal.
> 
> 2012/2/27 jamal <hadi@cyberus.ca>:
> >
> > Looks like a reasonable feature to have - but it has to continue working
> > as before for things that dont need the mask.
> >
> >>
> >> diff --git a/tc/m_pedit.c b/tc/m_pedit.c
> >> index 7499846..f081eff 100644
> >> --- a/tc/m_pedit.c
> >> +++ b/tc/m_pedit.c
> >
> >>
> >>       tkey->val = htonl(tkey->val & retain);
> >> -     tkey->mask = htonl(tkey->mask | ~retain);
> >> +     tkey->mask = htonl(~tkey->mask | ~retain);
> >
> > A change like the above worries me that this may work for your use case
> > but will break other things that used to work. I'd like to run some
> > tests but dont have the cycles for the next few days. Alternatively, I
> > could explain some tests to you and you could validate and fix whatever
> > breaks.
> > Let me know what is best for you.
> >
> 
> Can You explain tests to me? I'll check behavior with my changes and fix breaks
> 
> 

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[Anton 'EvilMan' Danilov]

Hi, Jamal.

2012/2/28 Jamal Hadi Salim <jhs@mojatatu.com>:
>
> Thinking about this some more:
> Did you try the "retain" option? it is the xor mask to set operation.
>
> I was going to ask you to test the retain in addition to the
> extra mask you are adding on different sets - but i am beginning
> to think the mask you are adding is not needed at all.
>

You are right, mask option isn't needed. Retain option must be used instead it.

>
> As a side note: I think we need an action to work on VLANs since
> they are such an important field. The action would push/pop and edit
> VLAN fields. If you are interested in this, I can help provide guidance
> (It is one of those things on my todo longtimenow but i cant seem to get
> around to it).
>

Action to work on Vlans is fine idea. I'm very interested in this
action. I'll be grateful for any help.

-- 
Anton.

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[jamal]

Hi Anton,

On Wed, 2012-02-29 at 16:12 +0300, Anton 'EvilMan' Danilov wrote:

> 
> You are right, mask option isn't needed. Retain option must be used instead it.
> 

Thanks for verifying.

> Action to work on Vlans is fine idea. I'm very interested in this
> action. I'll be grateful for any help.

Lets start by defining the interface to be seen by user space
such as vlan editing/popping/pushing etc. Then i will help you
build it slowly until you are comfortable with moving with it.

As a matter of fact, lets talk offline so we dont add more noise
to the list; you can always post the RFC to the list when you are
ready.

cheers,
jamal

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[Stephen Hemminger]

On Wed, 29 Feb 2012 09:01:20 -0500
jamal <hadi@cyberus.ca> wrote:

> Hi Anton,
> 
> On Wed, 2012-02-29 at 16:12 +0300, Anton 'EvilMan' Danilov wrote:
> 
> > 
> > You are right, mask option isn't needed. Retain option must be used instead it.
> > 
> 
> Thanks for verifying.
> 
> > Action to work on Vlans is fine idea. I'm very interested in this
> > action. I'll be grateful for any help.
> 
> Lets start by defining the interface to be seen by user space
> such as vlan editing/popping/pushing etc. Then i will help you
> build it slowly until you are comfortable with moving with it.
> 
> As a matter of fact, lets talk offline so we dont add more noise
> to the list; you can always post the RFC to the list when you are
> ready.
> 
> cheers,
> jamal

Did you come to a conclusion, should it be applied as is?

Re: [PATCH iproute2] pedit action: add mask for changing bits.

[Jamal Hadi Salim]

On Wed, 2012-04-25 at 13:15 -0700, Stephen Hemminger wrote:

> Did you come to a conclusion, should it be applied as is?

No, dont apply it as is please. 
EvilMan should have a new version when he gets to it.

cheers,
jamal