linux-kernel - Re: [PATCH v4 12/14] selftests: harness: Stop using setjmp()/longjmp()

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250610141252-1ee7ae72-dbad-4a80-931c-5b4b14fb07ce@linutronix.de>
Date: Tue, 10 Jun 2025 14:21:25 +0200
From: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: Shuah Khan <shuah@...nel.org>, Shuah Khan <skhan@...uxfoundation.org>, 
	Willy Tarreau <w@....eu>, Thomas Weißschuh <linux@...ssschuh.net>, 
	Kees Cook <kees@...nel.org>, Andy Lutomirski <luto@...capital.net>, 
	Will Drewry <wad@...omium.org>, Mark Brown <broonie@...nel.org>, 
	Muhammad Usama Anjum <usama.anjum@...labora.com>, linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH v4 12/14] selftests: harness: Stop using
 setjmp()/longjmp()

Hi Nicolin,

On Mon, Jun 09, 2025 at 11:40:34PM -0700, Nicolin Chen wrote:
> Hi Thomas,
> 
> CC += Jason
> 
> On Mon, May 05, 2025 at 05:15:30PM +0200, Thomas Weißschuh wrote:
> > Usage of longjmp() was added to ensure that teardown is always run in
> > commit 63e6b2a42342 ("selftests/harness: Run TEARDOWN for ASSERT failures")
> > However instead of calling longjmp() to the teardown handler it is easier to
> > just call the teardown handler directly from __bail().
> > Any potential duplicate teardown invocations are harmless as the actual
> > handler will only ever be executed once since
> > commit fff37bd32c76 ("selftests/harness: Fix fixture teardown").
> > 
> > Additionally this removes a incompatibility with nolibc,
> > which does not support setjmp()/longjmp().
> > 
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> 
> The iommufd selftest (CONFIG_IOMMUFD_TEST) starts to give warnings
> when building with v6.16-rc1, though the test code wasn't changed
> at these two functions:

Thanks for the report.

> ------------------------------------------------------------------
> make: Entering directory '/nicolinc/linux-stable/tools/testing/selftests/iommu'
>   CC       iommufd
> iommufd.c: In function ‘wrapper_iommufd_mock_domain_all_aligns’:
> iommufd.c:1806:17: warning: ‘mfd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>  1806 |                 close(mfd);
>       |                 ^~~~~~~~~~
> iommufd.c:1766:13: note: ‘mfd’ was declared here
>  1766 |         int mfd;
>       |             ^~~
> iommufd.c: In function ‘wrapper_iommufd_mock_domain_all_aligns_copy’:
> iommufd.c:1869:17: warning: ‘mfd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>  1869 |                 close(mfd);
>       |                 ^~~~~~~~~~
> iommufd.c:1818:13: note: ‘mfd’ was declared here
>  1818 |         int mfd;
>       |             ^~~
>   CC       iommufd_fail_nth
> make: Leaving directory '/nicolinc/linux-stable/tools/testing/selftests/iommu'
> ------------------------------------------------------------------
> 
> Git bisect points to this patch, and reverting it fixes these.
> 
> Both mfds are under the same "if (variant->driver)" check, so the
> warnings don't seem legit to me.
> 
> Do you have any idea why this happens?

It does look very weird. I could understand if the compiler assumes that
variant->file changes during the runtime of the function.
But even if I work around this, by introducing a local variable "bool file =
variant->file" the issue persists.
However as soon as the value of of "bool file" is fixed to either "true" or
"false" it goes away.


The following diff *alone* also prevents the warning, but that doesn't
make any sense either:

--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -65,7 +65,7 @@ static inline void *memfd_mmap(size_t length, int prot, int flags, int *mfd_p)
                return MAP_FAILED;
        if (ftruncate(mfd, length))
                return MAP_FAILED;
-       *mfd_p = mfd;
+       *mfd_p = 0;
        return mmap(0, length, prot, flags, mfd, 0);
 }


Maybe the logic became too complex for GCC?
Case in point, when trying with an older GCC 13.2, the following warning appeared:

In file included from iommufd_utils.h:14,
                 from iommufd.c:12:
In function 'iommufd_viommu_vdevice_alloc',
    inlined from 'wrapper_iommufd_viommu_vdevice_alloc' at iommufd.c:2731:1:
../kselftest_harness.h:760:12: warning: 'ret' may be used uninitialized [-Wmaybe-uninitialized]
  760 |         if (!(__exp _t __seen)) { \
      |            ^
../kselftest_harness.h:513:9: note: in expansion of macro '__EXPECT'
  513 |         __EXPECT(expected, #expected, seen, #seen, ==, 1)
      |         ^~~~~~~~
iommufd_utils.h:1005:9: note: in expansion of macro 'ASSERT_EQ'
 1005 |         ASSERT_EQ(0, _test_cmd_trigger_vevents(self->fd, dev_id, nvevents))
      |         ^~~~~~~~~
iommufd.c:2766:17: note: in expansion of macro 'test_cmd_trigger_vevents'
 2766 |                 test_cmd_trigger_vevents(dev_id, 3);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~
iommufd_utils.h: In function 'wrapper_iommufd_viommu_vdevice_alloc':
iommufd_utils.h:993:13: note: 'ret' was declared here
  993 |         int ret;
      |             ^~~

Which is also just a false-positive and fixed with newer GCCs.


Thomas