Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
adds functionality that architectures can use to optionally allocate and
build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
arch specific early level initializer") lets secondary CPUs correct (and
reallocate memory) cacheinfo data if needed.
If the early build functionality is not used and cacheinfo does not need
correction, memory for cacheinfo is never allocated. x86 does not use the
early build functionality. Consequently, during the cacheinfo CPU hotplug
callback, last_level_cache_is_valid() attempts to dereference a NULL
pointer:
BUG: kernel NULL pointer dereference, address: 0000000000000100
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not present page
PGD 0 P4D 0
Oops: 0000 [#1] PREEPMT SMP NOPTI
CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
not done earlier.
Cc: Andreas Herrmann <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Chen Yu <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Radu Rendec <[email protected]>
Cc: Pierre Gondois <[email protected]>
Cc: Pu Wen <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Srinivas Pandruvada <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Zhang Rui <[email protected]>
Cc: [email protected]
Cc: [email protected]
Acked-by: Len Brown <[email protected]>
Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
Signed-off-by: Ricardo Neri <[email protected]>
---
The motivation for commit 5944ce092b97 was to prevent a BUG splat in
PREEMPT_RT kernels during memory allocation. This splat is not observed on
x86 because the memory allocation for cacheinfo happens in
detect_cache_attributes() from the cacheinfo CPU hotplug callback.
The dereference of a NULL pointer is not observed today because
cache_leaves(cpu) is zero until after init_cache_level() is called (also
during the CPU hotplug callback). Patch2 will set it earlier and the NULL-
pointer dereference will be observed.
---
Changes since v2:
* Introduced this patch.
Changes since v1:
* N/A
---
drivers/base/cacheinfo.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index cbae8be1fe52..461a77ece4b0 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -554,7 +554,11 @@ static inline int init_level_allocate_ci(unsigned int cpu)
*/
ci_cacheinfo(cpu)->early_ci_levels = false;
- if (cache_leaves(cpu) <= early_leaves)
+ /*
+ * Some architectures (e.g., x86) do not use early initialization.
+ * Allocate memory now in such case.
+ */
+ if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
return 0;
kfree(per_cpu_cacheinfo(cpu));
--
2.25.1
On Fri, 2023-08-04 at 18:24 -0700, Ricardo Neri wrote:
> Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> adds functionality that architectures can use to optionally allocate and
> build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> arch specific early level initializer") lets secondary CPUs correct (and
> reallocate memory) cacheinfo data if needed.
>
> If the early build functionality is not used and cacheinfo does not need
> correction, memory for cacheinfo is never allocated. x86 does not use the
> early build functionality. Consequently, during the cacheinfo CPU hotplug
> callback, last_level_cache_is_valid() attempts to dereference a NULL
> pointer:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000100
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not present page
> PGD 0 P4D 0
> Oops: 0000 [#1] PREEPMT SMP NOPTI
> CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
> RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
>
> Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> not done earlier.
>
> Cc: Andreas Herrmann <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Chen Yu <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Radu Rendec <[email protected]>
> Cc: Pierre Gondois <[email protected]>
> Cc: Pu Wen <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Srinivas Pandruvada <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Zhang Rui <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Acked-by: Len Brown <[email protected]>
> Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
> Signed-off-by: Ricardo Neri <[email protected]>
> ---
> The motivation for commit 5944ce092b97 was to prevent a BUG splat in
> PREEMPT_RT kernels during memory allocation. This splat is not observed on
> x86 because the memory allocation for cacheinfo happens in
> detect_cache_attributes() from the cacheinfo CPU hotplug callback.
>
> The dereference of a NULL pointer is not observed today because
> cache_leaves(cpu) is zero until after init_cache_level() is called (also
> during the CPU hotplug callback). Patch2 will set it earlier and the NULL-
> pointer dereference will be observed.
> ---
> Changes since v2:
> * Introduced this patch.
>
> Changes since v1:
> * N/A
> ---
> drivers/base/cacheinfo.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index cbae8be1fe52..461a77ece4b0 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -554,7 +554,11 @@ static inline int init_level_allocate_ci(unsigned int cpu)
> */
> ci_cacheinfo(cpu)->early_ci_levels = false;
>
> - if (cache_leaves(cpu) <= early_leaves)
> + /*
> + * Some architectures (e.g., x86) do not use early initialization.
> + * Allocate memory now in such case.
> + */
> + if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
> return 0;
>
> kfree(per_cpu_cacheinfo(cpu));
For this patch only:
Reviewed-by: Radu Rendec <[email protected]>
Thanks for submitting!
Best regards,
Radu
On Sat, Aug 05, 2023 at 10:28:30AM -0400, Radu Rendec wrote:
> On Fri, 2023-08-04 at 18:24 -0700, Ricardo Neri wrote:
> > Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> > adds functionality that architectures can use to optionally allocate and
> > build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> > arch specific early level initializer") lets secondary CPUs correct (and
> > reallocate memory) cacheinfo data if needed.
> >
> > If the early build functionality is not used and cacheinfo does not need
> > correction, memory for cacheinfo is never allocated. x86 does not use the
> > early build functionality. Consequently, during the cacheinfo CPU hotplug
> > callback, last_level_cache_is_valid() attempts to dereference a NULL
> > pointer:
> >
> > ???? BUG: kernel NULL pointer dereference, address: 0000000000000100
> > ???? #PF: supervisor read access in kernel mode
> > ???? #PF: error_code(0x0000) - not present page
> > ???? PGD 0 P4D 0
> > ???? Oops: 0000 [#1] PREEPMT SMP NOPTI
> > ???? CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
> > ???? RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
> >
> > Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> > not done earlier.
> >
> > Cc: Andreas Herrmann <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Chen Yu <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Cc: Radu Rendec <[email protected]>
> > Cc: Pierre Gondois <[email protected]>
> > Cc: Pu Wen <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Sudeep Holla <[email protected]>
> > Cc: Srinivas Pandruvada <[email protected]>
> > Cc: Will Deacon <[email protected]>
> > Cc: Zhang Rui <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Acked-by: Len Brown <[email protected]>
> > Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
> > Signed-off-by: Ricardo Neri <[email protected]>
> > ---
> > The motivation for commit 5944ce092b97 was to prevent a BUG splat in
> > PREEMPT_RT kernels during memory allocation. This splat is not observed on
> > x86 because the memory allocation for cacheinfo happens in
> > detect_cache_attributes() from the cacheinfo CPU hotplug callback.
> >
> > The dereference of a NULL pointer is not observed today because
> > cache_leaves(cpu) is zero until after init_cache_level() is called (also
> > during the CPU hotplug callback). Patch2 will set it earlier and the NULL-
> > pointer dereference will be observed.
> > ---
> > Changes since v2:
> > ?* Introduced this patch.
> >
> > Changes since v1:
> > ?* N/A
> > ---
> > ?drivers/base/cacheinfo.c | 6 +++++-
> > ?1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> > index cbae8be1fe52..461a77ece4b0 100644
> > --- a/drivers/base/cacheinfo.c
> > +++ b/drivers/base/cacheinfo.c
> > @@ -554,7 +554,11 @@ static inline int init_level_allocate_ci(unsigned int cpu)
> > ???????? */
> > ????????ci_cacheinfo(cpu)->early_ci_levels = false;
> > ?
> > -???????if (cache_leaves(cpu) <= early_leaves)
> > +???????/*
> > +??????? * Some architectures (e.g., x86) do not use early initialization.
> > +??????? * Allocate memory now in such case.
> > +??????? */
> > +???????if (cache_leaves(cpu) <= early_leaves && per_cpu_cacheinfo(cpu))
> > ????????????????return 0;
> > ?
> > ????????kfree(per_cpu_cacheinfo(cpu));
>
> For this patch only:
>
> Reviewed-by: Radu Rendec <[email protected]>
>
> Thanks for submitting!
Thank you!
On Wed, Aug 30, 2023 at 08:13:09AM -0400, Radu Rendec wrote:
> On Wed, 2023-08-30 at 12:49 +0100, Sudeep Holla wrote:
> > On Fri, Aug 04, 2023 at 06:24:19PM -0700, Ricardo Neri wrote:
> > > Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> > > adds functionality that architectures can use to optionally allocate and
> > > build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> > > arch specific early level initializer") lets secondary CPUs correct (and
> > > reallocate memory) cacheinfo data if needed.
> > >
> > > If the early build functionality is not used and cacheinfo does not need
> > > correction, memory for cacheinfo is never allocated. x86 does not use the
> > > early build functionality. Consequently, during the cacheinfo CPU hotplug
> > > callback, last_level_cache_is_valid() attempts to dereference a NULL
> > > pointer:
> > >
> > > ???? BUG: kernel NULL pointer dereference, address: 0000000000000100
> > > ???? #PF: supervisor read access in kernel mode
> > > ???? #PF: error_code(0x0000) - not present page
> > > ???? PGD 0 P4D 0
> > > ???? Oops: 0000 [#1] PREEPMT SMP NOPTI
> > > ???? CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
> > > ???? RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
> > >
> > > Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> > > not done earlier.
> > >
> > > Cc: Andreas Herrmann <[email protected]>
> > > Cc: Catalin Marinas <[email protected]>
> > > Cc: Chen Yu <[email protected]>
> > > Cc: Len Brown <[email protected]>
> > > Cc: Radu Rendec <[email protected]>
> > > Cc: Pierre Gondois <[email protected]>
> > > Cc: Pu Wen <[email protected]>
> > > Cc: "Rafael J. Wysocki" <[email protected]>
> > > Cc: Sudeep Holla <[email protected]>
> > > Cc: Srinivas Pandruvada <[email protected]>
> > > Cc: Will Deacon <[email protected]>
> > > Cc: Zhang Rui <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Acked-by: Len Brown <[email protected]>
> > > Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
> >
> > Not sure if we strictly need this(details below), but I am fine either way.
> >
> > > Signed-off-by: Ricardo Neri <[email protected]>
> > > ---
> > > The motivation for commit 5944ce092b97 was to prevent a BUG splat in
> > > PREEMPT_RT kernels during memory allocation. This splat is not observed on
> > > x86 because the memory allocation for cacheinfo happens in
> > > detect_cache_attributes() from the cacheinfo CPU hotplug callback.
> > >
> > > The dereference of a NULL pointer is not observed today because
> > > cache_leaves(cpu) is zero until after init_cache_level() is called (also
> > > during the CPU hotplug callback). Patch2 will set it earlier and the NULL-
> > > pointer dereference will be observed.
> >
> > Right, this is the information I have been asking in the previous versions.
> > This clarifies a lot. The trigger is in the patch 2/3 which is why it didn't
> > make complete sense to me without it when you posted this patch independently.
> > Thanks for posting it together and sorry for the delay(both reviewing this
> > and in understanding the issue).
> >
> > Given the trigger for NULL pointer dereference is in 2/3, I am not sure
> > if it is really worth applying this to all the stable kernels with the
> > commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU").
> > That is the reason why I asked to drop fixes tag if you agree with me.
> > It is simple fix, so I am OK if you prefer to see that in the stable kernels
> > as well.
>
> Thanks for reviewing, Sudeep. Since my previous commit 6539cffa9495
> ("cacheinfo: Add arch specific early level initializer") opens a door
> for the NULL pointer dereference, I would sleep better at night if the
> fix was included in the stable kernels :) But seriously, I am concerned
> that with the fix applied in mainline and not in stable, something else
> could be backported to the stable in the future, that could trigger the
> NULL pointer dereference there. Ricardo's patch 2/3 is one way to
> trigger it, but you never know what other patch lands in mainline in
> the future that assumes it's safe to set the cache leaves earlier.
>
Fair enough. I agree with you, so please retain the fixes tag as is.
Please work with x86 maintainers to get it merged along with other patches.
Let me know if you have other plans.
--
Regards,
Sudeep
On Fri, Aug 04, 2023 at 06:24:19PM -0700, Ricardo Neri wrote:
> Commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU")
> adds functionality that architectures can use to optionally allocate and
> build cacheinfo early during boot. Commit 6539cffa9495 ("cacheinfo: Add
> arch specific early level initializer") lets secondary CPUs correct (and
> reallocate memory) cacheinfo data if needed.
>
> If the early build functionality is not used and cacheinfo does not need
> correction, memory for cacheinfo is never allocated. x86 does not use the
> early build functionality. Consequently, during the cacheinfo CPU hotplug
> callback, last_level_cache_is_valid() attempts to dereference a NULL
> pointer:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000100
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not present page
> PGD 0 P4D 0
> Oops: 0000 [#1] PREEPMT SMP NOPTI
> CPU: 0 PID 19 Comm: cpuhp/0 Not tainted 6.4.0-rc2 #1
> RIP: 0010: last_level_cache_is_valid+0x95/0xe0a
>
> Allocate memory for cacheinfo during the cacheinfo CPU hotplug callback if
> not done earlier.
>
> Cc: Andreas Herrmann <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Chen Yu <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Radu Rendec <[email protected]>
> Cc: Pierre Gondois <[email protected]>
> Cc: Pu Wen <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Srinivas Pandruvada <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Zhang Rui <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Acked-by: Len Brown <[email protected]>
> Fixes: 6539cffa9495 ("cacheinfo: Add arch specific early level initializer")
Not sure if we strictly need this(details below), but I am fine either way.
> Signed-off-by: Ricardo Neri <[email protected]>
> ---
> The motivation for commit 5944ce092b97 was to prevent a BUG splat in
> PREEMPT_RT kernels during memory allocation. This splat is not observed on
> x86 because the memory allocation for cacheinfo happens in
> detect_cache_attributes() from the cacheinfo CPU hotplug callback.
>
> The dereference of a NULL pointer is not observed today because
> cache_leaves(cpu) is zero until after init_cache_level() is called (also
> during the CPU hotplug callback). Patch2 will set it earlier and the NULL-
> pointer dereference will be observed.
Right, this is the information I have been asking in the previous versions.
This clarifies a lot. The trigger is in the patch 2/3 which is why it didn't
make complete sense to me without it when you posted this patch independently.
Thanks for posting it together and sorry for the delay(both reviewing this
and in understanding the issue).
Given the trigger for NULL pointer dereference is in 2/3, I am not sure
if it is really worth applying this to all the stable kernels with the
commit 5944ce092b97 ("arch_topology: Build cacheinfo from primary CPU").
That is the reason why I asked to drop fixes tag if you agree with me.
It is simple fix, so I am OK if you prefer to see that in the stable kernels
as well.
Since there are x86 changes and patch 2/3 triggers NULL pointer dereference
without this patch, I prefer you route all 3 via x86. So,
Reviewed-by: Sudeep Holla <[email protected]>
--
Regards,
Sudeep