diff mbox series

[v2,1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()

Message ID 20230116130210.34490-1-frank.li@vivo.com
State New
Headers
Series [v2,1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency() |

Commit Message

李扬韬 Jan. 16, 2023, 1:02 p.m. UTC 
  Add iotype sanity check to avoid potential memory corruption.
This is to fix the compile error below:

fs/f2fs/iostat.c:231 __update_iostat_latency() error: buffer overflow
'io_lat->peak_lat[type]' 3 <= 3

vim +228 fs/f2fs/iostat.c

  211  static inline void __update_iostat_latency(struct bio_iostat_ctx
	*iostat_ctx,
  212					enum iostat_lat_type type)
  213  {
  214		unsigned long ts_diff;
  215		unsigned int page_type = iostat_ctx->type;
  216		struct f2fs_sb_info *sbi = iostat_ctx->sbi;
  217		struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
  218		unsigned long flags;
  219
  220		if (!sbi->iostat_enable)
  221			return;
  222
  223		ts_diff = jiffies - iostat_ctx->submit_ts;
  224		if (page_type >= META_FLUSH)
                                 ^^^^^^^^^^

  225			page_type = META;
  226
  227		spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 @228		io_lat->sum_lat[type][page_type] += ts_diff;
                                      ^^^^^^^^^
Mixup between META_FLUSH and NR_PAGE_TYPE leads to memory corruption.

Fixes: a4b6817625e7 ("f2fs: introduce periodic iostat io latency traces")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/iostat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Chao Yu Jan. 16, 2023, 1:25 p.m. UTC | #1 
On 2023/1/16 21:02, Yangtao Li wrote:
> Add iotype sanity check to avoid potential memory corruption.
> This is to fix the compile error below:
> 
> fs/f2fs/iostat.c:231 __update_iostat_latency() error: buffer overflow
> 'io_lat->peak_lat[type]' 3 <= 3
> 
> vim +228 fs/f2fs/iostat.c
> 
>    211  static inline void __update_iostat_latency(struct bio_iostat_ctx
> 	*iostat_ctx,
>    212					enum iostat_lat_type type)
>    213  {
>    214		unsigned long ts_diff;
>    215		unsigned int page_type = iostat_ctx->type;
>    216		struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>    217		struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>    218		unsigned long flags;
>    219
>    220		if (!sbi->iostat_enable)
>    221			return;
>    222
>    223		ts_diff = jiffies - iostat_ctx->submit_ts;
>    224		if (page_type >= META_FLUSH)
>                                   ^^^^^^^^^^
> 
>    225			page_type = META;
>    226
>    227		spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   @228		io_lat->sum_lat[type][page_type] += ts_diff;
>                                        ^^^^^^^^^
> Mixup between META_FLUSH and NR_PAGE_TYPE leads to memory corruption.
> 
> Fixes: a4b6817625e7 ("f2fs: introduce periodic iostat io latency traces")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   fs/f2fs/iostat.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> index ed8176939aa5..e9a3df7ce4d9 100644
> --- a/fs/f2fs/iostat.c
> +++ b/fs/f2fs/iostat.c
> @@ -223,7 +223,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   		return;
>   
>   	ts_diff = jiffies - iostat_ctx->submit_ts;
> -	if (iotype >= META_FLUSH)
> +	if (iotype == META_FLUSH)

Maybe it's betterr to merge these two check condition as below?

if (iotype >= NR_PAGE_TYPE) {
	f2fs_bug_on(sbi, iotype != META_FLUSH);
	iotype = META;
}

For CHECK_FS is off case, I guess it's fine to not return and just print
warning message for notice.

Thanks,

>   		iotype = META;
>   
>   	if (rw == 0) {
> @@ -235,6 +235,11 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   			idx = WRITE_ASYNC_IO;
>   	}
>   
> +	if (iotype >= NR_PAGE_TYPE) {
> +		f2fs_bug_on(sbi, 1);
> +		return;
> +	}
> +
>   	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   	io_lat->sum_lat[idx][iotype] += ts_diff;
>   	io_lat->bio_cnt[idx][iotype]++;
李扬韬 Jan. 16, 2023, 1:47 p.m. UTC | #2 
Hi Chao,

> Maybe it's betterr to merge these two check condition as below?
>
> if (iotype >= NR_PAGE_TYPE) {
> 	f2fs_bug_on(sbi, iotype != META_FLUSH);
> 	iotype = META;
> }

For normal , only META_FLUSH will be greater than NR_PAGE_TYPE,
there is no problem with this logic.

>
> For CHECK_FS is off case, I guess it's fine to not return and just print
> warning message for notice.

But if there is an exception, we don't know the type we originally wanted to count.
If they are all changed to meta, it is possible to get a wrong statistic. I think
there is no need to make statistics on this kind of error scene. Just like in the
next patch, if iostat_lat_type is wrong, we should return directly instead of changing
the value beyond the range to WRITE_ASYNC_IO.

So it's no need tp merge these two check condition?

Thx,
Yangtao
Jaegeuk Kim Jan. 19, 2023, 1:49 a.m. UTC | #3 
On 01/16, Yangtao Li wrote:
> Hi Chao,
> 
> > Maybe it's betterr to merge these two check condition as below?
> >
> > if (iotype >= NR_PAGE_TYPE) {
> > 	f2fs_bug_on(sbi, iotype != META_FLUSH);
> > 	iotype = META;
> > }
> 
> For normal , only META_FLUSH will be greater than NR_PAGE_TYPE,
> there is no problem with this logic.
> 
> >
> > For CHECK_FS is off case, I guess it's fine to not return and just print
> > warning message for notice.
> 
> But if there is an exception, we don't know the type we originally wanted to count.
> If they are all changed to meta, it is possible to get a wrong statistic. I think
> there is no need to make statistics on this kind of error scene. Just like in the
> next patch, if iostat_lat_type is wrong, we should return directly instead of changing
> the value beyond the range to WRITE_ASYNC_IO.
> 
> So it's no need tp merge these two check condition?

I also prefer to do like Chao's comment. We don't need to expect such exception
at all.

And, it seems we just need to do like:

	enum page_type iotype;

	if (iotype == META_FLUSH) {
		iotype = META;
	} else if (iotype >= NR_PAGE_TYPE) {
		f2fs_warn();
		return;
	}

> 
> Thx,
> Yangtao
diff mbox series

Patch

diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
index ed8176939aa5..e9a3df7ce4d9 100644
--- a/fs/f2fs/iostat.c
+++ b/fs/f2fs/iostat.c
@@ -223,7 +223,7 @@  static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 		return;
 
 	ts_diff = jiffies - iostat_ctx->submit_ts;
-	if (iotype >= META_FLUSH)
+	if (iotype == META_FLUSH)
 		iotype = META;
 
 	if (rw == 0) {
@@ -235,6 +235,11 @@  static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 			idx = WRITE_ASYNC_IO;
 	}
 
+	if (iotype >= NR_PAGE_TYPE) {
+		f2fs_bug_on(sbi, 1);
+		return;
+	}
+
 	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 	io_lat->sum_lat[idx][iotype] += ts_diff;
 	io_lat->bio_cnt[idx][iotype]++;