We got the following UBSAN report:
====================================================================
UBSAN: shift-out-of-bounds in block/blk-iocost.c:1294:23
shift exponent 18446744073709 is too large for 64-bit type ......
CPU: 1 PID: 1088217 Comm: fsstress Kdump: loaded Not tainted ......
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) ......
Call Trace:
dump_stack+0x9c/0xd3
ubsan_epilogue+0xa/0x4e
__ubsan_handle_shift_out_of_bounds.cold+0x87/0x137
iocg_kick_delay.cold+0x18/0x60
ioc_rqos_throttle+0x7f8/0x870
__rq_qos_throttle+0x40/0x60
blk_mq_submit_bio+0x24d/0xd60
__submit_bio_noacct_mq+0x10b/0x270
submit_bio_noacct+0x13d/0x150
submit_bio+0xbf/0x280
submit_bh_wbc+0x3aa/0x450
ext4_read_bh_nowait+0xdb/0x180 [ext4]
ext4_read_bh_lock+0x6d/0x90 [ext4]
ext4_bread_batch+0x24c/0x2e0 [ext4]
__ext4_find_entry+0x2d2/0x880 [ext4]
ext4_lookup.part.0+0xbf/0x370 [ext4]
ext4_lookup+0x3e/0x60 [ext4]
lookup_open.isra.0+0x343/0x630
open_last_lookups+0x1f2/0x750
path_openat+0x133/0x330
do_filp_open+0x122/0x270
do_sys_openat2+0x3a8/0x550
__x64_sys_creat+0xae/0xe0
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x61/0xc6
===================================================================
The result of E1 >> E2 is E1 right-shifted E2 bit positions. From the
report, we know E2 is greater than the width of E1. In the C99 standard,
if the value of the E2 is negative or is greater than or equal to the
width of E1, the behavior is undefined.
In the actual test, if the E2 is greater than or equal to the width of
E1, the result of E1 >> E2 is E1 >> (E2 %/ E1width), which is not what we
want.
So letting the value of the right operand be less than the width of u64
in this expression.
Signed-off-by: Li Jinlin <[email protected]>
---
block/blk-iocost.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 07c1a31dd495..2b837ac4b2ba 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -1332,7 +1332,7 @@ static bool iocg_kick_delay(struct ioc_gq *iocg, struct ioc_now *now)
/* calculate the current delay in effect - 1/2 every second */
tdelta = now->now - iocg->delay_at;
if (iocg->delay)
- delay = iocg->delay >> div64_u64(tdelta, USEC_PER_SEC);
+ delay = iocg->delay >> min(div64_u64(tdelta, USEC_PER_SEC), 63);
else
delay = 0;
--
2.30.2
Hi Li,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on linus/master v6.1-rc6 next-20221125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Jinlin/blk-iocost-fix-shift-out-of-bounds-in-iocg_hick_delay/20221126-201700
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/r/20221126121458.3564942-1-lijinlin3%40huawei.com
patch subject: [PATCH] blk-iocost: fix shift-out-of-bounds in iocg_hick_delay()
config: x86_64-randconfig-a012
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5d3fe813cc0a541d089a4c869e85cfb7ce5220f3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Li-Jinlin/blk-iocost-fix-shift-out-of-bounds-in-iocg_hick_delay/20221126-201700
git checkout 5d3fe813cc0a541d089a4c869e85cfb7ce5220f3
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> block/blk-iocost.c:1334:26: warning: comparison of distinct pointer types ('typeof (div64_u64(tdelta, 1000000L)) *' (aka 'unsigned long long *') and 'typeof (63) *' (aka 'int *')) [-Wcompare-distinct-pointer-types]
delay = iocg->delay >> min(div64_u64(tdelta, USEC_PER_SEC), 63);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:45:19: note: expanded from macro 'min'
#define min(x, y) __careful_cmp(x, y, <)
^~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
__builtin_choose_expr(__safe_cmp(x, y), \
^~~~~~~~~~~~~~~~
include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
(__typecheck(x, y) && __no_side_effects(x, y))
^~~~~~~~~~~~~~~~~
include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
1 warning generated.
vim +1334 block/blk-iocost.c
1320
1321 static bool iocg_kick_delay(struct ioc_gq *iocg, struct ioc_now *now)
1322 {
1323 struct ioc *ioc = iocg->ioc;
1324 struct blkcg_gq *blkg = iocg_to_blkg(iocg);
1325 u64 tdelta, delay, new_delay;
1326 s64 vover, vover_pct;
1327 u32 hwa;
1328
1329 lockdep_assert_held(&iocg->waitq.lock);
1330
1331 /* calculate the current delay in effect - 1/2 every second */
1332 tdelta = now->now - iocg->delay_at;
1333 if (iocg->delay)
> 1334 delay = iocg->delay >> min(div64_u64(tdelta, USEC_PER_SEC), 63);
1335 else
1336 delay = 0;
1337
1338 /* calculate the new delay from the debt amount */
1339 current_hweight(iocg, &hwa, NULL);
1340 vover = atomic64_read(&iocg->vtime) +
1341 abs_cost_to_cost(iocg->abs_vdebt, hwa) - now->vnow;
1342 vover_pct = div64_s64(100 * vover,
1343 ioc->period_us * ioc->vtime_base_rate);
1344
1345 if (vover_pct <= MIN_DELAY_THR_PCT)
1346 new_delay = 0;
1347 else if (vover_pct >= MAX_DELAY_THR_PCT)
1348 new_delay = MAX_DELAY;
1349 else
1350 new_delay = MIN_DELAY +
1351 div_u64((MAX_DELAY - MIN_DELAY) *
1352 (vover_pct - MIN_DELAY_THR_PCT),
1353 MAX_DELAY_THR_PCT - MIN_DELAY_THR_PCT);
1354
1355 /* pick the higher one and apply */
1356 if (new_delay > delay) {
1357 iocg->delay = new_delay;
1358 iocg->delay_at = now->now;
1359 delay = new_delay;
1360 }
1361
1362 if (delay >= MIN_DELAY) {
1363 if (!iocg->indelay_since)
1364 iocg->indelay_since = now->now;
1365 blkcg_set_delay(blkg, delay * NSEC_PER_USEC);
1366 return true;
1367 } else {
1368 if (iocg->indelay_since) {
1369 iocg->stat.indelay_us += now->now - iocg->indelay_since;
1370 iocg->indelay_since = 0;
1371 }
1372 iocg->delay = 0;
1373 blkcg_clear_delay(blkg);
1374 return false;
1375 }
1376 }
1377
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Sat, Nov 26, 2022 at 08:14:58PM +0800, Li Jinlin wrote:
> if (iocg->delay)
> - delay = iocg->delay >> div64_u64(tdelta, USEC_PER_SEC);
> + delay = iocg->delay >> min(div64_u64(tdelta, USEC_PER_SEC), 63);
Let's just set delay to 0 if the shift is too big.
Thanks.
--
tejun