[PATCH] stats: fix io_u_plat out-of-bound accesses (round 2)

[PATCH] stats: fix io_u_plat out-of-bound accesses (round 2)

[Eric Gouriou]

Commit 833491908a1afd67 introduced the ability to report completion
latency percentiles. It also caused a memory corruption when running
with multiple threads due to out of bound accesses in show_run_stats().
The major index of the io_u_plat two-dimensional array is meant
to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The
code in show_run_stats() incorrectly wrote into the array using
a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed
the out of bound accesses by increasing the size of the major
dimension of the io_u_plat array from 2 to 3.

This patch reverts the size change from 0a0b49007cbce8d1 in favor
of avoiding the out-of-bound accesses in show_run_stats().

Signed-off-by: Eric Gouriou <egouriou@google.com>
---
Jens, Zhu,

Yu-Ju is unlikely to be checking fio email traffic this week,
hence my follow-up. The error was introduced while porting
the patch between different versions of fio. The internal version
was tested appropriately but not the upstream version.

Apologies for the trouble.

Regards - Eric

---
 fio.h  |    2 +-
 stat.c |    5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fio.h b/fio.h
index c741162..6c57496 100644
--- a/fio.h
+++ b/fio.h
@@ -217,7 +217,7 @@ struct thread_stat {
 	unsigned int io_u_complete[FIO_IO_U_MAP_NR];
 	unsigned int io_u_lat_u[FIO_IO_U_LAT_U_NR];
 	unsigned int io_u_lat_m[FIO_IO_U_LAT_M_NR];
-	unsigned int io_u_plat[3][FIO_IO_U_PLAT_NR];
+	unsigned int io_u_plat[2][FIO_IO_U_PLAT_NR];
 	unsigned long total_io_u[3];
 	unsigned long short_io_u[3];
 	unsigned long total_submit;
diff --git a/stat.c b/stat.c
index ee6ee51..ae3c71a 100644
--- a/stat.c
+++ b/stat.c
@@ -773,11 +773,12 @@ void show_run_stats(void)
 
 
 		for (k = 0; k <= 2; k++) {
-			int m;
-
 			ts->total_io_u[k] += td->ts.total_io_u[k];
 			ts->short_io_u[k] += td->ts.short_io_u[k];
+		}
 
+		for (k = 0; k <= DDIR_WRITE; k++) {
+			int m;
 			for (m = 0; m < FIO_IO_U_PLAT_NR; m++)
 				ts->io_u_plat[k][m] += td->ts.io_u_plat[k][m];
 		}
-- 
1.7.3.1

Re: [PATCH] stats: fix io_u_plat out-of-bound accesses (round 2)

[Jens Axboe]

On 2011-08-16 01:23, Eric Gouriou wrote:
> Commit 833491908a1afd67 introduced the ability to report completion
> latency percentiles. It also caused a memory corruption when running
> with multiple threads due to out of bound accesses in show_run_stats().
> The major index of the io_u_plat two-dimensional array is meant
> to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The
> code in show_run_stats() incorrectly wrote into the array using
> a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed
> the out of bound accesses by increasing the size of the major
> dimension of the io_u_plat array from 2 to 3.
> 
> This patch reverts the size change from 0a0b49007cbce8d1 in favor
> of avoiding the out-of-bound accesses in show_run_stats().

This makes more sense, I didn't look carefully enough at this, the 3rd
index is usually for trim.

> Jens, Zhu,
> 
> Yu-Ju is unlikely to be checking fio email traffic this week,
> hence my follow-up. The error was introduced while porting
> the patch between different versions of fio. The internal version
> was tested appropriately but not the upstream version.
> 
> Apologies for the trouble.

No worries, thanks for following up on this so quickly. My group replies
bounced on Yu-Ju's email.

-- 
Jens Axboe

Re: [PATCH] stats: fix io_u_plat out-of-bound accesses (round 2)

[Yu-Ju Hong]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

Hi Jens, Zhu,

Sorry for all the troubles I caused for not checking the differences between
the Google internal version and the external version of fio. I couldn't
respond last week because I was out of town and did not have Internet access
nor my laptop last week. Also, my google.com account had been suspended
after I finished my internship, so I am currently using my gmail.com account
(which explains why the email got bounced). Thank you for discovering and
fixing the problem, and sorry again!

Eric,

Thank you very much for the follow-up!

Regards,
Yu-Ju


On Tue, Aug 16, 2011 at 2:38 AM, Jens Axboe <axboe@kernel.dk> wrote:

> On 2011-08-16 01:23, Eric Gouriou wrote:
> > Commit 833491908a1afd67 introduced the ability to report completion
> > latency percentiles. It also caused a memory corruption when running
> > with multiple threads due to out of bound accesses in show_run_stats().
> > The major index of the io_u_plat two-dimensional array is meant
> > to be DDIR_ value in {DDIR_READ, DDIR_WRITE} (i.e., {0, 1}). The
> > code in show_run_stats() incorrectly wrote into the array using
> > a major index with values {0, 1, 2}. Commit 0a0b49007cbce8d1 fixed
> > the out of bound accesses by increasing the size of the major
> > dimension of the io_u_plat array from 2 to 3.
> >
> > This patch reverts the size change from 0a0b49007cbce8d1 in favor
> > of avoiding the out-of-bound accesses in show_run_stats().
>
> This makes more sense, I didn't look carefully enough at this, the 3rd
> index is usually for trim.
>
> > Jens, Zhu,
> >
> > Yu-Ju is unlikely to be checking fio email traffic this week,
> > hence my follow-up. The error was introduced while porting
> > the patch between different versions of fio. The internal version
> > was tested appropriately but not the upstream version.
> >
> > Apologies for the trouble.
>
> No worries, thanks for following up on this so quickly. My group replies
> bounced on Yu-Ju's email.
>
> --
> Jens Axboe
>
>

[-- Attachment #2: Type: text/html, Size: 2715 bytes --]