On 02/24/2019 10:00 PM, Li,Rongqing wrote: > > >> -----邮件原件----- >> 发件人: Eric Dumazet [mailto:eric.dumazet@xxxxxxxxx] >> 发送时间: 2019年2月25日 12:11 >> 收件人: Li,Rongqing <lirongqing@xxxxxxxxx>; netfilter-devel@xxxxxxxxxxxxxxx >> 主题: Re: 答复: [PATCH] netfilter: force access of RCU protected data in >> nft_update_chain_stats >> >> >> >> On 02/24/2019 08:03 PM, Li,Rongqing wrote: >>> >>> >>>> -----邮件原件----- >>>> 发件人: Eric Dumazet [mailto:eric.dumazet@xxxxxxxxx] >>>> 发送时间: 2019年2月25日 11:50 >>>> 收件人: Li,Rongqing <lirongqing@xxxxxxxxx>; >>>> netfilter-devel@xxxxxxxxxxxxxxx >>>> 主题: 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 >>>>> basechain->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() ? >>>> >>> >>> 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. > > > [RFC] netfilter: check the result of dereferencing base_chain->stats > > check the result of dereferencing base_chain->stats, instead of > result of this_cpu_ptr > > base_chain->stats maybe change to NULL when a chain is updated, > a NULL counter can be attached > > and we do not need to check returning of this_cpu_ptr since > base_chain->stats is allocated by percpu allocator if it is > non-NULL, this_cpu_ptr returns a valid value > > diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c > index 2a00aef7b6d4..ed8a382d1012 100644 > --- a/net/netfilter/nf_tables_core.c > +++ b/net/netfilter/nf_tables_core.c > @@ -98,15 +98,16 @@ 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, *pstats; OK but then make sure to add proper sparse annotations ( aka __percpu ) > > base_chain = nft_base_chain(chain); > if (!rcu_access_pointer(base_chain->stats)) > return; > > local_bh_disable(); > - stats = this_cpu_ptr(rcu_dereference(base_chain->stats)); > - if (stats) { > + pstats = rcu_dereference(base_chain->stats); > + if (pstats) { if (likely(pstats)) { > + stats = this_cpu_ptr(pstats); > u64_stats_update_begin(&stats->syncp); > stats->pkts++; > stats->bytes += pkt->skb->len; >
