Re: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()

Qu Wenruo
2023-12-26T07:40:08Z

On 2023/12/26 17:06, kernel test robot wrote:
> Hi Qu,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on kdave/for-next]
> [also build test WARNING on akpm-mm/mm-everything linus/master v6.7-rc7 next-20231222]
> [cannot apply to akpm-mm/mm-nonmm-unstable]
> [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/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
> base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> patch link: https://lore.kernel.org/r/56ea15d8b430f4fe3f8e55509ad0bc72b1d9356f.1703324146.git.wqu%40suse.com
> patch subject: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()
> config: m68k-randconfig-r133-20231226 (https://download.01.org/0day-ci/archive/20231226/202312261423.zqIlU2hn-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231226/202312261423.zqIlU2hn-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312261423.zqIlU2hn-lkp@intel.com/
>
> sparse warnings: (new ones prefixed by >>)
>>> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
> lib/test-kstrtox.c:351:39: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)

Any way to suppress the warning? As long as the constant value (u64) is
checked against the same truncated value (u32), the result should be fine.

I really want to make sure the pointer is not incorrectly updated in the
failure case.

Thanks,
Qu
>
> vim +339 lib/test-kstrtox.c
>
> 275
> 276 /* Want to include "E" suffix for full coverage. */
> 277 #define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
> 278 MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
> 279 MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
> 280
> 281 static void __init test_memparse_safe_fail(void)
> 282 {
> 283 struct memparse_test_fail {
> 284 const char *str;
> 285 /* Expected error number, either -EINVAL or -ERANGE. */
> 286 unsigned int expected_ret;
> 287 };
> 288 static const struct memparse_test_fail tests[] __initconst = {
> 289 /* No valid string can be found at all. */
> 290 {"", -EINVAL},
> 291 {"\n", -EINVAL},
> 292 {"\n0", -EINVAL},
> 293 {"+", -EINVAL},
> 294 {"-", -EINVAL},
> 295
> 296 /*
> 297 * No support for any leading "+-" chars, even followed by a valid
> 298 * number.
> 299 */
> 300 {"-0", -EINVAL},
> 301 {"+0", -EINVAL},
> 302 {"-1", -EINVAL},
> 303 {"+1", -EINVAL},
> 304
> 305 /* Stray suffix would also be rejected. */
> 306 {"K", -EINVAL},
> 307 {"P", -EINVAL},
> 308
> 309 /* Overflow in the string itself*/
> 310 {"18446744073709551616", -ERANGE},
> 311 {"02000000000000000000000", -ERANGE},
> 312 {"0x10000000000000000", -ERANGE},
> 313
> 314 /*
> 315 * Good string but would overflow with suffix.
> 316 *
> 317 * Note, for "E" suffix, one should not use with hex, or "0x1E"
> 318 * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix.
> 319 * Another reason "E" suffix is cursed.
> 320 */
> 321 {"16E", -ERANGE},
> 322 {"020E", -ERANGE},
> 323 {"16384P", -ERANGE},
> 324 {"040000P", -ERANGE},
> 325 {"16777216T", -ERANGE},
> 326 {"0100000000T", -ERANGE},
> 327 {"17179869184G", -ERANGE},
> 328 {"0200000000000G", -ERANGE},
> 329 {"17592186044416M", -ERANGE},
> 330 {"0400000000000000M", -ERANGE},
> 331 {"18014398509481984K", -ERANGE},
> 332 {"01000000000000000000K", -ERANGE},
> 333 };
> 334 unsigned int i;
> 335
> 336 for_each_test(i, tests) {
> 337 const struct memparse_test_fail *t = &tests[i];
> 338 unsigned long long tmp = ULL_PATTERN;
> > 339 char *retptr = (char *)ULL_PATTERN;
> 340 int ret;
> 341
> 342 ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
> 343 if (ret != t->expected_ret) {
> 344 WARN(1, "str '%s', expected ret %d got %d\n", t->str,
> 345 t->expected_ret, ret);
> 346 continue;
> 347 }
> 348 if (tmp != ULL_PATTERN)
> 349 WARN(1, "str '%s' failed as expected, but result got modified",
> 350 t->str);
> 351 if (retptr != (char *)ULL_PATTERN)
> 352 WARN(1, "str '%s' failed as expected, but pointer got modified",
> 353 t->str);
> 354 }
> 355 }
> 356
>