| Message ID | 20171019194133.27485-1-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0b167f11085a34281349f395d200052b61a7e221 |
| Headers | show |
| Series | powerpc/perf: Fix IMC allocation routine | expand |
Hi Guilherme, Nice catch. On Friday 20 October 2017 01:11 AM, Guilherme G. Piccoli wrote: > When setting nr_cpus=1, we observed a crash in IMC code during boot > due to a missing allocation: basically, IMC code is taking the number > of threads into account in imc_mem_init() and if we manually set > nr_cpus for a value that is not multiple of the number of threads per > core, an integer division in that function will discard the decimal > portion, leading IMC to not allocate one mem_info struct. This causes > a NULL pointer dereference later, on is_core_imc_mem_inited(). > > This patch just rounds that division up, fixing the bug. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> Acked-by: Anju T Sudhakar <anju@linux.vnet.ibm.com> > --- > Anju, looks good to you? Tested in P9 with latest FW available. > > arch/powerpc/perf/imc-pmu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c > index 88126245881b..92ae5de0bbac 100644 > --- a/arch/powerpc/perf/imc-pmu.c > +++ b/arch/powerpc/perf/imc-pmu.c > @@ -1104,7 +1104,7 @@ static int init_nest_pmu_ref(void) > > static void cleanup_all_core_imc_memory(void) > { > - int i, nr_cores = num_present_cpus() / threads_per_core; > + int i, nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); > struct imc_mem_info *ptr = core_imc_pmu->mem_info; > int size = core_imc_pmu->counter_mem_size; > > @@ -1212,7 +1212,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent, > if (!pmu_ptr->pmu.name) > return -ENOMEM; > > - nr_cores = num_present_cpus() / threads_per_core; > + nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); > pmu_ptr->mem_info = kcalloc(nr_cores, sizeof(struct imc_mem_info), > GFP_KERNEL); > Thanks, Anju <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p>Hi Guilherme,</p> Nice catch.<br> <br> <div class="moz-cite-prefix">On Friday 20 October 2017 01:11 AM, Guilherme G. Piccoli wrote:<br> </div> <blockquote type="cite" cite="mid:20171019194133.27485-1-gpiccoli@linux.vnet.ibm.com"> <pre wrap="">When setting nr_cpus=1, we observed a crash in IMC code during boot due to a missing allocation: basically, IMC code is taking the number of threads into account in imc_mem_init() and if we manually set nr_cpus for a value that is not multiple of the number of threads per core, an integer division in that function will discard the decimal portion, leading IMC to not allocate one mem_info struct. This causes a NULL pointer dereference later, on is_core_imc_mem_inited(). This patch just rounds that division up, fixing the bug. Signed-off-by: Guilherme G. Piccoli <a class="moz-txt-link-rfc2396E" href="mailto:gpiccoli@linux.vnet.ibm.com"><gpiccoli@linux.vnet.ibm.com></a></pre> </blockquote> Acked-by: Anju T Sudhakar <a class="moz-txt-link-rfc2396E" href="mailto:anju@linux.vnet.ibm.com"><anju@linux.vnet.ibm.com></a> <blockquote type="cite" cite="mid:20171019194133.27485-1-gpiccoli@linux.vnet.ibm.com"> <pre wrap=""> --- Anju, looks good to you? Tested in P9 with latest FW available. arch/powerpc/perf/imc-pmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c index 88126245881b..92ae5de0bbac 100644 --- a/arch/powerpc/perf/imc-pmu.c +++ b/arch/powerpc/perf/imc-pmu.c @@ -1104,7 +1104,7 @@ static int init_nest_pmu_ref(void) static void cleanup_all_core_imc_memory(void) { - int i, nr_cores = num_present_cpus() / threads_per_core; + int i, nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); struct imc_mem_info *ptr = core_imc_pmu->mem_info; int size = core_imc_pmu->counter_mem_size; @@ -1212,7 +1212,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent, if (!pmu_ptr->pmu.name) return -ENOMEM; - nr_cores = num_present_cpus() / threads_per_core; + nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); pmu_ptr->mem_info = kcalloc(nr_cores, sizeof(struct imc_mem_info), GFP_KERNEL); </pre> </blockquote> <br> Thanks,<br> Anju<br> </body> </html>
On 10/24/2017 06:34 AM, Anju T Sudhakar wrote: > Hi Guilherme, > > Nice catch. > > On Friday 20 October 2017 01:11 AM, Guilherme G. Piccoli wrote: >> When setting nr_cpus=1, we observed a crash in IMC code during boot >> due to a missing allocation: basically, IMC code is taking the number >> of threads into account in imc_mem_init() and if we manually set >> nr_cpus for a value that is not multiple of the number of threads per >> core, an integer division in that function will discard the decimal >> portion, leading IMC to not allocate one mem_info struct. This causes >> a NULL pointer dereference later, on is_core_imc_mem_inited(). >> >> This patch just rounds that division up, fixing the bug. >> >> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > Acked-by: Anju T Sudhakar <anju@linux.vnet.ibm.com> Thanks Anju :) >> --- >> Anju, looks good to you? Tested in P9 with latest FW available. >> >> arch/powerpc/perf/imc-pmu.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c >> index 88126245881b..92ae5de0bbac 100644 >> --- a/arch/powerpc/perf/imc-pmu.c >> +++ b/arch/powerpc/perf/imc-pmu.c >> @@ -1104,7 +1104,7 @@ static int init_nest_pmu_ref(void) >> >> static void cleanup_all_core_imc_memory(void) >> { >> - int i, nr_cores = num_present_cpus() / threads_per_core; >> + int i, nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); >> struct imc_mem_info *ptr = core_imc_pmu->mem_info; >> int size = core_imc_pmu->counter_mem_size; >> >> @@ -1212,7 +1212,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent, >> if (!pmu_ptr->pmu.name) >> return -ENOMEM; >> >> - nr_cores = num_present_cpus() / threads_per_core; >> + nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); >> pmu_ptr->mem_info = kcalloc(nr_cores, sizeof(struct imc_mem_info), >> GFP_KERNEL); >> > > Thanks, > Anju
On Thu, 2017-10-19 at 19:41:33 UTC, "Guilherme G. Piccoli" wrote: > When setting nr_cpus=1, we observed a crash in IMC code during boot > due to a missing allocation: basically, IMC code is taking the number > of threads into account in imc_mem_init() and if we manually set > nr_cpus for a value that is not multiple of the number of threads per > core, an integer division in that function will discard the decimal > portion, leading IMC to not allocate one mem_info struct. This causes > a NULL pointer dereference later, on is_core_imc_mem_inited(). > > This patch just rounds that division up, fixing the bug. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/0b167f11085a34281349f395d20005 cheers
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c index 88126245881b..92ae5de0bbac 100644 --- a/arch/powerpc/perf/imc-pmu.c +++ b/arch/powerpc/perf/imc-pmu.c @@ -1104,7 +1104,7 @@ static int init_nest_pmu_ref(void) static void cleanup_all_core_imc_memory(void) { - int i, nr_cores = num_present_cpus() / threads_per_core; + int i, nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); struct imc_mem_info *ptr = core_imc_pmu->mem_info; int size = core_imc_pmu->counter_mem_size; @@ -1212,7 +1212,7 @@ static int imc_mem_init(struct imc_pmu *pmu_ptr, struct device_node *parent, if (!pmu_ptr->pmu.name) return -ENOMEM; - nr_cores = num_present_cpus() / threads_per_core; + nr_cores = DIV_ROUND_UP(num_present_cpus(), threads_per_core); pmu_ptr->mem_info = kcalloc(nr_cores, sizeof(struct imc_mem_info), GFP_KERNEL);
When setting nr_cpus=1, we observed a crash in IMC code during boot due to a missing allocation: basically, IMC code is taking the number of threads into account in imc_mem_init() and if we manually set nr_cpus for a value that is not multiple of the number of threads per core, an integer division in that function will discard the decimal portion, leading IMC to not allocate one mem_info struct. This causes a NULL pointer dereference later, on is_core_imc_mem_inited(). This patch just rounds that division up, fixing the bug. Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- Anju, looks good to you? Tested in P9 with latest FW available. arch/powerpc/perf/imc-pmu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)