The cacheinfo sets up the shared_cpu_map by checking whether the caches
with the same index are shared between CPUs. However, this will trigger
slab-out-of-bounds access if the CPUs do not have the same cache hierarchy.
Another problem is the mismatched shared_cpu_map when the shared cache does
not have the same index between CPUs.
CPU0 I D L3
index 0 1 2 x
^ ^ ^ ^
index 0 1 2 3
CPU1 I D L2 L3
This patch checks each cache is shared with all caches on other CPUs.
Signed-off-by: Yong-Xuan Wang <[email protected]>
---
drivers/base/cacheinfo.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index 950b22cdb5f7..d38f80f6fff1 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -256,7 +256,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
{
struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
struct cacheinfo *this_leaf, *sib_leaf;
- unsigned int index;
+ unsigned int index, sib_index;
int ret = 0;
if (this_cpu_ci->cpu_map_populated)
@@ -284,11 +284,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
if (i == cpu || !sib_cpu_ci->info_list)
continue;/* skip if itself or no cacheinfo */
-
- sib_leaf = per_cpu_cacheinfo_idx(i, index);
- if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
- cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
- cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
+ for (sib_index = 0; sib_index < cache_leaves(i); sib_index++) {
+ sib_leaf = per_cpu_cacheinfo_idx(i, sib_index);;
+ if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
+ cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
+ cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
+ }
}
}
/* record the maximum cache line size */
@@ -302,7 +303,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
static void cache_shared_cpu_map_remove(unsigned int cpu)
{
struct cacheinfo *this_leaf, *sib_leaf;
- unsigned int sibling, index;
+ unsigned int sibling, index, sib_index;
for (index = 0; index < cache_leaves(cpu); index++) {
this_leaf = per_cpu_cacheinfo_idx(cpu, index);
@@ -313,9 +314,13 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
if (sibling == cpu || !sib_cpu_ci->info_list)
continue;/* skip if itself or no cacheinfo */
- sib_leaf = per_cpu_cacheinfo_idx(sibling, index);
- cpumask_clear_cpu(cpu, &sib_leaf->shared_cpu_map);
- cpumask_clear_cpu(sibling, &this_leaf->shared_cpu_map);
+ for (sib_index = 0; sib_index < cache_leaves(sibling); sib_index++) {
+ sib_leaf = per_cpu_cacheinfo_idx(sibling, sib_index);;
+ if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
+ cpumask_clear_cpu(cpu, &sib_leaf->shared_cpu_map);
+ cpumask_clear_cpu(sibling, &this_leaf->shared_cpu_map);
+ }
+ }
}
}
}
--
2.17.1
Hello Yong-Xuan,
Except for the nit below, I tried the patch and everything seemed ok, so
with that:
Reviewed-by: Pierre Gondois <[email protected]>
Regards,
Pierre
On 12/19/22 11:51, Yong-Xuan Wang wrote:
> The cacheinfo sets up the shared_cpu_map by checking whether the caches
> with the same index are shared between CPUs. However, this will trigger
> slab-out-of-bounds access if the CPUs do not have the same cache hierarchy.
> Another problem is the mismatched shared_cpu_map when the shared cache does
> not have the same index between CPUs.
>
> CPU0 I D L3
> index 0 1 2 x
> ^ ^ ^ ^
> index 0 1 2 3
> CPU1 I D L2 L3
>
> This patch checks each cache is shared with all caches on other CPUs.
>
> Signed-off-by: Yong-Xuan Wang <[email protected]>
> ---
> drivers/base/cacheinfo.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index 950b22cdb5f7..d38f80f6fff1 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -256,7 +256,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> {
> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> struct cacheinfo *this_leaf, *sib_leaf;
> - unsigned int index;
> + unsigned int index, sib_index;
> int ret = 0;
>
> if (this_cpu_ci->cpu_map_populated)
> @@ -284,11 +284,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>
> if (i == cpu || !sib_cpu_ci->info_list)
> continue;/* skip if itself or no cacheinfo */
> -
> - sib_leaf = per_cpu_cacheinfo_idx(i, index);
> - if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> - cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
> - cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
> + for (sib_index = 0; sib_index < cache_leaves(i); sib_index++) {
> + sib_leaf = per_cpu_cacheinfo_idx(i, sib_index);;
It seems there are 2 ';' above (same in the block below).
> + if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> + cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
> + cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
> + }
> }
> }
> /* record the maximum cache line size */
> @@ -302,7 +303,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> static void cache_shared_cpu_map_remove(unsigned int cpu)
> {
> struct cacheinfo *this_leaf, *sib_leaf;
> - unsigned int sibling, index;
> + unsigned int sibling, index, sib_index;
>
> for (index = 0; index < cache_leaves(cpu); index++) {
> this_leaf = per_cpu_cacheinfo_idx(cpu, index);
> @@ -313,9 +314,13 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
> if (sibling == cpu || !sib_cpu_ci->info_list)
> continue;/* skip if itself or no cacheinfo */
>
> - sib_leaf = per_cpu_cacheinfo_idx(sibling, index);
> - cpumask_clear_cpu(cpu, &sib_leaf->shared_cpu_map);
> - cpumask_clear_cpu(sibling, &this_leaf->shared_cpu_map);
> + for (sib_index = 0; sib_index < cache_leaves(sibling); sib_index++) {
> + sib_leaf = per_cpu_cacheinfo_idx(sibling, sib_index);;
> + if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> + cpumask_clear_cpu(cpu, &sib_leaf->shared_cpu_map);
> + cpumask_clear_cpu(sibling, &this_leaf->shared_cpu_map);
> + }
> + }
> }
> }
> }
Hi Yong-Xuan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on driver-core/driver-core-next driver-core/driver-core-linus staging/staging-testing staging/staging-next staging/staging-linus linus/master v6.1 next-20221220]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Yong-Xuan-Wang/drivers-base-cacheinfo-fix-shared_cpu_map/20221219-191334
patch link: https://lore.kernel.org/r/20221219105132.27690-2-yongxuan.wang%40sifive.com
patch subject: [PATCH v2 1/1] drivers: base: cacheinfo: fix shared_cpu_map
config: nios2-randconfig-c041-20221224
compiler: nios2-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
cocci warnings: (new ones prefixed by >>)
>> drivers/base/cacheinfo.c:318:57-58: Unneeded semicolon
drivers/base/cacheinfo.c:288:51-52: Unneeded semicolon
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, Dec 21, 2022 at 10:00:39AM +0100, Pierre Gondois wrote:
> Hello Yong-Xuan,
> Except for the nit below, I tried the patch and everything seemed ok, so
> with that:
> Reviewed-by: Pierre Gondois <[email protected]>
>
> Regards,
> Pierre
>
> On 12/19/22 11:51, Yong-Xuan Wang wrote:
> > The cacheinfo sets up the shared_cpu_map by checking whether the caches
> > with the same index are shared between CPUs. However, this will trigger
> > slab-out-of-bounds access if the CPUs do not have the same cache hierarchy.
> > Another problem is the mismatched shared_cpu_map when the shared cache does
> > not have the same index between CPUs.
> >
> > CPU0 I D L3
> > index 0 1 2 x
> > ^ ^ ^ ^
> > index 0 1 2 3
> > CPU1 I D L2 L3
> >
> > This patch checks each cache is shared with all caches on other CPUs.
> >
> > Signed-off-by: Yong-Xuan Wang <[email protected]>
> > ---
> > drivers/base/cacheinfo.c | 25 +++++++++++++++----------
> > 1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> > index 950b22cdb5f7..d38f80f6fff1 100644
> > --- a/drivers/base/cacheinfo.c
> > +++ b/drivers/base/cacheinfo.c
> > @@ -256,7 +256,7 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> > {
> > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> > struct cacheinfo *this_leaf, *sib_leaf;
> > - unsigned int index;
> > + unsigned int index, sib_index;
> > int ret = 0;
> > if (this_cpu_ci->cpu_map_populated)
> > @@ -284,11 +284,12 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> > if (i == cpu || !sib_cpu_ci->info_list)
> > continue;/* skip if itself or no cacheinfo */
> > -
> > - sib_leaf = per_cpu_cacheinfo_idx(i, index);
> > - if (cache_leaves_are_shared(this_leaf, sib_leaf)) {
> > - cpumask_set_cpu(cpu, &sib_leaf->shared_cpu_map);
> > - cpumask_set_cpu(i, &this_leaf->shared_cpu_map);
> > + for (sib_index = 0; sib_index < cache_leaves(i); sib_index++) {
> > + sib_leaf = per_cpu_cacheinfo_idx(i, sib_index);;
>
> It seems there are 2 ';' above (same in the block below).
Yes, the kernel test robot complains about this as well.
It needs to be fixed before this change can be accepted.
thanks,
greg k-h
Hi Greg,
On Fri, Jan 20, 2023 at 12:27:26PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 21, 2022 at 10:00:39AM +0100, Pierre Gondois wrote:
[...]
> >
> > It seems there are 2 ';' above (same in the block below).
>
> Yes, the kernel test robot complains about this as well.
>
> It needs to be fixed before this change can be accepted.
>
Just FYI, v3 and v4 was posted and I have pulled v4 which includes all
the suggested changes. I will send a pull request with all cacheinfo and
associated changes later today. They have been in the next for a while, need
to tag them and send it to you.
--
Regards,
Sudeep
On Fri, Jan 20, 2023 at 11:31:55AM +0000, Sudeep Holla wrote:
> Hi Greg,
>
> On Fri, Jan 20, 2023 at 12:27:26PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Dec 21, 2022 at 10:00:39AM +0100, Pierre Gondois wrote:
>
> [...]
>
> > >
> > > It seems there are 2 ';' above (same in the block below).
> >
> > Yes, the kernel test robot complains about this as well.
> >
> > It needs to be fixed before this change can be accepted.
> >
>
> Just FYI, v3 and v4 was posted and I have pulled v4 which includes all
> the suggested changes. I will send a pull request with all cacheinfo and
> associated changes later today. They have been in the next for a while, need
> to tag them and send it to you.
Ah, good, sorry, I missed that. Am catching up on old patches in my
queue...