* [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
> -----邮件原件-----
> 发件人: 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
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
> -----邮件原件-----
> 发件人: 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
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
> -----邮件原件-----
> 发件人: 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.