> -----邮件原件-----
> 发件人: Eric Dumazet [mailto:[email protected]]
> 发送时间: 2019年2月25日 12:11
> 收件人: Li,Rongqing <[email protected]>; [email protected]
> 主题: 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:[email protected]]
> >> 发送时间: 2019年2月25日 11:50
> >> 收件人: Li,Rongqing <[email protected]>;
> >> [email protected]
> >> 主题: 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 <[email protected]>
> >>> Signed-off-by: Li RongQing <[email protected]>
> >>> ---
> >>> 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;
