| Message ID | 1d8d5089-32a9-9c42-5949-8f9cd20f33e0@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix build when NR_CPUS == 1 | expand |
On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: > In this case the compiler is recognizing that no valid array indexes > remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > ...)), but oddly enough isn't really consistent about the checking it > does (see the code comment). I assume this is because of the underlying per_cpu access to __per_cpu_offset using cpu as the index, in which case wouldn't it be better to place the BUG_ON there? Also I wonder why the accesses the same function does to the per_cpu area before the modified chunk using this_cpu as index don't also trigger such warnings. Thanks, Roger.
On 01.03.2021 15:01, Roger Pau Monné wrote: > On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: >> In this case the compiler is recognizing that no valid array indexes >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >> ...)), but oddly enough isn't really consistent about the checking it >> does (see the code comment). > > I assume this is because of the underlying per_cpu access to > __per_cpu_offset using cpu as the index, in which case wouldn't it be > better to place the BUG_ON there? Not sure, to be honest. It seemed more logical to me to place it next to where the issue is. > Also I wonder why the accesses the same function does to the per_cpu > area before the modified chunk using this_cpu as index don't also > trigger such warnings. The compiler appears to be issuing the warning when it can prove that no legitimate index can make it to a respective use. in this case it means that is is if ( this_cpu == cpu ) continue; which makes it possible for the compiler to know that what gets past this would be an out of bounds access, since for NR_CPUS=1 both this_cpu and cpu can only validly both be zero. (This also plays into my choice of placement, because it is not x2apic_cluster() on its own which has this issue.) Jan
Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
> In this case the compiler is recognizing that no valid array indexes
> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> ...)), but oddly enough isn't really consistent about the checking it
> does (see the code comment).
...
> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
> + if ( this_cpu == cpu )
> + continue;
> + /*
> + * Guard in particular against the compiler suspecting out-of-bounds
> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
> + * is the 1st of these alone which actually helps, not the 2nd, nor
> + * are both required together there).
> + */
> + BUG_ON(this_cpu >= NR_CPUS);
> + BUG_ON(cpu >= NR_CPUS);
> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
> continue;
Is there some particular reason for not putting the BUG_ON before the
if test ? That would avoid the refactoring.
Of course putting it in next to the array indexing would address that
too.
Ian.
On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: > On 01.03.2021 15:01, Roger Pau Monné wrote: > > On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: > >> In this case the compiler is recognizing that no valid array indexes > >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > >> ...)), but oddly enough isn't really consistent about the checking it > >> does (see the code comment). > > > > I assume this is because of the underlying per_cpu access to > > __per_cpu_offset using cpu as the index, in which case wouldn't it be > > better to place the BUG_ON there? > > Not sure, to be honest. It seemed more logical to me to place it > next to where the issue is. I'm not sure whether I would prefer to place it in per_cpu directly to avoid similar issues popping up in other parts of the code in the future? Maybe that's not likely. TBH it seems kind of random to be placing this BUG_ON conditionals to make the compilers happy, but maybe there's no other option. > > Also I wonder why the accesses the same function does to the per_cpu > > area before the modified chunk using this_cpu as index don't also > > trigger such warnings. > > The compiler appears to be issuing the warning when it can prove > that no legitimate index can make it to a respective use. in this > case it means that is is > > if ( this_cpu == cpu ) > continue; > > which makes it possible for the compiler to know that what gets > past this would be an out of bounds access, since for NR_CPUS=1 > both this_cpu and cpu can only validly both be zero. (This also > plays into my choice of placement, because it is not > x2apic_cluster() on its own which has this issue.) I see, thanks for the explanation. It makes me wonder whether other random issues like this will pop up in other parts of the code. We should have a gitlab build with NR_CPUS=1 in order to assert we don't regress it. Speaking for myself I certainly won't be able to detect whether something broke this support in the future. Thanks, Roger.
On 01.03.2021 19:00, Roger Pau Monné wrote: > On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: >> On 01.03.2021 15:01, Roger Pau Monné wrote: >>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: >>>> In this case the compiler is recognizing that no valid array indexes >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >>>> ...)), but oddly enough isn't really consistent about the checking it >>>> does (see the code comment). >>> >>> I assume this is because of the underlying per_cpu access to >>> __per_cpu_offset using cpu as the index, in which case wouldn't it be >>> better to place the BUG_ON there? >> >> Not sure, to be honest. It seemed more logical to me to place it >> next to where the issue is. > > I'm not sure whether I would prefer to place it in per_cpu directly to > avoid similar issues popping up in other parts of the code in the > future? That's going to be a lot of BUG_ON(), and hence a lot of "ud2" instances. Not something I'm keen on having. The more that it's not the per_cpu() which triggers the warning, but separate conditionals allowing the compiler to deduce value ranges of variables. > Maybe that's not likely. TBH it seems kind of random to be placing > this BUG_ON conditionals to make the compilers happy, but maybe > there's no other option. In principle I agree - hence the longish comment. >>> Also I wonder why the accesses the same function does to the per_cpu >>> area before the modified chunk using this_cpu as index don't also >>> trigger such warnings. >> >> The compiler appears to be issuing the warning when it can prove >> that no legitimate index can make it to a respective use. in this >> case it means that is is >> >> if ( this_cpu == cpu ) >> continue; >> >> which makes it possible for the compiler to know that what gets >> past this would be an out of bounds access, since for NR_CPUS=1 >> both this_cpu and cpu can only validly both be zero. (This also >> plays into my choice of placement, because it is not >> x2apic_cluster() on its own which has this issue.) > > I see, thanks for the explanation. It makes me wonder whether other > random issues like this will pop up in other parts of the code. We > should have a gitlab build with NR_CPUS=1 in order to assert we don't > regress it. Speaking for myself I certainly won't be able to detect > whether something broke this support in the future. I guess I'll carry on having such a build test locally. Jan
On 01.03.2021 17:03, Ian Jackson wrote: > Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): >> In this case the compiler is recognizing that no valid array indexes >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >> ...)), but oddly enough isn't really consistent about the checking it >> does (see the code comment). > ... >> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) >> + if ( this_cpu == cpu ) >> + continue; >> + /* >> + * Guard in particular against the compiler suspecting out-of-bounds >> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it >> + * is the 1st of these alone which actually helps, not the 2nd, nor >> + * are both required together there). >> + */ >> + BUG_ON(this_cpu >= NR_CPUS); >> + BUG_ON(cpu >= NR_CPUS); >> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) >> continue; > > Is there some particular reason for not putting the BUG_ON before the > if test ? That would avoid the refactoring. I want it to be as close as possible to the place where the issue is. I also view the refactoring as a good thing, since it allows a style correction as a side effect. > Of course putting it in next to the array indexing would address that > too. See my earlier reply to Roger's similar remark (which still was along the lines of what I've said above). Jan
On Tue, Mar 02, 2021 at 10:59:37AM +0100, Jan Beulich wrote: > On 01.03.2021 19:00, Roger Pau Monné wrote: > > On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: > >> On 01.03.2021 15:01, Roger Pau Monné wrote: > >>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: > >>>> In this case the compiler is recognizing that no valid array indexes > >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > >>>> ...)), but oddly enough isn't really consistent about the checking it > >>>> does (see the code comment). > >>> > >>> I assume this is because of the underlying per_cpu access to > >>> __per_cpu_offset using cpu as the index, in which case wouldn't it be > >>> better to place the BUG_ON there? > >> > >> Not sure, to be honest. It seemed more logical to me to place it > >> next to where the issue is. > > > > I'm not sure whether I would prefer to place it in per_cpu directly to > > avoid similar issues popping up in other parts of the code in the > > future? > > That's going to be a lot of BUG_ON(), and hence a lot of "ud2" > instances. Not something I'm keen on having. The more that it's > not the per_cpu() which triggers the warning, but separate > conditionals allowing the compiler to deduce value ranges of > variables. Right. I don't see much other way around this then. Did you check with clang also? Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 02.03.2021 11:57, Roger Pau Monné wrote: > On Tue, Mar 02, 2021 at 10:59:37AM +0100, Jan Beulich wrote: >> On 01.03.2021 19:00, Roger Pau Monné wrote: >>> On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: >>>> On 01.03.2021 15:01, Roger Pau Monné wrote: >>>>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: >>>>>> In this case the compiler is recognizing that no valid array indexes >>>>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >>>>>> ...)), but oddly enough isn't really consistent about the checking it >>>>>> does (see the code comment). >>>>> >>>>> I assume this is because of the underlying per_cpu access to >>>>> __per_cpu_offset using cpu as the index, in which case wouldn't it be >>>>> better to place the BUG_ON there? >>>> >>>> Not sure, to be honest. It seemed more logical to me to place it >>>> next to where the issue is. >>> >>> I'm not sure whether I would prefer to place it in per_cpu directly to >>> avoid similar issues popping up in other parts of the code in the >>> future? >> >> That's going to be a lot of BUG_ON(), and hence a lot of "ud2" >> instances. Not something I'm keen on having. The more that it's >> not the per_cpu() which triggers the warning, but separate >> conditionals allowing the compiler to deduce value ranges of >> variables. > > Right. I don't see much other way around this then. Did you check with > clang also? No, I didn't. But if anything it would require further additions, surely no less. > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
Jan Beulich writes ("Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
> On 01.03.2021 17:03, Ian Jackson wrote:
> > Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"):
> >> In this case the compiler is recognizing that no valid array indexes
> >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid,
> >> ...)), but oddly enough isn't really consistent about the checking it
> >> does (see the code comment).
> > ...
> >> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu))
> >> + if ( this_cpu == cpu )
> >> + continue;
> >> + /*
> >> + * Guard in particular against the compiler suspecting out-of-bounds
> >> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it
> >> + * is the 1st of these alone which actually helps, not the 2nd, nor
> >> + * are both required together there).
> >> + */
> >> + BUG_ON(this_cpu >= NR_CPUS);
> >> + BUG_ON(cpu >= NR_CPUS);
> >> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) )
> >> continue;
> >
> > Is there some particular reason for not putting the BUG_ON before the
> > if test ? That would avoid the refactoring.
>
> I want it to be as close as possible to the place where the issue
> is. I also view the refactoring as a good thing, since it allows
> a style correction as a side effect.
I'm afraid that at this stage of the release I would prefer changes to
be as small as reasonably sensible. So unless there is some
reason, other than taste, style or formatting, could we please just
introduce the new BUG_ON and not also do other refactoring.
Ian.
On 02.03.2021 13:28, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): >> On 01.03.2021 17:03, Ian Jackson wrote: >>> Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): >>>> In this case the compiler is recognizing that no valid array indexes >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >>>> ...)), but oddly enough isn't really consistent about the checking it >>>> does (see the code comment). >>> ... >>>> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) >>>> + if ( this_cpu == cpu ) >>>> + continue; >>>> + /* >>>> + * Guard in particular against the compiler suspecting out-of-bounds >>>> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it >>>> + * is the 1st of these alone which actually helps, not the 2nd, nor >>>> + * are both required together there). >>>> + */ >>>> + BUG_ON(this_cpu >= NR_CPUS); >>>> + BUG_ON(cpu >= NR_CPUS); >>>> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) >>>> continue; >>> >>> Is there some particular reason for not putting the BUG_ON before the >>> if test ? That would avoid the refactoring. >> >> I want it to be as close as possible to the place where the issue >> is. I also view the refactoring as a good thing, since it allows >> a style correction as a side effect. > > I'm afraid that at this stage of the release I would prefer changes to > be as small as reasonably sensible. So unless there is some > reason, other than taste, style or formatting, could we please just > introduce the new BUG_ON and not also do other refactoring. FAOD: That's fine - I'll keep this queued for 4.16 then. I did put a question mark behind the version anyway. Jan
--- a/xen/arch/x86/genapic/x2apic.c +++ b/xen/arch/x86/genapic/x2apic.c @@ -54,7 +54,17 @@ static void init_apic_ldr_x2apic_cluster per_cpu(cluster_cpus, this_cpu) = cluster_cpus_spare; for_each_online_cpu ( cpu ) { - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) + if ( this_cpu == cpu ) + continue; + /* + * Guard in particular against the compiler suspecting out-of-bounds + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it + * is the 1st of these alone which actually helps, not the 2nd, nor + * are both required together there). + */ + BUG_ON(this_cpu >= NR_CPUS); + BUG_ON(cpu >= NR_CPUS); + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) continue; per_cpu(cluster_cpus, this_cpu) = per_cpu(cluster_cpus, cpu); break;
In this case the compiler is recognizing that no valid array indexes remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, ...)), but oddly enough isn't really consistent about the checking it does (see the code comment). Signed-off-by: Jan Beulich <jbeulich@suse.com>