* broken behaviour of TC filter delete
@ 2018-08-23 21:39 Roman Mashak
2018-08-24 8:17 ` Jiri Pirko
0 siblings, 1 reply; 7+ messages in thread
From: Roman Mashak @ 2018-08-23 21:39 UTC (permalink / raw)
To: netdev; +Cc: jiri, Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: broken behaviour of TC filter delete
2018-08-23 21:39 broken behaviour of TC filter delete Roman Mashak
@ 2018-08-24 8:17 ` Jiri Pirko
2018-08-24 16:18 ` Roman Mashak
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Pirko @ 2018-08-24 8:17 UTC (permalink / raw)
To: Roman Mashak; +Cc: netdev, jiri, Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: broken behaviour of TC filter delete
2018-08-24 8:17 ` Jiri Pirko
@ 2018-08-24 16:18 ` Roman Mashak
2018-08-24 18:11 ` Cong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Roman Mashak @ 2018-08-24 16:18 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, jiri, Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: broken behaviour of TC filter delete
2018-08-24 16:18 ` Roman Mashak
@ 2018-08-24 18:11 ` Cong Wang
2018-08-25 13:02 ` Jiri Pirko
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2018-08-24 18:11 UTC (permalink / raw)
To: Roman Mashak
Cc: Jiri Pirko, Linux Kernel Network Developers, Jiri Pirko,
Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: broken behaviour of TC filter delete
2018-08-24 18:11 ` Cong Wang
@ 2018-08-25 13:02 ` Jiri Pirko
2018-08-26 17:48 ` Jamal Hadi Salim
2018-08-27 18:30 ` Cong Wang
0 siblings, 2 replies; 7+ messages in thread
From: Jiri Pirko @ 2018-08-25 13:02 UTC (permalink / raw)
To: Cong Wang
Cc: Roman Mashak, Linux Kernel Network Developers, Jiri Pirko,
Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: broken behaviour of TC filter delete
2018-08-25 13:02 ` Jiri Pirko
@ 2018-08-26 17:48 ` Jamal Hadi Salim
2018-08-27 18:30 ` Cong Wang
1 sibling, 0 replies; 7+ messages in thread
From: Jamal Hadi Salim @ 2018-08-26 17:48 UTC (permalink / raw)
To: Jiri Pirko, Cong Wang
Cc: Roman Mashak, Linux Kernel Network Developers, Jiri Pirko
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: broken behaviour of TC filter delete
2018-08-25 13:02 ` Jiri Pirko
2018-08-26 17:48 ` Jamal Hadi Salim
@ 2018-08-27 18:30 ` Cong Wang
1 sibling, 0 replies; 7+ messages in thread
From: Cong Wang @ 2018-08-27 18:30 UTC (permalink / raw)
To: Jiri Pirko
Cc: Roman Mashak, Linux Kernel Network Developers, Jiri Pirko,
Jamal Hadi Salim
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-27 22:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-23 21:39 broken behaviour of TC filter delete Roman Mashak
2018-08-24 8:17 ` Jiri Pirko
2018-08-24 16:18 ` Roman Mashak
2018-08-24 18:11 ` Cong Wang
2018-08-25 13:02 ` Jiri Pirko
2018-08-26 17:48 ` Jamal Hadi Salim
2018-08-27 18:30 ` Cong Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.