| Message ID | 20210319122414.129364-2-nikos.nikoleris@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Minor arm/arm64 MMU fixes and checks | expand |
On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote: > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> > --- > lib/arm/asm/cpumask.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h > index 6683bb6..02124de 100644 > --- a/lib/arm/asm/cpumask.h > +++ b/lib/arm/asm/cpumask.h > @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src) > > static inline int cpumask_next(int cpu, const cpumask_t *mask) > { > - while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask)) > + while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask)) > ; > return cpu; This looks like the right thing to do, but I'm surprised that I've never seen an assert in cpumask_test_cpu, even though it looks like we call cpumask_next with cpu == nr_cpus - 1 in several places. Can you please add a commit message explaining how you found this bug? Thanks, drew
Hi Drew, On 22/03/2021 09:31, Andrew Jones wrote: > On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote: >> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> >> --- >> lib/arm/asm/cpumask.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h >> index 6683bb6..02124de 100644 >> --- a/lib/arm/asm/cpumask.h >> +++ b/lib/arm/asm/cpumask.h >> @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src) >> >> static inline int cpumask_next(int cpu, const cpumask_t *mask) >> { >> - while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask)) >> + while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask)) >> ; >> return cpu; > Thanks for reviewing this! > This looks like the right thing to do, but I'm surprised that > I've never seen an assert in cpumask_test_cpu, even though > it looks like we call cpumask_next with cpu == nr_cpus - 1 > in several places. > cpumask_next() would trigger one of the assertions in the 4th patch in this series without this fix. The 4th patch is a way to demonstrate (if we apply it without the rest) the problem of using cpu0's thread_info->cpu uninitialized. > Can you please add a commit message explaining how you found > this bug? > Yes I'll do that. Thanks, Nikos > Thanks, > drew >
On Mon, Mar 22, 2021 at 09:45:09AM +0000, Nikos Nikoleris wrote: > Hi Drew, > > On 22/03/2021 09:31, Andrew Jones wrote: > > On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote: > > > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> > > > --- > > > lib/arm/asm/cpumask.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h > > > index 6683bb6..02124de 100644 > > > --- a/lib/arm/asm/cpumask.h > > > +++ b/lib/arm/asm/cpumask.h > > > @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src) > > > static inline int cpumask_next(int cpu, const cpumask_t *mask) > > > { > > > - while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask)) > > > + while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask)) > > > ; > > > return cpu; > > > > Thanks for reviewing this! > > > > This looks like the right thing to do, but I'm surprised that > > I've never seen an assert in cpumask_test_cpu, even though > > it looks like we call cpumask_next with cpu == nr_cpus - 1 > > in several places. > > > > cpumask_next() would trigger one of the assertions in the 4th patch in this > series without this fix. The 4th patch is a way to demonstrate (if we apply > it without the rest) the problem of using cpu0's thread_info->cpu > uninitialized. Ah, I see my error. I had already applied your 4th patch but hadn't reviewed it yet, so I didn't realize it was new code. Now it makes sense that we didn't hit that assert before (it didn't exist before :-) > > > Can you please add a commit message explaining how you found > > this bug? > > > > Yes I'll do that. If you just write one here then I'll add it while applying. The rest of the patches look good to me. So no need to respin. Thanks, drew
On 22/03/2021 10:12, Andrew Jones wrote: > On Mon, Mar 22, 2021 at 09:45:09AM +0000, Nikos Nikoleris wrote: >> Hi Drew, >> >> On 22/03/2021 09:31, Andrew Jones wrote: >>> On Fri, Mar 19, 2021 at 12:24:11PM +0000, Nikos Nikoleris wrote: >>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> >>>> --- >>>> lib/arm/asm/cpumask.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h >>>> index 6683bb6..02124de 100644 >>>> --- a/lib/arm/asm/cpumask.h >>>> +++ b/lib/arm/asm/cpumask.h >>>> @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src) >>>> static inline int cpumask_next(int cpu, const cpumask_t *mask) >>>> { >>>> - while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask)) >>>> + while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask)) >>>> ; >>>> return cpu; >>> >> >> Thanks for reviewing this! >> >> >>> This looks like the right thing to do, but I'm surprised that >>> I've never seen an assert in cpumask_test_cpu, even though >>> it looks like we call cpumask_next with cpu == nr_cpus - 1 >>> in several places. >>> >> >> cpumask_next() would trigger one of the assertions in the 4th patch in this >> series without this fix. The 4th patch is a way to demonstrate (if we apply >> it without the rest) the problem of using cpu0's thread_info->cpu >> uninitialized. > > Ah, I see my error. I had already applied your 4th patch but hadn't > reviewed it yet, so I didn't realize it was new code. Now it makes > sense that we didn't hit that assert before (it didn't exist > before :-) > >> >>> Can you please add a commit message explaining how you found >>> this bug? >>> >> >> Yes I'll do that. > > If you just write one here then I'll add it while applying. The rest of > the patches look good to me. So no need to respin. > Sounds good! Maybe we can add something along the lines of: Prior to this change, a call of cpumask_next(cpu, mask) where cpu=nr_cpu - 1 (assuming all cpus are enumerated in the range 0..nr_cpus - 1) would make an out-of-bounds access to the mask. In many cases, this is still a valid memory location due the implementation of cpumask_t, however, in certain configurations (for example, nr_cpus == sizeof(long)) this would cause an access outside the bounds of the mask too. This patch changes the way we guard calls to cpumask_test_cpu() in cpumask_next() to avoid the above condition. A following change adds assertions to catch out-of-bounds accesses to cpumask_t. Thanks, Nikos > Thanks, > drew > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, Mar 22, 2021 at 10:40:26AM +0000, Nikos Nikoleris wrote: > > Prior to this change, a call of cpumask_next(cpu, mask) where cpu=nr_cpu > - 1 (assuming all cpus are enumerated in the range 0..nr_cpus - 1) would > make an out-of-bounds access to the mask. In many cases, this is still a > valid memory location due the implementation of cpumask_t, however, in > certain configurations (for example, nr_cpus == sizeof(long)) this would > cause an access outside the bounds of the mask too. > > This patch changes the way we guard calls to cpumask_test_cpu() in > cpumask_next() to avoid the above condition. A following change adds > assertions to catch out-of-bounds accesses to cpumask_t. > Thanks, I've added it. It looks like Alexandru would also like commit message improvements. I can add those too. Thanks, drew
diff --git a/lib/arm/asm/cpumask.h b/lib/arm/asm/cpumask.h index 6683bb6..02124de 100644 --- a/lib/arm/asm/cpumask.h +++ b/lib/arm/asm/cpumask.h @@ -105,7 +105,7 @@ static inline void cpumask_copy(cpumask_t *dst, const cpumask_t *src) static inline int cpumask_next(int cpu, const cpumask_t *mask) { - while (cpu < nr_cpus && !cpumask_test_cpu(++cpu, mask)) + while (++cpu < nr_cpus && !cpumask_test_cpu(cpu, mask)) ; return cpu; }
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com> --- lib/arm/asm/cpumask.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)