diff mbox series

[next] drm/mm/selftests: fix unsigned comparison with less than zero

Message ID 20200617155959.231740-1-colin.king@canonical.com (mailing list archive)
State New, archived
Headers show
Series [next] drm/mm/selftests: fix unsigned comparison with less than zero | expand

Commit Message

Colin King June 17, 2020, 3:59 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Function get_insert_time can return error values that are cast
to a u64. The checks of insert_time1 and insert_time2 check for
the errors but because they are u64 variables the check for less
than zero can never be true. Fix this by casting the value to s64
to allow of the negative error check to succeed.

Addresses-Coverity: ("Unsigned compared against 0, no effect")
Fixes: 6e60d5ded06b ("drm/mm: add ig_frag selftest")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/selftests/test-drm_mm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dan Carpenter June 18, 2020, 10:39 a.m. UTC | #1 
On Wed, Jun 17, 2020 at 04:59:59PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Function get_insert_time can return error values that are cast
> to a u64. The checks of insert_time1 and insert_time2 check for
> the errors but because they are u64 variables the check for less
> than zero can never be true. Fix this by casting the value to s64
> to allow of the negative error check to succeed.
> 
> Addresses-Coverity: ("Unsigned compared against 0, no effect")
> Fixes: 6e60d5ded06b ("drm/mm: add ig_frag selftest")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/selftests/test-drm_mm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c
> index 3846b0f5bae3..671a152a6df2 100644
> --- a/drivers/gpu/drm/selftests/test-drm_mm.c
> +++ b/drivers/gpu/drm/selftests/test-drm_mm.c
> @@ -1124,12 +1124,12 @@ static int igt_frag(void *ignored)
>  
>  		insert_time1 = get_insert_time(&mm, insert_size,
>  					       nodes + insert_size, mode);
> -		if (insert_time1 < 0)
> +		if ((s64)insert_time1 < 0)
>  			goto err;

The error codes in this function seem pretty messed up.

Speaking of error codes, what the heck is going on with
prepare_igt_frag().

  1037  static int prepare_igt_frag(struct drm_mm *mm,
  1038                              struct drm_mm_node *nodes,
  1039                              unsigned int num_insert,
  1040                              const struct insert_mode *mode)
  1041  {
  1042          unsigned int size = 4096;
  1043          unsigned int i;
  1044          u64 ret = -EINVAL;
                ^^^^^^^^^^^^^^^^^^
Why is it u64?

  1045  
  1046          for (i = 0; i < num_insert; i++) {
  1047                  if (!expect_insert(mm, &nodes[i], size, 0, i,
  1048                                     mode) != 0) {
  1049                          pr_err("%s insert failed\n", mode->name);
  1050                          goto out;
                                ^^^^^^^^
One of the common bugs with do nothing gotos is that we forget to set
the error code.  If we did a direct "return -EINVAL;" here, then there
would be no ambiguity.

  1051                  }
  1052          }
  1053  
  1054          /* introduce fragmentation by freeing every other node */
  1055          for (i = 0; i < num_insert; i++) {
  1056                  if (i % 2 == 0)
  1057                          drm_mm_remove_node(&nodes[i]);
  1058          }
  1059  
  1060  out:
  1061          return ret;
  1062  
  1063  }

regards,
dan carpenter
Christian König June 21, 2020, 3:33 p.m. UTC | #2 
Am 17.06.20 um 17:59 schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
>
> Function get_insert_time can return error values that are cast
> to a u64. The checks of insert_time1 and insert_time2 check for
> the errors but because they are u64 variables the check for less
> than zero can never be true. Fix this by casting the value to s64
> to allow of the negative error check to succeed.
>
> Addresses-Coverity: ("Unsigned compared against 0, no effect")
> Fixes: 6e60d5ded06b ("drm/mm: add ig_frag selftest")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

Going to pick that up for drm-misc-fixes tomorrow.

> ---
>   drivers/gpu/drm/selftests/test-drm_mm.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c
> index 3846b0f5bae3..671a152a6df2 100644
> --- a/drivers/gpu/drm/selftests/test-drm_mm.c
> +++ b/drivers/gpu/drm/selftests/test-drm_mm.c
> @@ -1124,12 +1124,12 @@ static int igt_frag(void *ignored)
>   
>   		insert_time1 = get_insert_time(&mm, insert_size,
>   					       nodes + insert_size, mode);
> -		if (insert_time1 < 0)
> +		if ((s64)insert_time1 < 0)
>   			goto err;
>   
>   		insert_time2 = get_insert_time(&mm, (insert_size * 2),
>   					       nodes + insert_size * 2, mode);
> -		if (insert_time2 < 0)
> +		if ((s64)insert_time2 < 0)
>   			goto err;
>   
>   		pr_info("%s fragmented insert of %u and %u insertions took %llu and %llu nsecs\n",
Nirmoy June 21, 2020, 9:38 p.m. UTC | #3 
On 6/18/20 12:39 PM, Dan Carpenter wrote:
> On Wed, Jun 17, 2020 at 04:59:59PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Function get_insert_time can return error values that are cast
>> to a u64. The checks of insert_time1 and insert_time2 check for
>> the errors but because they are u64 variables the check for less
>> than zero can never be true. Fix this by casting the value to s64
>> to allow of the negative error check to succeed.
>>
>> Addresses-Coverity: ("Unsigned compared against 0, no effect")
>> Fixes: 6e60d5ded06b ("drm/mm: add ig_frag selftest")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>   drivers/gpu/drm/selftests/test-drm_mm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c
>> index 3846b0f5bae3..671a152a6df2 100644
>> --- a/drivers/gpu/drm/selftests/test-drm_mm.c
>> +++ b/drivers/gpu/drm/selftests/test-drm_mm.c
>> @@ -1124,12 +1124,12 @@ static int igt_frag(void *ignored)
>>   
>>   		insert_time1 = get_insert_time(&mm, insert_size,
>>   					       nodes + insert_size, mode);
>> -		if (insert_time1 < 0)
>> +		if ((s64)insert_time1 < 0)
>>   			goto err;
> The error codes in this function seem pretty messed up.
>
> Speaking of error codes, what the heck is going on with
> prepare_igt_frag().


This is on me. I will send a patch to correct this mistake.


Thanks,

Nirmoy


>
>    1037  static int prepare_igt_frag(struct drm_mm *mm,
>    1038                              struct drm_mm_node *nodes,
>    1039                              unsigned int num_insert,
>    1040                              const struct insert_mode *mode)
>    1041  {
>    1042          unsigned int size = 4096;
>    1043          unsigned int i;
>    1044          u64 ret = -EINVAL;
>                  ^^^^^^^^^^^^^^^^^^
> Why is it u64?
>
>    1045
>    1046          for (i = 0; i < num_insert; i++) {
>    1047                  if (!expect_insert(mm, &nodes[i], size, 0, i,
>    1048                                     mode) != 0) {
>    1049                          pr_err("%s insert failed\n", mode->name);
>    1050                          goto out;
>                                  ^^^^^^^^
> One of the common bugs with do nothing gotos is that we forget to set
> the error code.  If we did a direct "return -EINVAL;" here, then there
> would be no ambiguity.
>
>    1051                  }
>    1052          }
>    1053
>    1054          /* introduce fragmentation by freeing every other node */
>    1055          for (i = 0; i < num_insert; i++) {
>    1056                  if (i % 2 == 0)
>    1057                          drm_mm_remove_node(&nodes[i]);
>    1058          }
>    1059
>    1060  out:
>    1061          return ret;
>    1062
>    1063  }
>
> regards,
> dan carpenter
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cnirmoy.das%40amd.com%7C74bcb0163ea04eaf0ca008d8137403ac%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280736306420244&amp;sdata=kZ7BUVaFWI5aV4OztJr8GMS8QWjz%2F7JIb9jwRM3ct5g%3D&amp;reserved=0
Christian König June 22, 2020, 5:36 p.m. UTC | #4 
Am 22.06.20 um 15:45 schrieb Nirmoy Das:
> Function prepare_igt_frag() and get_insert_time() were casting
> signed value to unsigned value before returning error.
> So error check in igt_frag() would not work with unsigned
> return value from get_insert_time() compared against negative
> value.
>
> Addresses-Coverity: ("Unsigned compared against 0, no effect")
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> Reported-by: Colin Ian King <colin.king@canonical.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/selftests/test-drm_mm.c | 21 +++++++--------------
>   1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c
> index 3846b0f5bae3..3306d8bd0544 100644
> --- a/drivers/gpu/drm/selftests/test-drm_mm.c
> +++ b/drivers/gpu/drm/selftests/test-drm_mm.c
> @@ -1041,13 +1041,12 @@ static int prepare_igt_frag(struct drm_mm *mm,
>   {
>   	unsigned int size = 4096;
>   	unsigned int i;
> -	u64 ret = -EINVAL;
>
>   	for (i = 0; i < num_insert; i++) {
>   		if (!expect_insert(mm, &nodes[i], size, 0, i,
>   				   mode) != 0) {
>   			pr_err("%s insert failed\n", mode->name);
> -			goto out;
> +			return -EINVAL;
>   		}
>   	}
>
> @@ -1057,8 +1056,7 @@ static int prepare_igt_frag(struct drm_mm *mm,
>   			drm_mm_remove_node(&nodes[i]);
>   	}
>
> -out:
> -	return ret;
> +	return 0;
>
>   }
>
> @@ -1070,21 +1068,16 @@ static u64 get_insert_time(struct drm_mm *mm,
>   	unsigned int size = 8192;
>   	ktime_t start;
>   	unsigned int i;
> -	u64 ret = -EINVAL;
>
>   	start = ktime_get();
>   	for (i = 0; i < num_insert; i++) {
>   		if (!expect_insert(mm, &nodes[i], size, 0, i, mode) != 0) {
>   			pr_err("%s insert failed\n", mode->name);
> -			goto out;
> +			return 0;
>   		}
>   	}
>
> -	ret = ktime_to_ns(ktime_sub(ktime_get(), start));
> -
> -out:
> -	return ret;
> -
> +	return ktime_to_ns(ktime_sub(ktime_get(), start));
>   }
>
>   static int igt_frag(void *ignored)
> @@ -1119,17 +1112,17 @@ static int igt_frag(void *ignored)
>   			continue;
>
>   		ret = prepare_igt_frag(&mm, nodes, insert_size, mode);
> -		if (!ret)
> +		if (ret)
>   			goto err;
>
>   		insert_time1 = get_insert_time(&mm, insert_size,
>   					       nodes + insert_size, mode);
> -		if (insert_time1 < 0)
> +		if (insert_time1 == 0)
>   			goto err;
>
>   		insert_time2 = get_insert_time(&mm, (insert_size * 2),
>   					       nodes + insert_size * 2, mode);
> -		if (insert_time2 < 0)
> +		if (insert_time2 == 0)
>   			goto err;
>
>   		pr_info("%s fragmented insert of %u and %u insertions took %llu and %llu nsecs\n",
> --
> 2.27.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c
index 3846b0f5bae3..671a152a6df2 100644
--- a/drivers/gpu/drm/selftests/test-drm_mm.c
+++ b/drivers/gpu/drm/selftests/test-drm_mm.c
@@ -1124,12 +1124,12 @@  static int igt_frag(void *ignored)
 
 		insert_time1 = get_insert_time(&mm, insert_size,
 					       nodes + insert_size, mode);
-		if (insert_time1 < 0)
+		if ((s64)insert_time1 < 0)
 			goto err;
 
 		insert_time2 = get_insert_time(&mm, (insert_size * 2),
 					       nodes + insert_size * 2, mode);
-		if (insert_time2 < 0)
+		if ((s64)insert_time2 < 0)
 			goto err;
 
 		pr_info("%s fragmented insert of %u and %u insertions took %llu and %llu nsecs\n",