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?
regards,
dan carpenter
