net: sched: Remove TCA_OPTIONS from policy

PATCH In review David Ahern 6 participants 9 messages Oct 25, 2018 – Oct 31, 2018

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

David Ahern

author

2018-10-25 00:01 UTC #0 lore raw

PATCH 1/1 net: sched: Remove TCA_OPTIONS from policy

From: David Ahern <[email protected]>

Marco reported an error with hfsc:

root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.

Apparently a few implementations pass TCA_OPTIONS as a binary instead of nested attribute, so drop TCA_OPTIONS from the policy.

Fixes 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")

Reported-by Marco Berizzi <[email protected]>

Signed-off-by David Ahern <[email protected]>

---
 net/sched/sch_api.c | 1 -
 1 file changed, 1 deletion(-)

net/sched/sch_api.c +0 -1

index 3dc0acf54245..be7cd140b2a3 100644
@@ -1309,7 +1309,6 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
 
 const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
 	[TCA_KIND]		= { .type = NLA_STRING },
-	[TCA_OPTIONS]		= { .type = NLA_NESTED },
 	[TCA_RATE]		= { .type = NLA_BINARY,
 				    .len = sizeof(struct tc_estimator) },
 	[TCA_STAB]		= { .type = NLA_NESTED },

Signature · 1 line

2.11.0

David Miller

maintainer

2018-10-25 06:05 UTC #1 lore raw

From: David Ahern <[email protected]>
Date: Wed, 24 Oct 2018 08:32:49 -0700

Quoted 6 lines

From: David Ahern <[email protected]>
Marco reported an error with hfsc:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.

Quoted 6 lines

Apparently a few implementations pass TCA_OPTIONS as a binary instead of nested attribute, so drop TCA_OPTIONS from the policy.
Fixes 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
Reported-by Marco Berizzi <[email protected]>
Signed-off-by David Ahern <[email protected]>

That's unfortunate... applied, thanks.

Jiri Pirko

maintainer

2018-10-25 15:09 UTC #2 lore raw

In reply to David Ahern

Quoted 8 lines · Wed, Oct 24, 2018 at 05:32:49PM CEST, [email protected] wrote:

From: David Ahern <[email protected]>
Marco reported an error with hfsc:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.
Apparently a few implementations pass TCA_OPTIONS as a binary instead of nested attribute, so drop TCA_OPTIONS from the policy.

Yeah, this is nice example of a case, where I think it wouldn't hurt to be a bit more strict. Apparently, the userspace app is buggy. It should be fixed. Note that I'm aware of the bw compatibility.

Quoted 13 lines

Fixes 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
Reported-by Marco Berizzi <[email protected]>
Signed-off-by David Ahern <[email protected]>
---
net/sched/sch_api.c | 1 -
1 file changed, 1 deletion(-)
net/sched/sch_api.c +0 -0
index 3dc0acf54245..be7cd140b2a3 100644

net/sched/sch_api.c check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w) · line 1312

Quoted 4 lines

net/sched/sch_api.c +0 -1

@@ -1309,7 +1309,6 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
 
 const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
 	[TCA_KIND]		= { .type = NLA_STRING },
-	[TCA_OPTIONS]		= { .type = NLA_NESTED },

A future developer might add this again. A comment why not would be good here.

Quoted 6 lines

net/sched/sch_api.c +0 -0

 	[TCA_RATE]		= { .type = NLA_BINARY,
 				    .len = sizeof(struct tc_estimator) },
 	[TCA_STAB]		= { .type = NLA_NESTED },

Signature · 1 line

2.11.0

David Ahern

2018-10-27 00:39 UTC #3 lore raw

Quoted 7 lines · Jiri Pirko

Quoted 6 lines · Wed, Oct 24, 2018 at 05:32:49PM CEST, [email protected] wrote:
From: David Ahern <[email protected]>
Marco reported an error with hfsc:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.

Quoted 6 lines

Quoted 2 lines
Apparently a few implementations pass TCA_OPTIONS as a binary instead of nested attribute, so drop TCA_OPTIONS from the policy.
Yeah, this is nice example of a case, where I think it wouldn't hurt to be a bit more strict. Apparently, the userspace app is buggy. It should be fixed. Note that I'm aware of the bw compatibility.

Kernel side for hfsc expects TCA_OPTIONS as a binary as well - a struct tc_hfsc_qopt. Nothing that can be done.

Jiri Pirko

maintainer

2018-10-27 00:52 UTC #4 lore raw

Quoted 11 lines · Fri, Oct 26, 2018 at 06:02:01PM CEST, [email protected] wrote:

Quoted 10 lines · Jiri Pirko
Quoted 8 lines · Wed, Oct 24, 2018 at 05:32:49PM CEST, [email protected] wrote:
From: David Ahern <[email protected]>
Marco reported an error with hfsc:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.
Apparently a few implementations pass TCA_OPTIONS as a binary instead of nested attribute, so drop TCA_OPTIONS from the policy.

Quoted 6 lines

Quoted 3 lines
Yeah, this is nice example of a case, where I think it wouldn't hurt to be a bit more strict. Apparently, the userspace app is buggy. It should be fixed. Note that I'm aware of the bw compatibility.
Kernel side for hfsc expects TCA_OPTIONS as a binary as well - a struct tc_hfsc_qopt. Nothing that can be done.

Marco Berizzi

2018-10-26 20:11 UTC #5 lore raw

In reply to David Ahern

Quoted 16 lines

Il 24 ottobre 2018 alle 17.32 David Ahern <[email protected]> ha scritto:

 net/sched/sch_api.c | 1 -
 1 file changed, 1 deletion(-)

net/sched/sch_api.c +0 -0

index 3dc0acf54245..be7cd140b2a3 100644
@@ -1309,7 +1309,6 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
 
 const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
 [TCA_KIND] = { .type = NLA_STRING },
 
 *   [TCA_OPTIONS] = { .type = NLA_NESTED },
 [TCA_RATE] = { .type = NLA_BINARY,
 .len = sizeof(struct tc_estimator) },

Quoted 3 lines

[TCA_STAB] = { .type = NLA_NESTED },
--
2.11.0

David,

Apologies for bothering you again.
I applied your patch to 4.19, but after issuing this command:

root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
root@Calimero:~# ping 10.81.104.1
PING 10.81.104.1 (10.81.104.1) 56(84) bytes of data.
^C
--- 10.81.104.1 ping statistics ---
2 packets transmitted, 0 received, 100% packet loss, time 1001ms

I'm losing ipv4 connectivity.
If I remove the qdisc everything is going to work again:

root@Calimero:~# tc qdisc del dev eth0 root                   
root@Calimero:~# ping 10.81.104.1
PING 10.81.104.1 (10.81.104.1) 56(84) bytes of data.
64 bytes from 10.81.104.1: icmp_seq=1 ttl=255 time=0.711 ms
^C
--- 10.81.104.1 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.711/0.711/0.711/0.000 ms

David Ahern

2018-10-27 00:26 UTC #6 lore raw

Quoted 14 lines · Marco Berizzi

Apologies for bothering you again.
I applied your patch to 4.19, but after issuing this command:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
root@Calimero:~# ping 10.81.104.1
PING 10.81.104.1 (10.81.104.1) 56(84) bytes of data.
^C
--- 10.81.104.1 ping statistics ---
2 packets transmitted, 0 received, 100% packet loss, time 1001ms
I'm losing ipv4 connectivity.
If I remove the qdisc everything is going to work again:

Quoted 9 lines

root@Calimero:~# tc qdisc del dev eth0 root                   
root@Calimero:~# ping 10.81.104.1
PING 10.81.104.1 (10.81.104.1) 56(84) bytes of data.
64 bytes from 10.81.104.1: icmp_seq=1 ttl=255 time=0.711 ms
^C
--- 10.81.104.1 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 0.711/0.711/0.711/0.000 ms

I backed up to 95278ddaa15cfa23e4a06ee9ed7b6ee0197c500b which is the commit before the validation patch and it does not work there.

Can you bisect and find out when it stopped working?

Cong Wang

2018-10-27 02:57 UTC #7 lore raw

In reply to Marco Berizzi

Quoted 11 lines · Marco Berizzi

Apologies for bothering you again.
I applied your patch to 4.19, but after issuing this command:

root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
root@Calimero:~# ping 10.81.104.1
PING 10.81.104.1 (10.81.104.1) 56(84) bytes of data.
^C
--- 10.81.104.1 ping statistics ---
2 packets transmitted, 0 received, 100% packet loss, time 1001ms

Quoted 2 lines

I'm losing ipv4 connectivity.
If I remove the qdisc everything is going to work again:

Did this really work before?

You specify a default class without adding it, so the packets are dropped.

How would you expect this to work? :)

Marco Berizzi

2018-10-31 21:40 UTC #8 lore raw

Quoted 15 lines

Il 26 ottobre 2018 alle 20.19 Cong Wang <[email protected]> ha scritto:
Quoted 11 lines · Marco Berizzi
Apologies for bothering you again.
I applied your patch to 4.19, but after issuing this command:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
root@Calimero:~# ping 10.81.104.1
PING 10.81.104.1 (10.81.104.1) 56(84) bytes of data.
^C
--- 10.81.104.1 ping statistics ---
2 packets transmitted, 0 received, 100% packet loss, time 1001ms

Quoted 8 lines

Quoted 2 lines
I'm losing ipv4 connectivity.
If I remove the qdisc everything is going to work again:
Did this really work before?
You specify a default class without adding it, so the packets are dropped.
How would you expect this to work? :)

:-) yes indeed. Apologies for the noise.