[PATCH net-next 0/5] net/sched: improve class lifetime handling

[PATCH net-next 0/5] net/sched: improve class lifetime handling

[Pedro Tammela]

Valis says[0]:
============
Three classifiers (cls_fw, cls_u32 and cls_route) always copy
tcf_result struct into the new instance of the filter on update.

This causes a problem when updating a filter bound to a class,
as tcf_unbind_filter() is always called on the old instance in the
success path, decreasing filter_cnt of the still referenced class
and allowing it to be deleted, leading to a use-after-free.
============

Turns out these could have been spotted easily with proper warnings.
Improve the current class lifetime with wrappers that check for
overflow/underflow.

While at it add an extack for when a class in use is deleted.

[0] https://lore.kernel.org/all/20230721174856.3045-1-sec@valis.email/


Pedro Tammela (5):
  net/sched: wrap open coded Qdics class filter counter
  net/sched: sch_drr: warn about class in use while deleting
  net/sched: sch_hfsc: warn about class in use while deleting
  net/sched: sch_htb: warn about class in use while deleting
  net/sched: sch_qfq: warn about class in use while deleting

 include/net/sch_generic.h |  1 +
 include/net/tc_class.h    | 33 +++++++++++++++++++++++++++++++++
 net/sched/sch_drr.c       | 12 +++++++-----
 net/sched/sch_hfsc.c      | 11 +++++++----
 net/sched/sch_htb.c       | 11 ++++++-----
 net/sched/sch_qfq.c       | 11 ++++++-----
 6 files changed, 60 insertions(+), 19 deletions(-)
 create mode 100644 include/net/tc_class.h

-- 
2.39.2

[PATCH net-next 1/5] net/sched: wrap open coded Qdics class filter counter

[Pedro Tammela]

The 'filter_cnt' counter is used to control a Qdisc class lifetime.
Each filter referecing this class by its id will eventually
increment/decrement this counter in their respective
'add/update/delete' routines.
As these operations are always serialized under rtnl lock, we don't
need an atomic type like 'refcount_t'.

It also means that we lose the overflow/underflow checks already
present in refcount_t, which are valuable to hunt down bugs
where the unsigned counter wraps around as it aids automated tools
like syzkaller to scream in such situations.

Wrap the open coded increment/decrement into helper functions and
add overflow checks to the operations.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/net/sch_generic.h |  1 +
 include/net/tc_class.h    | 33 +++++++++++++++++++++++++++++++++
 net/sched/sch_drr.c       | 10 +++++-----
 net/sched/sch_hfsc.c      |  9 +++++----
 net/sched/sch_htb.c       |  9 ++++-----
 net/sched/sch_qfq.c       |  9 ++++-----
 6 files changed, 52 insertions(+), 19 deletions(-)
 create mode 100644 include/net/tc_class.h

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 15be2d96b06d..891ee8637a92 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -599,6 +599,7 @@ get_default_qdisc_ops(const struct net_device *dev, int ntx)
 
 struct Qdisc_class_common {
 	u32			classid;
+	unsigned int		filter_cnt;
 	struct hlist_node	hnode;
 };
 
diff --git a/include/net/tc_class.h b/include/net/tc_class.h
new file mode 100644
index 000000000000..2ab4aa2dba30
--- /dev/null
+++ b/include/net/tc_class.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __NET_TC_CLASS_H
+#define __NET_TC_CLASS_H
+
+#include <linux/overflow.h>
+#include <net/sch_generic.h>
+
+static inline bool qdisc_class_in_use(const struct Qdisc_class_common *cl)
+{
+	return cl->filter_cnt > 0;
+}
+
+static inline void qdisc_class_get(struct Qdisc_class_common *cl)
+{
+	unsigned int res;
+
+	if (check_add_overflow(cl->filter_cnt, 1, &res))
+		WARN(1, "Qdisc class overflow");
+
+	cl->filter_cnt = res;
+}
+
+static inline void qdisc_class_put(struct Qdisc_class_common *cl)
+{
+	unsigned int res;
+
+	if (check_sub_overflow(cl->filter_cnt, 1, &res))
+		WARN(1, "Qdisc class underflow");
+
+	cl->filter_cnt = res;
+}
+
+#endif
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index e35a4e90f4e6..0bffa01c61bb 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -14,10 +14,10 @@
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/tc_class.h>
 
 struct drr_class {
 	struct Qdisc_class_common	common;
-	unsigned int			filter_cnt;
 
 	struct gnet_stats_basic_sync		bstats;
 	struct gnet_stats_queue		qstats;
@@ -150,7 +150,7 @@ static int drr_delete_class(struct Qdisc *sch, unsigned long arg,
 	struct drr_sched *q = qdisc_priv(sch);
 	struct drr_class *cl = (struct drr_class *)arg;
 
-	if (cl->filter_cnt > 0)
+	if (qdisc_class_in_use(&cl->common))
 		return -EBUSY;
 
 	sch_tree_lock(sch);
@@ -187,8 +187,8 @@ static unsigned long drr_bind_tcf(struct Qdisc *sch, unsigned long parent,
 {
 	struct drr_class *cl = drr_find_class(sch, classid);
 
-	if (cl != NULL)
-		cl->filter_cnt++;
+	if (cl)
+		qdisc_class_get(&cl->common);
 
 	return (unsigned long)cl;
 }
@@ -197,7 +197,7 @@ static void drr_unbind_tcf(struct Qdisc *sch, unsigned long arg)
 {
 	struct drr_class *cl = (struct drr_class *)arg;
 
-	cl->filter_cnt--;
+	qdisc_class_put(&cl->common);
 }
 
 static int drr_graft_class(struct Qdisc *sch, unsigned long arg,
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 70b0c5873d32..122fe775c7ab 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -67,6 +67,7 @@
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 #include <asm/div64.h>
+#include <net/tc_class.h>
 
 /*
  * kernel internal service curve representation:
@@ -116,7 +117,6 @@ struct hfsc_class {
 	struct net_rate_estimator __rcu *rate_est;
 	struct tcf_proto __rcu *filter_list; /* filter list */
 	struct tcf_block *block;
-	unsigned int	filter_cnt;	/* filter count */
 	unsigned int	level;		/* class level in hierarchy */
 
 	struct hfsc_sched *sched;	/* scheduler data */
@@ -1094,7 +1094,8 @@ hfsc_delete_class(struct Qdisc *sch, unsigned long arg,
 	struct hfsc_sched *q = qdisc_priv(sch);
 	struct hfsc_class *cl = (struct hfsc_class *)arg;
 
-	if (cl->level > 0 || cl->filter_cnt > 0 || cl == &q->root)
+	if (cl->level > 0 || qdisc_class_in_use(&cl->cl_common) ||
+	    cl == &q->root)
 		return -EBUSY;
 
 	sch_tree_lock(sch);
@@ -1223,7 +1224,7 @@ hfsc_bind_tcf(struct Qdisc *sch, unsigned long parent, u32 classid)
 	if (cl != NULL) {
 		if (p != NULL && p->level <= cl->level)
 			return 0;
-		cl->filter_cnt++;
+		qdisc_class_get(&cl->cl_common);
 	}
 
 	return (unsigned long)cl;
@@ -1234,7 +1235,7 @@ hfsc_unbind_tcf(struct Qdisc *sch, unsigned long arg)
 {
 	struct hfsc_class *cl = (struct hfsc_class *)arg;
 
-	cl->filter_cnt--;
+	qdisc_class_put(&cl->cl_common);
 }
 
 static struct tcf_block *hfsc_tcf_block(struct Qdisc *sch, unsigned long arg,
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 325c29041c7d..5223b63cec00 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -37,6 +37,7 @@
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/tc_class.h>
 
 /* HTB algorithm.
     Author: devik@cdi.cz
@@ -102,7 +103,6 @@ struct htb_class {
 
 	struct tcf_proto __rcu	*filter_list;	/* class attached filters */
 	struct tcf_block	*block;
-	int			filter_cnt;
 
 	int			level;		/* our level (see above) */
 	unsigned int		children;
@@ -1710,7 +1710,7 @@ static int htb_delete(struct Qdisc *sch, unsigned long arg,
 	 * tc subsys guarantee us that in htb_destroy it holds no class
 	 * refs so that we can remove children safely there ?
 	 */
-	if (cl->children || cl->filter_cnt)
+	if (cl->children || qdisc_class_in_use(&cl->common))
 		return -EBUSY;
 
 	if (!cl->level && htb_parent_last_child(cl))
@@ -2108,7 +2108,7 @@ static unsigned long htb_bind_filter(struct Qdisc *sch, unsigned long parent,
 	 * be broken by class during destroy IIUC.
 	 */
 	if (cl)
-		cl->filter_cnt++;
+		qdisc_class_get(&cl->common);
 	return (unsigned long)cl;
 }
 
@@ -2116,8 +2116,7 @@ static void htb_unbind_filter(struct Qdisc *sch, unsigned long arg)
 {
 	struct htb_class *cl = (struct htb_class *)arg;
 
-	if (cl)
-		cl->filter_cnt--;
+	qdisc_class_put(&cl->common);
 }
 
 static void htb_walk(struct Qdisc *sch, struct qdisc_walker *arg)
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index befaf74b33ca..2515828d99a6 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -15,6 +15,7 @@
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/tc_class.h>
 
 
 /*  Quick Fair Queueing Plus
@@ -130,8 +131,6 @@ struct qfq_aggregate;
 struct qfq_class {
 	struct Qdisc_class_common common;
 
-	unsigned int filter_cnt;
-
 	struct gnet_stats_basic_sync bstats;
 	struct gnet_stats_queue qstats;
 	struct net_rate_estimator __rcu *rate_est;
@@ -545,7 +544,7 @@ static int qfq_delete_class(struct Qdisc *sch, unsigned long arg,
 	struct qfq_sched *q = qdisc_priv(sch);
 	struct qfq_class *cl = (struct qfq_class *)arg;
 
-	if (cl->filter_cnt > 0)
+	if (qdisc_class_in_use(&cl->common))
 		return -EBUSY;
 
 	sch_tree_lock(sch);
@@ -581,7 +580,7 @@ static unsigned long qfq_bind_tcf(struct Qdisc *sch, unsigned long parent,
 	struct qfq_class *cl = qfq_find_class(sch, classid);
 
 	if (cl != NULL)
-		cl->filter_cnt++;
+		qdisc_class_get(&cl->common);
 
 	return (unsigned long)cl;
 }
@@ -590,7 +589,7 @@ static void qfq_unbind_tcf(struct Qdisc *sch, unsigned long arg)
 {
 	struct qfq_class *cl = (struct qfq_class *)arg;
 
-	cl->filter_cnt--;
+	qdisc_class_put(&cl->common);
 }
 
 static int qfq_graft_class(struct Qdisc *sch, unsigned long arg,
-- 
2.39.2

[PATCH net-next 2/5] net/sched: sch_drr: warn about class in use while deleting

[Pedro Tammela]

Add extack to warn that delete was rejected because
the class is still in use

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_drr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 0bffa01c61bb..32165c8a3bd9 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -150,8 +150,10 @@ static int drr_delete_class(struct Qdisc *sch, unsigned long arg,
 	struct drr_sched *q = qdisc_priv(sch);
 	struct drr_class *cl = (struct drr_class *)arg;
 
-	if (qdisc_class_in_use(&cl->common))
+	if (qdisc_class_in_use(&cl->common)) {
+		NL_SET_ERR_MSG(extack, "DRR class is in use");
 		return -EBUSY;
+	}
 
 	sch_tree_lock(sch);
 
-- 
2.39.2

[PATCH net-next 3/5] net/sched: sch_hfsc: warn about class in use while deleting

[Pedro Tammela]

Add extack to warn that delete was rejected because
the class is still in use

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_hfsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 122fe775c7ab..4800191f7c49 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1095,8 +1095,10 @@ hfsc_delete_class(struct Qdisc *sch, unsigned long arg,
 	struct hfsc_class *cl = (struct hfsc_class *)arg;
 
 	if (cl->level > 0 || qdisc_class_in_use(&cl->cl_common) ||
-	    cl == &q->root)
+	    cl == &q->root) {
+		NL_SET_ERR_MSG(extack, "HFSC class in use");
 		return -EBUSY;
+	}
 
 	sch_tree_lock(sch);
 
-- 
2.39.2

[PATCH net-next 4/5] net/sched: sch_htb: warn about class in use while deleting

[Pedro Tammela]

Add extack to warn that delete was rejected because
the class is still in use

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_htb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 5223b63cec00..8e8660a145c3 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1710,8 +1710,10 @@ static int htb_delete(struct Qdisc *sch, unsigned long arg,
 	 * tc subsys guarantee us that in htb_destroy it holds no class
 	 * refs so that we can remove children safely there ?
 	 */
-	if (cl->children || qdisc_class_in_use(&cl->common))
+	if (cl->children || qdisc_class_in_use(&cl->common)) {
+		NL_SET_ERR_MSG(extack, "HTB class in use");
 		return -EBUSY;
+	}
 
 	if (!cl->level && htb_parent_last_child(cl))
 		last_child = 1;
-- 
2.39.2

[PATCH net-next 5/5] net/sched: sch_qfq: warn about class in use while deleting

[Pedro Tammela]

Add extack to warn that delete was rejected because
the class is still in use

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_qfq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 2515828d99a6..7a0427353cf8 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -544,8 +544,10 @@ static int qfq_delete_class(struct Qdisc *sch, unsigned long arg,
 	struct qfq_sched *q = qdisc_priv(sch);
 	struct qfq_class *cl = (struct qfq_class *)arg;
 
-	if (qdisc_class_in_use(&cl->common))
+	if (qdisc_class_in_use(&cl->common)) {
+		NL_SET_ERR_MSG_MOD(extack, "QFQ class in use");
 		return -EBUSY;
+	}
 
 	sch_tree_lock(sch);
 
-- 
2.39.2

Re: [PATCH net-next 0/5] net/sched: improve class lifetime handling

[Jamal Hadi Salim]

On Fri, Jul 21, 2023 at 3:14 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>
> Valis says[0]:
> ============
> Three classifiers (cls_fw, cls_u32 and cls_route) always copy
> tcf_result struct into the new instance of the filter on update.
>
> This causes a problem when updating a filter bound to a class,
> as tcf_unbind_filter() is always called on the old instance in the
> success path, decreasing filter_cnt of the still referenced class
> and allowing it to be deleted, leading to a use-after-free.
> ============
>
> Turns out these could have been spotted easily with proper warnings.
> Improve the current class lifetime with wrappers that check for
> overflow/underflow.
>
> While at it add an extack for when a class in use is deleted.
>
> [0] https://lore.kernel.org/all/20230721174856.3045-1-sec@valis.email/
>

For the series:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal
>
> Pedro Tammela (5):
>   net/sched: wrap open coded Qdics class filter counter
>   net/sched: sch_drr: warn about class in use while deleting
>   net/sched: sch_hfsc: warn about class in use while deleting
>   net/sched: sch_htb: warn about class in use while deleting
>   net/sched: sch_qfq: warn about class in use while deleting
>
>  include/net/sch_generic.h |  1 +
>  include/net/tc_class.h    | 33 +++++++++++++++++++++++++++++++++
>  net/sched/sch_drr.c       | 12 +++++++-----
>  net/sched/sch_hfsc.c      | 11 +++++++----
>  net/sched/sch_htb.c       | 11 ++++++-----
>  net/sched/sch_qfq.c       | 11 ++++++-----
>  6 files changed, 60 insertions(+), 19 deletions(-)
>  create mode 100644 include/net/tc_class.h
>
> --
> 2.39.2
>

Re: [PATCH net-next 1/5] net/sched: wrap open coded Qdics class filter counter

[Cong Wang]

On Fri, Jul 21, 2023 at 04:13:28PM -0300, Pedro Tammela wrote:
> The 'filter_cnt' counter is used to control a Qdisc class lifetime.
> Each filter referecing this class by its id will eventually
> increment/decrement this counter in their respective
> 'add/update/delete' routines.
> As these operations are always serialized under rtnl lock, we don't
> need an atomic type like 'refcount_t'.
> 
> It also means that we lose the overflow/underflow checks already
> present in refcount_t, which are valuable to hunt down bugs
> where the unsigned counter wraps around as it aids automated tools
> like syzkaller to scream in such situations.
> 
> Wrap the open coded increment/decrement into helper functions and
> add overflow checks to the operations.

So what's the concern of using refcount_t here? Since we have RTNL lock,
I don't think performance is a concern.

I'd prefer to reuse the overflow/underflow with refcount_t than
open-coding new ones.


> diff --git a/include/net/tc_class.h b/include/net/tc_class.h
> new file mode 100644
> index 000000000000..2ab4aa2dba30
> --- /dev/null
> +++ b/include/net/tc_class.h

Why not put these helpers togethre with other qdisc class helpers in
include/net/sch_generic.h?

Thanks.

Re: [PATCH net-next 1/5] net/sched: wrap open coded Qdics class filter counter

[Pedro Tammela]

On 23/07/2023 15:10, Cong Wang wrote:
> On Fri, Jul 21, 2023 at 04:13:28PM -0300, Pedro Tammela wrote:
>> The 'filter_cnt' counter is used to control a Qdisc class lifetime.
>> Each filter referecing this class by its id will eventually
>> increment/decrement this counter in their respective
>> 'add/update/delete' routines.
>> As these operations are always serialized under rtnl lock, we don't
>> need an atomic type like 'refcount_t'.
>>
>> It also means that we lose the overflow/underflow checks already
>> present in refcount_t, which are valuable to hunt down bugs
>> where the unsigned counter wraps around as it aids automated tools
>> like syzkaller to scream in such situations.
>>
>> Wrap the open coded increment/decrement into helper functions and
>> add overflow checks to the operations.
> 
> So what's the concern of using refcount_t here? Since we have RTNL lock,
> I don't think performance is a concern.
> 
> I'd prefer to reuse the overflow/underflow with refcount_t than
> open-coding new ones.

I see.
There's a need for another minor adaption as well.
As the 'filter_cnt' starts from 0, it can do 0->1 transitions, which the 
refcount API warns.
So in a refcount_t 'filter_cnt' we would need to initialize it to 1.

I think it's confusing for a variable that represents a count of filters 
have 1 as a nil value.

> 
> 
>> diff --git a/include/net/tc_class.h b/include/net/tc_class.h
>> new file mode 100644
>> index 000000000000..2ab4aa2dba30
>> --- /dev/null
>> +++ b/include/net/tc_class.h
> 
> Why not put these helpers togethre with other qdisc class helpers in
> include/net/sch_generic.h?

OK

> 
> Thanks.