iproute2/tc invalid JSON in 6.0.0 for flowid

iproute2/tc invalid JSON in 6.0.0 for flowid

[Christian Pössinger]

Dear Maintainers,

I am new to the reporting process for iproute2 and this is only my second post
thus please bear with me and my MTA.

  $ tc -V
  tc utility, iproute2-6.0.0, libbpf 0.3.0

  $ tc qdisc add dev eth1 handle ffff: ingress
  $ tc filter add dev eth1 parent ffff: prio 20 protocol all u32 match ip dport 80 \
       0xffff action police rate 100000 conform-exceed drop burst 15k flowid ffff:1

  $ tc qdisc show dev eth1
  qdisc mq 0: root
  qdisc pfifo_fast 0: parent :2 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
  qdisc pfifo_fast 0: parent :1 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
  qdisc ingress ffff: parent ffff:fff1 ----------------
  
  $ tc filter show dev eth1 ingress
  filter parent ffff: protocol all pref 20 u32 chain 0
  filter parent ffff: protocol all pref 20 u32 chain 0 fh 800: ht divisor 1
  filter parent ffff: protocol all pref 20 u32 chain 0 fh 800::800 order 2048 key ht 800 bkt 0 flowid ffff:1 not_in_hw
    match 00000050/0000ffff at 20
          action order 1:  police 0x1 rate 100Kbit burst 15Kb mtu 2Kb action drop overhead 0b
          ref 1 bind 1
   
  That output looks good but my use-case is machine reading (JSON) the output
  of tc.
  
  $ tc -json filter show dev eth1 ingress
  [{"parent":"ffff:","protocol":"all","pref":20,"kind":"u32","chain":0},
   {"parent":"ffff:","protocol":"all","pref":20,"kind":"u32","chain":0,"options":{"fh":"800:","ht_divisor":1}},
   {"parent":"ffff:","protocol":"all","pref":20,"kind":"u32","chain":0,"options":{"fh":"800::800","order":2048,"key_ht":"800","bkt":"0"flowid ffff:1 ,"not_in_hw":true,"match":{"value":"50","mask":"ffff","offmask":"","off":20},"actions":[{"order":1,"kind":"police","index":1,"control_action":{"type":"drop"},"overhead":0,"ref":1,"bind":1}]}}]
   
This actually contains invalid JSON here

... "bkt":"0"flowid ffff:1 ,"not_in_hw":true, ...

It should actually read:

... "bkt":"0","flowid":"ffff:1","not_in_hw":true, ...

If you can point me to the location which could be responsible for this issue, I am happy to submit a fix to the net tree.

Thanks in advance,
Christian Poessinger

Re: iproute2/tc invalid JSON in 6.0.0 for flowid

[Francois Romieu]

Christian Pössinger <christian@poessinger.com> :
[...]
> If you can point me to the location which could be responsible for this issue, I am happy to submit a fix to the net tree.

If the completely untested patch below does not work, you may also
dig the bits in include/json_print.h and lib/json_print.c 

(Ccing Stephen as the unidentified "both" in README.devel)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index d787eb91..70098bcd 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1275,11 +1275,11 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 		fprintf(stderr, "divisor and hash missing ");
 	}
 	if (tb[TCA_U32_CLASSID]) {
+		char *fmt = !sel || !(sel->flags & TC_U32_TERMINAL) ? "*flowid %s" : "flowid %s";
 		SPRINT_BUF(b1);
-		fprintf(f, "%sflowid %s ",
-			!sel || !(sel->flags & TC_U32_TERMINAL) ? "*" : "",
-			sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]),
-					  b1));
+
+		print_string(PRINT_ANY, "flowid", fmt,
+			    sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]), b1));
 	} else if (sel && sel->flags & TC_U32_TERMINAL) {
 		print_string(PRINT_FP, NULL, "terminal flowid ", NULL);
 	}

-- 
Ueimor

Re: iproute2/tc invalid JSON in 6.0.0 for flowid

[Stephen Hemminger]

On Thu, 13 Oct 2022 17:18:13 +0200
Francois Romieu <romieu@fr.zoreil.com> wrote:

> Christian Pössinger <christian@poessinger.com> :
> [...]
> > If you can point me to the location which could be responsible for this issue, I am happy to submit a fix to the net tree.  
> 
> If the completely untested patch below does not work, you may also
> dig the bits in include/json_print.h and lib/json_print.c 
> 
> (Ccing Stephen as the unidentified "both" in README.devel)
> 
> diff --git a/tc/f_u32.c b/tc/f_u32.c
> index d787eb91..70098bcd 100644
> --- a/tc/f_u32.c
> +++ b/tc/f_u32.c
> @@ -1275,11 +1275,11 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
>  		fprintf(stderr, "divisor and hash missing ");
>  	}
>  	if (tb[TCA_U32_CLASSID]) {
> +		char *fmt = !sel || !(sel->flags & TC_U32_TERMINAL) ? "*flowid %s" : "flowid %s";
>  		SPRINT_BUF(b1);
> -		fprintf(f, "%sflowid %s ",
> -			!sel || !(sel->flags & TC_U32_TERMINAL) ? "*" : "",
> -			sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]),
> -					  b1));
> +
> +		print_string(PRINT_ANY, "flowid", fmt,
> +			    sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]), b1));
>  	} else if (sel && sel->flags & TC_U32_TERMINAL) {
>  		print_string(PRINT_FP, NULL, "terminal flowid ", NULL);
>  	}
> 


Already sent patch.

[PATCH iproute2] u32: fix json formatting of flowid

[Stephen Hemminger]

The code to print json was not done for the flow id.
This would lead to incorrect JSON format output.

Reported-by: Christian Pössinger <christian@poessinger.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 tc/f_u32.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index d787eb915603..e4e0ab121c57 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1275,12 +1275,14 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 		fprintf(stderr, "divisor and hash missing ");
 	}
 	if (tb[TCA_U32_CLASSID]) {
+		__u32 classid = rta_getattr_u32(tb[TCA_U32_CLASSID]);
 		SPRINT_BUF(b1);
-		fprintf(f, "%sflowid %s ",
-			!sel || !(sel->flags & TC_U32_TERMINAL) ? "*" : "",
-			sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]),
-					  b1));
-	} else if (sel && sel->flags & TC_U32_TERMINAL) {
+		if (sel && (sel->flags & TC_U32_TERMINAL))
+			print_string(PRINT_FP, NULL, "*", NULL);
+
+		print_string(PRINT_ANY, "flowid", "flowid %s ",
+			     sprint_tc_classid(classid, b1));
+	} else if (sel && (sel->flags & TC_U32_TERMINAL)) {
 		print_string(PRINT_FP, NULL, "terminal flowid ", NULL);
 	}
 	if (tb[TCA_U32_LINK]) {
-- 
2.35.1

RE: [PATCH iproute2] u32: fix json formatting of flowid

[Christian Pössinger]

Hi Stephen,

I can confirm this fix working!

You may include 
Tested-by: Christian Pössinger <christian@poessinger.com>

Thanks,
Christian

-----Original Message-----
From: Stephen Hemminger <stephen@networkplumber.org> 
Sent: Thursday, 13 October 2022 17:33
To: netdev@vger.kernel.org
Cc: Stephen Hemminger <stephen@networkplumber.org>; Christian Pössinger <christian@poessinger.com>
Subject: [PATCH iproute2] u32: fix json formatting of flowid

The code to print json was not done for the flow id.
This would lead to incorrect JSON format output.

Reported-by: Christian Pössinger <christian@poessinger.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 tc/f_u32.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index d787eb915603..e4e0ab121c57 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1275,12 +1275,14 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 		fprintf(stderr, "divisor and hash missing ");
 	}
 	if (tb[TCA_U32_CLASSID]) {
+		__u32 classid = rta_getattr_u32(tb[TCA_U32_CLASSID]);
 		SPRINT_BUF(b1);
-		fprintf(f, "%sflowid %s ",
-			!sel || !(sel->flags & TC_U32_TERMINAL) ? "*" : "",
-			sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]),
-					  b1));
-	} else if (sel && sel->flags & TC_U32_TERMINAL) {
+		if (sel && (sel->flags & TC_U32_TERMINAL))
+			print_string(PRINT_FP, NULL, "*", NULL);
+
+		print_string(PRINT_ANY, "flowid", "flowid %s ",
+			     sprint_tc_classid(classid, b1));
+	} else if (sel && (sel->flags & TC_U32_TERMINAL)) {
 		print_string(PRINT_FP, NULL, "terminal flowid ", NULL);
 	}
 	if (tb[TCA_U32_LINK]) {
-- 
2.35.1