Message ID | 20190411161220.5951-1-vladbu@mellanox.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net: sched: flower: use correct ht function to prevent duplicates | expand |
On Thu, 11 Apr 2019 19:12:20 +0300, Vlad Buslov wrote: > Implementation of function rhashtable_insert_fast() check if its internal > helper function __rhashtable_insert_fast() returns non-NULL pointer and > seemingly return -EEXIST in such case. However, since > __rhashtable_insert_fast() is called with NULL key pointer, it never > actually checks for duplicates, which means that -EEXIST is never returned > to the user. Use rhashtable_lookup_insert_fast() hash table API instead. In > order to verify that it works as expected and prevent the problem from > happening in future, extend tc-tests with new test that verifies that no > new filters with existing key can be inserted to flower classifier. > > Fixes: 1f17f7742eeb ("net: sched: flower: insert filter to ht before offloading it to hw") > Signed-off-by: Vlad Buslov <vladbu@mellanox.com> Ah, John just posted the same patch internally this morning.. Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com> Date: Thu, 11 Apr 2019 10:14:15 -0700 > On Thu, 11 Apr 2019 19:12:20 +0300, Vlad Buslov wrote: >> Implementation of function rhashtable_insert_fast() check if its internal >> helper function __rhashtable_insert_fast() returns non-NULL pointer and >> seemingly return -EEXIST in such case. However, since >> __rhashtable_insert_fast() is called with NULL key pointer, it never >> actually checks for duplicates, which means that -EEXIST is never returned >> to the user. Use rhashtable_lookup_insert_fast() hash table API instead. In >> order to verify that it works as expected and prevent the problem from >> happening in future, extend tc-tests with new test that verifies that no >> new filters with existing key can be inserted to flower classifier. >> >> Fixes: 1f17f7742eeb ("net: sched: flower: insert filter to ht before offloading it to hw") >> Signed-off-by: Vlad Buslov <vladbu@mellanox.com> > > Ah, John just posted the same patch internally this morning.. > > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> Applied.
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 2763176e369c..9cd8122a5c38 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -1466,9 +1466,9 @@ static int fl_ht_insert_unique(struct cls_fl_filter *fnew, struct fl_flow_mask *mask = fnew->mask; int err; - err = rhashtable_insert_fast(&mask->ht, - &fnew->ht_node, - mask->filter_ht_params); + err = rhashtable_lookup_insert_fast(&mask->ht, + &fnew->ht_node, + mask->filter_ht_params); if (err) { *in_ht = false; /* It is okay if filter with same key exists when diff --git a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json index 99a5ffca1088..1b6501983018 100644 --- a/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json +++ b/tools/testing/selftests/tc-testing/tc-tests/filters/tests.json @@ -38,5 +38,25 @@ "$TC qdisc del dev $DEV2 ingress", "/bin/rm $BATCH_FILE" ] + }, + { + "id": "4cbd", + "name": "Try to add filter with duplicate key", + "category": [ + "filter", + "flower" + ], + "setup": [ + "$TC qdisc add dev $DEV2 ingress", + "$TC filter add dev $DEV2 protocol ip prio 1 parent ffff: flower dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50 ip_proto tcp src_ip 1.1.1.1 dst_ip 2.2.2.2 action drop" + ], + "cmdUnderTest": "$TC filter add dev $DEV2 protocol ip prio 1 parent ffff: flower dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50 ip_proto tcp src_ip 1.1.1.1 dst_ip 2.2.2.2 action drop", + "expExitCode": "2", + "verifyCmd": "$TC -s filter show dev $DEV2 ingress", + "matchPattern": "filter protocol ip pref 1 flower chain 0 handle", + "matchCount": "1", + "teardown": [ + "$TC qdisc del dev $DEV2 ingress" + ] } ]
Implementation of function rhashtable_insert_fast() check if its internal helper function __rhashtable_insert_fast() returns non-NULL pointer and seemingly return -EEXIST in such case. However, since __rhashtable_insert_fast() is called with NULL key pointer, it never actually checks for duplicates, which means that -EEXIST is never returned to the user. Use rhashtable_lookup_insert_fast() hash table API instead. In order to verify that it works as expected and prevent the problem from happening in future, extend tc-tests with new test that verifies that no new filters with existing key can be inserted to flower classifier. Fixes: 1f17f7742eeb ("net: sched: flower: insert filter to ht before offloading it to hw") Signed-off-by: Vlad Buslov <vladbu@mellanox.com> --- net/sched/cls_flower.c | 6 +++--- .../tc-testing/tc-tests/filters/tests.json | 20 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)