broken behaviour of TC filter delete

broken behaviour of TC filter delete

[Roman Mashak]

It appears that the following commit changed the behaviour of scenario where a
filter is deleted twice:

commit f71e0ca4db187af7c44987e9d21e9042c3046070
Author: Jiri Pirko <jiri@mellanox.com>
Date:   Mon Jul 23 09:23:05 2018 +0200

    net: sched: Avoid implicit chain 0 creation


Steps to reproduce :

1) create dummy device
   $ ip link add dev dummy0 type dummy

2) create qdisc
   $ tc qdisc add dev dummy0 ingress

3) create simple u32 filter with action attached
   $ tc filter add dev dummy0 parent ffff: protocol ip prio 1 u32 match ip src 10.10.10.1/32 action ok

4) list the filter
   $ tc filter ls dev dummy0 parent ffff:

5) delete the filter with the given protocol and priority
   $ tc filter del dev dummy0 parent ffff: protocol ip prio 1

6) repeat step 5, this will return -ENOENT ("Error: Filter with specified priority/protocol not found.")
However, before the change at step 6 we would get -EINVAL (Error: Cannot find specified filter chain.)
and that makes sense.

The change breaks a number of our internal TC tests.

Re: broken behaviour of TC filter delete

[Jiri Pirko]

Thu, Aug 23, 2018 at 11:39:22PM CEST, mrv@mojatatu.com wrote:
>
>
>It appears that the following commit changed the behaviour of scenario where a
>filter is deleted twice:
>
>commit f71e0ca4db187af7c44987e9d21e9042c3046070
>Author: Jiri Pirko <jiri@mellanox.com>
>Date:   Mon Jul 23 09:23:05 2018 +0200
>
>    net: sched: Avoid implicit chain 0 creation
>
>
>Steps to reproduce :
>
>1) create dummy device
>   $ ip link add dev dummy0 type dummy
>
>2) create qdisc
>   $ tc qdisc add dev dummy0 ingress
>
>3) create simple u32 filter with action attached
>   $ tc filter add dev dummy0 parent ffff: protocol ip prio 1 u32 match ip src 10.10.10.1/32 action ok
>
>4) list the filter
>   $ tc filter ls dev dummy0 parent ffff:
>
>5) delete the filter with the given protocol and priority
>   $ tc filter del dev dummy0 parent ffff: protocol ip prio 1
>
>6) repeat step 5, this will return -ENOENT ("Error: Filter with specified priority/protocol not found.")
>However, before the change at step 6 we would get -EINVAL (Error: Cannot find specified filter chain.)
>and that makes sense.

Wait, this now returns:
Error: Cannot find specified filter chain.
So you want it to return -EINVAL (Error: Cannot find specified filter chain.) ?
How about for other chains?


>
>The change breaks a number of our internal TC tests.

Re: broken behaviour of TC filter delete

[Roman Mashak]

Jiri Pirko <jiri@resnulli.us> writes:

> Thu, Aug 23, 2018 at 11:39:22PM CEST, mrv@mojatatu.com wrote:
>>
>>
>>It appears that the following commit changed the behaviour of scenario where a
>>filter is deleted twice:
>>
>>commit f71e0ca4db187af7c44987e9d21e9042c3046070
>>Author: Jiri Pirko <jiri@mellanox.com>
>>Date:   Mon Jul 23 09:23:05 2018 +0200
>>
>>    net: sched: Avoid implicit chain 0 creation
>>
>>
>>Steps to reproduce :
>>
>>1) create dummy device
>>   $ ip link add dev dummy0 type dummy
>>
>>2) create qdisc
>>   $ tc qdisc add dev dummy0 ingress
>>
>>3) create simple u32 filter with action attached
>>   $ tc filter add dev dummy0 parent ffff: protocol ip prio 1 u32 match ip src 10.10.10.1/32 action ok
>>
>>4) list the filter
>>   $ tc filter ls dev dummy0 parent ffff:
>>
>>5) delete the filter with the given protocol and priority
>>   $ tc filter del dev dummy0 parent ffff: protocol ip prio 1
>>
>>6) repeat step 5, this will return -ENOENT ("Error: Filter with specified priority/protocol not found.")
>>However, before the change at step 6 we would get -EINVAL (Error: Cannot find specified filter chain.)
>>and that makes sense.
>
> Wait, this now returns:
> Error: Cannot find specified filter chain.
> So you want it to return -EINVAL (Error: Cannot find specified filter chain.) ?
> How about for other chains?
>

I must've mixed up the return codes and messages while typing the
message.

So _before_ commit f71e0ca4db187af7c44987e9d21e9042c3046070 step 6 would
return -ENOENT with "Error: Filter with specified priority/protocol not
found." and _after_ the commit it returns -EINVAL (Error: Cannot find
specified filter chain.)

ENOENT seems to be more logical to return when there's no more filter to delete.

Re: broken behaviour of TC filter delete

[Cong Wang]

On Fri, Aug 24, 2018 at 9:21 AM Roman Mashak <mrv@mojatatu.com> wrote:
>
> So _before_ commit f71e0ca4db187af7c44987e9d21e9042c3046070 step 6 would
> return -ENOENT with "Error: Filter with specified priority/protocol not
> found." and _after_ the commit it returns -EINVAL (Error: Cannot find
> specified filter chain.)
>
> ENOENT seems to be more logical to return when there's no more filter to delete.

Yeah, at least we should keep ENOENT for compatibility.

The bug here is chain 0 is gone after the last filter is gone,
so when you delete the filter again, it treats it as you specify
chain 0 which does not exist, so it hits EINVAL before ENOENT.

I am not sure how to fix this properly.

Re: broken behaviour of TC filter delete

[Jiri Pirko]

Fri, Aug 24, 2018 at 08:11:07PM CEST, xiyou.wangcong@gmail.com wrote:
>On Fri, Aug 24, 2018 at 9:21 AM Roman Mashak <mrv@mojatatu.com> wrote:
>>
>> So _before_ commit f71e0ca4db187af7c44987e9d21e9042c3046070 step 6 would
>> return -ENOENT with "Error: Filter with specified priority/protocol not
>> found." and _after_ the commit it returns -EINVAL (Error: Cannot find
>> specified filter chain.)
>>
>> ENOENT seems to be more logical to return when there's no more filter to delete.
>
>Yeah, at least we should keep ENOENT for compatibility.
>
>The bug here is chain 0 is gone after the last filter is gone,
>so when you delete the filter again, it treats it as you specify
>chain 0 which does not exist, so it hits EINVAL before ENOENT.

I understand. My concern is about consistency with other chains. Perhaps
-ENOENT for all chains in this case would be doable. What do you think?

>
>I am not sure how to fix this properly.

Re: broken behaviour of TC filter delete

[Jamal Hadi Salim]

On 2018-08-25 9:02 a.m., Jiri Pirko wrote:
> Fri, Aug 24, 2018 at 08:11:07PM CEST, xiyou.wangcong@gmail.com wrote:
>
>>> ENOENT seems to be more logical to return when there's no more filter to delete.
>>
>> Yeah, at least we should keep ENOENT for compatibility.
>>
>> The bug here is chain 0 is gone after the last filter is gone,
>> so when you delete the filter again, it treats it as you specify
>> chain 0 which does not exist, so it hits EINVAL before ENOENT.
> 
> I understand. My concern is about consistency with other chains. Perhaps
> -ENOENT for all chains in this case would be doable. What do you think?
>

ENOENT with extack describing whether chain or filter not found.

cheers,
jamal

Re: broken behaviour of TC filter delete

[Cong Wang]

On Sat, Aug 25, 2018 at 6:06 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Fri, Aug 24, 2018 at 08:11:07PM CEST, xiyou.wangcong@gmail.com wrote:
> >On Fri, Aug 24, 2018 at 9:21 AM Roman Mashak <mrv@mojatatu.com> wrote:
> >>
> >> So _before_ commit f71e0ca4db187af7c44987e9d21e9042c3046070 step 6 would
> >> return -ENOENT with "Error: Filter with specified priority/protocol not
> >> found." and _after_ the commit it returns -EINVAL (Error: Cannot find
> >> specified filter chain.)
> >>
> >> ENOENT seems to be more logical to return when there's no more filter to delete.
> >
> >Yeah, at least we should keep ENOENT for compatibility.
> >
> >The bug here is chain 0 is gone after the last filter is gone,
> >so when you delete the filter again, it treats it as you specify
> >chain 0 which does not exist, so it hits EINVAL before ENOENT.
>
> I understand. My concern is about consistency with other chains. Perhaps
> -ENOENT for all chains in this case would be doable. What do you think?

Yeah, I think -ENOENT makes more sense than EINVAL here too.