答复: 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats

["Li,Rongqing" <lirongqing@xxxxxxxxx>]

> -----邮件原件-----
> 发件人: 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;
 
        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) {
+               stats = this_cpu_ptr(pstats);
                u64_stats_update_begin(&stats->syncp);
                stats->pkts++;
                stats->bytes += pkt->skb->len;