[PKT_SCHED]: police action: fix multiple bugs in init path

- Return proper error codes
- Some attribute sizes are not checked
- rta may by NULL
- multiple leaks
- possible unbalanced unlock
- used action is freed after if an error happens while trying to replace
- estimator can't be replaced

This patch makes replacement atomic, so the old action is either
replaced entirely or not touched at all.

Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: David S. Miller <davem@davemloft.net>

1 files changed, 49 insertions, 37 deletions

diff --git a/net/sched/police.c b/net/sched/police.c
index a6c8d42adfcdea..e52e48333cc378 100644
--- a/net/sched/police.c
+++ b/net/sched/police.c
@@ -165,20 +165,28 @@ static int tcf_act_police_locate(struct rtattr *rta, struct rtattr *est,
                                  struct tc_action *a, int ovr, int bind)
 {
 	unsigned h;
-	int ret = 0;
+	int ret = 0, err;
 	struct rtattr *tb[TCA_POLICE_MAX];
 	struct tc_police *parm;
 	struct tcf_police *p;
+	struct qdisc_rate_table *R_tab = NULL, *P_tab = NULL;
 
-	if (rtattr_parse(tb, TCA_POLICE_MAX, RTA_DATA(rta),
-	                 RTA_PAYLOAD(rta)) < 0)
-		return -1;
+	if (rta == NULL || rtattr_parse(tb, TCA_POLICE_MAX, RTA_DATA(rta),
+	                                RTA_PAYLOAD(rta)) < 0)
+		return -EINVAL;
 
 	if (tb[TCA_POLICE_TBF-1] == NULL ||
 	    RTA_PAYLOAD(tb[TCA_POLICE_TBF-1]) != sizeof(*parm))
-		return -1;
-
+		return -EINVAL;
 	parm = RTA_DATA(tb[TCA_POLICE_TBF-1]);
+
+	if (tb[TCA_POLICE_RESULT-1] != NULL &&
+	    RTA_PAYLOAD(tb[TCA_POLICE_RESULT-1]) != sizeof(u32))
+		return -EINVAL;
+	if (tb[TCA_POLICE_RESULT-1] != NULL &&
+	    RTA_PAYLOAD(tb[TCA_POLICE_RESULT-1]) != sizeof(u32))
+		return -EINVAL;
+
 	if (parm->index && (p = tcf_police_lookup(parm->index)) != NULL) {
 		a->priv = p;
 		spin_lock(&p->lock);
@@ -186,18 +194,18 @@ static int tcf_act_police_locate(struct rtattr *rta, struct rtattr *est,
 			p->bindcnt += 1;
 			p->refcnt += 1;
 		}
+		spin_unlock(&p->lock);
 		if (ovr)
 			goto override;
-		spin_unlock(&p->lock);
 		return ret;
 	}
 
 	p = kmalloc(sizeof(*p), GFP_KERNEL);
 	if (p == NULL)
-		return -1;
+		return -ENOMEM;
 	memset(p, 0, sizeof(*p));
 
-	ret = 1;
+	ret = ACT_P_CREATED;
 	p->refcnt = 1;
 	spin_lock_init(&p->lock);
 	p->stats_lock = &p->lock;
@@ -205,28 +213,32 @@ static int tcf_act_police_locate(struct rtattr *rta, struct rtattr *est,
 		p->bindcnt = 1;
 override:
 	if (parm->rate.rate) {
-		p->R_tab = qdisc_get_rtab(&parm->rate, tb[TCA_POLICE_RATE-1]);
-		if (p->R_tab == NULL)
+		err = -ENOMEM;
+		R_tab = qdisc_get_rtab(&parm->rate, tb[TCA_POLICE_RATE-1]);
+		if (R_tab == NULL)
 			goto failure;
 		if (parm->peakrate.rate) {
-			p->P_tab = qdisc_get_rtab(&parm->peakrate,
-					          tb[TCA_POLICE_PEAKRATE-1]);
-			if (p->P_tab == NULL)
+			P_tab = qdisc_get_rtab(&parm->peakrate,
+					       tb[TCA_POLICE_PEAKRATE-1]);
+			if (p->P_tab == NULL) {
+				qdisc_put_rtab(R_tab);
 				goto failure;
+			}
 		}
 	}
-	if (tb[TCA_POLICE_RESULT-1]) {
-		if (RTA_PAYLOAD(tb[TCA_POLICE_RESULT-1]) != sizeof(u32))
-			goto failure;
-		p->result = *(u32*)RTA_DATA(tb[TCA_POLICE_RESULT-1]);
+	/* No failure allowed after this point */
+	spin_lock(&p->lock);
+	if (R_tab != NULL) {
+		qdisc_put_rtab(p->R_tab);
+		p->R_tab = R_tab;
 	}
-#ifdef CONFIG_NET_ESTIMATOR
-	if (tb[TCA_POLICE_AVRATE-1]) {
-		if (RTA_PAYLOAD(tb[TCA_POLICE_AVRATE-1]) != sizeof(u32))
-			goto failure;
-		p->ewma_rate = *(u32*)RTA_DATA(tb[TCA_POLICE_AVRATE-1]);
+	if (P_tab != NULL) {
+		qdisc_put_rtab(p->P_tab);
+		p->P_tab = P_tab;
 	}
-#endif
+
+	if (tb[TCA_POLICE_RESULT-1])
+		p->result = *(u32*)RTA_DATA(tb[TCA_POLICE_RESULT-1]);
 	p->toks = p->burst = parm->burst;
 	p->mtu = parm->mtu;
 	if (p->mtu == 0) {
@@ -238,16 +250,19 @@ override:
 		p->ptoks = L2T_P(p, p->mtu);
 	p->action = parm->action;
 
-	if (ovr) {
-		spin_unlock(&p->lock);
-		return ret;
-	}
-	PSCHED_GET_TIME(p->t_c);
-	p->index = parm->index ? : tcf_police_new_index();
 #ifdef CONFIG_NET_ESTIMATOR
+	if (tb[TCA_POLICE_AVRATE-1])
+		p->ewma_rate = *(u32*)RTA_DATA(tb[TCA_POLICE_AVRATE-1]);
 	if (est)
-		gen_new_estimator(&p->bstats, &p->rate_est, p->stats_lock, est);
+		gen_replace_estimator(&p->bstats, &p->rate_est, p->stats_lock, est);
 #endif
+
+	spin_unlock(&p->lock);
+	if (ret != ACT_P_CREATED)
+		return ret;
+
+	PSCHED_GET_TIME(p->t_c);
+	p->index = parm->index ? : tcf_police_new_index();
 	h = tcf_police_hash(p->index);
 	write_lock_bh(&police_lock);
 	p->next = tcf_police_ht[h];
@@ -258,12 +273,9 @@ override:
 	return ret;
 
 failure:
-	if (p->R_tab)
-		qdisc_put_rtab(p->R_tab);
-	if (ovr)
-		spin_unlock(&p->lock);
-	kfree(p);
-	return -1;
+	if (ret == ACT_P_CREATED)
+		kfree(p);
+	return err;
 }
 
 static int tcf_act_police_cleanup(struct tc_action *a, int bind)