[PATCH net-next] net: sched: fix use before alloc of per cpu stats

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 <redacted>
---
 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

On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
[off-list ref] 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.

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.

On 1/16/2018 2:08 PM, Cong Wang wrote:

On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
[off-list ref] wrote:

quoted

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.

quoted

Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath")
Signed-off-by: Prashant Bhole <redacted>
---
  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

On Mon, Jan 15, 2018 at 9:47 PM, Prashant Bhole
[off-list ref] wrote:

On 1/16/2018 2:08 PM, Cong Wang wrote:

quoted

On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
[off-list ref] wrote:

quoted

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.

quoted

Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage
of tp->q for clsact fastpath")
Signed-off-by: Prashant Bhole <redacted>
---
  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.

On 1/16/2018 2:57 PM, Cong Wang wrote:

On Mon, Jan 15, 2018 at 9:47 PM, Prashant Bhole
[off-list ref] wrote:

quoted

On 1/16/2018 2:08 PM, Cong Wang wrote:

quoted

On Sun, Jan 14, 2018 at 9:52 PM, Prashant Bhole
[off-list ref] wrote:

quoted

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.

quoted

Fixes: 46209401f8f6 ("net: core: introduce mini_Qdisc and eliminate usage
of tp->q for clsact fastpath")
Signed-off-by: Prashant Bhole <redacted>
---
   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...

quoted

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

On Mon, Jan 15, 2018 at 10:33 PM, Prashant Bhole
[off-list ref] 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.