| Message ID | 20230116130210.34490-1-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [f2fs-dev,v2,1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency() | expand |
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]++;
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
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 --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]++;
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(-)