Re: [bug report] lib/test_hmm: add zone device private THP test infrastructure

[Dan Carpenter <dan.carpenter@linaro.org>]

On Mon, Sep 15, 2025 at 07:49:33PM +1000, Balbir Singh wrote:
> On 9/15/25 15:50, Dan Carpenter wrote:
> > Hello Balbir Singh,
> > 
> > Commit 6098c878b175 ("lib/test_hmm: add zone device private THP test
> > infrastructure") from Sep 8, 2025 (linux-next), leads to the
> > following Smatch static checker warning:
> > 
> > 	lib/test_hmm.c:721 dmirror_migrate_alloc_and_copy()
> > 	error: we previously assumed 'spage' could be null (see line 718)
> > 
> > lib/test_hmm.c
> >     675 static void dmirror_migrate_alloc_and_copy(struct migrate_vma *args,
> >     676                                            struct dmirror *dmirror)
> >     677 {
> >     678         const unsigned long *src = args->src;
> >     679         unsigned long *dst = args->dst;
> >     680         unsigned long addr;
> >     681 
> >     682         for (addr = args->start; addr < args->end; ) {
> >     683                 struct page *spage;
> >     684                 struct page *dpage;
> >     685                 struct page *rpage;
> >     686                 bool is_large = *src & MIGRATE_PFN_COMPOUND;
> >     687                 int write = (*src & MIGRATE_PFN_WRITE) ? MIGRATE_PFN_WRITE : 0;
> >     688                 unsigned long nr = 1;
> >     689 
> >     690                 if (!(*src & MIGRATE_PFN_MIGRATE))
> >     691                         goto next;
> >     692 
> >     693                 /*
> >     694                  * Note that spage might be NULL which is OK since it is an
> >     695                  * unallocated pte_none() or read-only zero page.
> >     696                  */
> >     697                 spage = migrate_pfn_to_page(*src);
> >     698                 if (WARN(spage && is_zone_device_page(spage),
> >     699                      "page already in device spage pfn: 0x%lx\n",
> >     700                      page_to_pfn(spage)))
> >     701                         goto next;
> >     702 
> >     703                 if (dmirror->flags & HMM_DMIRROR_FLAG_FAIL_ALLOC) {
> >     704                         dmirror->flags &= ~HMM_DMIRROR_FLAG_FAIL_ALLOC;
> >     705                         dpage = NULL;
> >     706                 } else
> >     707                         dpage = dmirror_devmem_alloc_page(dmirror, is_large);
> >     708 
> >     709                 if (!dpage) {
> >     710                         struct folio *folio;
> >     711                         unsigned long i;
> >     712                         unsigned long spfn = *src >> MIGRATE_PFN_SHIFT;
> >     713                         struct page *src_page;
> >     714 
> >     715                         if (!is_large)
> >     716                                 goto next;
> >     717 
> >     718                         if (!spage && is_large) {
> >                                     ^^^^^^    ^^^^^^^^
> > If spage is NULL but is_large is false
> 
> Isn't is_large being false ruled out two lines above?
> 

Ah, yes.  When Smatch sees an impossible condition like that, the
assumption is that probably the static checker has messed up.  So there
isn't special code to filter out impossible conditions.

For some Smatch checks like the uninitialized variable warning, then
warnings in dead code are filtered out.  But the line with the warning
is not dead code so it's more complicated.

regards,
dan carpenter

> > 
> >     719                                 nr = HPAGE_PMD_NR;
> >     720                         } else {
> > --> 721                                 folio = page_folio(spage);
> >                                                            ^^^^^
> > the this will crash.
> > 
> >     722                                 nr = folio_nr_pages(folio);
> >     723                         }
> >     724 
> >     725                         for (i = 0; i < nr && addr < args->end; i++) {
> > 
> > regards,
> > dan carpenter
> 
> Thanks,
> Balbir