netdev - [PATCH net 2/2] net/sched: add delete_empty() to filters and use it in cls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Dec 2019 10:30:53 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Vlad Buslov <vladbu@...lanox.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: [PATCH net 2/2] net/sched: add delete_empty() to filters and use it in cls_flower

on tc filters that don't support lockless insertion/removal, there is no
need to guard against concurrent insertion when a removal is in progress.
Therefore, we can avoid taking the filter lock and doing a full walk()
when deleting: it's sufficient to decrease the refcount.
This fixes situations where walk() was wrongly detecting a non-empty
filter on deletion, like it happened with cls_u32 in the error path of
change(), thus leading to failures in the following tdc selftests:

 6aa7: (filter, u32) Add/Replace u32 with source match and invalid indev
 6658: (filter, u32) Add/Replace u32 with custom hash table and invalid handle
 74c2: (filter, u32) Add/Replace u32 filter with invalid hash table id

On cls_flower, and on (future) lockless filters, this check is necessary:
move all the check_empty() logic in a callback so that each filter
can have its own implementation.

Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
Suggested-by: Jamal Hadi Salim <jhs@...atatu.com>
Suggested-by: Vlad Buslov <vladbu@...lanox.com>
Signed-off-by: Davide Caratti <dcaratti@...hat.com>
---
 include/net/sch_generic.h |  2 ++
 net/sched/cls_api.c       | 29 ++++-------------------------
 net/sched/cls_flower.c    | 23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 144f264ea394..5e294da0967e 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -308,6 +308,8 @@ struct tcf_proto_ops {
 	int			(*delete)(struct tcf_proto *tp, void *arg,
 					  bool *last, bool rtnl_held,
 					  struct netlink_ext_ack *);
+	bool			(*delete_empty)(struct tcf_proto *tp,
+						bool rtnl_held);
 	void			(*walk)(struct tcf_proto *tp,
 					struct tcf_walker *arg, bool rtnl_held);
 	int			(*reoffload)(struct tcf_proto *tp, bool add,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6a0eacafdb19..7900db8d4c06 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -308,33 +308,12 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
 		tcf_proto_destroy(tp, rtnl_held, true, extack);
 }
 
-static int walker_check_empty(struct tcf_proto *tp, void *fh,
-			      struct tcf_walker *arg)
-{
-	if (fh) {
-		arg->nonempty = true;
-		return -1;
-	}
-	return 0;
-}
-
-static bool tcf_proto_is_empty(struct tcf_proto *tp, bool rtnl_held)
-{
-	struct tcf_walker walker = { .fn = walker_check_empty, };
-
-	if (tp->ops->walk) {
-		tp->ops->walk(tp, &walker, rtnl_held);
-		return !walker.nonempty;
-	}
-	return true;
-}
-
 static bool tcf_proto_check_delete(struct tcf_proto *tp, bool rtnl_held)
 {
-	spin_lock(&tp->lock);
-	if (tcf_proto_is_empty(tp, rtnl_held))
-		tp->deleting = true;
-	spin_unlock(&tp->lock);
+	if (tp->ops->delete_empty)
+		return tp->ops->delete_empty(tp, rtnl_held);
+
+	tp->deleting = true;
 	return tp->deleting;
 }
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 0d125de54285..e0316d18529e 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2773,6 +2773,28 @@ static void fl_bind_class(void *fh, u32 classid, unsigned long cl)
 		f->res.class = cl;
 }
 
+static int walker_check_empty(struct tcf_proto *tp, void *fh,
+			      struct tcf_walker *arg)
+{
+	if (fh) {
+		arg->nonempty = true;
+		return -1;
+	}
+	return 0;
+}
+
+static bool fl_delete_empty(struct tcf_proto *tp, bool rtnl_held)
+{
+	struct tcf_walker walker = { .fn = walker_check_empty, };
+
+	spin_lock(&tp->lock);
+	fl_walk(tp, &walker, rtnl_held);
+	tp->deleting = !walker.nonempty;
+	spin_unlock(&tp->lock);
+
+	return tp->deleting;
+}
+
 static struct tcf_proto_ops cls_fl_ops __read_mostly = {
 	.kind		= "flower",
 	.classify	= fl_classify,
@@ -2782,6 +2804,7 @@ static struct tcf_proto_ops cls_fl_ops __read_mostly = {
 	.put		= fl_put,
 	.change		= fl_change,
 	.delete		= fl_delete,
+	.delete_empty	= fl_delete_empty,
 	.walk		= fl_walk,
 	.reoffload	= fl_reoffload,
 	.hw_add		= fl_hw_add,
-- 
2.24.1