Re: [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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