Hello Rijo Thomas,
The patch 757cc3e9ff1d: "tee: add AMD-TEE driver" from Dec 27, 2019, leads to the following static checker warning:
drivers/tee/amdtee/core.c:447 amdtee_driver_init() warn: 'drv_data' isn't an ERR_PTR
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
440 int rc; 441 442 rc = psp_check_tee_status(); 443 if (rc) 444 goto err_fail; ^^^^^^^^^^^^^ Ideally psp_check_tee_status() would print its own error warning and we could return directly here.
445 446 drv_data = kzalloc(sizeof(*drv_data), GFP_KERNEL); 447 if (IS_ERR(drv_data)) ^^^^^^^^^^^^^^^^ This should check for NULL instead of error pointer.
448 return -ENOMEM; 449 450 amdtee = kzalloc(sizeof(*amdtee), GFP_KERNEL); 451 if (IS_ERR(amdtee)) { ^^^^^^^^^^^^^^ Same.
452 rc = -ENOMEM; 453 goto err_kfree_drv_data; 454 } 455 456 pool = amdtee_config_shm(); 457 if (IS_ERR(pool)) { 458 pr_err("shared pool configuration error\n"); 459 rc = PTR_ERR(pool); 460 goto err_kfree_amdtee; 461 } 462 463 teedev = tee_device_alloc(&amdtee_desc, NULL, pool, amdtee); 464 if (IS_ERR(teedev)) { 465 rc = PTR_ERR(teedev); 466 goto err;
Better to create a "err_free_pool" label. The tee_device_unregister() is a no-op on this path so that's fine, but it's easier to read if we have the err_free_pool label.
467 } 468 amdtee->teedev = teedev; 469 470 rc = tee_device_register(amdtee->teedev); 471 if (rc) 472 goto err;
"goto err;" is very vague. We are unregistering a device which was never registered which feels wrong but actually works fine...
473 474 amdtee->pool = pool; 475 476 drv_data->amdtee = amdtee; 477 478 pr_info("amd-tee driver initialization successful\n"); 479 return 0; 480 481 err: 482 tee_device_unregister(amdtee->teedev); 483 if (pool) ^^^^^^^^^ Always true.
484 tee_shm_pool_free(pool); 485 486 err_kfree_amdtee: 487 kfree(amdtee); 488 489 err_kfree_drv_data: 490 kfree(drv_data); 491 drv_data = NULL; 492 493 err_fail: 494 pr_err("amd-tee driver initialization failed\n"); 495 return rc; 496 }
regards, dan carpenter
+Herbert Xu, Deva
Hello Dan,
On 07/01/20 12:53 pm, Dan Carpenter wrote:
Hello Rijo Thomas,
The patch 757cc3e9ff1d: "tee: add AMD-TEE driver" from Dec 27, 2019, leads to the following static checker warning:
drivers/tee/amdtee/core.c:447 amdtee_driver_init() warn: 'drv_data' isn't an ERR_PTR
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
Okay, I will remove unused assignments in the driver.
440 int rc; 441 442 rc = psp_check_tee_status(); 443 if (rc) 444 goto err_fail; ^^^^^^^^^^^^^ Ideally psp_check_tee_status() would print its own error warning and we could return directly here.
Sure, I can add a warning here and return directly.
445 446 drv_data = kzalloc(sizeof(*drv_data), GFP_KERNEL); 447 if (IS_ERR(drv_data)) ^^^^^^^^^^^^^^^^ This should check for NULL instead of error pointer.
Okay I shall fix this.
448 return -ENOMEM; 449 450 amdtee = kzalloc(sizeof(*amdtee), GFP_KERNEL); 451 if (IS_ERR(amdtee)) { ^^^^^^^^^^^^^^ Same.
Okay.
452 rc = -ENOMEM; 453 goto err_kfree_drv_data; 454 } 455 456 pool = amdtee_config_shm(); 457 if (IS_ERR(pool)) { 458 pr_err("shared pool configuration error\n"); 459 rc = PTR_ERR(pool); 460 goto err_kfree_amdtee; 461 } 462 463 teedev = tee_device_alloc(&amdtee_desc, NULL, pool, amdtee); 464 if (IS_ERR(teedev)) { 465 rc = PTR_ERR(teedev); 466 goto err;
Better to create a "err_free_pool" label. The tee_device_unregister() is a no-op on this path so that's fine, but it's easier to read if we have the err_free_pool label.
Okay, I will add err_free_pool label just after tee_device_unregister.
467 } 468 amdtee->teedev = teedev; 469 470 rc = tee_device_register(amdtee->teedev); 471 if (rc) 472 goto err;
"goto err;" is very vague. We are unregistering a device which was never registered which feels wrong but actually works fine...
I shall rename err to err_device_unregister.
473 474 amdtee->pool = pool; 475 476 drv_data->amdtee = amdtee; 477 478 pr_info("amd-tee driver initialization successful\n"); 479 return 0; 480 481 err: 482 tee_device_unregister(amdtee->teedev); 483 if (pool) ^^^^^^^^^ Always true.
Agree, I shall remove this check.
484 tee_shm_pool_free(pool); 485 486 err_kfree_amdtee: 487 kfree(amdtee); 488 489 err_kfree_drv_data: 490 kfree(drv_data); 491 drv_data = NULL; 492 493 err_fail: 494 pr_err("amd-tee driver initialization failed\n"); 495 return rc; 496 }
regards, dan carpenter
I shall update the driver with these fixes in cryptodev-2.6 tree.
Thanks, Rijo
Hi all,
[+Joakim, Jerome]
On Tue, Jan 7, 2020 at 1:26 PM Thomas, Rijo-john Rijo-john.Thomas@amd.com wrote:
+Herbert Xu, Deva
Hello Dan,
On 07/01/20 12:53 pm, Dan Carpenter wrote:
Hello Rijo Thomas,
The patch 757cc3e9ff1d: "tee: add AMD-TEE driver" from Dec 27, 2019, leads to the following static checker warning:
drivers/tee/amdtee/core.c:447 amdtee_driver_init() warn: 'drv_data' isn't an ERR_PTRdrivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
Okay, I will remove unused assignments in the driver.
Is this really such a good idea? I had the impression we were moving in the direction of always assigning default values to variables, unless they are implicitly zero initialized.
Thanks, Jens
440 int rc; 441 442 rc = psp_check_tee_status(); 443 if (rc) 444 goto err_fail; ^^^^^^^^^^^^^ Ideally psp_check_tee_status() would print its own error warning and we could return directly here.
Sure, I can add a warning here and return directly.
445 446 drv_data = kzalloc(sizeof(*drv_data), GFP_KERNEL); 447 if (IS_ERR(drv_data)) ^^^^^^^^^^^^^^^^ This should check for NULL instead of error pointer.
Okay I shall fix this.
448 return -ENOMEM; 449 450 amdtee = kzalloc(sizeof(*amdtee), GFP_KERNEL); 451 if (IS_ERR(amdtee)) { ^^^^^^^^^^^^^^ Same.
Okay.
452 rc = -ENOMEM; 453 goto err_kfree_drv_data; 454 } 455 456 pool = amdtee_config_shm(); 457 if (IS_ERR(pool)) { 458 pr_err("shared pool configuration error\n"); 459 rc = PTR_ERR(pool); 460 goto err_kfree_amdtee; 461 } 462 463 teedev = tee_device_alloc(&amdtee_desc, NULL, pool, amdtee); 464 if (IS_ERR(teedev)) { 465 rc = PTR_ERR(teedev); 466 goto err;
Better to create a "err_free_pool" label. The tee_device_unregister() is a no-op on this path so that's fine, but it's easier to read if we have the err_free_pool label.
Okay, I will add err_free_pool label just after tee_device_unregister.
467 } 468 amdtee->teedev = teedev; 469 470 rc = tee_device_register(amdtee->teedev); 471 if (rc) 472 goto err;
"goto err;" is very vague. We are unregistering a device which was never registered which feels wrong but actually works fine...
I shall rename err to err_device_unregister.
473 474 amdtee->pool = pool; 475 476 drv_data->amdtee = amdtee; 477 478 pr_info("amd-tee driver initialization successful\n"); 479 return 0; 480 481 err: 482 tee_device_unregister(amdtee->teedev); 483 if (pool) ^^^^^^^^^ Always true.
Agree, I shall remove this check.
484 tee_shm_pool_free(pool); 485 486 err_kfree_amdtee: 487 kfree(amdtee); 488 489 err_kfree_drv_data: 490 kfree(drv_data); 491 drv_data = NULL; 492 493 err_fail: 494 pr_err("amd-tee driver initialization failed\n"); 495 return rc; 496 }
regards, dan carpenter
I shall update the driver with these fixes in cryptodev-2.6 tree.
Thanks, Rijo _______________________________________________ Tee-dev mailing list Tee-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/tee-dev
On Fri, Jan 10, 2020 at 08:57:35AM +0100, Jens Wiklander wrote:
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
Okay, I will remove unused assignments in the driver.
Is this really such a good idea? I had the impression we were moving in the direction of always assigning default values to variables, unless they are implicitly zero initialized.
All that does is it disables GCC and other static analysis tools from finding uninitialized error bugs. It means we won't be able to turn on the GCC warning about unused assignments. That warning finds bugs where we accidentally use the wrong variable etc. Assigning bogus unused values makes the code more difficult to read.
regards, dan carpenter
On Fri, Jan 10, 2020 at 9:22 AM Dan Carpenter dan.carpenter@oracle.com wrote:
On Fri, Jan 10, 2020 at 08:57:35AM +0100, Jens Wiklander wrote:
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
We're bound to have corner cases with false positives. How will those be handled?
Okay, I will remove unused assignments in the driver.
Is this really such a good idea? I had the impression we were moving in the direction of always assigning default values to variables, unless they are implicitly zero initialized.
All that does is it disables GCC and other static analysis tools from finding uninitialized error bugs. It means we won't be able to turn on the GCC warning about unused assignments. That warning finds bugs where we accidentally use the wrong variable etc. Assigning bogus unused values makes the code more difficult to read.
This is a tradeoff, tools cannot find 100% of the cases with uninitialized variables on the stack. So in the cases where tools doesn't catch this we're going to use garbage from the stack. This is boiling down to a matter of taste and policy since both approaches have pros and cons. Myself I prefer initializing locals since I think it's more safe and then I'm also guarded against the false positives about usage of uninitialized variables you get from compilers from time to time. However, I'm flexible if this is what we really want, but I don't want to change back and forth as it's error prone and annoying.
Cheers, Jens
On Fri, Jan 10, 2020 at 12:11:18PM +0100, Jens Wiklander wrote:
On Fri, Jan 10, 2020 at 9:22 AM Dan Carpenter dan.carpenter@oracle.com wrote:
On Fri, Jan 10, 2020 at 08:57:35AM +0100, Jens Wiklander wrote:
drivers/tee/amdtee/core.c 435 static int __init amdtee_driver_init(void) 436 { 437 struct amdtee *amdtee = NULL; ^^^^^^^^^^^^^ 438 struct tee_device *teedev; 439 struct tee_shm_pool *pool = ERR_PTR(-EINVAL); ^^^^^^^^^^^^^^^^^^^^^^^^
These look like they are left overs from one error label style error handling. We're trying to enable the warning about unused assignments so it would be good to get rid of these.
We're bound to have corner cases with false positives. How will those be handled?
There aren't a lot of "false positives". GCC knows when an assignment is really never used.
There are some times where people think unused assignments help readability but probably we'll just silence the warning. We generally just silence GCC warnings even when GCC is wrong (like for uninitialized variables).
Okay, I will remove unused assignments in the driver.
Is this really such a good idea? I had the impression we were moving in the direction of always assigning default values to variables, unless they are implicitly zero initialized.
All that does is it disables GCC and other static analysis tools from finding uninitialized error bugs. It means we won't be able to turn on the GCC warning about unused assignments. That warning finds bugs where we accidentally use the wrong variable etc. Assigning bogus unused values makes the code more difficult to read.
This is a tradeoff, tools cannot find 100% of the cases with uninitialized variables on the stack. So in the cases where tools doesn't catch this we're going to use garbage from the stack. This is boiling down to a matter of taste and policy since both approaches have pros and cons. Myself I prefer initializing locals since I think it's more safe and then I'm also guarded against the false positives about usage of uninitialized variables you get from compilers from time to time. However, I'm flexible if this is what we really want, but I don't want to change back and forth as it's error prone and annoying.
GCC is actually broken currently with regards to uninitialized variables. It often initializes stack variables to zero and doesn't print a warning. But Smatch and other static checkers are working so we end up detecting most of the bugs I think.
regards, dan carpenter
-
Dan Carpenter -
Jens Wiklander -
Thomas, Rijo-john