On 06/02/2023 14:19, Dan Carpenter wrote:
[ Ancient code but the warning showed up again because the function was
renamed or something? - dan ]
Hello Chris Wilson,
The patch 871dfbd67d4e: "drm/i915: Allow compaction upto SWIOTLB max
segment size" from Oct 11, 2016, leads to the following Smatch static
checker warning:
drivers/gpu/drm/i915/gem/i915_gem_shmem.c:164 shmem_sg_alloc_table()
warn: variable dereferenced before check 'sg' (see line 155)
Is smatch getting confused here? Not entirely sure but lets see below..
drivers/gpu/drm/i915/gem/i915_gem_shmem.c
58 int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
59 size_t size, struct intel_memory_region *mr,
60 struct address_space *mapping,
61 unsigned int max_segment)
62 {
63 unsigned int page_count; /* restricted by sg_alloc_table */
64 unsigned long i;
65 struct scatterlist *sg;
66 struct page *page;
67 unsigned long last_pfn = 0; /* suppress gcc warning */
68 gfp_t noreclaim;
69 int ret;
70
71 if (overflows_type(size / PAGE_SIZE, page_count))
72 return -E2BIG;
73
74 page_count = size / PAGE_SIZE;
75 /*
76 * If there's no chance of allocating enough pages for the whole
77 * object, bail early.
78 */
79 if (size > resource_size(&mr->region))
80 return -ENOMEM;
81
82 if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
83 return -ENOMEM;
84
85 /*
86 * Get the list of pages out of our struct file. They'll be pinned
87 * at this point until we release them.
88 *
89 * Fail silently without starting the shrinker
90 */
91 mapping_set_unevictable(mapping);
92 noreclaim = mapping_gfp_constraint(mapping, ~__GFP_RECLAIM);
93 noreclaim |= __GFP_NORETRY | __GFP_NOWARN;
94
95 sg = st->sgl;
^^^^^^^^^^^^
"sg" set here.
It is guaranteed to be non-NULL since sg_alloc_table succeeded.
96 st->nents = 0;
97 for (i = 0; i < page_count; i++) {
98 const unsigned int shrink[] = {
99 I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
100 0,
101 }, *s = shrink;
102 gfp_t gfp = noreclaim;
103
104 do {
105 cond_resched();
106 page = shmem_read_mapping_page_gfp(mapping, i, gfp);
107 if (!IS_ERR(page))
108 break;
This should probably break out of the outer loop instead of the inner
loop?
Don't think so, the loop has allocated a page and wants to break out the
inner loop so the page can be appended to the sg list.
109
110 if (!*s) {
111 ret = PTR_ERR(page);
112 goto err_sg;
113 }
114
115 i915_gem_shrink(NULL, i915, 2 * page_count, NULL, *s++);
116
117 /*
118 * We've tried hard to allocate the memory by reaping
119 * our own buffer, now let the real VM do its job and
120 * go down in flames if truly OOM.
121 *
122 * However, since graphics tend to be disposable,
123 * defer the oom here by reporting the ENOMEM back
124 * to userspace.
125 */
126 if (!*s) {
127 /* reclaim and warn, but no oom */
128 gfp = mapping_gfp_mask(mapping);
129
130 /*
131 * Our bo are always dirty and so we require
132 * kswapd to reclaim our pages (direct reclaim
133 * does not effectively begin pageout of our
134 * buffers on its own). However, direct reclaim
135 * only waits for kswapd when under allocation
136 * congestion. So as a result __GFP_RECLAIM is
137 * unreliable and fails to actually reclaim our
138 * dirty pages -- unless you try over and over
139 * again with !__GFP_NORETRY. However, we still
140 * want to fail this allocation rather than
141 * trigger the out-of-memory killer and for
142 * this we want __GFP_RETRY_MAYFAIL.
143 */
144 gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
145 }
146 } while (1);
147
148 if (!i ||
149 sg->length >= max_segment ||
150 page_to_pfn(page) != last_pfn + 1) {
151 if (i)
152 sg = sg_next(sg);
153
154 st->nents++;
155 sg_set_page(sg, page, PAGE_SIZE, 0);
^^
Dereferenced.
Does smatch worry about the sg = sg_next(sg) here, or the initially
highlighted state? Even for the former we know we have allocated enough
entries (always allocate assuming worst possible fragmentation) so this
will still be valid whatever happens.
156 } else {
157 sg->length += PAGE_SIZE;
^^
Here too.
158 }
159 last_pfn = page_to_pfn(page);
160
161 /* Check that the i965g/gm workaround works. */
162 GEM_BUG_ON(gfp & __GFP_DMA32 && last_pfn >= 0x00100000UL);
163 }
--> 164 if (sg) /* loop terminated early; short sg table */
If "sg" were NULL then we are already toasted.
AFAICT it can never be since the loop can never try to iterate past the
last sg entry.
Regards,
Tvrtko
165 sg_mark_end(sg);
166
167 /* Trim unused sg entries to avoid wasting memory. */
168 i915_sg_trim(st);
169
170 return 0;
171 err_sg:
172 sg_mark_end(sg);
173 if (sg != st->sgl) {
174 shmem_sg_free_table(st, mapping, false, false);
175 } else {
176 mapping_clear_unevictable(mapping);
177 sg_free_table(st);
178 }
179
180 /*
181 * shmemfs first checks if there is enough memory to allocate the page
182 * and reports ENOSPC should there be insufficient, along with the usual
183 * ENOMEM for a genuine allocation failure.
184 *
185 * We use ENOSPC in our driver to mean that we have run out of aperture
186 * space and so want to translate the error from shmemfs back to our
187 * usual understanding of ENOMEM.
188 */
189 if (ret == -ENOSPC)
190 ret = -ENOMEM;
191
192 return ret;
193 }
regards,
dan carpenter
