| Message ID | 20250313153029.93347-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ubsan: fix ubsan on clang + code fixes | expand |
On 13/03/2025 3:30 pm, Roger Pau Monne wrote: > When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the > compiler emits the following errors: > > arch/x86/setup.c: In function '__start_xen': > arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > 1504 | end = consider_modules(s, e, reloc_size + mask, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1505 | bi->mods, bi->nr_modules, -1); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]' > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > 686 | static uint64_t __init consider_modules( > | ^~~~~~~~~~~~~~~~ > arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > 1535 | end = consider_modules(s, e, size, bi->mods, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1536 | bi->nr_modules + relocated, j); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]' > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > 686 | static uint64_t __init consider_modules( > | ^~~~~~~~~~~~~~~~ > > This seems to be the result of some function manipulation done by UBSAN > triggering GCC stringops related errors. Placate the errors by declaring > the function parameter as `const struct *boot_module` instead of `const > struct boot_module[]`. > > Note that GCC 13 seems to be fixed, and doesn't trigger the error when > using `[]`. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> (I swear I've seen this before, and already fixed it once by switching to a pointer...)
On 13.03.2025 16:30, Roger Pau Monne wrote: > When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the > compiler emits the following errors: > > arch/x86/setup.c: In function '__start_xen': > arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > 1504 | end = consider_modules(s, e, reloc_size + mask, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1505 | bi->mods, bi->nr_modules, -1); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]' > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > 686 | static uint64_t __init consider_modules( > | ^~~~~~~~~~~~~~~~ > arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > 1535 | end = consider_modules(s, e, size, bi->mods, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1536 | bi->nr_modules + relocated, j); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]' > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > 686 | static uint64_t __init consider_modules( > | ^~~~~~~~~~~~~~~~ > > This seems to be the result of some function manipulation done by UBSAN > triggering GCC stringops related errors. Placate the errors by declaring > the function parameter as `const struct *boot_module` instead of `const > struct boot_module[]`. > > Note that GCC 13 seems to be fixed, and doesn't trigger the error when > using `[]`. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/setup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 4a32d8491186..bde5d75ea6ab 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -684,7 +684,7 @@ static void __init noinline move_xen(void) > #undef BOOTSTRAP_MAP_LIMIT > > static uint64_t __init consider_modules( > - uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[], > + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, > unsigned int nr_mods, unsigned int this_mod) > { > unsigned int i; While I'm okay-ish with the change, how are we going to make sure it won't be re-introduced? Or something similar be introduced elsewhere? Jan
On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote: > On 13.03.2025 16:30, Roger Pau Monne wrote: > > When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the > > compiler emits the following errors: > > > > arch/x86/setup.c: In function '__start_xen': > > arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > > 1504 | end = consider_modules(s, e, reloc_size + mask, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 1505 | bi->mods, bi->nr_modules, -1); > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]' > > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > > 686 | static uint64_t __init consider_modules( > > | ^~~~~~~~~~~~~~~~ > > arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > > 1535 | end = consider_modules(s, e, size, bi->mods, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 1536 | bi->nr_modules + relocated, j); > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]' > > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > > 686 | static uint64_t __init consider_modules( > > | ^~~~~~~~~~~~~~~~ > > > > This seems to be the result of some function manipulation done by UBSAN > > triggering GCC stringops related errors. Placate the errors by declaring > > the function parameter as `const struct *boot_module` instead of `const > > struct boot_module[]`. > > > > Note that GCC 13 seems to be fixed, and doesn't trigger the error when > > using `[]`. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > xen/arch/x86/setup.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > > index 4a32d8491186..bde5d75ea6ab 100644 > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -684,7 +684,7 @@ static void __init noinline move_xen(void) > > #undef BOOTSTRAP_MAP_LIMIT > > > > static uint64_t __init consider_modules( > > - uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[], > > + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, > > unsigned int nr_mods, unsigned int this_mod) > > { > > unsigned int i; > > While I'm okay-ish with the change, how are we going to make sure it won't be > re-introduced? Or something similar be introduced elsewhere? I'm afraid I don't have a good response, as I don't even know exactly why the error triggers. We will rely on the CI to start doing randconfig builds with UBSAN enabled (see patch 7/7). Thanks, Roger.
On 14.03.2025 09:27, Roger Pau Monné wrote: > On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote: >> On 13.03.2025 16:30, Roger Pau Monne wrote: >>> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the >>> compiler emits the following errors: >>> >>> arch/x86/setup.c: In function '__start_xen': >>> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] >>> 1504 | end = consider_modules(s, e, reloc_size + mask, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 1505 | bi->mods, bi->nr_modules, -1); >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]' >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' >>> 686 | static uint64_t __init consider_modules( >>> | ^~~~~~~~~~~~~~~~ >>> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] >>> 1535 | end = consider_modules(s, e, size, bi->mods, >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 1536 | bi->nr_modules + relocated, j); >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]' >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' >>> 686 | static uint64_t __init consider_modules( >>> | ^~~~~~~~~~~~~~~~ >>> >>> This seems to be the result of some function manipulation done by UBSAN >>> triggering GCC stringops related errors. Placate the errors by declaring >>> the function parameter as `const struct *boot_module` instead of `const >>> struct boot_module[]`. >>> >>> Note that GCC 13 seems to be fixed, and doesn't trigger the error when >>> using `[]`. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> xen/arch/x86/setup.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>> index 4a32d8491186..bde5d75ea6ab 100644 >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void) >>> #undef BOOTSTRAP_MAP_LIMIT >>> >>> static uint64_t __init consider_modules( >>> - uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[], >>> + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, >>> unsigned int nr_mods, unsigned int this_mod) >>> { >>> unsigned int i; >> >> While I'm okay-ish with the change, how are we going to make sure it won't be >> re-introduced? Or something similar be introduced elsewhere? > > I'm afraid I don't have a good response, as I don't even know exactly > why the error triggers. One option might be to amend ./CODING_STYLE for dis-encourage [] notation in function parameters. I wouldn't be happy about us doing so, as I think that serves a documentation purpose, but compiler deficiencies getting in the way is certainly higher priority here. Trying to abstract this (vaguely along the lines of gcc11_wrap()), otoh, wouldn't be desirable imo, as it would still lose the doc effect, at least to some degree. > We will rely on the CI to start doing > randconfig builds with UBSAN enabled (see patch 7/7). Right. Just that randconfig is, well, random in what it covers. Jan
On Fri, Mar 14, 2025 at 09:33:01AM +0100, Jan Beulich wrote: > On 14.03.2025 09:27, Roger Pau Monné wrote: > > On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote: > >> On 13.03.2025 16:30, Roger Pau Monne wrote: > >>> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the > >>> compiler emits the following errors: > >>> > >>> arch/x86/setup.c: In function '__start_xen': > >>> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > >>> 1504 | end = consider_modules(s, e, reloc_size + mask, > >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>> 1505 | bi->mods, bi->nr_modules, -1); > >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]' > >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > >>> 686 | static uint64_t __init consider_modules( > >>> | ^~~~~~~~~~~~~~~~ > >>> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] > >>> 1535 | end = consider_modules(s, e, size, bi->mods, > >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>> 1536 | bi->nr_modules + relocated, j); > >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]' > >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' > >>> 686 | static uint64_t __init consider_modules( > >>> | ^~~~~~~~~~~~~~~~ > >>> > >>> This seems to be the result of some function manipulation done by UBSAN > >>> triggering GCC stringops related errors. Placate the errors by declaring > >>> the function parameter as `const struct *boot_module` instead of `const > >>> struct boot_module[]`. > >>> > >>> Note that GCC 13 seems to be fixed, and doesn't trigger the error when > >>> using `[]`. > >>> > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >>> --- > >>> xen/arch/x86/setup.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > >>> index 4a32d8491186..bde5d75ea6ab 100644 > >>> --- a/xen/arch/x86/setup.c > >>> +++ b/xen/arch/x86/setup.c > >>> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void) > >>> #undef BOOTSTRAP_MAP_LIMIT > >>> > >>> static uint64_t __init consider_modules( > >>> - uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[], > >>> + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, > >>> unsigned int nr_mods, unsigned int this_mod) > >>> { > >>> unsigned int i; > >> > >> While I'm okay-ish with the change, how are we going to make sure it won't be > >> re-introduced? Or something similar be introduced elsewhere? > > > > I'm afraid I don't have a good response, as I don't even know exactly > > why the error triggers. > > One option might be to amend ./CODING_STYLE for dis-encourage [] notation > in function parameters. I wouldn't be happy about us doing so, as I think > that serves a documentation purpose, but compiler deficiencies getting in > the way is certainly higher priority here. > > Trying to abstract this (vaguely along the lines of gcc11_wrap()), otoh, > wouldn't be desirable imo, as it would still lose the doc effect, at least > to some degree. This is a very specific case, I don't think we should change our coding style based on it. I think our only option is to deal with such compiler bugs when we detect them. Thanks, Roger.
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 4a32d8491186..bde5d75ea6ab 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -684,7 +684,7 @@ static void __init noinline move_xen(void) #undef BOOTSTRAP_MAP_LIMIT static uint64_t __init consider_modules( - uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[], + uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods, unsigned int nr_mods, unsigned int this_mod) { unsigned int i;
When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the compiler emits the following errors: arch/x86/setup.c: In function '__start_xen': arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] 1504 | end = consider_modules(s, e, reloc_size + mask, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1505 | bi->mods, bi->nr_modules, -1); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]' arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' 686 | static uint64_t __init consider_modules( | ^~~~~~~~~~~~~~~~ arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread] 1535 | end = consider_modules(s, e, size, bi->mods, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1536 | bi->nr_modules + relocated, j); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]' arch/x86/setup.c:686:24: note: in a call to function 'consider_modules' 686 | static uint64_t __init consider_modules( | ^~~~~~~~~~~~~~~~ This seems to be the result of some function manipulation done by UBSAN triggering GCC stringops related errors. Placate the errors by declaring the function parameter as `const struct *boot_module` instead of `const struct boot_module[]`. Note that GCC 13 seems to be fixed, and doesn't trigger the error when using `[]`. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)