netdev - Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 May 2017 08:53:42 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

On Tue, May 23, 2017 at 11:37 PM, Jiri Pirko <jiri@...nulli.us> wrote:
> Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangcong@...il.com wrote:
>>tcf_chain_get() always creates a new filter chain if not found
>>in existing ones. This is totally unnecessary when we get or
>>delete filters, new chain should be only created for new filters
>>(or new actions).
>>
>>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>Cc: Jamal Hadi Salim <jhs@...atatu.com>
>>Cc: Jiri Pirko <jiri@...lanox.com>
>>Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
>>---
>> include/net/pkt_cls.h |  3 ++-
>> net/sched/act_api.c   |  2 +-
>> net/sched/cls_api.c   | 13 +++++++++----
>> 3 files changed, 12 insertions(+), 6 deletions(-)
>>
>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>index 2c213a6..f776229 100644
>>--- a/include/net/pkt_cls.h
>>+++ b/include/net/pkt_cls.h
>>@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
>> int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
>>
>> #ifdef CONFIG_NET_CLS
>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>+                              bool create);
>> void tcf_chain_put(struct tcf_chain *chain);
>> int tcf_block_get(struct tcf_block **p_block,
>>                 struct tcf_proto __rcu **p_filter_chain);
>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>index 0ecf2a8..aed6cf2 100644
>>--- a/net/sched/act_api.c
>>+++ b/net/sched/act_api.c
>>@@ -34,7 +34,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
>>
>>       if (!tp)
>>               return -EINVAL;
>>-      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index);
>>+      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
>>       if (!a->goto_chain)
>>               return -ENOMEM;
>>       return 0;
>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>index 01a8b8b..23d2236 100644
>>--- a/net/sched/cls_api.c
>>+++ b/net/sched/cls_api.c
>>@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
>>       kfree(chain);
>> }
>>
>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>+                              bool create)
>> {
>>       struct tcf_chain *chain;
>>
>>@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>                       return chain;
>>               }
>>       }
>>-      return tcf_chain_create(block, chain_index);
>>+      if (create)
>>+              return tcf_chain_create(block, chain_index);
>>+      else
>>+              return NULL;
>> }
>> EXPORT_SYMBOL(tcf_chain_get);
>>
>>@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>               err = -EINVAL;
>>               goto errout;
>>       }
>>-      chain = tcf_chain_get(block, chain_index);
>>+      chain = tcf_chain_get(block, chain_index,
>>+                            n->nlmsg_type == RTM_NEWTFILTER);
>
> First of all, I really hate all these true/false arg dances. Totaly
> confusing all the time.

Sounds like you are able to understand the code at all.
Sigh, I bet you never even read the changelog. ;)


>
>
>
>>       if (!chain) {
>>-              err = -ENOMEM;
>>+              err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
>
> Confusing. Please do not obfuscate the code for a corner cases. Thanks.

Either you don't understand the changelog or you don't understand
ternary conditional operator. I can't help you if the latter.

Sorry.