netfilter: force access of RCU protected data in nft_update_chain_stats

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@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 net/netfilter/nf_tables_core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

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@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  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();
>  }
>  
>

> -----邮件原件-----
> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> 发送时间: 2019年2月25日 11:50
> 收件人: Li,Rongqing <lirongqing@baidu.com>; netfilter-devel@vger.kernel.org
> 主题: Re: [PATCH] netfilter: force access of RCU protected data in
> nft_update_chain_stats
> 
> 
> 
> 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@baidu.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  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() ?
> 

Ok, I will check it carefully.

> This rcu_access_pointer() test is fine, and avoids a
> local_bh_disable()/local_bh_enable()
> if they are not needed.



But it can not assure that rcu_dereference(base_chain->stats) returns NULL since nft_chain_stats_replace, should we check it again, other than check the returning of this_cpu_ptr?


thanks

-RongQing

On 02/24/2019 08:03 PM, Li,Rongqing wrote:
> 
> 
>> -----邮件原件-----
>> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com]
>> 发送时间: 2019年2月25日 11:50
>> 收件人: Li,Rongqing <lirongqing@baidu.com>; netfilter-devel@vger.kernel.org
>> 主题: Re: [PATCH] netfilter: force access of RCU protected data in
>> nft_update_chain_stats
>>
>>
>>
>> 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@baidu.com>
>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>>> ---
>>>  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() ?
>>
> 
> Ok, I will check it carefully.
> 
>> This rcu_access_pointer() test is fine, and avoids a
>> local_bh_disable()/local_bh_enable()
>> if they are not needed.
> 
> 
> 
> But it can not assure that rcu_dereference(base_chain->stats) returns NULL since nft_chain_stats_replace, should we check it again, other than check the returning of this_cpu_ptr?
> 

Sorry I do not understand your concern.