[PATCH iproute2-next 0/3] Support pedit of ip6 dsfield

[PATCH iproute2-next 0/3] Support pedit of ip6 dsfield

[Petr Machata]

The pedit action supports dsfield (and its synonyms, such as tos) for IPv4,
but not for IPv6. Patch #1 of this series adds the IPv6 support. Patch #2
then adds two related examples to man page, and patch #3 removes from the
man page a claim that the extended notation is only available for IPv4.

Petr Machata (3):
  tc: p_ip6: Support pedit of IPv6 dsfield
  man: tc-pedit: Add examples for dsfield and retain
  man: tc-pedit: Drop the claim that pedit ex is only for IPv4

 man/man8/tc-pedit.8 | 39 +++++++++++++++++++++++++++++++++++----
 tc/p_ip6.c          | 16 ++++++++++++++++
 2 files changed, 51 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH iproute2-next 1/3] tc: p_ip6: Support pedit of IPv6 dsfield

[Petr Machata]

Support keywords dsfield, traffic_class and tos in the IPv6 context.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 man/man8/tc-pedit.8 | 14 ++++++++++++--
 tc/p_ip6.c          | 16 ++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/man/man8/tc-pedit.8 b/man/man8/tc-pedit.8
index bbd725c4..b44b0263 100644
--- a/man/man8/tc-pedit.8
+++ b/man/man8/tc-pedit.8
@@ -60,8 +60,8 @@ pedit - generic packet editor action
 
 .ti -8
 .IR IP6HDR_FIELD " := { "
-.BR src " | " dst " | " flow_lbl " | " payload_len " | " nexthdr " |"
-.BR hoplimit " }"
+.BR src " | " dst " | " tos " | " dsfield " | " traffic_class " | "
+.BR flow_lbl " | " payload_len " | " nexthdr " |" hoplimit " }"
 
 .ti -8
 .IR TCPHDR_FIELD " := { "
@@ -228,6 +228,16 @@ are:
 .B src
 .TQ
 .B dst
+.TP
+.B tos
+.TQ
+.B dsfield
+.TQ
+.B traffic_class
+Traffic Class, an 8-bit quantity. Because the field is shifted by 4 bits,
+it is necessary to specify the full 16-bit halfword, with the actual
+dsfield value sandwiched between 4-bit zeroes.
+.TP
 .TQ
 .B flow_lbl
 .TQ
diff --git a/tc/p_ip6.c b/tc/p_ip6.c
index 7cc7997b..b6fe81f5 100644
--- a/tc/p_ip6.c
+++ b/tc/p_ip6.c
@@ -56,6 +56,22 @@ parse_ip6(int *argc_p, char ***argv_p,
 		res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey);
 		goto done;
 	}
+	if (strcmp(*argv, "traffic_class") == 0 ||
+	    strcmp(*argv, "tos") == 0 ||
+	    strcmp(*argv, "dsfield") == 0) {
+		NEXT_ARG();
+		tkey->off = 1;
+		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
+
+		/* Shift the field by 4 bits on success. */
+		if (!res) {
+			int nkeys = sel->sel.nkeys;
+			struct tc_pedit_key *key = &sel->sel.keys[nkeys - 1];
+			key->mask = htonl(ntohl(key->mask) << 4 | 0xf);
+			key->val = htonl(ntohl(key->val) << 4);
+		}
+		goto done;
+	}
 	if (strcmp(*argv, "payload_len") == 0) {
 		NEXT_ARG();
 		tkey->off = 4;
-- 
2.20.1

[PATCH iproute2-next 2/3] man: tc-pedit: Add examples for dsfield and retain

[Petr Machata]

Describe a way to update just the DSCP and just the ECN part of the
dsfield. That is useful on its own, but also it shows how retain works.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 man/man8/tc-pedit.8 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/man/man8/tc-pedit.8 b/man/man8/tc-pedit.8
index b44b0263..54b91d3d 100644
--- a/man/man8/tc-pedit.8
+++ b/man/man8/tc-pedit.8
@@ -377,6 +377,28 @@ tc filter add dev eth0 parent ffff: u32 \\
 	action pedit ex munge tcp dport set 22
 .EE
 .RE
+
+To rewrite just part of a field, use the
+.B retain
+directive. E.g. to overwrite the DSCP part of a dsfield with $DSCP, without
+touching ECN:
+
+.RS
+.EX
+tc filter add dev eth0 ingress flower ... \\
+	action pedit ex munge ip dsfield set $((DSCP << 2)) retain 0xfc
+.EE
+.RE
+
+And vice versa, to set ECN to e.g. 1 without impacting DSCP:
+
+.RS
+.EX
+tc filter add dev eth0 ingress flower ... \\
+	action pedit ex munge ip dsfield set 1 retain 0x3
+.EE
+.RE
+
 .SH SEE ALSO
 .BR tc (8),
 .BR tc-htb (8),
-- 
2.20.1

[PATCH iproute2-next 3/3] man: tc-pedit: Drop the claim that pedit ex is only for IPv4

[Petr Machata]

This sentence predates addition of extended pedit for IPv6 packets.

Reported-by: Ido Schimmel <idosch@mellanox.com>
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 man/man8/tc-pedit.8 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/man/man8/tc-pedit.8 b/man/man8/tc-pedit.8
index 54b91d3d..ae9f7c2e 100644
--- a/man/man8/tc-pedit.8
+++ b/man/man8/tc-pedit.8
@@ -90,8 +90,7 @@ action can be used to change arbitrary packet data. The location of data to
 change can either be specified by giving an offset and size as in
 .IR RAW_OP ,
 or for header values by naming the header and field to edit the size is then
-chosen automatically based on the header field size. Currently this is supported
-only for IPv4 headers.
+chosen automatically based on the header field size.
 .SH OPTIONS
 .TP
 .B ex
-- 
2.20.1

Re: [PATCH iproute2-next 1/3] tc: p_ip6: Support pedit of IPv6 dsfield

[Stephen Hemminger]

On Fri, 27 Mar 2020 20:55:08 +0300
Petr Machata <petrm@mellanox.com> wrote:

> Support keywords dsfield, traffic_class and tos in the IPv6 context.
> 
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
>  man/man8/tc-pedit.8 | 14 ++++++++++++--
>  tc/p_ip6.c          | 16 ++++++++++++++++
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/man/man8/tc-pedit.8 b/man/man8/tc-pedit.8
> index bbd725c4..b44b0263 100644
> --- a/man/man8/tc-pedit.8
> +++ b/man/man8/tc-pedit.8
> @@ -60,8 +60,8 @@ pedit - generic packet editor action
>  
>  .ti -8
>  .IR IP6HDR_FIELD " := { "
> -.BR src " | " dst " | " flow_lbl " | " payload_len " | " nexthdr " |"
> -.BR hoplimit " }"
> +.BR src " | " dst " | " tos " | " dsfield " | " traffic_class " | "
> +.BR flow_lbl " | " payload_len " | " nexthdr " |" hoplimit " }"
>  
>  .ti -8
>  .IR TCPHDR_FIELD " := { "
> @@ -228,6 +228,16 @@ are:
>  .B src
>  .TQ
>  .B dst
> +.TP
> +.B tos
> +.TQ
> +.B dsfield
> +.TQ
> +.B traffic_class
> +Traffic Class, an 8-bit quantity. Because the field is shifted by 4 bits,
> +it is necessary to specify the full 16-bit halfword, with the actual
> +dsfield value sandwiched between 4-bit zeroes.
> +.TP
>  .TQ
>  .B flow_lbl
>  .TQ
> diff --git a/tc/p_ip6.c b/tc/p_ip6.c
> index 7cc7997b..b6fe81f5 100644
> --- a/tc/p_ip6.c
> +++ b/tc/p_ip6.c
> @@ -56,6 +56,22 @@ parse_ip6(int *argc_p, char ***argv_p,
>  		res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey);
>  		goto done;
>  	}
> +	if (strcmp(*argv, "traffic_class") == 0 ||
> +	    strcmp(*argv, "tos") == 0 ||
> +	    strcmp(*argv, "dsfield") == 0) {
> +		NEXT_ARG();
> +		tkey->off = 1;
> +		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
> +
> +		/* Shift the field by 4 bits on success. */
> +		if (!res) {
> +			int nkeys = sel->sel.nkeys;
> +			struct tc_pedit_key *key = &sel->sel.keys[nkeys - 1];
> +			key->mask = htonl(ntohl(key->mask) << 4 | 0xf);
> +			key->val = htonl(ntohl(key->val) << 4);
> +		}
> +		goto done;
> +	}
Why in the middle of the list? Why three aliases for the same value?
Since this is new code choose one and make it match what IPv6 standard
calls that field.

Re: [PATCH iproute2-next 1/3] tc: p_ip6: Support pedit of IPv6 dsfield

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

>> diff --git a/tc/p_ip6.c b/tc/p_ip6.c
>> index 7cc7997b..b6fe81f5 100644
>> --- a/tc/p_ip6.c
>> +++ b/tc/p_ip6.c
>> @@ -56,6 +56,22 @@ parse_ip6(int *argc_p, char ***argv_p,
>>  		res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey);
>>  		goto done;
>>  	}
>> +	if (strcmp(*argv, "traffic_class") == 0 ||
>> +	    strcmp(*argv, "tos") == 0 ||
>> +	    strcmp(*argv, "dsfield") == 0) {
>> +		NEXT_ARG();
>> +		tkey->off = 1;
>> +		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
>> +
>> +		/* Shift the field by 4 bits on success. */
>> +		if (!res) {
>> +			int nkeys = sel->sel.nkeys;
>> +			struct tc_pedit_key *key = &sel->sel.keys[nkeys - 1];
>> +			key->mask = htonl(ntohl(key->mask) << 4 | 0xf);
>> +			key->val = htonl(ntohl(key->val) << 4);
>> +		}
>> +		goto done;
>> +	}
> Why in the middle of the list?

Because that's the order IPv4 code does them.

> Why three aliases for the same value?
> Since this is new code choose one and make it match what IPv6 standard
> calls that field.

TOS because flower also calls it TOS, even if it's the IPv6 field.
dsfield, because the IPv4 pedit also accepts this. I'm fine with just
accepting traffic_class though.

Re: [PATCH iproute2-next 1/3] tc: p_ip6: Support pedit of IPv6 dsfield

[David Ahern]

On 3/30/20 2:32 AM, Petr Machata wrote:
> 
> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
>>> diff --git a/tc/p_ip6.c b/tc/p_ip6.c
>>> index 7cc7997b..b6fe81f5 100644
>>> --- a/tc/p_ip6.c
>>> +++ b/tc/p_ip6.c
>>> @@ -56,6 +56,22 @@ parse_ip6(int *argc_p, char ***argv_p,
>>>  		res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey);
>>>  		goto done;
>>>  	}
>>> +	if (strcmp(*argv, "traffic_class") == 0 ||
>>> +	    strcmp(*argv, "tos") == 0 ||
>>> +	    strcmp(*argv, "dsfield") == 0) {
>>> +		NEXT_ARG();
>>> +		tkey->off = 1;
>>> +		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
>>> +
>>> +		/* Shift the field by 4 bits on success. */
>>> +		if (!res) {
>>> +			int nkeys = sel->sel.nkeys;
>>> +			struct tc_pedit_key *key = &sel->sel.keys[nkeys - 1];
>>> +			key->mask = htonl(ntohl(key->mask) << 4 | 0xf);
>>> +			key->val = htonl(ntohl(key->val) << 4);
>>> +		}
>>> +		goto done;
>>> +	}
>> Why in the middle of the list?
> 
> Because that's the order IPv4 code does them.

neither parse function uses matches() so the order should not matter.
> 
>> Why three aliases for the same value?
>> Since this is new code choose one and make it match what IPv6 standard
>> calls that field.
> 
> TOS because flower also calls it TOS, even if it's the IPv6 field.
> dsfield, because the IPv4 pedit also accepts this. I'm fine with just
> accepting traffic_class though.
> 

that's probably the right thing to do since this is ipv6 related

Re: [PATCH iproute2-next 1/3] tc: p_ip6: Support pedit of IPv6 dsfield

[Petr Machata]

David Ahern <dsahern@gmail.com> writes:

> On 3/30/20 2:32 AM, Petr Machata wrote:
>> 
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>>>> diff --git a/tc/p_ip6.c b/tc/p_ip6.c
>>>> index 7cc7997b..b6fe81f5 100644
>>>> --- a/tc/p_ip6.c
>>>> +++ b/tc/p_ip6.c
>>>> @@ -56,6 +56,22 @@ parse_ip6(int *argc_p, char ***argv_p,
>>>>  		res = parse_cmd(&argc, &argv, 4, TU32, 0x0007ffff, sel, tkey);
>>>>  		goto done;
>>>>  	}
>>>> +	if (strcmp(*argv, "traffic_class") == 0 ||
>>>> +	    strcmp(*argv, "tos") == 0 ||
>>>> +	    strcmp(*argv, "dsfield") == 0) {
>>>> +		NEXT_ARG();
>>>> +		tkey->off = 1;
>>>> +		res = parse_cmd(&argc, &argv, 1, TU32, RU8, sel, tkey);
>>>> +
>>>> +		/* Shift the field by 4 bits on success. */
>>>> +		if (!res) {
>>>> +			int nkeys = sel->sel.nkeys;
>>>> +			struct tc_pedit_key *key = &sel->sel.keys[nkeys - 1];
>>>> +			key->mask = htonl(ntohl(key->mask) << 4 | 0xf);
>>>> +			key->val = htonl(ntohl(key->val) << 4);
>>>> +		}
>>>> +		goto done;
>>>> +	}
>>> Why in the middle of the list?
>> 
>> Because that's the order IPv4 code does them.
>
> neither parse function uses matches() so the order should not matter.

It was purely a consistency thing. So you both seem to imply I should
move it to the end, so I'll do that in v2.

>> 
>>> Why three aliases for the same value?
>>> Since this is new code choose one and make it match what IPv6 standard
>>> calls that field.
>> 
>> TOS because flower also calls it TOS, even if it's the IPv6 field.
>> dsfield, because the IPv4 pedit also accepts this. I'm fine with just
>> accepting traffic_class though.
>> 
>
> that's probably the right thing to do since this is ipv6 related

All right, I'll send v2 with this fix.