| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <52D293A2.2070703@mojatatu.com>
Date: Sun, 12 Jan 2014 08:07:46 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
CC: Thomas Graf <tgraf@...g.ch>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [Patch net-next 6/7] net_sched: cls: move allocation in ->init
to generic layer
On 01/09/14 19:14, Cong Wang wrote:
> Most of the filters need allocation of tp->root in ->init()
> and free it in ->destroy(), make this generic.
>
> Also we could reduce the use of tcf_tree_lock a bit.
>
> Cc: Thomas Graf <tgraf@...g.ch>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
Hrm. This one worries me a little.
I dont see how just pre-allocing the private head of the classifier
magically allows you to get rid of locks. Have you tested against those
classifiers you changed?
If those locks are useless - then that is a separate patch to kill
them (sorry, dont have time to test myself right now).
cheers,
jamal
> ---
> include/net/sch_generic.h | 1 +
> net/sched/cls_api.c | 7 +++++++
> net/sched/cls_basic.c | 8 ++------
> net/sched/cls_bpf.c | 11 ++---------
> net/sched/cls_cgroup.c | 21 +++++++--------------
> net/sched/cls_flow.c | 8 ++------
> net/sched/cls_fw.c | 14 ++++----------
> net/sched/cls_route.c | 15 ++-------------
> net/sched/cls_rsvp.h | 10 ++--------
> net/sched/cls_tcindex.c | 13 ++-----------
> net/sched/cls_u32.c | 9 ++-------
> net/sched/sch_api.c | 1 +
> 12 files changed, 34 insertions(+), 84 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index d062f81..819dc1d 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -208,6 +208,7 @@ struct tcf_proto_ops {
> struct sk_buff *skb, struct tcmsg*);
>
> struct module *owner;
> + size_t root_size;
> };
>
> struct tcf_proto {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 29a30a1..8460c75 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -262,6 +262,13 @@ replay:
> tp->q = q;
> tp->classify = tp_ops->classify;
> tp->classid = parent;
> + tp->root = kzalloc(tp_ops->root_size, GFP_KERNEL);
> + if (!tp->root) {
> + err = -ENOBUFS;
> + module_put(tp_ops->owner);
> + kfree(tp);
> + goto errout;
> + }
>
> err = tp_ops->init(tp);
> if (err != 0) {
> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index e98ca99..318f672 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -75,13 +75,9 @@ static void basic_put(struct tcf_proto *tp, unsigned long f)
>
> static int basic_init(struct tcf_proto *tp)
> {
> - struct basic_head *head;
> + struct basic_head *head = tp->root;
>
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> INIT_LIST_HEAD(&head->flist);
> - tp->root = head;
> return 0;
> }
>
> @@ -102,7 +98,6 @@ static void basic_destroy(struct tcf_proto *tp)
> list_del(&f->link);
> basic_delete_filter(tp, f);
> }
> - kfree(head);
> }
>
> static int basic_delete(struct tcf_proto *tp, unsigned long arg)
> @@ -288,6 +283,7 @@ static struct tcf_proto_ops cls_basic_ops __read_mostly = {
> .walk = basic_walk,
> .dump = basic_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct basic_head),
> };
>
> static int __init init_basic(void)
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index 8e3cf49..eedd296 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -75,15 +75,9 @@ static int cls_bpf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>
> static int cls_bpf_init(struct tcf_proto *tp)
> {
> - struct cls_bpf_head *head;
> -
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> + struct cls_bpf_head *head = tp->root;
>
> INIT_LIST_HEAD(&head->plist);
> - tp->root = head;
> -
> return 0;
> }
>
> @@ -126,8 +120,6 @@ static void cls_bpf_destroy(struct tcf_proto *tp)
> list_del(&prog->link);
> cls_bpf_delete_prog(tp, prog);
> }
> -
> - kfree(head);
> }
>
> static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
> @@ -366,6 +358,7 @@ static struct tcf_proto_ops cls_bpf_ops __read_mostly = {
> .delete = cls_bpf_delete,
> .walk = cls_bpf_walk,
> .dump = cls_bpf_dump,
> + .root_size = sizeof(struct cls_bpf_head),
> };
>
> static int __init cls_bpf_init_mod(void)
> diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
> index 8e2158a..4b7e083 100644
> --- a/net/sched/cls_cgroup.c
> +++ b/net/sched/cls_cgroup.c
> @@ -22,6 +22,7 @@ struct cls_cgroup_head {
> u32 handle;
> struct tcf_exts exts;
> struct tcf_ematch_tree ematches;
> + bool init;
> };
>
> static int cls_cgroup_classify(struct sk_buff *skb, const struct tcf_proto *tp,
> @@ -73,6 +74,9 @@ static void cls_cgroup_put(struct tcf_proto *tp, unsigned long f)
>
> static int cls_cgroup_init(struct tcf_proto *tp)
> {
> + struct cls_cgroup_head *head = tp->root;
> +
> + tcf_exts_init(&head->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
> return 0;
> }
>
> @@ -94,20 +98,9 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
> if (!tca[TCA_OPTIONS])
> return -EINVAL;
>
> - if (head == NULL) {
> - if (!handle)
> - return -EINVAL;
> -
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> -
> - tcf_exts_init(&head->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
> + if (!head->init) {
> head->handle = handle;
> -
> - tcf_tree_lock(tp);
> - tp->root = head;
> - tcf_tree_unlock(tp);
> + head->init = true;
> }
>
> if (handle != head->handle)
> @@ -140,7 +133,6 @@ static void cls_cgroup_destroy(struct tcf_proto *tp)
> if (head) {
> tcf_exts_destroy(tp, &head->exts);
> tcf_em_tree_destroy(tp, &head->ematches);
> - kfree(head);
> }
> }
>
> @@ -205,6 +197,7 @@ static struct tcf_proto_ops cls_cgroup_ops __read_mostly = {
> .walk = cls_cgroup_walk,
> .dump = cls_cgroup_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct cls_cgroup_head),
> };
>
> static int __init init_cgroup_cls(void)
> diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
> index 257029c..b39080a 100644
> --- a/net/sched/cls_flow.c
> +++ b/net/sched/cls_flow.c
> @@ -526,13 +526,9 @@ static int flow_delete(struct tcf_proto *tp, unsigned long arg)
>
> static int flow_init(struct tcf_proto *tp)
> {
> - struct flow_head *head;
> + struct flow_head *head = tp->root;
>
> - head = kzalloc(sizeof(*head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> INIT_LIST_HEAD(&head->filters);
> - tp->root = head;
> return 0;
> }
>
> @@ -545,7 +541,6 @@ static void flow_destroy(struct tcf_proto *tp)
> list_del(&f->list);
> flow_destroy_filter(tp, f);
> }
> - kfree(head);
> }
>
> static unsigned long flow_get(struct tcf_proto *tp, u32 handle)
> @@ -653,6 +648,7 @@ static struct tcf_proto_ops cls_flow_ops __read_mostly = {
> .dump = flow_dump,
> .walk = flow_walk,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct flow_head),
> };
>
> static int __init cls_flow_init(void)
> diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
> index ed00e8c..73cd277 100644
> --- a/net/sched/cls_fw.c
> +++ b/net/sched/cls_fw.c
> @@ -34,6 +34,7 @@
> struct fw_head {
> struct fw_filter *ht[HTSIZE];
> u32 mask;
> + bool init;
> };
>
> struct fw_filter {
> @@ -155,7 +156,6 @@ static void fw_destroy(struct tcf_proto *tp)
> fw_delete_filter(tp, f);
> }
> }
> - kfree(head);
> }
>
> static int fw_delete(struct tcf_proto *tp, unsigned long arg)
> @@ -259,19 +259,12 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
> if (!handle)
> return -EINVAL;
>
> - if (head == NULL) {
> + if (!head->init) {
> u32 mask = 0xFFFFFFFF;
> if (tb[TCA_FW_MASK])
> mask = nla_get_u32(tb[TCA_FW_MASK]);
> -
> - head = kzalloc(sizeof(struct fw_head), GFP_KERNEL);
> - if (head == NULL)
> - return -ENOBUFS;
> head->mask = mask;
> -
> - tcf_tree_lock(tp);
> - tp->root = head;
> - tcf_tree_unlock(tp);
> + head->init = true;
> }
>
> f = kzalloc(sizeof(struct fw_filter), GFP_KERNEL);
> @@ -388,6 +381,7 @@ static struct tcf_proto_ops cls_fw_ops __read_mostly = {
> .walk = fw_walk,
> .dump = fw_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct fw_head),
> };
>
> static int __init init_fw(void)
> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
> index 1ad3068..038f35f 100644
> --- a/net/sched/cls_route.c
> +++ b/net/sched/cls_route.c
> @@ -279,7 +279,6 @@ static void route4_destroy(struct tcf_proto *tp)
> kfree(b);
> }
> }
> - kfree(head);
> }
>
> static int route4_delete(struct tcf_proto *tp, unsigned long arg)
> @@ -462,20 +461,9 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
> goto reinsert;
> }
>
> - err = -ENOBUFS;
> - if (head == NULL) {
> - head = kzalloc(sizeof(struct route4_head), GFP_KERNEL);
> - if (head == NULL)
> - goto errout;
> -
> - tcf_tree_lock(tp);
> - tp->root = head;
> - tcf_tree_unlock(tp);
> - }
> -
> f = kzalloc(sizeof(struct route4_filter), GFP_KERNEL);
> if (f == NULL)
> - goto errout;
> + return -ENOBUFS;
>
> tcf_exts_init(&f->exts, TCA_ROUTE4_ACT, TCA_ROUTE4_POLICE);
> err = route4_set_parms(net, tp, base, f, handle, head, tb,
> @@ -613,6 +601,7 @@ static struct tcf_proto_ops cls_route4_ops __read_mostly = {
> .walk = route4_walk,
> .dump = route4_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct route4_head),
> };
>
> static int __init init_route4(void)
> diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
> index 19f8e5d..47930bc 100644
> --- a/net/sched/cls_rsvp.h
> +++ b/net/sched/cls_rsvp.h
> @@ -242,14 +242,7 @@ static void rsvp_put(struct tcf_proto *tp, unsigned long f)
>
> static int rsvp_init(struct tcf_proto *tp)
> {
> - struct rsvp_head *data;
> -
> - data = kzalloc(sizeof(struct rsvp_head), GFP_KERNEL);
> - if (data) {
> - tp->root = data;
> - return 0;
> - }
> - return -ENOBUFS;
> + return 0;
> }
>
> static void
> @@ -656,6 +649,7 @@ static struct tcf_proto_ops RSVP_OPS __read_mostly = {
> .walk = rsvp_walk,
> .dump = rsvp_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct rsvp_head),
> };
>
> static int __init init_rsvp(void)
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index eed8404..6454158 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -118,18 +118,11 @@ static void tcindex_put(struct tcf_proto *tp, unsigned long f)
>
> static int tcindex_init(struct tcf_proto *tp)
> {
> - struct tcindex_data *p;
> -
> - pr_debug("tcindex_init(tp %p)\n", tp);
> - p = kzalloc(sizeof(struct tcindex_data), GFP_KERNEL);
> - if (!p)
> - return -ENOMEM;
> + struct tcindex_data *p = tp->root;
>
> p->mask = 0xffff;
> p->hash = DEFAULT_HASH_SIZE;
> p->fall_through = 1;
> -
> - tp->root = p;
> return 0;
> }
>
> @@ -407,15 +400,12 @@ static void tcindex_destroy(struct tcf_proto *tp)
> struct tcindex_data *p = tp->root;
> struct tcf_walker walker;
>
> - pr_debug("tcindex_destroy(tp %p),p %p\n", tp, p);
> walker.count = 0;
> walker.skip = 0;
> walker.fn = &tcindex_destroy_element;
> tcindex_walk(tp, &walker);
> kfree(p->perfect);
> kfree(p->h);
> - kfree(p);
> - tp->root = NULL;
> }
>
>
> @@ -491,6 +481,7 @@ static struct tcf_proto_ops cls_tcindex_ops __read_mostly = {
> .walk = tcindex_walk,
> .dump = tcindex_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct tcindex_data),
> };
>
> static int __init init_tcindex(void)
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 84c28da..678c2d72 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -300,15 +300,11 @@ static u32 gen_new_htid(struct tc_u_common *tp_c)
>
> static int u32_init(struct tcf_proto *tp)
> {
> - struct tc_u_hnode *root_ht;
> + struct tc_u_hnode *root_ht = tp->root;
> struct tc_u_common *tp_c;
>
> tp_c = tp->q->u32_node;
>
> - root_ht = kzalloc(sizeof(*root_ht), GFP_KERNEL);
> - if (root_ht == NULL)
> - return -ENOBUFS;
> -
> root_ht->divisor = 0;
> root_ht->refcnt++;
> root_ht->handle = tp_c ? gen_new_htid(tp_c) : 0x80000000;
> @@ -329,7 +325,6 @@ static int u32_init(struct tcf_proto *tp)
> tp_c->hlist = root_ht;
> root_ht->tp_c = tp_c;
>
> - tp->root = root_ht;
> tp->data = tp_c;
> return 0;
> }
> @@ -394,7 +389,6 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht)
> for (hn = &tp_c->hlist; *hn; hn = &(*hn)->next) {
> if (*hn == ht) {
> *hn = ht->next;
> - kfree(ht);
> return 0;
> }
> }
> @@ -801,6 +795,7 @@ static struct tcf_proto_ops cls_u32_ops __read_mostly = {
> .walk = u32_walk,
> .dump = u32_dump,
> .owner = THIS_MODULE,
> + .root_size = sizeof(struct tc_u_hnode),
> };
>
> static int __init init_u32(void)
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 1313145..5fef7f4 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1829,6 +1829,7 @@ EXPORT_SYMBOL(tc_classify);
> void tcf_destroy(struct tcf_proto *tp)
> {
> tp->ops->destroy(tp);
> + kfree(tp->root);
> module_put(tp->ops->owner);
> kfree(tp);
> }
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists