| 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: <4C15DDC1-F05E-4382-9AE9-20BD78D9677B@redhat.com>
Date: Mon, 31 Aug 2020 09:56:27 +0200
From: "Eelco Chaudron" <echaudro@...hat.com>
To: trix@...hat.com
Cc: pshelar@....org, davem@...emloft.net, kuba@...nel.org,
natechancellor@...il.com, ndesaulniers@...gle.com,
netdev@...r.kernel.org, dev@...nvswitch.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: openvswitch: pass NULL for unused parameters
On 31 Aug 2020, at 9:50, Eelco Chaudron wrote:
> On 30 Aug 2020, at 23:26, trix@...hat.com wrote:
>
>> From: Tom Rix <trix@...hat.com>
>>
>> clang static analysis flags these problems
>>
>> flow_table.c:713:2: warning: The expression is an uninitialized
>> value. The computed value will also be garbage
>> (*n_mask_hit)++;
>> ^~~~~~~~~~~~~~~
>> flow_table.c:748:5: warning: The expression is an uninitialized
>> value. The computed value will also be garbage
>> (*n_cache_hit)++;
>> ^~~~~~~~~~~~~~~~
>>
>> These are not problems because neither parameter is used
>> by the calling function.
>>
>> Looking at all of the calling functions, there are many
>> cases where the results are unused. Passing unused
>> parameters is a waste.
>>
>> In the case where the output mask index parameter of flow_lookup()
>> is not used by the caller, it is always has a value of 0.
>>
>> To avoid passing unused parameters, rework the
>> masked_flow_lookup() and flow_lookup() routines to check
>> for NULL parameters and change the unused parameters to NULL.
>>
>> For the mask index parameter, use a local pointer to a value of
>> 0 if user passed in NULL.
>
>
> Some of this code is fast path, and some of it is not. So maybe the
> original author did this to avoid the null checks?
>
> Can you do some performance runs and see if it impact the performance
> in a negative way?
Forgot to add that if you do some performance tests, make sure you have
some 70+ masks as this is where you do a lot of null checks vs only
one-time use of a variable, see ovs_flow_tbl_lookup_stats().
>> Signed-off-by: Tom Rix <trix@...hat.com>
>> ---
>> v2
>> - fix spelling
>> - add mask index to NULL parameters
>> ---
>> net/openvswitch/flow_table.c | 32 +++++++++++++++-----------------
>> 1 file changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/net/openvswitch/flow_table.c
>> b/net/openvswitch/flow_table.c
>> index e2235849a57e..eac25596e4f4 100644
>> --- a/net/openvswitch/flow_table.c
>> +++ b/net/openvswitch/flow_table.c
>> @@ -710,7 +710,8 @@ static struct sw_flow *masked_flow_lookup(struct
>> table_instance *ti,
>> ovs_flow_mask_key(&masked_key, unmasked, false, mask);
>> hash = flow_hash(&masked_key, &mask->range);
>> head = find_bucket(ti, hash);
>> - (*n_mask_hit)++;
>> + if (n_mask_hit)
>> + (*n_mask_hit)++;
>>
>> hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver],
>> lockdep_ovsl_is_held()) {
>> @@ -730,12 +731,17 @@ static struct sw_flow *flow_lookup(struct
>> flow_table *tbl,
>> const struct sw_flow_key *key,
>> u32 *n_mask_hit,
>> u32 *n_cache_hit,
>> - u32 *index)
>> + u32 *in_index)
>> {
>> u64 *usage_counters = this_cpu_ptr(ma->masks_usage_cntr);
>> struct sw_flow *flow;
>> struct sw_flow_mask *mask;
>> int i;
>> + u32 idx = 0;
>> + u32 *index = &idx;
>> +
>> + if (in_index)
>> + index = in_index;
>>
>> if (likely(*index < ma->max)) {
>> mask = rcu_dereference_ovsl(ma->masks[*index]);
>> @@ -745,7 +751,8 @@ static struct sw_flow *flow_lookup(struct
>> flow_table *tbl,
>> u64_stats_update_begin(&ma->syncp);
>> usage_counters[*index]++;
>> u64_stats_update_end(&ma->syncp);
>> - (*n_cache_hit)++;
>> + if (n_cache_hit)
>> + (*n_cache_hit)++;
>> return flow;
>> }
>> }
>> @@ -796,13 +803,9 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct
>> flow_table *tbl,
>>
>> *n_mask_hit = 0;
>> *n_cache_hit = 0;
>> - if (unlikely(!skb_hash || mc->cache_size == 0)) {
>> - u32 mask_index = 0;
>> - u32 cache = 0;
>> -
>> - return flow_lookup(tbl, ti, ma, key, n_mask_hit, &cache,
>> - &mask_index);
>> - }
>> + if (unlikely(!skb_hash || mc->cache_size == 0))
>> + return flow_lookup(tbl, ti, ma, key, n_mask_hit, NULL,
>> + NULL);
>>
>> /* Pre and post recirulation flows usually have the same skb_hash
>> * value. To avoid hash collisions, rehash the 'skb_hash' with
>> @@ -849,11 +852,7 @@ struct sw_flow *ovs_flow_tbl_lookup(struct
>> flow_table *tbl,
>> {
>> struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
>> struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
>> - u32 __always_unused n_mask_hit;
>> - u32 __always_unused n_cache_hit;
>> - u32 index = 0;
>> -
>> - return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &n_cache_hit,
>> &index);
>> + return flow_lookup(tbl, ti, ma, key, NULL, NULL, NULL);
>> }
>>
>> struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
>> @@ -865,7 +864,6 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct
>> flow_table *tbl,
>> /* Always called under ovs-mutex. */
>> for (i = 0; i < ma->max; i++) {
>> struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
>> - u32 __always_unused n_mask_hit;
>> struct sw_flow_mask *mask;
>> struct sw_flow *flow;
>>
>> @@ -873,7 +871,7 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct
>> flow_table *tbl,
>> if (!mask)
>> continue;
>>
>> - flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
>> + flow = masked_flow_lookup(ti, match->key, mask, NULL);
>> if (flow && ovs_identifier_is_key(&flow->id) &&
>> ovs_flow_cmp_unmasked_key(flow, match)) {
>> return flow;
>> --
>> 2.18.1
Powered by blists - more mailing lists