[PATCH net] net: sched: Remove TCA_OPTIONS from policy

[PATCH net] net: sched: Remove TCA_OPTIONS from policy

[David Ahern]

From: David Ahern <dsahern@gmail.com>

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 <pupilla@libero.it>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/sched/sch_api.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 3dc0acf54245..be7cd140b2a3 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -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 },
-- 
2.11.0

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[David Miller]

From: David Ahern <dsahern@kernel.org>
Date: Wed, 24 Oct 2018 08:32:49 -0700

> From: David Ahern <dsahern@gmail.com>
> 
> 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 <pupilla@libero.it>
> Signed-off-by: David Ahern <dsahern@gmail.com>

That's unfortunate... applied, thanks.

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[Jiri Pirko]

Wed, Oct 24, 2018 at 05:32:49PM CEST, dsahern@kernel.org wrote:
>From: David Ahern <dsahern@gmail.com>
>
>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.


>
>Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
>Reported-by: Marco Berizzi <pupilla@libero.it>
>Signed-off-by: David Ahern <dsahern@gmail.com>
>---
> net/sched/sch_api.c | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>index 3dc0acf54245..be7cd140b2a3 100644
>--- a/net/sched/sch_api.c
>+++ b/net/sched/sch_api.c
>@@ -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.


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

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[Marco Berizzi]

> Il 24 ottobre 2018 alle 17.32 David Ahern <dsahern@kernel.org> ha scritto:
>  net/sched/sch_api.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 3dc0acf54245..be7cd140b2a3 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -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 },
> --
> 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

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[David Ahern]

On 10/26/18 5:34 AM, Marco Berizzi wrote:
> 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
> 

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?

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[David Ahern]

On 10/25/18 12:31 AM, Jiri Pirko wrote:
> Wed, Oct 24, 2018 at 05:32:49PM CEST, dsahern@kernel.org wrote:
>> From: David Ahern <dsahern@gmail.com>
>>
>> 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.

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

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[Jiri Pirko]

Fri, Oct 26, 2018 at 06:02:01PM CEST, dsahern@gmail.com wrote:
>On 10/25/18 12:31 AM, Jiri Pirko wrote:
>> Wed, Oct 24, 2018 at 05:32:49PM CEST, dsahern@kernel.org wrote:
>>> From: David Ahern <dsahern@gmail.com>
>>>
>>> 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.
>
>Kernel side for hfsc expects TCA_OPTIONS as a binary as well - a struct
>tc_hfsc_qopt. Nothing that can be done.

:(

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[Cong Wang]

On Fri, Oct 26, 2018 at 4:35 AM Marco Berizzi <pupilla@libero.it> wrote:
> 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:

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? :)

Re: [PATCH net] net: sched: Remove TCA_OPTIONS from policy

[Marco Berizzi]

> Il 26 ottobre 2018 alle 20.19 Cong Wang <xiyou.wangcong@gmail.com> ha scritto:
> 
> On Fri, Oct 26, 2018 at 4:35 AM Marco Berizzi <pupilla@libero.it> wrote:
> 
> > 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:
> 
> 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.