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