Messages in this thread | | | Date | Tue, 19 Oct 2010 07:49:33 -0400 | From | Vivek Goyal <> | Subject | Re: [PATCH] Fix array overflow in CFQ |
| |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |