* [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats @ 2019-02-25 1:58 Li RongQing 2019-02-25 3:50 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Li RongQing @ 2019-02-25 1:58 UTC (permalink / raw) To: netfilter-devel 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); + 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(); } -- 2.16.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-25 1:58 [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats Li RongQing @ 2019-02-25 3:50 ` Eric Dumazet 2019-02-25 4:03 ` 答复: " Li,Rongqing 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2019-02-25 3:50 UTC (permalink / raw) To: Li RongQing, netfilter-devel 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(); > } > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-25 3:50 ` Eric Dumazet @ 2019-02-25 4:03 ` Li,Rongqing 2019-02-25 4:10 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Li,Rongqing @ 2019-02-25 4:03 UTC (permalink / raw) To: Eric Dumazet, netfilter-devel@vger.kernel.org > -----邮件原件----- > 发件人: 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-25 4:03 ` 答复: " Li,Rongqing @ 2019-02-25 4:10 ` Eric Dumazet 2019-02-25 6:00 ` 答复: " Li,Rongqing 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2019-02-25 4:10 UTC (permalink / raw) To: Li,Rongqing, netfilter-devel@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* 答复: 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-25 4:10 ` Eric Dumazet @ 2019-02-25 6:00 ` Li,Rongqing 2019-02-25 15:55 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Li,Rongqing @ 2019-02-25 6:00 UTC (permalink / raw) To: Eric Dumazet, netfilter-devel@vger.kernel.org > -----邮件原件----- > 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com] > 发送时间: 2019年2月25日 12:11 > 收件人: 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 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 > >>> basechain->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. [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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: 答复: 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-25 6:00 ` 答复: " Li,Rongqing @ 2019-02-25 15:55 ` Eric Dumazet 2019-02-26 2:53 ` 答复: " Li,Rongqing 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2019-02-25 15:55 UTC (permalink / raw) To: Li,Rongqing, netfilter-devel@vger.kernel.org On 02/24/2019 10:00 PM, Li,Rongqing wrote: > > >> -----邮件原件----- >> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com] >> 发送时间: 2019年2月25日 12:11 >> 收件人: 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 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 >>>>> basechain->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. > > > [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; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* 答复: 答复: 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-25 15:55 ` Eric Dumazet @ 2019-02-26 2:53 ` Li,Rongqing 2019-02-26 3:16 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Li,Rongqing @ 2019-02-26 2:53 UTC (permalink / raw) To: Eric Dumazet, netfilter-devel@vger.kernel.org > -----邮件原件----- > 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com] > 发送时间: 2019年2月25日 23:56 > 收件人: 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 10:00 PM, Li,Rongqing wrote: > > > > > >> -----邮件原件----- > >> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com] > >> 发送时间: 2019年2月25日 12:11 > >> 收件人: 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 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 > >>>> > >>>> > >>>> > > > > > > [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 ) > I test this and found , there is always sparse error whether add __percpu or not Like: struct nft_stats __percpu *pstats; pstats = rcu_dereference(base_chain->stats); error: incompatible types in comparison expression (different address spaces) -RongQing ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: 答复: 答复: 答复: [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats 2019-02-26 2:53 ` 答复: " Li,Rongqing @ 2019-02-26 3:16 ` Eric Dumazet 0 siblings, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2019-02-26 3:16 UTC (permalink / raw) To: Li,Rongqing, netfilter-devel@vger.kernel.org On 02/25/2019 06:53 PM, Li,Rongqing wrote: > > >> -----邮件原件----- >> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com] >> 发送时间: 2019年2月25日 23:56 >> 收件人: 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 10:00 PM, Li,Rongqing wrote: >>> >>> >>>> -----邮件原件----- >>>> 发件人: Eric Dumazet [mailto:eric.dumazet@gmail.com] >>>> 发送时间: 2019年2月25日 12:11 >>>> 收件人: 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 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 >>>>>> >>>>>> >>>>>> >>> >>> >>> [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 ) >> > > I test this and found , there is always sparse error whether add __percpu or not > > Like: > struct nft_stats __percpu *pstats; > > pstats = rcu_dereference(base_chain->stats); I would suggest : diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c index 2a00aef7b6d4aaabfc538f24b0c36b50f76ce2b4..c29a8ce2c6bcdd683400f0b03dcc0c4440ab8fc8 100644 --- a/net/netfilter/nf_tables_core.c +++ b/net/netfilter/nf_tables_core.c @@ -98,21 +98,22 @@ 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 __percpu *pstats; struct nft_stats *stats; 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) { + rcu_read_lock(); + pstats = READ_ONCE(base_chain->stats); + if (pstats) { + local_bh_disable(); + stats = this_cpu_ptr(pstats); u64_stats_update_begin(&stats->syncp); stats->pkts++; stats->bytes += pkt->skb->len; u64_stats_update_end(&stats->syncp); + local_bh_enable(); } - local_bh_enable(); + rcu_read_unlock(); } struct nft_jumpstack { ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-26 3:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-25 1:58 [PATCH] netfilter: force access of RCU protected data in nft_update_chain_stats Li RongQing 2019-02-25 3:50 ` Eric Dumazet 2019-02-25 4:03 ` 答复: " Li,Rongqing 2019-02-25 4:10 ` Eric Dumazet 2019-02-25 6:00 ` 答复: " Li,Rongqing 2019-02-25 15:55 ` Eric Dumazet 2019-02-26 2:53 ` 答复: " Li,Rongqing 2019-02-26 3:16 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).