Re: [PATCH] Fix array overflow in CFQ

[Vivek Goyal]

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@xxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/