On 02/24/2019 05:58 PM, Li RongQing wrote:
> basechain->stats is rcu protected data, cannot assure that
> twice accesses have the same result, so dereference it once.
>
> basechain->stats is allocated by percpu allocater, if it is not NULL,
> its percpu variable does not need to be checked with NULL
>
> Signed-off-by: Zhang Yu <zhangyu31@xxxxxxxxx>
> Signed-off-by: Li RongQing <lirongqing@xxxxxxxxx>
> ---
> net/netfilter/nf_tables_core.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
> index 2a00aef7b6d4..9be622c76a62 100644
> --- a/net/netfilter/nf_tables_core.c
> +++ b/net/netfilter/nf_tables_core.c
> @@ -98,20 +98,20 @@ static noinline void nft_update_chain_stats(const struct nft_chain *chain,
> const struct nft_pktinfo *pkt)
> {
> struct nft_base_chain *base_chain;
> - struct nft_stats *stats;
> + struct nft_stats *stats, *pstat;
>
> base_chain = nft_base_chain(chain);
> - if (!rcu_access_pointer(base_chain->stats))
> +
> + stats = rcu_dereference(base_chain->stats);
This looks bogus to me.
Where is the needed rcu_read_lock() before rcu_dereference() ?
This rcu_access_pointer() test is fine, and avoids a local_bh_disable()/local_bh_enable()
if they are not needed.
> + if (!stats)
> return;
>
> local_bh_disable();
> - stats = this_cpu_ptr(rcu_dereference(base_chain->stats));
> - if (stats) {
> - u64_stats_update_begin(&stats->syncp);
> - stats->pkts++;
> - stats->bytes += pkt->skb->len;
> - u64_stats_update_end(&stats->syncp);
> - }
> + pstat = this_cpu_ptr(stats);
> + u64_stats_update_begin(&pstat->syncp);
> + pstat->pkts++;
> + pstat->bytes += pkt->skb->len;
> + u64_stats_update_end(&pstat->syncp);
> local_bh_enable();
> }
>
>
