David Laight david.laight.linux@gmail.com wrote:
--- a/tools/testing/selftests/mm/write_to_hugetlbfs.c +++ b/tools/testing/selftests/mm/write_to_hugetlbfs.c @@ -86,10 +86,17 @@ int main(int argc, char **argv) while ((c = getopt(argc, argv, "s:p:m:owlrn")) != -1) { switch (c) { case 's':
if (sscanf(optarg, "%zu", &size) != 1) {perror("Invalid -s.");
char *end = NULL;Initialiser not needed.
unsigned long tmp = strtoul(optarg, &end, 10);if (errno || end == optarg || *end != '\0') {I doubt that use of errno is correct. Library functions that set errno on error don't set it to zero. The only test needed there is *end != ''. (end == optarg will be picked up by size == 0 later - if that is actually needed to stop things breaking.)
Good point!
perror("Invalid -s size"); exit_usage(); }if (tmp == 0) {No point checking for zero before the assigning the 'unsigned long' to 'size_t'. So the result of strtoul() can just be just assigned to 'size'. (Ignoring the fact that size_t will be unsigned long.)
perror("size not found");exit_usage();}size = (size_t)tmp; break; case 'p':Geeze guys, it's just a selftest.
hp2:/usr/src/linux-6.19-rc1> grep -r scanf tools/testing/selftests | wc -l 177
if your command line breaks the selftest, fix your command line?
Yes, I am ok with sscanf() :-).
What was wrong with atoi() ?
As the patch summary described, write_to_hugetlbfs previously parsed -s via atoi() into an int, which can overflow and print negative sizes. This problem was found on our kernel-64k platform and
#./charge_reserved_hugetlb.sh -cgroup-v2 # ----------------------------------------- ... # nr hugepages = 10 # writing cgroup limit: 5368709120 # writing reseravation limit: 5368709120 ... # Writing to this path: /mnt/huge/test # Writing this size: -1610612736 <--------
Or, at most, strtoul() with a check that *end == 0.
True, this will work as well, but as Andrew pointed, it is a tiny test issue and also resolved by sscanf() so let's just go with it. (He has added patchset to the mm-new branch and queued it for linux-next)
If the issue remains controversial, you could send a separate patch later. Anyway, thank you for the review comments.