[PATCH net-next 0/2] net: sched: remove redundant resource cleanup when init() fails

[PATCH net-next 0/2] net: sched: remove redundant resource cleanup when init() fails

[Zhengchao Shao]

qdisc_create() calls .init() to initialize qdisc. If the initialization
fails, qdisc_create() will call .destroy() to release resources.

Zhengchao Shao (2):
  net: sched: fq_codel: remove redundant resource cleanup in
    fq_codel_init()
  net: sched: htb: remove redundant resource cleanup in htb_init()

 net/sched/sch_fq_codel.c | 17 ++++-------------
 net/sched/sch_htb.c      | 36 +++++++++---------------------------
 2 files changed, 13 insertions(+), 40 deletions(-)

-- 
2.17.1

[PATCH net-next 1/2] net: sched: fq_codel: remove redundant resource cleanup in fq_codel_init()

[Zhengchao Shao]

If fq_codel_init() fails, qdisc_create() invokes fq_codel_destroy() to
clear resources. Therefore, remove redundant resource cleanup in
fq_codel_init().

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 net/sched/sch_fq_codel.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 23a042adb74d..90abbca5aeb0 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -481,25 +481,23 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
 	if (opt) {
 		err = fq_codel_change(sch, opt, extack);
 		if (err)
-			goto init_failure;
+			return err;
 	}
 
 	err = tcf_block_get(&q->block, &q->filter_list, sch, extack);
 	if (err)
-		goto init_failure;
+		return err;
 
 	if (!q->flows) {
 		q->flows = kvcalloc(q->flows_cnt,
 				    sizeof(struct fq_codel_flow),
 				    GFP_KERNEL);
 		if (!q->flows) {
-			err = -ENOMEM;
-			goto init_failure;
+			return -ENOMEM;
 		}
 		q->backlogs = kvcalloc(q->flows_cnt, sizeof(u32), GFP_KERNEL);
 		if (!q->backlogs) {
-			err = -ENOMEM;
-			goto alloc_failure;
+			return -ENOMEM;
 		}
 		for (i = 0; i < q->flows_cnt; i++) {
 			struct fq_codel_flow *flow = q->flows + i;
@@ -513,13 +511,6 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
 	else
 		sch->flags &= ~TCQ_F_CAN_BYPASS;
 	return 0;
-
-alloc_failure:
-	kvfree(q->flows);
-	q->flows = NULL;
-init_failure:
-	q->flows_cnt = 0;
-	return err;
 }
 
 static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
-- 
2.17.1

[PATCH net-next 2/2] net: sched: htb: remove redundant resource cleanup in htb_init()

[Zhengchao Shao]

If htb_init() fails, qdisc_create() invokes htb_destroy() to clear
resources. Therefore, remove redundant resource cleanup in htb_init().

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 net/sched/sch_htb.c | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index cb5872d22ecf..da75885bbffd 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1102,7 +1102,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 
 	err = qdisc_class_hash_init(&q->clhash);
 	if (err < 0)
-		goto err_free_direct_qdiscs;
+		return err;
 
 	qdisc_skb_head_init(&q->direct_queue);
 
@@ -1125,8 +1125,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 		qdisc = qdisc_create_dflt(dev_queue, &pfifo_qdisc_ops,
 					  TC_H_MAKE(sch->handle, 0), extack);
 		if (!qdisc) {
-			err = -ENOMEM;
-			goto err_free_qdiscs;
+			return -ENOMEM;
 		}
 
 		htb_set_lockdep_class_child(qdisc);
@@ -1144,7 +1143,7 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 	};
 	err = htb_offload(dev, &offload_opt);
 	if (err)
-		goto err_free_qdiscs;
+		return err;
 
 	/* Defer this assignment, so that htb_destroy skips offload-related
 	 * parts (especially calling ndo_setup_tc) on errors.
@@ -1152,22 +1151,6 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt,
 	q->offload = true;
 
 	return 0;
-
-err_free_qdiscs:
-	for (ntx = 0; ntx < q->num_direct_qdiscs && q->direct_qdiscs[ntx];
-	     ntx++)
-		qdisc_put(q->direct_qdiscs[ntx]);
-
-	qdisc_class_hash_destroy(&q->clhash);
-	/* Prevent use-after-free and double-free when htb_destroy gets called.
-	 */
-	q->clhash.hash = NULL;
-	q->clhash.hashsize = 0;
-
-err_free_direct_qdiscs:
-	kfree(q->direct_qdiscs);
-	q->direct_qdiscs = NULL;
-	return err;
 }
 
 static void htb_attach_offload(struct Qdisc *sch)
@@ -1690,13 +1673,12 @@ static void htb_destroy(struct Qdisc *sch)
 	qdisc_class_hash_destroy(&q->clhash);
 	__qdisc_reset_queue(&q->direct_queue);
 
-	if (!q->offload)
-		return;
-
-	offload_opt = (struct tc_htb_qopt_offload) {
-		.command = TC_HTB_DESTROY,
-	};
-	htb_offload(dev, &offload_opt);
+	if (q->offload) {
+		offload_opt = (struct tc_htb_qopt_offload) {
+			.command = TC_HTB_DESTROY,
+		};
+		htb_offload(dev, &offload_opt);
+	}
 
 	if (!q->direct_qdiscs)
 		return;
-- 
2.17.1

Re: [PATCH net-next 0/2] net: sched: remove redundant resource cleanup when init() fails

[Jakub Kicinski]

On Tue, 30 Aug 2022 08:56:36 +0800 Zhengchao Shao wrote:
> qdisc_create() calls .init() to initialize qdisc. If the initialization
> fails, qdisc_create() will call .destroy() to release resources.

Looks like this set does not apply cleanly, please rebase on top 
of latest net-next/main and repost.