Hi,
Static analysis with Coverity on linux-next has found a potential issue
in drivers/gpu/drm/i915/selftests/intel_memory_region.c in function
igt_mock_fill - the problematic commit is as follows:
commit d148738923fdb5077089e48ec15555e6008100d0
Author: Thomas Hellström <[email protected]>
Date: Wed Jun 2 10:38:08 2021 +0200
drm/i915/ttm Initialize the ttm device and memory managers
The analysis is as follows:
49 static int igt_mock_fill(void *arg)
50 {
51 struct intel_memory_region *mem = arg;
52 resource_size_t total = resource_size(&mem->region);
53 resource_size_t page_size;
54 resource_size_t rem;
55 unsigned long max_pages;
56 unsigned long page_num;
57 LIST_HEAD(objects);
58 int err = 0;
59
60 page_size = mem->chunk_size;
61 rem = total;
62 retry:
value_overwrite: Overwriting previous write to max_pages with value from
div64_u64(rem, page_size).
63 max_pages = div64_u64(rem, page_size);
64
65 for_each_prime_number_from(page_num, 1, max_pages) {
66 resource_size_t size = page_num * page_size;
67 struct drm_i915_gem_object *obj;
68
69 obj = i915_gem_object_create_region(mem, size, 0);
70 if (IS_ERR(obj)) {
71 err = PTR_ERR(obj);
72 break;
73 }
74
75 err = i915_gem_object_pin_pages_unlocked(obj);
76 if (err) {
77 i915_gem_object_put(obj);
78 break;
79 }
80
81 list_add(&obj->st_link, &objects);
82 rem -= size;
83 }
84
85 if (err == -ENOMEM)
86 err = 0;
87 if (err == -ENXIO) {
88 if (page_num * page_size <= rem) {
89 if (mem->is_range_manager && max_pages > 1) {
Unused value (UNUSED_VALUE)
assigned_value: Assigning value from max_pages >> 1 to max_pages here,
but that stored value is overwritten before it can be used.
90 max_pages >>= 1;
91 goto retry;
92 }
93
The right shift to max_pages on line 90 is being overwritten on line 64
on the retry.
Colin
On Mon, 12 Jul 2021 at 16:17, Colin Ian King <[email protected]> wrote:
>
> Hi,
>
> Static analysis with Coverity on linux-next has found a potential issue
> in drivers/gpu/drm/i915/selftests/intel_memory_region.c in function
> igt_mock_fill - the problematic commit is as follows:
>
> commit d148738923fdb5077089e48ec15555e6008100d0
> Author: Thomas Hellström <[email protected]>
> Date: Wed Jun 2 10:38:08 2021 +0200
>
> drm/i915/ttm Initialize the ttm device and memory managers
>
> The analysis is as follows:
>
>
> 49 static int igt_mock_fill(void *arg)
> 50 {
> 51 struct intel_memory_region *mem = arg;
> 52 resource_size_t total = resource_size(&mem->region);
> 53 resource_size_t page_size;
> 54 resource_size_t rem;
> 55 unsigned long max_pages;
> 56 unsigned long page_num;
> 57 LIST_HEAD(objects);
> 58 int err = 0;
> 59
> 60 page_size = mem->chunk_size;
> 61 rem = total;
> 62 retry:
>
> value_overwrite: Overwriting previous write to max_pages with value from
> div64_u64(rem, page_size).
>
> 63 max_pages = div64_u64(rem, page_size);
> 64
> 65 for_each_prime_number_from(page_num, 1, max_pages) {
> 66 resource_size_t size = page_num * page_size;
> 67 struct drm_i915_gem_object *obj;
> 68
> 69 obj = i915_gem_object_create_region(mem, size, 0);
> 70 if (IS_ERR(obj)) {
> 71 err = PTR_ERR(obj);
> 72 break;
> 73 }
> 74
> 75 err = i915_gem_object_pin_pages_unlocked(obj);
> 76 if (err) {
> 77 i915_gem_object_put(obj);
> 78 break;
> 79 }
> 80
> 81 list_add(&obj->st_link, &objects);
> 82 rem -= size;
> 83 }
> 84
> 85 if (err == -ENOMEM)
> 86 err = 0;
> 87 if (err == -ENXIO) {
> 88 if (page_num * page_size <= rem) {
> 89 if (mem->is_range_manager && max_pages > 1) {
>
> Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from max_pages >> 1 to max_pages here,
> but that stored value is overwritten before it can be used.
Yeah, that doesn't look good.
AFAIK this should be fixed with d53ec322dc7d ("drm/i915/ttm: switch
over to ttm_buddy_man"), which is in drm-tip, but I guess has not made
its way over to linux-next yet.
Thanks for the report.
>
> 90 max_pages >>= 1;
> 91 goto retry;
> 92 }
> 93
>
> The right shift to max_pages on line 90 is being overwritten on line 64
> on the retry.
>
> Colin