On 07/02/2023 08:49, Dan Carpenter wrote:
On Mon, Feb 06, 2023 at 04:59:36PM +0000, Tvrtko Ursulin wrote:
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..
Reading through your comments, it seems like you're saying the NULL
check should be deleted. I don't really consider that a "false positive".
Hopefully, we both agree that by the time we get to the check the "sg"
pointer has been dereferenced.
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.
Yeah. This doesn't matter. When I originally wrote this, I thought it
mattered but then I re-read the code but forgot to delete this comment.
In theory Smatch is supposed to be able to be tracking that "If
sg_alloc_table() succeeds, then "st->sgl" is non-NULL," but
__sg_alloc_table() has a do { } while() loop and Smatch is bad at
parsing loops.
However, Smatch does say that if sg_alloc_table() succeeds it means
page_count is non-zero.
96 st->nents = 0;
97 for (i = 0; i < page_count; i++) {
Since page_count is non-zero we definitely enter this loop.
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.
None of that really matters. What matters is that we dereference "sg"
at the end of every iteration through the loop.
Technically, it does slightly matter. If Smatch were able to determine
that a dereference is safe, then it doesn't print a warning. But Smatch
is probably always never going to know that sg_next() can't return NULL
here.
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 */
^^^^^^
165 sg_mark_end(sg);
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.
Right. So this if statement can be deleted?
I think so, I don't see loop can exit with a null sg. Sg_mark_end()
still has to stay in case of i915_sg_trim below is not able to
re-allocate a more compact list.
Regards,
Tvrtko
