Re: [PATCH] Fix array overflow in CFQ

Vivek Goyal
2010-10-19T11:50:25Z

On Tue, Oct 19, 2010 at 12:01:40PM +0200, Jens Axboe wrote:
> On 2010-10-19 11:10, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> >
> > gcc 4.5 complains when compiling a recent rc with
> >
> > linux/block/cfq-iosched.c: In function ‘cfq_dispatch_requests’:
> > linux/block/cfq-iosched.c:2156:3: warning: array subscript is above array bounds
> >
> > and it is right:
> >
> > slice = group_slice * count /
> > max_t(unsigned, cfqg->busy_queues_avg[cfqd->serving_prio],
> > cfq_group_busy_queues_wl(cfqd->serving_prio, cfqd, cfqg));
> >
> > busy_queues_avg can be indexed by this enum
> >
> > enum wl_prio_t {
> > BE_WORKLOAD = 0,
> > RT_WORKLOAD = 1,
> > IDLE_WORKLOAD = 2,
> > };
> >
> > in cfqd->serving_prio, but is only declared as
> >
> > unsigned int busy_queues_avg[2];
> >
> > which is clearly off by one. Fix this here.
>
> Indeed, that is definitely buggy. ->service_trees[][] looks buggy, too.
> WTF?!

Hi Jens,

busy_queues_avg[] definitely looks buggy. Looks like I introduced this bug
while converting corrado's logic to group logic. I will fix it in a while.
Sorry for the goof up here.

->service_trees[][] is not buggy. We maintain workload subclassification
only for RT and BE class. For IDLE class, there are no ASYNC_WORKLOAD,
SYNC_NOIDLE_WORKLOAD or SYNC_WORKLOAD. All the type of idle queues
go onto a separate service tree, service_tree_idle.

Thanks
Vivek

>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 9eba291..8ce9f52 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -160,6 +160,7 @@ enum wl_prio_t {
> BE_WORKLOAD = 0,
> RT_WORKLOAD = 1,
> IDLE_WORKLOAD = 2,
> + CFQ_PRIO_NR,
> };
>
> /*
> @@ -168,7 +169,8 @@ enum wl_prio_t {
> enum wl_type_t {
> ASYNC_WORKLOAD = 0,
> SYNC_NOIDLE_WORKLOAD = 1,
> - SYNC_WORKLOAD = 2
> + SYNC_WORKLOAD = 2,
> + CFQ_TYPE_NR,
> };
>
> /* This is per cgroup per device grouping structure */
> @@ -185,12 +187,12 @@ struct cfq_group {
> int nr_cfqq;
>
> /* Per group busy queus average. Useful for workload slice calc. */
> - unsigned int busy_queues_avg[2];
> + unsigned int busy_queues_avg[CFQ_PRIO_NR];
> /*
> * rr lists of queues with requests, onle rr for each priority class.
> * Counts are embedded in the cfq_rb_root
> */
> - struct cfq_rb_root service_trees[2][3];
> + struct cfq_rb_root service_trees[CFQ_PRIO_NR][CFQ_TYPE_NR];
> struct cfq_rb_root service_tree_idle;
>
> unsigned long saved_workload_slice;
>
> --
> Jens Axboe