[PATCH 1/2] tc: check for errors from gen_replace_estimator

[PATCH 1/2] tc: check for errors from gen_replace_estimator

[Stephen Hemminger]

gen_replace_estimator can return errors, but they were being ignored.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/sched/act_police.c	2008-11-25 10:02:05.000000000 -0800
+++ b/net/sched/act_police.c	2008-11-25 10:15:50.000000000 -0800
@@ -185,14 +185,21 @@ override:
 		if (parm->peakrate.rate) {
 			P_tab = qdisc_get_rtab(&parm->peakrate,
 					       tb[TCA_POLICE_PEAKRATE]);
-			if (P_tab == NULL) {
-				qdisc_put_rtab(R_tab);
+			if (P_tab == NULL)
 				goto failure;
-			}
 		}
 	}
-	/* No failure allowed after this point */
+
 	spin_lock_bh(&police->tcf_lock);
+	if (est) {
+		err = gen_replace_estimator(&police->tcf_bstats,
+					    &police->tcf_rate_est,
+					    &police->tcf_lock, est);
+		if (err)
+			goto failure_unlock;
+	}
+
+	/* No failure allowed after this point */
 	if (R_tab != NULL) {
 		qdisc_put_rtab(police->tcfp_R_tab);
 		police->tcfp_R_tab = R_tab;
@@ -217,10 +224,6 @@ override:
 
 	if (tb[TCA_POLICE_AVRATE])
 		police->tcfp_ewma_rate = nla_get_u32(tb[TCA_POLICE_AVRATE]);
-	if (est)
-		gen_replace_estimator(&police->tcf_bstats,
-				      &police->tcf_rate_est,
-				      &police->tcf_lock, est);
 
 	spin_unlock_bh(&police->tcf_lock);
 	if (ret != ACT_P_CREATED)
@@ -238,7 +241,13 @@ override:
 	a->priv = police;
 	return ret;
 
+failure_unlock:
+	spin_unlock_bh(&police->tcf_lock);
 failure:
+	if (P_tab)
+		qdisc_put_rtab(P_tab);
+	if (R_tab)
+		qdisc_put_rtab(R_tab);
 	if (ret == ACT_P_CREATED)
 		kfree(police);
 	return err;
--- a/net/sched/sch_api.c	2008-11-25 10:02:05.000000000 -0800
+++ b/net/sched/sch_api.c	2008-11-25 10:07:21.000000000 -0800
@@ -897,10 +897,10 @@ static int qdisc_change(struct Qdisc *sc
 	sch->stab = stab;
 
 	if (tca[TCA_RATE])
-		gen_replace_estimator(&sch->bstats, &sch->rate_est,
-				      qdisc_root_sleeping_lock(sch),
-				      tca[TCA_RATE]);
-	return 0;
+		err = gen_replace_estimator(&sch->bstats, &sch->rate_est,
+					    qdisc_root_sleeping_lock(sch),
+					    tca[TCA_RATE]);
+	return err;
 }
 
 struct check_loop_arg
--- a/net/sched/sch_cbq.c	2008-11-25 10:02:05.000000000 -0800
+++ b/net/sched/sch_cbq.c	2008-11-25 10:07:21.000000000 -0800
@@ -1806,10 +1806,10 @@ cbq_change_class(struct Qdisc *sch, u32 
 		sch_tree_unlock(sch);
 
 		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
-		return 0;
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+		return err;
 	}
 
 	if (parentid == TC_H_ROOT)
--- a/net/sched/sch_drr.c	2008-11-25 10:02:05.000000000 -0800
+++ b/net/sched/sch_drr.c	2008-11-25 10:07:21.000000000 -0800
@@ -88,10 +88,10 @@ static int drr_change_class(struct Qdisc
 		sch_tree_unlock(sch);
 
 		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
-		return 0;
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+		return err;
 	}
 
 	cl = kzalloc(sizeof(struct drr_class), GFP_KERNEL);
@@ -106,10 +106,15 @@ static int drr_change_class(struct Qdisc
 	if (cl->qdisc == NULL)
 		cl->qdisc = &noop_qdisc;
 
-	if (tca[TCA_RATE])
-		gen_replace_estimator(&cl->bstats, &cl->rate_est,
-				      qdisc_root_sleeping_lock(sch),
-				      tca[TCA_RATE]);
+	if (tca[TCA_RATE]) {
+		err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+					    qdisc_root_sleeping_lock(sch),
+					    tca[TCA_RATE]);
+		if (err) {
+			kfree(cl);
+			return err;
+		}
+	}
 
 	sch_tree_lock(sch);
 	qdisc_class_hash_insert(&q->clhash, &cl->common);
--- a/net/sched/sch_hfsc.c	2008-11-25 10:02:05.000000000 -0800
+++ b/net/sched/sch_hfsc.c	2008-11-25 10:07:21.000000000 -0800
@@ -1035,10 +1035,10 @@ hfsc_change_class(struct Qdisc *sch, u32
 		sch_tree_unlock(sch);
 
 		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
 					      qdisc_root_sleeping_lock(sch),
 					      tca[TCA_RATE]);
-		return 0;
+		return err;
 	}
 
 	if (parentid == TC_H_ROOT)

[PATCH 2/2] tc: policing requires a rate estimator

[Stephen Hemminger]

Found that while trying average rate policing, it was possible to
request average rate policing without a rate estimator. This results
in no policing which is harmless but incorrect.

Since policing could be setup in two steps, need to check
in the kernel.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 include/net/gen_stats.h  |    1 +
 net/core/gen_estimator.c |   30 +++++++++++++++++++++++++++---
 net/sched/act_police.c   |   16 ++++++++++++----
 3 files changed, 40 insertions(+), 7 deletions(-)

--- a/net/sched/act_police.c	2008-11-25 10:15:50.000000000 -0800
+++ b/net/sched/act_police.c	2008-11-25 10:17:54.000000000 -0800
@@ -182,6 +182,12 @@ override:
 		R_tab = qdisc_get_rtab(&parm->rate, tb[TCA_POLICE_RATE]);
 		if (R_tab == NULL)
 			goto failure;
+
+		if (!est && !gen_estimator_active(&police->tcf_rate_est)) {
+			err = -EINVAL;
+			goto failure;
+		}
+
 		if (parm->peakrate.rate) {
 			P_tab = qdisc_get_rtab(&parm->peakrate,
 					       tb[TCA_POLICE_PEAKRATE]);
--- a/include/net/gen_stats.h	2008-11-25 10:14:52.000000000 -0800
+++ b/include/net/gen_stats.h	2008-11-25 10:16:52.000000000 -0800
@@ -45,5 +45,6 @@ extern void gen_kill_estimator(struct gn
 extern int gen_replace_estimator(struct gnet_stats_basic *bstats,
 				 struct gnet_stats_rate_est *rate_est,
 				 spinlock_t *stats_lock, struct nlattr *opt);
+extern int gen_estimator_active(const struct gnet_stats_rate_est *rate_est);
 
 #endif
--- a/net/core/gen_estimator.c	2008-11-25 10:14:52.000000000 -0800
+++ b/net/core/gen_estimator.c	2008-11-25 10:16:52.000000000 -0800
@@ -242,6 +242,7 @@ int gen_new_estimator(struct gnet_stats_
 
 	return 0;
 }
+EXPORT_SYMBOL(gen_new_estimator);
 
 static void __gen_kill_estimator(struct rcu_head *head)
 {
@@ -275,6 +276,7 @@ void gen_kill_estimator(struct gnet_stat
 		call_rcu(&e->e_rcu, __gen_kill_estimator);
 	}
 }
+EXPORT_SYMBOL(gen_kill_estimator);
 
 /**
  * gen_replace_estimator - replace rate estimator configuration
@@ -295,8 +297,30 @@ int gen_replace_estimator(struct gnet_st
 	gen_kill_estimator(bstats, rate_est);
 	return gen_new_estimator(bstats, rate_est, stats_lock, opt);
 }
+EXPORT_SYMBOL(gen_replace_estimator);
+
+/**
+ * gen_estimator_active - test if estimator is currently in use
+ * @rate_est: rate estimator statistics
+ *
+ * Returns 1 if estimator is active, and 0 if not.
+ */
+int gen_estimator_active(const struct gnet_stats_rate_est *rate_est)
+{
+	int idx;
+	struct gen_estimator *e;
 
+	ASSERT_RTNL();
 
-EXPORT_SYMBOL(gen_kill_estimator);
-EXPORT_SYMBOL(gen_new_estimator);
-EXPORT_SYMBOL(gen_replace_estimator);
+	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
+		if (!elist[idx].timer.function)
+			continue;
+
+		list_for_each_entry(e, &elist[idx].list, list) {
+			if (e->rate_est == rate_est)
+				return 1;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL(gen_estimator_active);

Re: [PATCH 2/2] tc: policing requires a rate estimator

[Patrick McHardy]

Stephen Hemminger wrote:
> Found that while trying average rate policing, it was possible to
> request average rate policing without a rate estimator. This results
> in no policing which is harmless but incorrect.
> 
> Since policing could be setup in two steps, need to check
> in the kernel.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> ---
>  include/net/gen_stats.h  |    1 +
>  net/core/gen_estimator.c |   30 +++++++++++++++++++++++++++---
>  net/sched/act_police.c   |   16 ++++++++++++----
>  3 files changed, 40 insertions(+), 7 deletions(-)
> 
> --- a/net/sched/act_police.c	2008-11-25 10:15:50.000000000 -0800
> +++ b/net/sched/act_police.c	2008-11-25 10:17:54.000000000 -0800
> @@ -182,6 +182,12 @@ override:
>  		R_tab = qdisc_get_rtab(&parm->rate, tb[TCA_POLICE_RATE]);
>  		if (R_tab == NULL)
>  			goto failure;
> +
> +		if (!est && !gen_estimator_active(&police->tcf_rate_est)) {
> +			err = -EINVAL;
> +			goto failure;

This leaks the R_tab reference.

Re: [PATCH 2/2] tc: policing requires a rate estimator

[Stephen Hemminger]

On Tue, 25 Nov 2008 19:32:13 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Stephen Hemminger wrote:
> > Found that while trying average rate policing, it was possible to
> > request average rate policing without a rate estimator. This results
> > in no policing which is harmless but incorrect.
> > 
> > Since policing could be setup in two steps, need to check
> > in the kernel.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > 
> > ---
> >  include/net/gen_stats.h  |    1 +
> >  net/core/gen_estimator.c |   30 +++++++++++++++++++++++++++---
> >  net/sched/act_police.c   |   16 ++++++++++++----
> >  3 files changed, 40 insertions(+), 7 deletions(-)
> > 
> > --- a/net/sched/act_police.c	2008-11-25 10:15:50.000000000 -0800
> > +++ b/net/sched/act_police.c	2008-11-25 10:17:54.000000000 -0800
> > @@ -182,6 +182,12 @@ override:
> >  		R_tab = qdisc_get_rtab(&parm->rate, tb[TCA_POLICE_RATE]);
> >  		if (R_tab == NULL)
> >  			goto failure;
> > +
> > +		if (!est && !gen_estimator_active(&police->tcf_rate_est)) {
> > +			err = -EINVAL;
> > +			goto failure;
> 
> This leaks the R_tab reference.

The previous patch added a put at the failure: tag.

Re: [PATCH 2/2] tc: policing requires a rate estimator

[Patrick McHardy]

Stephen Hemminger wrote:
> On Tue, 25 Nov 2008 19:32:13 +0100
> Patrick McHardy <kaber@trash.net> wrote:
> 
>>>
>>> --- a/net/sched/act_police.c	2008-11-25 10:15:50.000000000 -0800
>>> +++ b/net/sched/act_police.c	2008-11-25 10:17:54.000000000 -0800
>>> @@ -182,6 +182,12 @@ override:
>>>  		R_tab = qdisc_get_rtab(&parm->rate, tb[TCA_POLICE_RATE]);
>>>  		if (R_tab == NULL)
>>>  			goto failure;
>>> +
>>> +		if (!est && !gen_estimator_active(&police->tcf_rate_est)) {
>>> +			err = -EINVAL;
>>> +			goto failure;
>> This leaks the R_tab reference.
> 
> The previous patch added a put at the failure: tag.

I missed that, sorry.

Re: [PATCH 2/2] tc: policing requires a rate estimator

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 25 Nov 2008 10:21:13 -0800

> Found that while trying average rate policing, it was possible to
> request average rate policing without a rate estimator. This results
> in no policing which is harmless but incorrect.
> 
> Since policing could be setup in two steps, need to check
> in the kernel.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

[PATCH] pkt_sched: gen_estimator: Optimize gen_estimator_active()

[Jarek Poplawski]

pkt_sched: gen_estimator: Optimize gen_estimator_active()

Since all other gen_estimator functions use bstats and rate_est params
together, and searching for them is optimized now, let's use this also
in gen_estimator_active(). The return type of gen_estimator_active()
is changed to bool, and gen_find_node() parameters to const, btw.

In tcf_act_police_locate() a check for ACT_P_CREATED is added before
calling gen_estimator_active().

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 include/net/gen_stats.h  |    4 ++--
 net/core/gen_estimator.c |   25 ++++++++-----------------
 net/sched/act_police.c   |    4 +++-
 3 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
index dcf5bfa..d136b52 100644
--- a/include/net/gen_stats.h
+++ b/include/net/gen_stats.h
@@ -45,6 +45,6 @@ extern void gen_kill_estimator(struct gnet_stats_basic *bstats,
 extern int gen_replace_estimator(struct gnet_stats_basic *bstats,
 				 struct gnet_stats_rate_est *rate_est,
 				 spinlock_t *stats_lock, struct nlattr *opt);
-extern int gen_estimator_active(const struct gnet_stats_rate_est *rate_est);
-
+extern bool gen_estimator_active(const struct gnet_stats_basic *bstats,
+				 const struct gnet_stats_rate_est *rate_est);
 #endif
diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index 3885550..9cc9f95 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -163,8 +163,9 @@ static void gen_add_node(struct gen_estimator *est)
 	rb_insert_color(&est->node, &est_root);
 }
 
-static struct gen_estimator *gen_find_node(struct gnet_stats_basic *bstats,
-					   struct gnet_stats_rate_est *rate_est)
+static
+struct gen_estimator *gen_find_node(const struct gnet_stats_basic *bstats,
+				    const struct gnet_stats_rate_est *rate_est)
 {
 	struct rb_node *p = est_root.rb_node;
 
@@ -301,26 +302,16 @@ EXPORT_SYMBOL(gen_replace_estimator);
 
 /**
  * gen_estimator_active - test if estimator is currently in use
+ * @bstats: basic statistics
  * @rate_est: rate estimator statistics
  *
- * Returns 1 if estimator is active, and 0 if not.
+ * Returns true if estimator is active, and false if not.
  */
-int gen_estimator_active(const struct gnet_stats_rate_est *rate_est)
+bool gen_estimator_active(const struct gnet_stats_basic *bstats,
+			  const struct gnet_stats_rate_est *rate_est)
 {
-	int idx;
-	struct gen_estimator *e;
-
 	ASSERT_RTNL();
 
-	for (idx=0; idx <= EST_MAX_INTERVAL; idx++) {
-		if (!elist[idx].timer.function)
-			continue;
-
-		list_for_each_entry(e, &elist[idx].list, list) {
-			if (e->rate_est == rate_est)
-				return 1;
-		}
-	}
-	return 0;
+	return gen_find_node(bstats, rate_est) != NULL;
 }
 EXPORT_SYMBOL(gen_estimator_active);
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 40df1d0..40f9a81 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -183,7 +183,9 @@ override:
 		if (R_tab == NULL)
 			goto failure;
 
-		if (!est && !gen_estimator_active(&police->tcf_rate_est)) {
+		if (!est && (ret == ACT_P_CREATED ||
+			     !gen_estimator_active(&police->tcf_bstats,
+						   &police->tcf_rate_est))) {
 			err = -EINVAL;
 			goto failure;
 		}

Re: [PATCH] pkt_sched: gen_estimator: Optimize gen_estimator_active()

[Stephen Hemminger]

On Wed, 26 Nov 2008 11:10:22 +0000
Jarek Poplawski <jarkao2@gmail.com> wrote:

> pkt_sched: gen_estimator: Optimize gen_estimator_active()
> 
> Since all other gen_estimator functions use bstats and rate_est params
> together, and searching for them is optimized now, let's use this also
> in gen_estimator_active(). The return type of gen_estimator_active()
> is changed to bool, and gen_find_node() parameters to const, btw.
> 
> In tcf_act_police_locate() a check for ACT_P_CREATED is added before
> calling gen_estimator_active().
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

yeah sure, i just coded the obvious way.

Re: [PATCH] pkt_sched: gen_estimator: Optimize gen_estimator_active()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 26 Nov 2008 11:10:22 +0000

> pkt_sched: gen_estimator: Optimize gen_estimator_active()
> 
> Since all other gen_estimator functions use bstats and rate_est params
> together, and searching for them is optimized now, let's use this also
> in gen_estimator_active(). The return type of gen_estimator_active()
> is changed to bool, and gen_find_node() parameters to const, btw.
> 
> In tcf_act_police_locate() a check for ACT_P_CREATED is added before
> calling gen_estimator_active().
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied, thanks Jarek.

Re: [PATCH 1/2] tc: check for errors from gen_replace_estimator

[Patrick McHardy]

Stephen Hemminger wrote:
> gen_replace_estimator can return errors, but they were being ignored.
> 
> --- a/net/sched/sch_hfsc.c	2008-11-25 10:02:05.000000000 -0800
> +++ b/net/sched/sch_hfsc.c	2008-11-25 10:07:21.000000000 -0800
> @@ -1035,10 +1035,10 @@ hfsc_change_class(struct Qdisc *sch, u32
>  		sch_tree_unlock(sch);
>  
>  		if (tca[TCA_RATE])
> -			gen_replace_estimator(&cl->bstats, &cl->rate_est,
> +			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
>  					      qdisc_root_sleeping_lock(sch),
>  					      tca[TCA_RATE]);
> -		return 0;
> +		return err;

At this point we've already made changes to the class, so
returning an error without undoing them is wrong.

I'd suggest to move the estimator replacement up a bit, the
other changes can't fail.

There's also gen_new_estimator() that should be treated
similary for consistency.

[PATCH 1a/2] tc: propogate errors from tcf_hash_create

[Stephen Hemminger]

Allow tcf_hash_create to return different errors on estimator failure.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 net/sched/act_api.c     |   18 +++++++++++++-----
 net/sched/act_gact.c    |    4 ++--
 net/sched/act_ipt.c     |    4 ++--
 net/sched/act_mirred.c  |    4 ++--
 net/sched/act_nat.c     |    4 ++--
 net/sched/act_pedit.c   |    4 ++--
 net/sched/act_simple.c  |    4 ++--
 net/sched/act_skbedit.c |    4 ++--
 8 files changed, 27 insertions(+), 19 deletions(-)

--- a/net/sched/act_api.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_api.c	2008-11-25 10:50:56.000000000 -0800
@@ -214,12 +214,14 @@ struct tcf_common *tcf_hash_check(u32 in
 }
 EXPORT_SYMBOL(tcf_hash_check);
 
-struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a, int size, int bind, u32 *idx_gen, struct tcf_hashinfo *hinfo)
+struct tcf_common *tcf_hash_create(u32 index, struct nlattr *est,
+				   struct tc_action *a, int size, int bind,
+				   u32 *idx_gen, struct tcf_hashinfo *hinfo)
 {
 	struct tcf_common *p = kzalloc(size, GFP_KERNEL);
 
 	if (unlikely(!p))
-		return p;
+		return ERR_PTR(-ENOMEM);
 	p->tcfc_refcnt = 1;
 	if (bind)
 		p->tcfc_bindcnt = 1;
@@ -228,9 +230,15 @@ struct tcf_common *tcf_hash_create(u32 i
 	p->tcfc_index = index ? index : tcf_hash_new_index(idx_gen, hinfo);
 	p->tcfc_tm.install = jiffies;
 	p->tcfc_tm.lastuse = jiffies;
-	if (est)
-		gen_new_estimator(&p->tcfc_bstats, &p->tcfc_rate_est,
-				  &p->tcfc_lock, est);
+	if (est) {
+		int err = gen_new_estimator(&p->tcfc_bstats, &p->tcfc_rate_est,
+					    &p->tcfc_lock, est);
+		if (err) {
+			kfree(p);
+			return ERR_PTR(err);
+		}
+	}
+
 	a->priv = (void *) p;
 	return p;
 }
--- a/net/sched/act_gact.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_gact.c	2008-11-25 10:52:19.000000000 -0800
@@ -88,8 +88,8 @@ static int tcf_gact_init(struct nlattr *
 	if (!pc) {
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*gact),
 				     bind, &gact_idx_gen, &gact_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
--- a/net/sched/act_ipt.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_ipt.c	2008-11-25 10:52:31.000000000 -0800
@@ -136,8 +136,8 @@ static int tcf_ipt_init(struct nlattr *n
 	if (!pc) {
 		pc = tcf_hash_create(index, est, a, sizeof(*ipt), bind,
 				     &ipt_idx_gen, &ipt_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
--- a/net/sched/act_mirred.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_mirred.c	2008-11-25 10:52:25.000000000 -0800
@@ -105,8 +105,8 @@ static int tcf_mirred_init(struct nlattr
 			return -EINVAL;
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*m), bind,
 				     &mirred_idx_gen, &mirred_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 		ret = ACT_P_CREATED;
 	} else {
 		if (!ovr) {
--- a/net/sched/act_nat.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_nat.c	2008-11-25 10:52:37.000000000 -0800
@@ -68,8 +68,8 @@ static int tcf_nat_init(struct nlattr *n
 	if (!pc) {
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind,
 				     &nat_idx_gen, &nat_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 		p = to_tcf_nat(pc);
 		ret = ACT_P_CREATED;
 	} else {
--- a/net/sched/act_pedit.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_pedit.c	2008-11-25 10:52:44.000000000 -0800
@@ -68,8 +68,8 @@ static int tcf_pedit_init(struct nlattr 
 			return -EINVAL;
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*p), bind,
 				     &pedit_idx_gen, &pedit_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 		p = to_pedit(pc);
 		keys = kmalloc(ksize, GFP_KERNEL);
 		if (keys == NULL) {
--- a/net/sched/act_simple.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_simple.c	2008-11-25 10:52:51.000000000 -0800
@@ -124,8 +124,8 @@ static int tcf_simp_init(struct nlattr *
 	if (!pc) {
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind,
 				     &simp_idx_gen, &simp_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 
 		d = to_defact(pc);
 		ret = alloc_defdata(d, defdata);
--- a/net/sched/act_skbedit.c	2008-11-25 10:47:48.000000000 -0800
+++ b/net/sched/act_skbedit.c	2008-11-25 10:53:11.000000000 -0800
@@ -104,8 +104,8 @@ static int tcf_skbedit_init(struct nlatt
 	if (!pc) {
 		pc = tcf_hash_create(parm->index, est, a, sizeof(*d), bind,
 				     &skbedit_idx_gen, &skbedit_hash_info);
-		if (unlikely(!pc))
-			return -ENOMEM;
+		if (IS_ERR(pc))
+		    return PTR_ERR(pc);
 
 		d = to_skbedit(pc);
 		ret = ACT_P_CREATED;

Re: [PATCH 1a/2] tc: propogate errors from tcf_hash_create

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 25 Nov 2008 10:57:27 -0800

> Allow tcf_hash_create to return different errors on estimator failure.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

[PATCH 1b/2] tc: check for errors in gen_rate_estimator creation

[Stephen Hemminger]

The functions gen_new_estimator and gen_replace_estimator can return errors,
but they were being ignored.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 net/sched/act_police.c |   25 +++++++++++++++++--------
 net/sched/sch_api.c    |   12 ++++++++----
 net/sched/sch_cbq.c    |   33 ++++++++++++++++++++++++---------
 net/sched/sch_drr.c    |   26 ++++++++++++++++++--------
 net/sched/sch_hfsc.c   |   12 ++++++++----
 net/sched/sch_htb.c    |   22 +++++++++++++++-------
 6 files changed, 90 insertions(+), 40 deletions(-)

--- a/net/sched/act_police.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/act_police.c	2008-11-25 10:54:16.000000000 -0800
@@ -185,14 +185,21 @@ override:
 		if (parm->peakrate.rate) {
 			P_tab = qdisc_get_rtab(&parm->peakrate,
 					       tb[TCA_POLICE_PEAKRATE]);
-			if (P_tab == NULL) {
-				qdisc_put_rtab(R_tab);
+			if (P_tab == NULL)
 				goto failure;
-			}
 		}
 	}
-	/* No failure allowed after this point */
+
 	spin_lock_bh(&police->tcf_lock);
+	if (est) {
+		err = gen_replace_estimator(&police->tcf_bstats,
+					    &police->tcf_rate_est,
+					    &police->tcf_lock, est);
+		if (err)
+			goto failure_unlock;
+	}
+
+	/* No failure allowed after this point */
 	if (R_tab != NULL) {
 		qdisc_put_rtab(police->tcfp_R_tab);
 		police->tcfp_R_tab = R_tab;
@@ -217,10 +224,6 @@ override:
 
 	if (tb[TCA_POLICE_AVRATE])
 		police->tcfp_ewma_rate = nla_get_u32(tb[TCA_POLICE_AVRATE]);
-	if (est)
-		gen_replace_estimator(&police->tcf_bstats,
-				      &police->tcf_rate_est,
-				      &police->tcf_lock, est);
 
 	spin_unlock_bh(&police->tcf_lock);
 	if (ret != ACT_P_CREATED)
@@ -238,7 +241,13 @@ override:
 	a->priv = police;
 	return ret;
 
+failure_unlock:
+	spin_unlock_bh(&police->tcf_lock);
 failure:
+	if (P_tab)
+		qdisc_put_rtab(P_tab);
+	if (R_tab)
+		qdisc_put_rtab(R_tab);
 	if (ret == ACT_P_CREATED)
 		kfree(police);
 	return err;
--- a/net/sched/sch_api.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_api.c	2008-11-25 10:54:16.000000000 -0800
@@ -887,6 +887,14 @@ static int qdisc_change(struct Qdisc *sc
 			return err;
 	}
 
+	if (tca[TCA_RATE]) {
+		err = gen_replace_estimator(&sch->bstats, &sch->rate_est,
+					    qdisc_root_sleeping_lock(sch),
+					    tca[TCA_RATE]);
+		if (err)
+			return err;
+	}
+
 	if (tca[TCA_STAB]) {
 		stab = qdisc_get_stab(tca[TCA_STAB]);
 		if (IS_ERR(stab))
@@ -896,10 +904,6 @@ static int qdisc_change(struct Qdisc *sc
 	qdisc_put_stab(sch->stab);
 	sch->stab = stab;
 
-	if (tca[TCA_RATE])
-		gen_replace_estimator(&sch->bstats, &sch->rate_est,
-				      qdisc_root_sleeping_lock(sch),
-				      tca[TCA_RATE]);
 	return 0;
 }
 
--- a/net/sched/sch_cbq.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_cbq.c	2008-11-25 10:56:15.000000000 -0800
@@ -1765,11 +1765,23 @@ cbq_change_class(struct Qdisc *sch, u32 
 		}
 
 		if (tb[TCA_CBQ_RATE]) {
-			rtab = qdisc_get_rtab(nla_data(tb[TCA_CBQ_RATE]), tb[TCA_CBQ_RTAB]);
+			rtab = qdisc_get_rtab(nla_data(tb[TCA_CBQ_RATE]),
+					      tb[TCA_CBQ_RTAB]);
 			if (rtab == NULL)
 				return -EINVAL;
 		}
 
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+			if (err) {
+				if (rtab)
+					qdisc_put_rtab(rtab);
+				return err;
+			}
+		}
+
 		/* Change class parameters */
 		sch_tree_lock(sch);
 
@@ -1805,10 +1817,6 @@ cbq_change_class(struct Qdisc *sch, u32 
 
 		sch_tree_unlock(sch);
 
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
 		return 0;
 	}
 
@@ -1855,6 +1863,17 @@ cbq_change_class(struct Qdisc *sch, u32 
 	cl = kzalloc(sizeof(*cl), GFP_KERNEL);
 	if (cl == NULL)
 		goto failure;
+
+	if (tca[TCA_RATE]) {
+		err = gen_new_estimator(&cl->bstats, &cl->rate_est,
+					qdisc_root_sleeping_lock(sch),
+					tca[TCA_RATE]);
+		if (err) {
+			kfree(cl);
+			goto failure;
+		}
+	}
+
 	cl->R_tab = rtab;
 	rtab = NULL;
 	cl->refcnt = 1;
@@ -1896,10 +1915,6 @@ cbq_change_class(struct Qdisc *sch, u32 
 
 	qdisc_class_hash_grow(sch, &q->clhash);
 
-	if (tca[TCA_RATE])
-		gen_new_estimator(&cl->bstats, &cl->rate_est,
-				  qdisc_root_sleeping_lock(sch), tca[TCA_RATE]);
-
 	*arg = (unsigned long)cl;
 	return 0;
 
--- a/net/sched/sch_drr.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_drr.c	2008-11-25 10:54:16.000000000 -0800
@@ -82,15 +82,19 @@ static int drr_change_class(struct Qdisc
 		quantum = psched_mtu(qdisc_dev(sch));
 
 	if (cl != NULL) {
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+			if (err)
+				return err;
+		}
+
 		sch_tree_lock(sch);
 		if (tb[TCA_DRR_QUANTUM])
 			cl->quantum = quantum;
 		sch_tree_unlock(sch);
 
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
 		return 0;
 	}
 
@@ -106,10 +110,16 @@ static int drr_change_class(struct Qdisc
 	if (cl->qdisc == NULL)
 		cl->qdisc = &noop_qdisc;
 
-	if (tca[TCA_RATE])
-		gen_replace_estimator(&cl->bstats, &cl->rate_est,
-				      qdisc_root_sleeping_lock(sch),
-				      tca[TCA_RATE]);
+	if (tca[TCA_RATE]) {
+		err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+					    qdisc_root_sleeping_lock(sch),
+					    tca[TCA_RATE]);
+		if (err) {
+			qdisc_destroy(cl->qdisc);
+			kfree(cl);
+			return err;
+		}
+	}
 
 	sch_tree_lock(sch);
 	qdisc_class_hash_insert(&q->clhash, &cl->common);
--- a/net/sched/sch_hfsc.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_hfsc.c	2008-11-25 10:54:16.000000000 -0800
@@ -1018,6 +1018,14 @@ hfsc_change_class(struct Qdisc *sch, u32
 		}
 		cur_time = psched_get_time();
 
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+					      qdisc_root_sleeping_lock(sch),
+					      tca[TCA_RATE]);
+			if (err)
+				return err;
+		}
+
 		sch_tree_lock(sch);
 		if (rsc != NULL)
 			hfsc_change_rsc(cl, rsc, cur_time);
@@ -1034,10 +1042,6 @@ hfsc_change_class(struct Qdisc *sch, u32
 		}
 		sch_tree_unlock(sch);
 
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
 		return 0;
 	}
 
--- a/net/sched/sch_htb.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_htb.c	2008-11-25 10:54:16.000000000 -0800
@@ -1332,9 +1332,14 @@ static int htb_change_class(struct Qdisc
 		if ((cl = kzalloc(sizeof(*cl), GFP_KERNEL)) == NULL)
 			goto failure;
 
-		gen_new_estimator(&cl->bstats, &cl->rate_est,
-				  qdisc_root_sleeping_lock(sch),
-				  tca[TCA_RATE] ? : &est.nla);
+		err = gen_new_estimator(&cl->bstats, &cl->rate_est,
+					qdisc_root_sleeping_lock(sch),
+					tca[TCA_RATE] ? : &est.nla);
+		if (err) {
+			kfree(cl);
+			goto failure;
+		}
+
 		cl->refcnt = 1;
 		cl->children = 0;
 		INIT_LIST_HEAD(&cl->un.leaf.drop_list);
@@ -1386,10 +1391,13 @@ static int htb_change_class(struct Qdisc
 		if (parent)
 			parent->children++;
 	} else {
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+			if (err)
+				return err;
+		}
 		sch_tree_lock(sch);
 	}
 

Re: [PATCH 1b/2] tc: check for errors in gen_rate_estimator creation

[Patrick McHardy]

Stephen Hemminger wrote:
> The functions gen_new_estimator and gen_replace_estimator can return errors,
> but they were being ignored.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> ---
> --- a/net/sched/sch_api.c	2008-11-25 10:51:47.000000000 -0800
> +++ b/net/sched/sch_api.c	2008-11-25 10:54:16.000000000 -0800
> @@ -887,6 +887,14 @@ static int qdisc_change(struct Qdisc *sc
>  			return err;
>  	}
>  
> +	if (tca[TCA_RATE]) {
> +		err = gen_replace_estimator(&sch->bstats, &sch->rate_est,
> +					    qdisc_root_sleeping_lock(sch),
> +					    tca[TCA_RATE]);
> +		if (err)
> +			return err;

This appears to have the same problem as in HFSC unless I missed more
code movement - changes to the qdisc have already been performed.

> --- a/net/sched/sch_hfsc.c	2008-11-25 10:51:47.000000000 -0800
> +++ b/net/sched/sch_hfsc.c	2008-11-25 10:54:16.000000000 -0800
> @@ -1018,6 +1018,14 @@ hfsc_change_class(struct Qdisc *sch, u32
>  		}
>  		cur_time = psched_get_time();
>  
> +		if (tca[TCA_RATE]) {
> +			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
> +					      qdisc_root_sleeping_lock(sch),
> +					      tca[TCA_RATE]);
> +			if (err)
> +				return err;
> +		}
> +
>  		sch_tree_lock(sch);
>  		if (rsc != NULL)
>  			hfsc_change_rsc(cl, rsc, cur_time);
> @@ -1034,10 +1042,6 @@ hfsc_change_class(struct Qdisc *sch, u32
>  		}
>  		sch_tree_unlock(sch);
>  
> -		if (tca[TCA_RATE])
> -			gen_replace_estimator(&cl->bstats, &cl->rate_est,
> -					      qdisc_root_sleeping_lock(sch),
> -					      tca[TCA_RATE]);
>  		return 0;
>  	}

There's a gen_new_estimator() a few lines below that is still unchecked.

The remaining ones look fine.

Re: [PATCH 1b/2] tc: check for errors in gen_rate_estimator creation

[Stephen Hemminger]

On Tue, 25 Nov 2008 20:05:46 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Stephen Hemminger wrote:
> > The functions gen_new_estimator and gen_replace_estimator can return errors,
> > but they were being ignored.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > 
> > ---
> > --- a/net/sched/sch_api.c	2008-11-25 10:51:47.000000000 -0800
> > +++ b/net/sched/sch_api.c	2008-11-25 10:54:16.000000000 -0800
> > @@ -887,6 +887,14 @@ static int qdisc_change(struct Qdisc *sc
> >  			return err;
> >  	}
> >  
> > +	if (tca[TCA_RATE]) {
> > +		err = gen_replace_estimator(&sch->bstats, &sch->rate_est,
> > +					    qdisc_root_sleeping_lock(sch),
> > +					    tca[TCA_RATE]);
> > +		if (err)
> > +			return err;
> 
> This appears to have the same problem as in HFSC unless I missed more
> code movement - changes to the qdisc have already been performed.

That is not fixable since both change and replace_estimator are non-unwindable,
and either one could fail. Could split checking and initialization out of gen_new_estimator,
but is it worth it??

Re: [PATCH 1b/2] tc: check for errors in gen_rate_estimator creation

[Patrick McHardy]

Stephen Hemminger wrote:
> On Tue, 25 Nov 2008 20:05:46 +0100
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> Stephen Hemminger wrote:
>>> The functions gen_new_estimator and gen_replace_estimator can return errors,
>>> but they were being ignored.
>>>
>>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>>>
>>>
>>> ---
>>> --- a/net/sched/sch_api.c	2008-11-25 10:51:47.000000000 -0800
>>> +++ b/net/sched/sch_api.c	2008-11-25 10:54:16.000000000 -0800
>>> @@ -887,6 +887,14 @@ static int qdisc_change(struct Qdisc *sc
>>>  			return err;
>>>  	}
>>>  
>>> +	if (tca[TCA_RATE]) {
>>> +		err = gen_replace_estimator(&sch->bstats, &sch->rate_est,
>>> +					    qdisc_root_sleeping_lock(sch),
>>> +					    tca[TCA_RATE]);
>>> +		if (err)
>>> +			return err;
>> This appears to have the same problem as in HFSC unless I missed more
>> code movement - changes to the qdisc have already been performed.
> 
> That is not fixable since both change and replace_estimator are non-unwindable,
> and either one could fail. Could split checking and initialization out of gen_new_estimator,
> but is it worth it??

Probably not. I'd suggest to not return the estimator error in
this spot instead.

Re: [PATCH 1b/2] tc: check for errors in gen_rate_estimator creation

[Stephen Hemminger]

The functions gen_new_estimator and gen_replace_estimator can return errors,
but they were being ignored.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 net/sched/act_police.c |   25 +++++++++++++++++--------
 net/sched/sch_api.c    |    7 +++++--
 net/sched/sch_cbq.c    |   33 ++++++++++++++++++++++++---------
 net/sched/sch_drr.c    |   26 ++++++++++++++++++--------
 net/sched/sch_hfsc.c   |   25 ++++++++++++++++++-------
 net/sched/sch_htb.c    |   22 +++++++++++++++-------
 6 files changed, 97 insertions(+), 41 deletions(-)

--- a/net/sched/act_police.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/act_police.c	2008-11-25 10:54:16.000000000 -0800
@@ -185,14 +185,21 @@ override:
 		if (parm->peakrate.rate) {
 			P_tab = qdisc_get_rtab(&parm->peakrate,
 					       tb[TCA_POLICE_PEAKRATE]);
-			if (P_tab == NULL) {
-				qdisc_put_rtab(R_tab);
+			if (P_tab == NULL)
 				goto failure;
-			}
 		}
 	}
-	/* No failure allowed after this point */
+
 	spin_lock_bh(&police->tcf_lock);
+	if (est) {
+		err = gen_replace_estimator(&police->tcf_bstats,
+					    &police->tcf_rate_est,
+					    &police->tcf_lock, est);
+		if (err)
+			goto failure_unlock;
+	}
+
+	/* No failure allowed after this point */
 	if (R_tab != NULL) {
 		qdisc_put_rtab(police->tcfp_R_tab);
 		police->tcfp_R_tab = R_tab;
@@ -217,10 +224,6 @@ override:
 
 	if (tb[TCA_POLICE_AVRATE])
 		police->tcfp_ewma_rate = nla_get_u32(tb[TCA_POLICE_AVRATE]);
-	if (est)
-		gen_replace_estimator(&police->tcf_bstats,
-				      &police->tcf_rate_est,
-				      &police->tcf_lock, est);
 
 	spin_unlock_bh(&police->tcf_lock);
 	if (ret != ACT_P_CREATED)
@@ -238,7 +241,13 @@ override:
 	a->priv = police;
 	return ret;
 
+failure_unlock:
+	spin_unlock_bh(&police->tcf_lock);
 failure:
+	if (P_tab)
+		qdisc_put_rtab(P_tab);
+	if (R_tab)
+		qdisc_put_rtab(R_tab);
 	if (ret == ACT_P_CREATED)
 		kfree(police);
 	return err;
--- a/net/sched/sch_api.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_api.c	2008-11-25 11:23:19.000000000 -0800
@@ -897,9 +897,12 @@ static int qdisc_change(struct Qdisc *sc
 	sch->stab = stab;
 
 	if (tca[TCA_RATE])
+		/* NB: ignores errors from replace_estimator
+		   because change can't be undone. */
 		gen_replace_estimator(&sch->bstats, &sch->rate_est,
-				      qdisc_root_sleeping_lock(sch),
-				      tca[TCA_RATE]);
+					    qdisc_root_sleeping_lock(sch),
+					    tca[TCA_RATE]);
+
 	return 0;
 }
 
--- a/net/sched/sch_cbq.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_cbq.c	2008-11-25 10:56:15.000000000 -0800
@@ -1765,11 +1765,23 @@ cbq_change_class(struct Qdisc *sch, u32 
 		}
 
 		if (tb[TCA_CBQ_RATE]) {
-			rtab = qdisc_get_rtab(nla_data(tb[TCA_CBQ_RATE]), tb[TCA_CBQ_RTAB]);
+			rtab = qdisc_get_rtab(nla_data(tb[TCA_CBQ_RATE]),
+					      tb[TCA_CBQ_RTAB]);
 			if (rtab == NULL)
 				return -EINVAL;
 		}
 
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+			if (err) {
+				if (rtab)
+					qdisc_put_rtab(rtab);
+				return err;
+			}
+		}
+
 		/* Change class parameters */
 		sch_tree_lock(sch);
 
@@ -1805,10 +1817,6 @@ cbq_change_class(struct Qdisc *sch, u32 
 
 		sch_tree_unlock(sch);
 
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
 		return 0;
 	}
 
@@ -1855,6 +1863,17 @@ cbq_change_class(struct Qdisc *sch, u32 
 	cl = kzalloc(sizeof(*cl), GFP_KERNEL);
 	if (cl == NULL)
 		goto failure;
+
+	if (tca[TCA_RATE]) {
+		err = gen_new_estimator(&cl->bstats, &cl->rate_est,
+					qdisc_root_sleeping_lock(sch),
+					tca[TCA_RATE]);
+		if (err) {
+			kfree(cl);
+			goto failure;
+		}
+	}
+
 	cl->R_tab = rtab;
 	rtab = NULL;
 	cl->refcnt = 1;
@@ -1896,10 +1915,6 @@ cbq_change_class(struct Qdisc *sch, u32 
 
 	qdisc_class_hash_grow(sch, &q->clhash);
 
-	if (tca[TCA_RATE])
-		gen_new_estimator(&cl->bstats, &cl->rate_est,
-				  qdisc_root_sleeping_lock(sch), tca[TCA_RATE]);
-
 	*arg = (unsigned long)cl;
 	return 0;
 
--- a/net/sched/sch_drr.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_drr.c	2008-11-25 10:54:16.000000000 -0800
@@ -82,15 +82,19 @@ static int drr_change_class(struct Qdisc
 		quantum = psched_mtu(qdisc_dev(sch));
 
 	if (cl != NULL) {
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+			if (err)
+				return err;
+		}
+
 		sch_tree_lock(sch);
 		if (tb[TCA_DRR_QUANTUM])
 			cl->quantum = quantum;
 		sch_tree_unlock(sch);
 
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
 		return 0;
 	}
 
@@ -106,10 +110,16 @@ static int drr_change_class(struct Qdisc
 	if (cl->qdisc == NULL)
 		cl->qdisc = &noop_qdisc;
 
-	if (tca[TCA_RATE])
-		gen_replace_estimator(&cl->bstats, &cl->rate_est,
-				      qdisc_root_sleeping_lock(sch),
-				      tca[TCA_RATE]);
+	if (tca[TCA_RATE]) {
+		err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+					    qdisc_root_sleeping_lock(sch),
+					    tca[TCA_RATE]);
+		if (err) {
+			qdisc_destroy(cl->qdisc);
+			kfree(cl);
+			return err;
+		}
+	}
 
 	sch_tree_lock(sch);
 	qdisc_class_hash_insert(&q->clhash, &cl->common);
--- a/net/sched/sch_hfsc.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_hfsc.c	2008-11-25 11:08:27.000000000 -0800
@@ -1018,6 +1018,14 @@ hfsc_change_class(struct Qdisc *sch, u32
 		}
 		cur_time = psched_get_time();
 
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+					      qdisc_root_sleeping_lock(sch),
+					      tca[TCA_RATE]);
+			if (err)
+				return err;
+		}
+
 		sch_tree_lock(sch);
 		if (rsc != NULL)
 			hfsc_change_rsc(cl, rsc, cur_time);
@@ -1034,10 +1042,6 @@ hfsc_change_class(struct Qdisc *sch, u32
 		}
 		sch_tree_unlock(sch);
 
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
 		return 0;
 	}
 
@@ -1063,6 +1067,16 @@ hfsc_change_class(struct Qdisc *sch, u32
 	if (cl == NULL)
 		return -ENOBUFS;
 
+	if (tca[TCA_RATE]) {
+		err = gen_new_estimator(&cl->bstats, &cl->rate_est,
+					qdisc_root_sleeping_lock(sch),
+					tca[TCA_RATE]);
+		if (err) {
+			kfree(cl);
+			return err;
+		}
+	}
+
 	if (rsc != NULL)
 		hfsc_change_rsc(cl, rsc, 0);
 	if (fsc != NULL)
@@ -1093,9 +1107,6 @@ hfsc_change_class(struct Qdisc *sch, u32
 
 	qdisc_class_hash_grow(sch, &q->clhash);
 
-	if (tca[TCA_RATE])
-		gen_new_estimator(&cl->bstats, &cl->rate_est,
-				  qdisc_root_sleeping_lock(sch), tca[TCA_RATE]);
 	*arg = (unsigned long)cl;
 	return 0;
 }
--- a/net/sched/sch_htb.c	2008-11-25 10:51:47.000000000 -0800
+++ b/net/sched/sch_htb.c	2008-11-25 10:54:16.000000000 -0800
@@ -1332,9 +1332,14 @@ static int htb_change_class(struct Qdisc
 		if ((cl = kzalloc(sizeof(*cl), GFP_KERNEL)) == NULL)
 			goto failure;
 
-		gen_new_estimator(&cl->bstats, &cl->rate_est,
-				  qdisc_root_sleeping_lock(sch),
-				  tca[TCA_RATE] ? : &est.nla);
+		err = gen_new_estimator(&cl->bstats, &cl->rate_est,
+					qdisc_root_sleeping_lock(sch),
+					tca[TCA_RATE] ? : &est.nla);
+		if (err) {
+			kfree(cl);
+			goto failure;
+		}
+
 		cl->refcnt = 1;
 		cl->children = 0;
 		INIT_LIST_HEAD(&cl->un.leaf.drop_list);
@@ -1386,10 +1391,13 @@ static int htb_change_class(struct Qdisc
 		if (parent)
 			parent->children++;
 	} else {
-		if (tca[TCA_RATE])
-			gen_replace_estimator(&cl->bstats, &cl->rate_est,
-					      qdisc_root_sleeping_lock(sch),
-					      tca[TCA_RATE]);
+		if (tca[TCA_RATE]) {
+			err = gen_replace_estimator(&cl->bstats, &cl->rate_est,
+						    qdisc_root_sleeping_lock(sch),
+						    tca[TCA_RATE]);
+			if (err)
+				return err;
+		}
 		sch_tree_lock(sch);
 	}
 

Re: [PATCH 1b/2] tc: check for errors in gen_rate_estimator creation

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 25 Nov 2008 11:25:37 -0800

> The functions gen_new_estimator and gen_replace_estimator can return errors,
> but they were being ignored.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.