* [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero
@ 2020-06-17 15:59 Colin King
2020-06-18 10:39 ` Dan Carpenter
2020-06-21 15:33 ` Christian König
0 siblings, 2 replies; 4+ messages in thread
From: Colin King @ 2020-06-17 15:59 UTC (permalink / raw)
To: David Airlie, Daniel Vetter, Christian König, dri-devel
Cc: kernel-janitors, linux-kernel
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;
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",
--
2.27.0.rc0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero
2020-06-17 15:59 [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero Colin King
@ 2020-06-18 10:39 ` Dan Carpenter
2020-06-21 21:38 ` Nirmoy
2020-06-21 15:33 ` Christian König
1 sibling, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2020-06-18 10:39 UTC (permalink / raw)
To: Colin King
Cc: David Airlie, kernel-janitors, linux-kernel, dri-devel,
Christian König
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero
2020-06-17 15:59 [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero Colin King
2020-06-18 10:39 ` Dan Carpenter
@ 2020-06-21 15:33 ` Christian König
1 sibling, 0 replies; 4+ messages in thread
From: Christian König @ 2020-06-21 15:33 UTC (permalink / raw)
To: Colin King, David Airlie, Daniel Vetter, dri-devel
Cc: kernel-janitors, linux-kernel
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",
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero
2020-06-18 10:39 ` Dan Carpenter
@ 2020-06-21 21:38 ` Nirmoy
0 siblings, 0 replies; 4+ messages in thread
From: Nirmoy @ 2020-06-21 21:38 UTC (permalink / raw)
To: Dan Carpenter, Colin King
Cc: David Airlie, kernel-janitors, linux-kernel, dri-devel,
Christian König
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&data\x02%7C01%7Cnirmoy.das%40amd.com%7C74bcb0163ea04eaf0ca008d8137403ac%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637280736306420244&sdata=kZ7BUVaFWI5aV4OztJr8GMS8QWjz%2F7JIb9jwRM3ct5g%3D&reserved=0
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-21 21:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 15:59 [PATCH][next] drm/mm/selftests: fix unsigned comparison with less than zero Colin King
2020-06-18 10:39 ` Dan Carpenter
2020-06-21 21:38 ` Nirmoy
2020-06-21 15:33 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).