tc: check for errors from gen_replace_estimator

PATCH Applied Stephen Hemminger 4 participants 18 messages Nov 25, 2008 – Nov 26, 2008

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Stephen Hemminger

author

2008-11-25 18:21 UTC #0 lore raw

PATCH 1/2 tc: check for errors from gen_replace_estimator

gen_replace_estimator can return errors, but they were being ignored.

Signed-off-by Stephen Hemminger <[email protected]>

net/sched/act_police.c +17 -8

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

net/sched/sch_api.c +4 -4

@@ -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

net/sched/sch_cbq.c +4 -4

@@ -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)

net/sched/sch_drr.c +13 -8

@@ -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);

net/sched/sch_hfsc.c +2 -2

@@ -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)

Stephen Hemminger

author

2008-11-25 18:21 UTC #1 lore raw

PATCH 2/2 tc: policing requires a rate estimator

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 <[email protected]>

---
 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(-)

net/sched/act_police.c +6 -0

@@ -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]);

include/net/gen_stats.h +1 -0

@@ -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

net/core/gen_estimator.c +27 -3

@@ -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);

Stephen Hemminger

author

2008-11-25 18:33 UTC #3 lore raw

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

Quoted 17 lines · On Tue, 25 Nov 2008 19:32:13 +0100 Patrick McHardy <[email protected]> wrote:

Quoted 16 lines · 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 <[email protected]>
---
 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(-)

Quoted 12 lines

net/sched/act_police.c override: · line 188

Quoted 10 lines

net/sched/act_police.c +4 -0

@@ -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.

Patrick McHardy

2008-11-25 18:39 UTC #4 lore raw

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

Quoted 16 lines · Stephen Hemminger

Quoted 12 lines · On Tue, 25 Nov 2008 19:32:13 +0100 Patrick McHardy <[email protected]> wrote:
net/sched/act_police.c override: · line 188
Quoted 11 lines
net/sched/act_police.c +4 -0
@@ -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.

Quoted 1 line

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

I missed that, sorry.

David Miller

maintainer Applied

2008-11-26 05:14 UTC #5 lore raw

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

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Nov 2008 10:21:13 -0800

Quoted 8 lines

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 <[email protected]>

Applied.

Jarek Poplawski

2008-11-26 11:10 UTC #6 lore raw

PATCH 1/1 pkt_sched: gen_estimator: Optimize gen_estimator_active()

In reply to Stephen Hemminger

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 <[email protected]>

---

 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(-)

include/net/gen_stats.h +2 -2

index dcf5bfa..d136b52 100644
@@ -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

net/core/gen_estimator.c +8 ~~-17~~

index 3885550..9cc9f95 100644
@@ -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);

net/sched/act_police.c +3 -1

index 40df1d0..40f9a81 100644
@@ -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;
 		}

Stephen Hemminger

author

2008-11-26 16:29 UTC #7 lore raw

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

Quoted 11 lines · On Wed, 26 Nov 2008 11:10:22 +0000 Jarek Poplawski <[email protected]> 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 <[email protected]>

yeah sure, i just coded the obvious way.

David Miller

maintainer Applied

2008-11-26 23:24 UTC #8 lore raw

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

In reply to Jarek Poplawski

From: Jarek Poplawski <[email protected]>
Date: Wed, 26 Nov 2008 11:10:22 +0000

Quoted 11 lines

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 <[email protected]>

Applied, thanks Jarek.

Patrick McHardy

2008-11-25 18:28 UTC #9 lore raw

In reply to Stephen Hemminger

Quoted 5 lines · Stephen Hemminger

gen_replace_estimator can return errors, but they were being ignored.

net/sched/sch_hfsc.c hfsc_change_class(struct Qdisc *sch, u32 · line 1041

Quoted 9 lines

net/sched/sch_hfsc.c +2 -2

@@ -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.

Stephen Hemminger

author

2008-11-25 18:58 UTC #10 lore raw

PATCH 1/1 tc: check for errors in gen_rate_estimator creation

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 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(-)

net/sched/act_police.c +17 -8

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

net/sched/sch_api.c +8 -4

@@ -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;
 }

net/sched/sch_cbq.c +24 -9

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

net/sched/sch_drr.c +18 -8

@@ -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);

net/sched/sch_hfsc.c +8 -4

@@ -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;
 	}

net/sched/sch_htb.c +15 -7

@@ -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);
 	}

Patrick McHardy

2008-11-25 19:05 UTC #11 lore raw

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

Quoted 10 lines · Stephen Hemminger

The functions gen_new_estimator and gen_replace_estimator can return errors, but they were being ignored.
Signed-off-by Stephen Hemminger <[email protected]>
---

net/sched/sch_api.c static int qdisc_change(struct Qdisc *sc · line 895

Quoted 9 lines

net/sched/sch_api.c +6 -0

@@ -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.

Quoted 18 lines

net/sched/sch_hfsc.c +8 -0

@@ -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);

net/sched/sch_hfsc.c hfsc_change_class(struct Qdisc *sch, u32 · line 1046

Quoted 9 lines

net/sched/sch_hfsc.c +0 -4

@@ -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.

Stephen Hemminger

author

2008-11-25 19:14 UTC #12 lore raw

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

Quoted 14 lines · On Tue, 25 Nov 2008 20:05:46 +0100 Patrick McHardy <[email protected]> wrote:

Quoted 13 lines · Stephen Hemminger
The functions gen_new_estimator and gen_replace_estimator can return errors, but they were being ignored.
Signed-off-by Stephen Hemminger <[email protected]>
---
net/sched/sch_api.c +0 -0
@@ -887,6 +887,14 @@ static int qdisc_change(struct Qdisc *sc
 			return err;
 	}
 

Quoted 9 lines

net/sched/sch_api.c static int qdisc_change(struct Qdisc *sc · line 895
Quoted 6 lines
net/sched/sch_api.c +6 -0
+	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??

Patrick McHardy

2008-11-25 19:19 UTC #13 lore raw

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

Quoted 26 lines · Stephen Hemminger

Quoted 14 lines · On Tue, 25 Nov 2008 20:05:46 +0100 Patrick McHardy <[email protected]> wrote:
Quoted 13 lines · Stephen Hemminger
The functions gen_new_estimator and gen_replace_estimator can return errors, but they were being ignored.
Signed-off-by Stephen Hemminger <[email protected]>
---
net/sched/sch_api.c +0 -0
@@ -887,6 +887,14 @@ static int qdisc_change(struct Qdisc *sc
 			return err;
 	}
 
Quoted 8 lines
net/sched/sch_api.c static int qdisc_change(struct Qdisc *sc · line 895
Quoted 6 lines
net/sched/sch_api.c +6 -0
+	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.

Quoted 3 lines

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.

Stephen Hemminger

author

2008-11-25 19:25 UTC #14 lore raw

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

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 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(-)

net/sched/act_police.c +17 -8

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

net/sched/sch_api.c +5 -2

@@ -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;
 }

net/sched/sch_cbq.c +24 -9

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

net/sched/sch_drr.c +18 -8

@@ -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);

net/sched/sch_hfsc.c +18 -7

@@ -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;
 }

net/sched/sch_htb.c +15 -7

@@ -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);
 	}

David Miller

maintainer Applied

2008-11-26 05:13 UTC #15 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Nov 2008 11:25:37 -0800

Quoted 4 lines

The functions gen_new_estimator and gen_replace_estimator can return errors, but they were being ignored.
Signed-off-by Stephen Hemminger <[email protected]>

Applied.

Stephen Hemminger

author

2008-11-25 18:58 UTC #16 lore raw

PATCH 1/1 tc: propogate errors from tcf_hash_create

In reply to Patrick McHardy

Allow tcf_hash_create to return different errors on estimator failure.

Signed-off-by Stephen Hemminger <[email protected]>

---
 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(-)

net/sched/act_api.c +13 -5

@@ -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;
 }

net/sched/act_gact.c +2 -2

@@ -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) {

net/sched/act_ipt.c +2 -2

@@ -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) {

net/sched/act_mirred.c +2 -2

@@ -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) {

net/sched/act_nat.c +2 -2

@@ -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 {

net/sched/act_pedit.c +2 -2

@@ -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) {

net/sched/act_simple.c +2 -2

@@ -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);

net/sched/act_skbedit.c +2 -2

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

David Miller

maintainer Applied

2008-11-26 05:12 UTC #17 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Nov 2008 10:57:27 -0800

Quoted 3 lines

Allow tcf_hash_create to return different errors on estimator failure.
Signed-off-by Stephen Hemminger <[email protected]>

Applied.