linux-kernel - Re: [PATCH v3 1/3] selftests/mm/write_to_hugetlbfs: parse -s as size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEemH2e3C_oor-CK3_35m-gAp6uU5+75up7sL0wfX1MZdh3+jQ@mail.gmail.com>
Date: Tue, 23 Dec 2025 17:29:38 +0800
From: Li Wang <liwang@...hat.com>
To: David Laight <david.laight.linux@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	David Hildenbrand <david@...nel.org>, Mark Brown <broonie@...nel.org>, Shuah Khan <shuah@...nel.org>, 
	Waiman Long <longman@...hat.com>
Subject: Re: [PATCH v3 1/3] selftests/mm/write_to_hugetlbfs: parse -s as size_t

David Laight <david.laight.linux@...il.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.

-- 
Regards,
Li Wang