* [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()
@ 2023-01-16 13:02 Yangtao Li via Linux-f2fs-devel
2023-01-16 13:25 ` Chao Yu
0 siblings, 1 reply; 4+ messages in thread
From: Yangtao Li via Linux-f2fs-devel @ 2023-01-16 13:02 UTC (permalink / raw)
To: jaegeuk, chao
Cc: kernel test robot, Yangtao Li, Dan Carpenter, linux-kernel,
linux-f2fs-devel
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)
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]++;
--
2.25.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()
2023-01-16 13:02 [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency() Yangtao Li via Linux-f2fs-devel
@ 2023-01-16 13:25 ` Chao Yu
2023-01-16 13:47 ` Yangtao Li via Linux-f2fs-devel
0 siblings, 1 reply; 4+ messages in thread
From: Chao Yu @ 2023-01-16 13:25 UTC (permalink / raw)
To: Yangtao Li, jaegeuk
Cc: kernel test robot, Dan Carpenter, linux-kernel, linux-f2fs-devel
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]++;
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()
2023-01-16 13:25 ` Chao Yu
@ 2023-01-16 13:47 ` Yangtao Li via Linux-f2fs-devel
2023-01-19 1:49 ` Jaegeuk Kim
0 siblings, 1 reply; 4+ messages in thread
From: Yangtao Li via Linux-f2fs-devel @ 2023-01-16 13:47 UTC (permalink / raw)
To: jaegeuk, chao; +Cc: linux-kernel, linux-f2fs-devel
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()
2023-01-16 13:47 ` Yangtao Li via Linux-f2fs-devel
@ 2023-01-19 1:49 ` Jaegeuk Kim
0 siblings, 0 replies; 4+ messages in thread
From: Jaegeuk Kim @ 2023-01-19 1:49 UTC (permalink / raw)
To: Yangtao Li; +Cc: linux-kernel, linux-f2fs-devel
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
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-19 1:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-16 13:02 [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency() Yangtao Li via Linux-f2fs-devel
2023-01-16 13:25 ` Chao Yu
2023-01-16 13:47 ` Yangtao Li via Linux-f2fs-devel
2023-01-19 1:49 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).