* Re: [PATCH 2.6 PKT_SCHED]: Fix overflow on 64bit in times reported to userspace by tc actions
2004-11-22 13:24 ` jamal
@ 2004-11-22 19:40 ` Patrick McHardy
2004-11-23 13:39 ` jamal
0 siblings, 1 reply; 7+ messages in thread
From: Patrick McHardy @ 2004-11-22 19:40 UTC (permalink / raw)
To: hadi; +Cc: David S. Miller, netdev
[-- Attachment #1: Type: text/plain, Size: 871 bytes --]
jamal wrote:
>I think thats the one I am acking, no?
>
Sorry, seems my eyes were still closed when replying to your mail :)
>Can you resend it just to be
>sure. Note that i had no issues with any of the other patches (other
>than one i explicitly flagged to test).
>
Attached, description from the original mail:
This patch moves memory allocation for tc_actions to
tcf_action_init_1 and fixes multiple bugs in error paths:
- when memory allocation fails in tcf_action_init, the
action is destroyed twice, once in tcf_action_init and
once in tcf_action_add/tcf_change_act
- when tcf_action_init_1 fails for the first action, the action
is passed to tcf_action_destroy in an undefined state by
tcf_action_add/tcf_change_act/tcf_change_act_police
- when tcf_action_init_1 fails for any but the first action,
the action leaks in tcf_action_init
Regards
Patrick
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 7482 bytes --]
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/11/09 07:31:03+01:00 kaber@coreworks.de
# [PKT_SCHED]: Clean up tcf_action_init memory handling
#
# Signed-off-by: Patrick McHardy <kaber@trash.net>
#
# net/sched/act_api.c
# 2004/11/09 07:30:52+01:00 kaber@coreworks.de +47 -57
# [PKT_SCHED]: Clean up tcf_action_init memory handling
#
# Signed-off-by: Patrick McHardy <kaber@trash.net>
#
# include/net/pkt_cls.h
# 2004/11/09 07:30:52+01:00 kaber@coreworks.de +6 -20
# [PKT_SCHED]: Clean up tcf_action_init memory handling
#
# Signed-off-by: Patrick McHardy <kaber@trash.net>
#
# include/net/act_api.h
# 2004/11/09 07:30:52+01:00 kaber@coreworks.de +2 -2
# [PKT_SCHED]: Clean up tcf_action_init memory handling
#
# Signed-off-by: Patrick McHardy <kaber@trash.net>
#
diff -Nru a/include/net/act_api.h b/include/net/act_api.h
--- a/include/net/act_api.h 2004-11-09 09:31:36 +01:00
+++ b/include/net/act_api.h 2004-11-09 09:31:36 +01:00
@@ -87,8 +87,8 @@
extern int tcf_unregister_action(struct tc_action_ops *a);
extern void tcf_action_destroy(struct tc_action *a, int bind);
extern int tcf_action_exec(struct sk_buff *skb, struct tc_action *a, struct tcf_result *res);
-extern int tcf_action_init(struct rtattr *rta, struct rtattr *est, struct tc_action *a,char *n, int ovr, int bind);
-extern int tcf_action_init_1(struct rtattr *rta, struct rtattr *est, struct tc_action *a,char *n, int ovr, int bind);
+extern struct tc_action *tcf_action_init(struct rtattr *rta, struct rtattr *est, char *n, int ovr, int bind, int *err);
+extern struct tc_action *tcf_action_init_1(struct rtattr *rta, struct rtattr *est, char *n, int ovr, int bind, int *err);
extern int tcf_action_dump(struct sk_buff *skb, struct tc_action *a, int, int);
extern int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
extern int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
diff -Nru a/include/net/pkt_cls.h b/include/net/pkt_cls.h
--- a/include/net/pkt_cls.h 2004-11-09 09:31:36 +01:00
+++ b/include/net/pkt_cls.h 2004-11-09 09:31:36 +01:00
@@ -70,17 +70,10 @@
int ret;
struct tc_action *act;
- act = kmalloc(sizeof(*act), GFP_KERNEL);
- if (NULL == act)
- return -ENOMEM;
- memset(act, 0, sizeof(*act));
-
- ret = tcf_action_init_1(act_police_tlv, rate_tlv, act, "police",
- TCA_ACT_NOREPLACE, TCA_ACT_BIND);
- if (ret < 0) {
- tcf_action_destroy(act, TCA_ACT_UNBIND);
+ act = tcf_action_init_1(act_police_tlv, rate_tlv, "police",
+ TCA_ACT_NOREPLACE, TCA_ACT_BIND, &ret);
+ if (act == NULL)
return ret;
- }
act->type = TCA_OLD_COMPAT;
@@ -103,17 +96,10 @@
int ret;
struct tc_action *act;
- act = kmalloc(sizeof(*act), GFP_KERNEL);
- if (NULL == act)
- return -ENOMEM;
- memset(act, 0, sizeof(*act));
-
- ret = tcf_action_init(act_tlv, rate_tlv, act, NULL,
- TCA_ACT_NOREPLACE, TCA_ACT_BIND);
- if (ret < 0) {
- tcf_action_destroy(act, TCA_ACT_UNBIND);
+ act = tcf_action_init(act_tlv, rate_tlv, NULL,
+ TCA_ACT_NOREPLACE, TCA_ACT_BIND, &ret);
+ if (act == NULL)
return ret;
- }
if (*action) {
tcf_tree_lock(tp);
diff -Nru a/net/sched/act_api.c b/net/sched/act_api.c
--- a/net/sched/act_api.c 2004-11-09 09:31:36 +01:00
+++ b/net/sched/act_api.c 2004-11-09 09:31:36 +01:00
@@ -294,14 +294,16 @@
}
-int tcf_action_init_1(struct rtattr *rta, struct rtattr *est, struct tc_action *a, char *name, int ovr, int bind )
+struct tc_action *tcf_action_init_1(struct rtattr *rta, struct rtattr *est,
+ char *name, int ovr, int bind, int *err)
{
+ struct tc_action *a;
struct tc_action_ops *a_o;
char act_name[4 + IFNAMSIZ + 1];
struct rtattr *tb[TCA_ACT_MAX+1];
struct rtattr *kind = NULL;
- int err = -EINVAL;
+ *err = -EINVAL;
if (NULL == name) {
if (rtattr_parse(tb, TCA_ACT_MAX, RTA_DATA(rta), RTA_PAYLOAD(rta))<0)
@@ -337,22 +339,25 @@
goto err_out;
}
- if (NULL == a) {
+ a = kmalloc(sizeof(*a), GFP_KERNEL);
+ if (a == NULL) {
+ *err = -ENOMEM;
goto err_mod;
}
+ memset(a, 0, sizeof(*a));
/* backward compatibility for policer */
if (NULL == name) {
- err = a_o->init(tb[TCA_ACT_OPTIONS-1], est, a, ovr, bind);
- if (0 > err ) {
- err = -EINVAL;
- goto err_mod;
+ *err = a_o->init(tb[TCA_ACT_OPTIONS-1], est, a, ovr, bind);
+ if (*err < 0) {
+ *err = -EINVAL;
+ goto err_free;
}
} else {
- err = a_o->init(rta, est, a, ovr, bind);
- if (0 > err ) {
- err = -EINVAL;
- goto err_mod;
+ *err = a_o->init(rta, est, a, ovr, bind);
+ if (*err < 0) {
+ *err = -EINVAL;
+ goto err_free;
}
}
@@ -360,60 +365,58 @@
if it exists and is only bound to in a_o->init() then
ACT_P_CREATED is not returned (a zero is).
*/
- if (ACT_P_CREATED != err) {
+ if (*err != ACT_P_CREATED)
module_put(a_o->owner);
- }
a->ops = a_o;
DPRINTK("tcf_action_init_1: successfull %s \n",act_name);
- return 0;
+ *err = 0;
+ return a;
+
+err_free:
+ kfree(a);
err_mod:
module_put(a_o->owner);
err_out:
- return err;
+ return NULL;
}
-int tcf_action_init(struct rtattr *rta, struct rtattr *est, struct tc_action *a, char *name, int ovr , int bind)
+struct tc_action *tcf_action_init(struct rtattr *rta, struct rtattr *est,
+ char *name, int ovr, int bind, int *err)
{
struct rtattr *tb[TCA_ACT_MAX_PRIO+1];
+ struct tc_action *a = NULL, *act, *act_prev = NULL;
int i;
- struct tc_action *act = a, *a_s = a;
-
- int err = -EINVAL;
- if (rtattr_parse(tb, TCA_ACT_MAX_PRIO, RTA_DATA(rta), RTA_PAYLOAD(rta))<0)
- return err;
+ if (rtattr_parse(tb, TCA_ACT_MAX_PRIO, RTA_DATA(rta),
+ RTA_PAYLOAD(rta)) < 0) {
+ *err = -EINVAL;
+ return a;
+ }
- for (i=0; i < TCA_ACT_MAX_PRIO ; i++) {
+ for (i=0; i < TCA_ACT_MAX_PRIO; i++) {
if (tb[i]) {
- if (NULL == act) {
- act = kmalloc(sizeof(*act),GFP_KERNEL);
- if (NULL == act) {
- err = -ENOMEM;
- goto bad_ret;
- }
- memset(act, 0,sizeof(*act));
- }
- act->next = NULL;
- if (0 > tcf_action_init_1(tb[i],est,act,name,ovr,bind)) {
- printk("Error processing action order %d\n",i);
- return err;
+ act = tcf_action_init_1(tb[i], est, name, ovr, bind, err);
+ if (act == NULL) {
+ printk("Error processing action order %d\n", i);
+ goto bad_ret;
}
act->order = i+1;
- if (a_s != act) {
- a_s->next = act;
- a_s = act;
- }
- act = NULL;
+ if (a == NULL)
+ a = act;
+ else
+ act_prev->next = act;
+ act_prev = act;
}
}
+ return a;
- return 0;
bad_ret:
- tcf_action_destroy(a, bind);
- return err;
+ if (a != NULL)
+ tcf_action_destroy(a, bind);
+ return NULL;
}
int tcf_action_copy_stats (struct sk_buff *skb,struct tc_action *a)
@@ -849,21 +852,9 @@
struct tc_action *a = NULL;
u32 seq = n->nlmsg_seq;
- act = kmalloc(sizeof(*act),GFP_KERNEL);
- if (NULL == act)
- return -ENOMEM;
-
- memset(act, 0, sizeof(*act));
-
- ret = tcf_action_init(rta, NULL,act,NULL,ovr,0);
- /* NOTE: We have an all-or-none model
- * This means that of any of the actions fail
- * to update then all are undone.
- * */
- if (0 > ret) {
- tcf_action_destroy(act, 0);
+ act = tcf_action_init(rta, NULL, NULL, ovr, 0, &ret);
+ if (act == NULL)
goto done;
- }
/* dump then free all the actions after update; inserted policy
* stays intact
@@ -880,7 +871,6 @@
}
}
done:
-
return ret;
}
^ permalink raw reply [flat|nested] 7+ messages in thread