On Mon, 22 Dec 2025 09:45:41 +0800 Li Wang liwang@redhat.com wrote:
On Mon, Dec 22, 2025 at 6:11 AM David Laight david.laight.linux@gmail.com wrote:
On Sun, 21 Dec 2025 20:26:37 +0800 Li Wang liwang@redhat.com wrote:
write_to_hugetlbfs currently parses the -s size argument with atoi() into an int. This silently accepts malformed input, cannot report
overflow,
and can truncate large sizes.
And sscanf() will just ignore invalid trailing characters. Probably much the same as atoi() apart from a leading '-'.
Maybe you could use "%zu%c" and check the count is 1 - but I bet some static checker won't like that.
Yes, that would be stronger, since it would reject trailing garbage. But for a selftest this is probably sufficient: switching to size_t and parsing with "%zu" already avoids the int truncation issue.
Have you checked at what does sscanf() does with an overlong digit string? I'd guess that it just processes all the digits and then masks the result to fix (like the kernel one does).
It reality scanf() is 'not the function you are lookign for'.
IIRC the 'SUS' (used to) say that this was absolutely fine for command line parsing for 'standard utilities'.
It is best to use strtoul() and check the 'end' character is '\0'.
David
@Andrew Morton akpm@linux-foundation.org,
Hi Andrew, I noticed you have addedthe patches to your mm-new branch, Let me know if you prefer the "%zu%c" enhancement in a new version.
On Tue, 23 Dec 2025 17:29:38 +0800 Li Wang liwang@redhat.com wrote:
David Laight david.laight.linux@gmail.com wrote:
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 <--------
So the problem was that atoi() doesn't let you specify valid values over 2GB. That isn't how I read the patch summary. It read as though you were worried about detecting invalid input.
David
David Laight david.laight.linux@gmail.com wrote:
Maybe you could use "%zu%c" and check the count is 1 - but I bet some static checker won't like that.
Yes, that would be stronger, since it would reject trailing garbage. But for a selftest this is probably sufficient: switching to size_t and parsing with "%zu" already avoids the int truncation issue.
Have you checked at what does sscanf() does with an overlong digit string? I'd guess that it just processes all the digits and then masks the result to fix (like the kernel one does).
It will truncate the number to the size SIZE_MAX.
From my test:
# ./write_to_hugetlbfs -m 0 -s 99999999999999999999999999 -p /mnt/huge/test -o Writing to this path: /mnt/huge/test Writing this size: 18446744073709551615 <---------- SIZE_MAX Populating. Not writing to memory. Using method=0 Shared mapping. RESERVE mapping. Allocating using HUGETLBFS. write_to_hugetlbfs: Error mapping the file: Invalid argument
It reality scanf() is 'not the function you are lookign for'.
IIRC the 'SUS' (used to) say that this was absolutely fine for command line parsing for 'standard utilities'.
It is best to use strtoul() and check the 'end' character is '\0'.
Hmm, that sounds like we need to go back to the patch V1 [1] method. But I am not sure, @Andrew Morton, do you think so?
--- 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; + unsigned long tmp = strtoul(optarg, &end, 10); + if (errno || end == optarg || *end != '\0') { + perror("Invalid -s size"); exit_usage(); } + if (tmp == 0) { + perror("size not found"); + exit_usage(); + } + size = (size_t)tmp; break; case 'p':
[1] https://lore.kernel.org/linux-kselftest/20251220111645.2246009-3-liwang@redh...
-- Regards, Li Wang
On Mon, 22 Dec 2025 18:56:49 +0800 Li Wang liwang@redhat.com wrote:
It reality scanf() is 'not the function you are lookign for'.
IIRC the 'SUS' (used to) say that this was absolutely fine for command line parsing for 'standard utilities'.
It is best to use strtoul() and check the 'end' character is '\0'.
Hmm, that sounds like we need to go back to the patch V1 [1] method. But I am not sure, @Andrew Morton, do you think so?
--- 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;unsigned long tmp = strtoul(optarg, &end, 10);if (errno || end == optarg || *end != '\0') {perror("Invalid -s size"); exit_usage(); }if (tmp == 0) {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?
Andrew Morton akpm@linux-foundation.org wrote:
It is best to use strtoul() and check the 'end' character is '\0'.
Hmm, that sounds like we need to go back to the patch V1 [1] method. But I am not sure, @Andrew Morton, do you think so?
--- 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;unsigned long tmp = strtoul(optarg, &end, 10);if (errno || end == optarg || *end != '\0') {perror("Invalid -s size"); exit_usage(); }if (tmp == 0) {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() :-).
In fact, write_to hugetlbfs currently only accepts arguments from charge_reserved_hugetlb.sh, and the way the '-s' is used is not very diverse.
-- Regards, Li Wang
On Tue, 23 Dec 2025 10:41:22 +0800 Li Wang liwang@redhat.com wrote:
Andrew Morton akpm@linux-foundation.org wrote:
It is best to use strtoul() and check the 'end' character is '\0'.
Hmm, that sounds like we need to go back to the patch V1 [1] method. But I am not sure, @Andrew Morton, do you think so?
--- 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.)
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() ? Or, at most, strtoul() with a check that *end == 0.
David
In fact, write_to hugetlbfs currently only accepts arguments from charge_reserved_hugetlb.sh, and the way the '-s' is used is not very diverse.
-- Regards, Li Wang
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.
linux-kselftest-mirror@lists.linaro.org
-
Andrew Morton -
David Laight -
Li Wang