* [bug report] powerpc/perf: Add nest IMC PMU support
@ 2018-10-18 9:33 Dan Carpenter
2018-10-24 7:06 ` Anju T Sudhakar
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2018-10-18 9:33 UTC (permalink / raw)
To: anju; +Cc: linuxppc-dev
Hello Anju T Sudhakar,
The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
Jul 19, 2017, leads to the following static checker warning:
arch/powerpc/perf/imc-pmu.c:506 nest_imc_event_init()
warn: 'pcni' can't be NULL.
arch/powerpc/perf/imc-pmu.c
485 if (event->cpu < 0)
486 return -EINVAL;
487
488 pmu = imc_event_to_pmu(event);
489
490 /* Sanity check for config (event offset) */
491 if ((config & IMC_EVENT_OFFSET_MASK) > pmu->counter_mem_size)
492 return -EINVAL;
493
494 /*
495 * Nest HW counter memory resides in a per-chip reserve-memory (HOMER).
496 * Get the base memory addresss for this cpu.
497 */
498 chip_id = cpu_to_chip_id(event->cpu);
499 pcni = pmu->mem_info;
^^^^^^^^^^^^^^^^^^^^
500 do {
501 if (pcni->id == chip_id) {
502 flag = true;
503 break;
504 }
505 pcni++;
^^^^^^
506 } while (pcni);
^^^^
This will loop until we crash. I'm not sure what was intended.
507
508 if (!flag)
509 return -ENODEV;
510
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] powerpc/perf: Add nest IMC PMU support
2018-10-18 9:33 [bug report] powerpc/perf: Add nest IMC PMU support Dan Carpenter
@ 2018-10-24 7:06 ` Anju T Sudhakar
0 siblings, 0 replies; 5+ messages in thread
From: Anju T Sudhakar @ 2018-10-24 7:06 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linuxppc-dev
Hi,
On 10/18/18 3:03 PM, Dan Carpenter wrote:
> Hello Anju T Sudhakar,
>
> The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
> Jul 19, 2017, leads to the following static checker warning:
>
> arch/powerpc/perf/imc-pmu.c:506 nest_imc_event_init()
> warn: 'pcni' can't be NULL.
Unfortunately this warning didn't appear when I checked with smatch.
Could you please provide the steps to reproduce this?
This is the commit id with which I build smatch: commit
79fe36620a7a3a45d1a51d62238da250fb8db920
But anyway I am looking into the code part. Thanks for mentioning this.
I will update soon.
Thanks,
Anju
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] powerpc/perf: Add nest IMC PMU support
@ 2018-01-31 15:25 Dan Carpenter
2018-02-01 11:27 ` Madhavan Srinivasan
2018-02-08 6:31 ` Anju T Sudhakar
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2018-01-31 15:25 UTC (permalink / raw)
To: Anju T Sudhakar; +Cc: linuxppc-dev
Hello Anju T Sudhakar,
The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
Jul 19, 2017, leads to the following static checker warning:
arch/powerpc/perf/imc-pmu.c:1393 init_imc_pmu()
warn: 'pmu_ptr' was already freed.
arch/powerpc/perf/imc-pmu.c
1317 int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_idx)
1318 {
1319 int ret;
1320
1321 ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
1322 if (ret) {
1323 imc_common_mem_free(pmu_ptr);
1324 return ret;
1325 }
Change this to:
if (ret)
goto err_free_mpu_ptr;
Or something instead of a direct return. That's more normal kernel
style.
1326
1327 switch (pmu_ptr->domain) {
1328 case IMC_DOMAIN_NEST:
1329 /*
1330 * Nest imc pmu need only one cpu per chip, we initialize the
1331 * cpumask for the first nest imc pmu and use the same for the
1332 * rest. To handle the cpuhotplug callback unregister, we track
1333 * the number of nest pmus in "nest_pmus".
1334 */
1335 mutex_lock(&nest_init_lock);
1336 if (nest_pmus == 0) {
1337 ret = init_nest_pmu_ref();
1338 if (ret) {
1339 mutex_unlock(&nest_init_lock);
1340 goto err_free;
1341 }
1342 /* Register for cpu hotplug notification. */
1343 ret = nest_pmu_cpumask_init();
1344 if (ret) {
1345 mutex_unlock(&nest_init_lock);
1346 kfree(nest_imc_refc);
1347 kfree(per_nest_pmu_arr);
1348 goto err_free;
1349 }
1350 }
1351 nest_pmus++;
1352 mutex_unlock(&nest_init_lock);
1353 break;
1354 case IMC_DOMAIN_CORE:
1355 ret = core_imc_pmu_cpumask_init();
1356 if (ret) {
1357 cleanup_all_core_imc_memory();
1358 return ret;
These direct returns don't look correct...
1359 }
1360
1361 break;
1362 case IMC_DOMAIN_THREAD:
1363 ret = thread_imc_cpu_init();
1364 if (ret) {
1365 cleanup_all_thread_imc_memory();
1366 return ret;
1367 }
1368
1369 break;
1370 default:
1371 return -1; /* Unknown domain */
This one certainly looks like a memory leak. Plus -1 is -EPERM which is
probably not the correct error code.
1372 }
1373
1374 ret = update_events_in_group(parent, pmu_ptr);
1375 if (ret)
1376 goto err_free;
1377
1378 ret = update_pmu_ops(pmu_ptr);
1379 if (ret)
1380 goto err_free;
1381
1382 ret = perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
1383 if (ret)
1384 goto err_free;
1385
1386 pr_info("%s performance monitor hardware support registered\n",
1387 pmu_ptr->pmu.name);
1388
1389 return 0;
1390
1391 err_free:
1392 imc_common_mem_free(pmu_ptr);
1393 imc_common_cpuhp_mem_free(pmu_ptr);
^^^^^^^
This is a use after free, it should be in the reverse order.
err_free_cpuhp:
imc_common_cpuhp_mem_free(pmu_ptr);
err_free_pmu_ptr:
imc_common_mem_free(pmu_ptr);
1394 return ret;
1395 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] powerpc/perf: Add nest IMC PMU support
2018-01-31 15:25 Dan Carpenter
@ 2018-02-01 11:27 ` Madhavan Srinivasan
2018-02-08 6:31 ` Anju T Sudhakar
1 sibling, 0 replies; 5+ messages in thread
From: Madhavan Srinivasan @ 2018-02-01 11:27 UTC (permalink / raw)
To: linuxppc-dev
On Wednesday 31 January 2018 08:55 PM, Dan Carpenter wrote:
> Hello Anju T Sudhakar,
>
> The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
> Jul 19, 2017, leads to the following static checker warning:
>
> arch/powerpc/perf/imc-pmu.c:1393 init_imc_pmu()
> warn: 'pmu_ptr' was already freed.
>
> arch/powerpc/perf/imc-pmu.c
> 1317 int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_idx)
> 1318 {
> 1319 int ret;
> 1320
> 1321 ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
> 1322 if (ret) {
> 1323 imc_common_mem_free(pmu_ptr);
> 1324 return ret;
> 1325 }
>
> Change this to:
>
> if (ret)
> goto err_free_mpu_ptr;
>
> Or something instead of a direct return. That's more normal kernel
> style.
>
> 1326
> 1327 switch (pmu_ptr->domain) {
> 1328 case IMC_DOMAIN_NEST:
> 1329 /*
> 1330 * Nest imc pmu need only one cpu per chip, we initialize the
> 1331 * cpumask for the first nest imc pmu and use the same for the
> 1332 * rest. To handle the cpuhotplug callback unregister, we track
> 1333 * the number of nest pmus in "nest_pmus".
> 1334 */
> 1335 mutex_lock(&nest_init_lock);
> 1336 if (nest_pmus == 0) {
> 1337 ret = init_nest_pmu_ref();
> 1338 if (ret) {
> 1339 mutex_unlock(&nest_init_lock);
> 1340 goto err_free;
> 1341 }
> 1342 /* Register for cpu hotplug notification. */
> 1343 ret = nest_pmu_cpumask_init();
> 1344 if (ret) {
> 1345 mutex_unlock(&nest_init_lock);
> 1346 kfree(nest_imc_refc);
> 1347 kfree(per_nest_pmu_arr);
> 1348 goto err_free;
> 1349 }
> 1350 }
> 1351 nest_pmus++;
> 1352 mutex_unlock(&nest_init_lock);
> 1353 break;
> 1354 case IMC_DOMAIN_CORE:
> 1355 ret = core_imc_pmu_cpumask_init();
> 1356 if (ret) {
> 1357 cleanup_all_core_imc_memory();
> 1358 return ret;
>
> These direct returns don't look correct...
>
> 1359 }
> 1360
> 1361 break;
> 1362 case IMC_DOMAIN_THREAD:
> 1363 ret = thread_imc_cpu_init();
> 1364 if (ret) {
> 1365 cleanup_all_thread_imc_memory();
> 1366 return ret;
> 1367 }
> 1368
> 1369 break;
> 1370 default:
> 1371 return -1; /* Unknown domain */
>
> This one certainly looks like a memory leak. Plus -1 is -EPERM which is
> probably not the correct error code.
>
>
> 1372 }
> 1373
> 1374 ret = update_events_in_group(parent, pmu_ptr);
> 1375 if (ret)
> 1376 goto err_free;
> 1377
> 1378 ret = update_pmu_ops(pmu_ptr);
> 1379 if (ret)
> 1380 goto err_free;
> 1381
> 1382 ret = perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
> 1383 if (ret)
> 1384 goto err_free;
> 1385
> 1386 pr_info("%s performance monitor hardware support registered\n",
> 1387 pmu_ptr->pmu.name);
> 1388
> 1389 return 0;
> 1390
> 1391 err_free:
> 1392 imc_common_mem_free(pmu_ptr);
> 1393 imc_common_cpuhp_mem_free(pmu_ptr);
>
Yes, this doesn't looks right. Recent patch had re-factored this code.
ed8e443feee2b ('powerpc/perf: IMC code cleanup with some code
refactoring')
My bad. Should have looked at it more closely. I will look at this and
will rework it.
Thanks for reviewing.
Maddy
> ^^^^^^^
> This is a use after free, it should be in the reverse order.
>
> err_free_cpuhp:
> imc_common_cpuhp_mem_free(pmu_ptr);
> err_free_pmu_ptr:
> imc_common_mem_free(pmu_ptr);
>
> 1394 return ret;
> 1395 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] powerpc/perf: Add nest IMC PMU support
2018-01-31 15:25 Dan Carpenter
2018-02-01 11:27 ` Madhavan Srinivasan
@ 2018-02-08 6:31 ` Anju T Sudhakar
1 sibling, 0 replies; 5+ messages in thread
From: Anju T Sudhakar @ 2018-02-08 6:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linuxppc-dev
Hi Dan Carpenter,
On Wednesday 31 January 2018 08:55 PM, Dan Carpenter wrote:
> Hello Anju T Sudhakar,
>
> The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
> Jul 19, 2017, leads to the following static checker warning:
>
> arch/powerpc/perf/imc-pmu.c:1393 init_imc_pmu()
> warn: 'pmu_ptr' was already freed.
>
> arch/powerpc/perf/imc-pmu.c
> 1317 int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_idx)
> 1318 {
> 1319 int ret;
> 1320
> 1321 ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
> 1322 if (ret) {
> 1323 imc_common_mem_free(pmu_ptr);
> 1324 return ret;
> 1325 }
>
> Change this to:
>
> if (ret)
> goto err_free_mpu_ptr;
>
> Or something instead of a direct return. That's more normal kernel
> style.
>
> 1326
> 1327 switch (pmu_ptr->domain) {
> 1328 case IMC_DOMAIN_NEST:
> 1329 /*
> 1330 * Nest imc pmu need only one cpu per chip, we initialize the
> 1331 * cpumask for the first nest imc pmu and use the same for the
> 1332 * rest. To handle the cpuhotplug callback unregister, we track
> 1333 * the number of nest pmus in "nest_pmus".
> 1334 */
> 1335 mutex_lock(&nest_init_lock);
> 1336 if (nest_pmus == 0) {
> 1337 ret = init_nest_pmu_ref();
> 1338 if (ret) {
> 1339 mutex_unlock(&nest_init_lock);
> 1340 goto err_free;
> 1341 }
> 1342 /* Register for cpu hotplug notification. */
> 1343 ret = nest_pmu_cpumask_init();
> 1344 if (ret) {
> 1345 mutex_unlock(&nest_init_lock);
> 1346 kfree(nest_imc_refc);
> 1347 kfree(per_nest_pmu_arr);
> 1348 goto err_free;
> 1349 }
> 1350 }
> 1351 nest_pmus++;
> 1352 mutex_unlock(&nest_init_lock);
> 1353 break;
> 1354 case IMC_DOMAIN_CORE:
> 1355 ret = core_imc_pmu_cpumask_init();
> 1356 if (ret) {
> 1357 cleanup_all_core_imc_memory();
> 1358 return ret;
>
> These direct returns don't look correct...
>
> 1359 }
> 1360
> 1361 break;
> 1362 case IMC_DOMAIN_THREAD:
> 1363 ret = thread_imc_cpu_init();
> 1364 if (ret) {
> 1365 cleanup_all_thread_imc_memory();
> 1366 return ret;
> 1367 }
> 1368
> 1369 break;
> 1370 default:
> 1371 return -1; /* Unknown domain */
>
> This one certainly looks like a memory leak. Plus -1 is -EPERM which is
> probably not the correct error code.
>
>
> 1372 }
> 1373
> 1374 ret = update_events_in_group(parent, pmu_ptr);
> 1375 if (ret)
> 1376 goto err_free;
> 1377
> 1378 ret = update_pmu_ops(pmu_ptr);
> 1379 if (ret)
> 1380 goto err_free;
> 1381
> 1382 ret = perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
> 1383 if (ret)
> 1384 goto err_free;
> 1385
> 1386 pr_info("%s performance monitor hardware support registered\n",
> 1387 pmu_ptr->pmu.name);
> 1388
> 1389 return 0;
> 1390
> 1391 err_free:
> 1392 imc_common_mem_free(pmu_ptr);
> 1393 imc_common_cpuhp_mem_free(pmu_ptr);
> ^^^^^^^
> This is a use after free, it should be in the reverse order.
>
> err_free_cpuhp:
> imc_common_cpuhp_mem_free(pmu_ptr);
> err_free_pmu_ptr:
> imc_common_mem_free(pmu_ptr);
>
> 1394 return ret;
> 1395 }
>
> regards,
> dan carpenter
>
Apologies for the delayed response, I just got back from vacation.
Thank you for pointing out this, I will rework the code and send.
Regards,
Anju
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-24 7:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-18 9:33 [bug report] powerpc/perf: Add nest IMC PMU support Dan Carpenter
2018-10-24 7:06 ` Anju T Sudhakar
-- strict thread matches above, loose matches on Subject: below --
2018-01-31 15:25 Dan Carpenter
2018-02-01 11:27 ` Madhavan Srinivasan
2018-02-08 6:31 ` Anju T Sudhakar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.