* [PATCH net-next] net: sched: fix use before alloc of per cpu stats
@ 2018-01-15 5:52 Prashant Bhole
2018-01-16 5:08 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Prashant Bhole @ 2018-01-15 5:52 UTC (permalink / raw)
To: David S . Miller
Cc: Prashant Bhole, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
Daniel Borkmann, netdev
About bug:
During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
this flag is checked after init to allocate memory for stats.
Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
or this_cpu_ptr(NULL) gives a valid pointer.
About fix:
Currently stats memory is allocated at two places.
- in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
- in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this bug,
we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
allocation after init.
Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
net/sched/sch_api.c | 3 ++-
net/sched/sch_ingress.c | 6 ++----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 8a04c36e579f..de99a5e80944 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
goto err_out5;
}
- if (qdisc_is_percpu_stats(sch)) {
+ if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
+ qdisc_is_percpu_stats(sch)) {
sch->cpu_bstats =
netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu);
if (!sch->cpu_bstats)
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 7ca2be20dd6f..0a3fba46dfd3 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -82,8 +82,6 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt,
if (err)
return err;
- sch->flags |= TCQ_F_CPUSTATS;
-
return 0;
}
@@ -127,6 +125,7 @@ static struct Qdisc_ops ingress_qdisc_ops __read_mostly = {
.destroy = ingress_destroy,
.dump = ingress_dump,
.owner = THIS_MODULE,
+ .static_flags = TCQ_F_CPUSTATS,
};
struct clsact_sched_data {
@@ -202,8 +201,6 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt,
if (err)
return err;
- sch->flags |= TCQ_F_CPUSTATS;
-
return 0;
}
@@ -235,6 +232,7 @@ static struct Qdisc_ops clsact_qdisc_ops __read_mostly = {
.destroy = clsact_destroy,
.dump = ingress_dump,
.owner = THIS_MODULE,
+ .static_flags = TCQ_F_CPUSTATS,
};
static int __init ingress_module_init(void)
--
2.13.6
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: sched: fix use before alloc of per cpu stats
2018-01-15 5:52 [PATCH net-next] net: sched: fix use before alloc of per cpu stats Prashant Bhole
@ 2018-01-16 5:08 ` Cong Wang
2018-01-16 5:47 ` Prashant Bhole
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2018-01-16 5:08 UTC (permalink / raw)
To: Prashant Bhole
Cc: David S . Miller, Jamal Hadi Salim, Jiri Pirko, Daniel Borkmann,
Linux Kernel Network Developers
On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
<bhole_prashant_q7@lab.ntt.co.jp> wrote:
> About bug:
> During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
> with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
> this flag is checked after init to allocate memory for stats.
>
> Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
> while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
> or this_cpu_ptr(NULL) gives a valid pointer.
>
> About fix:
> Currently stats memory is allocated at two places.
> - in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
> - in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
>
> Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this bug,
> we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
> allocation after init.
Good catch! One nit below.
>
> Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
> net/sched/sch_api.c | 3 ++-
> net/sched/sch_ingress.c | 6 ++----
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 8a04c36e579f..de99a5e80944 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> goto err_out5;
> }
>
> - if (qdisc_is_percpu_stats(sch)) {
> + if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
> + qdisc_is_percpu_stats(sch)) {
Instead of checking both flags, it is simpler to just check if sch->cpu_bstats
and sch->cpu_qstats are NULL here?
Also, this patch should go to -net tree, not just net-next, but -net
doesn't have ops->static_flags yet. ;) Given this, consider moving
the sch->cpu_bstats allocation before ops->init(), which might be
simpler.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: sched: fix use before alloc of per cpu stats
2018-01-16 5:08 ` Cong Wang
@ 2018-01-16 5:47 ` Prashant Bhole
2018-01-16 5:57 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Prashant Bhole @ 2018-01-16 5:47 UTC (permalink / raw)
To: Cong Wang
Cc: David S . Miller, Jamal Hadi Salim, Jiri Pirko, Daniel Borkmann,
Linux Kernel Network Developers
On 1/16/2018 2:08 PM, Cong Wang wrote:
> On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
> <bhole_prashant_q7@lab.ntt.co.jp> wrote:
>> About bug:
>> During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
>> with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
>> this flag is checked after init to allocate memory for stats.
>>
>> Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
>> while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
>> or this_cpu_ptr(NULL) gives a valid pointer.
>>
>> About fix:
>> Currently stats memory is allocated at two places.
>> - in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
>> - in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
>>
>> Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this bug,
>> we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
>> allocation after init.
>
>
> Good catch! One nit below.
>
>
>>
>> Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>> net/sched/sch_api.c | 3 ++-
>> net/sched/sch_ingress.c | 6 ++----
>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index 8a04c36e579f..de99a5e80944 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>> goto err_out5;
>> }
>>
>> - if (qdisc_is_percpu_stats(sch)) {
>> + if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
>> + qdisc_is_percpu_stats(sch)) {
>
> Instead of checking both flags, it is simpler to just check if sch->cpu_bstats
> and sch->cpu_qstats are NULL here?
>
> Also, this patch should go to -net tree, not just net-next, but -net
> doesn't have ops->static_flags yet. ;) Given this, consider moving
> the sch->cpu_bstats allocation before ops->init(), which might be
> simpler.
Cong,
Thanks for reviewing. Based on this patch, Daniel submitted another
patch for -net:
http://patchwork.ozlabs.org/patch/861098/
Let me know if we still need v2 of my patch on -net-next (with your
suggested nit picks).
-Prashant
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: sched: fix use before alloc of per cpu stats
2018-01-16 5:47 ` Prashant Bhole
@ 2018-01-16 5:57 ` Cong Wang
2018-01-16 6:33 ` Prashant Bhole
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2018-01-16 5:57 UTC (permalink / raw)
To: Prashant Bhole
Cc: David S . Miller, Jamal Hadi Salim, Jiri Pirko, Daniel Borkmann,
Linux Kernel Network Developers
On Mon, Jan 15, 2018 at 9:47 PM, Prashant Bhole
<bhole_prashant_q7@lab.ntt.co.jp> wrote:
>
>
> On 1/16/2018 2:08 PM, Cong Wang wrote:
>>
>> On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
>> <bhole_prashant_q7@lab.ntt.co.jp> wrote:
>>>
>>> About bug:
>>> During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
>>> with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
>>> this flag is checked after init to allocate memory for stats.
>>>
>>> Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
>>> while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
>>> or this_cpu_ptr(NULL) gives a valid pointer.
>>>
>>> About fix:
>>> Currently stats memory is allocated at two places.
>>> - in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
>>> - in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
>>>
>>> Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this
>>> bug,
>>> we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
>>> allocation after init.
>>
>>
>>
>> Good catch! One nit below.
>>
>>
>>>
>>> Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage
>>> of tp->q for clsact fastpath")
>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>> ---
>>> net/sched/sch_api.c | 3 ++-
>>> net/sched/sch_ingress.c | 6 ++----
>>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>>> index 8a04c36e579f..de99a5e80944 100644
>>> --- a/net/sched/sch_api.c
>>> +++ b/net/sched/sch_api.c
>>> @@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device
>>> *dev,
>>> goto err_out5;
>>> }
>>>
>>> - if (qdisc_is_percpu_stats(sch)) {
>>> + if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
>>> + qdisc_is_percpu_stats(sch)) {
>>
>>
>> Instead of checking both flags, it is simpler to just check if
>> sch->cpu_bstats
>> and sch->cpu_qstats are NULL here?
>>
>> Also, this patch should go to -net tree, not just net-next, but -net
>> doesn't have ops->static_flags yet. ;) Given this, consider moving
>> the sch->cpu_bstats allocation before ops->init(), which might be
>> simpler.
>
>
> Cong,
> Thanks for reviewing. Based on this patch, Daniel submitted another patch
> for -net:
> http://patchwork.ozlabs.org/patch/861098/
Odd, I don't even see it in my inbox...
>
> Let me know if we still need v2 of my patch on -net-next (with your
> suggested nit picks).
It is okay, but it doesn't have to forward commit d59f5ffa59d8 to
net, reordering allocation before ->init() should be simpler.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: sched: fix use before alloc of per cpu stats
2018-01-16 5:57 ` Cong Wang
@ 2018-01-16 6:33 ` Prashant Bhole
2018-01-16 6:43 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Prashant Bhole @ 2018-01-16 6:33 UTC (permalink / raw)
To: Cong Wang
Cc: David S . Miller, Jamal Hadi Salim, Jiri Pirko, Daniel Borkmann,
Linux Kernel Network Developers
On 1/16/2018 2:57 PM, Cong Wang wrote:
> On Mon, Jan 15, 2018 at 9:47 PM, Prashant Bhole
> <bhole_prashant_q7@lab.ntt.co.jp> wrote:
>>
>>
>> On 1/16/2018 2:08 PM, Cong Wang wrote:
>>>
>>> On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
>>> <bhole_prashant_q7@lab.ntt.co.jp> wrote:
>>>>
>>>> About bug:
>>>> During init of clsact/ingress, it links qdisc's cpu_bstats,cpu_qstats
>>>> with mini qdisc. TCQ_F_CPUSTATS is set in qdisc->flags during init and
>>>> this flag is checked after init to allocate memory for stats.
>>>>
>>>> Hence mini qdisc points to null per-cpu-stats. The problem isn't caught
>>>> while updating stats via mini qdisc because per_cpu_ptr(NULL, cpu_num)
>>>> or this_cpu_ptr(NULL) gives a valid pointer.
>>>>
>>>> About fix:
>>>> Currently stats memory is allocated at two places.
>>>> - in qdisc_alloc() if TCQ_F_CPUSTATS is set in Qdisc_ops->static_flags
>>>> - in qdisc_create() if TCQ_F_CPUSTATS is set in Qdisc->flags
>>>>
>>>> Qdisc_ops->static_flags is propagated to Qdisc->flags. So to fix this
>>>> bug,
>>>> we set TCQ_F_CPUSTATS in static flags and additional condition to avoid
>>>> allocation after init.
>>>
>>>
>>>
>>> Good catch! One nit below.
>>>
>>>
>>>>
>>>> Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage
>>>> of tp->q for clsact fastpath")
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>> ---
>>>> net/sched/sch_api.c | 3 ++-
>>>> net/sched/sch_ingress.c | 6 ++----
>>>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>>>> index 8a04c36e579f..de99a5e80944 100644
>>>> --- a/net/sched/sch_api.c
>>>> +++ b/net/sched/sch_api.c
>>>> @@ -1094,7 +1094,8 @@ static struct Qdisc *qdisc_create(struct net_device
>>>> *dev,
>>>> goto err_out5;
>>>> }
>>>>
>>>> - if (qdisc_is_percpu_stats(sch)) {
>>>> + if (!(ops->static_flags & TCQ_F_CPUSTATS) &&
>>>> + qdisc_is_percpu_stats(sch)) {
>>>
>>>
>>> Instead of checking both flags, it is simpler to just check if
>>> sch->cpu_bstats
>>> and sch->cpu_qstats are NULL here?
>>>
>>> Also, this patch should go to -net tree, not just net-next, but -net
>>> doesn't have ops->static_flags yet. ;) Given this, consider moving
>>> the sch->cpu_bstats allocation before ops->init(), which might be
>>> simpler.
>>
>>
>> Cong,
>> Thanks for reviewing. Based on this patch, Daniel submitted another patch
>> for -net:
>> http://patchwork.ozlabs.org/patch/861098/
>
> Odd, I don't even see it in my inbox...
>
>
>>
>> Let me know if we still need v2 of my patch on -net-next (with your
>> suggested nit picks).
>
> It is okay, but it doesn't have to forward commit d59f5ffa59d8 to
> net, reordering allocation before ->init() should be simpler.
Pardon my ignorance, I think it is necessary to forward d59f5ffa59d8 to
-net. Because previously flags were set during init and checked after
init to allocate memory. static_flags makes the flags available before
init, hence we can allocate memory before init.
Sorry, but from your reply I couldn't understand whether we need v2 for
net-next.
-Prashant
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] net: sched: fix use before alloc of per cpu stats
2018-01-16 6:33 ` Prashant Bhole
@ 2018-01-16 6:43 ` Cong Wang
0 siblings, 0 replies; 6+ messages in thread
From: Cong Wang @ 2018-01-16 6:43 UTC (permalink / raw)
To: Prashant Bhole
Cc: David S . Miller, Jamal Hadi Salim, Jiri Pirko, Daniel Borkmann,
Linux Kernel Network Developers
On Mon, Jan 15, 2018 at 10:33 PM, Prashant Bhole
<bhole_prashant_q7@lab.ntt.co.jp> wrote:
> Pardon my ignorance, I think it is necessary to forward d59f5ffa59d8 to
> -net. Because previously flags were set during init and checked after init
> to allocate memory. static_flags makes the flags available before init,
> hence we can allocate memory before init.
Oh, right, but we can allocate it unconditionally and free it if not needed
after ->init(). It is not prettier though. ;)
> Sorry, but from your reply I couldn't understand whether we need v2 for
> net-next.
>
I don't think you need to send v2, because the one for -net will be merged
(with a conflict) into -net-next by DaveM.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-16 6:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-15 5:52 [PATCH net-next] net: sched: fix use before alloc of per cpu stats Prashant Bhole
2018-01-16 5:08 ` Cong Wang
2018-01-16 5:47 ` Prashant Bhole
2018-01-16 5:57 ` Cong Wang
2018-01-16 6:33 ` Prashant Bhole
2018-01-16 6:43 ` Cong Wang
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).