Commit 630e0afa authored by Paolo Abeni's avatar Paolo Abeni
Browse files

Merge branch 'net-sched-improve-class-lifetime-handling' 

Pedro Tammela says:

====================
net/sched: improve class lifetime handling

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/
====================

Link: https://lore.kernel.org/r/20230728153537.1865379-1-pctammela@mojatatu.com


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parents 05e1d8bd e20e7501

include/net/sch_generic.h

+26 −0
Original line number Diff line number Diff line
@@ -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;

};
@@ -633,6 +634,31 @@ qdisc_class_find(const struct Qdisc_class_hash *hash, u32 id) 
	return NULL;

}



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;

}



static inline int tc_classid_to_hwtc(struct net_device *dev, u32 classid)

{

	u32 hwtc = TC_H_MIN(classid) - TC_H_MIN_PRIORITY;

net/sched/sch_drr.c

+6 −5
Original line number Diff line number Diff line
@@ -17,7 +17,6 @@ 


struct drr_class {

	struct Qdisc_class_common	common;

	unsigned int			filter_cnt;



	struct gnet_stats_basic_sync		bstats;

	struct gnet_stats_queue		qstats;
@@ -150,8 +149,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 (cl->filter_cnt > 0)

	if (qdisc_class_in_use(&cl->common)) {

		NL_SET_ERR_MSG(extack, "DRR class is in use");

		return -EBUSY;

	}



	sch_tree_lock(sch);
@@ -187,8 +188,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 +198,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,

net/sched/sch_hfsc.c

+6 −4
Original line number Diff line number Diff line
@@ -116,7 +116,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,8 +1093,11 @@ 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) {

		NL_SET_ERR_MSG(extack, "HFSC class in use");

		return -EBUSY;

	}



	sch_tree_lock(sch);
@@ -1223,7 +1225,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 +1236,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,

net/sched/sch_htb.c

+5 −5
Original line number Diff line number Diff line
@@ -102,7 +102,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,8 +1709,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 || cl->filter_cnt)

	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;
@@ -2107,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;

}
@@ -2115,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)

net/sched/sch_qfq.c

+6 −6
Original line number Diff line number Diff line
@@ -130,8 +130,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,8 +543,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 (cl->filter_cnt > 0)

	if (qdisc_class_in_use(&cl->common)) {

		NL_SET_ERR_MSG_MOD(extack, "QFQ class in use");

		return -EBUSY;

	}



	sch_tree_lock(sch);
@@ -580,8 +580,8 @@ 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++;

	if (cl)

		qdisc_class_get(&cl->common);



	return (unsigned long)cl;

}
@@ -590,7 +590,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,