[bug report] mm: return an ERR_PTR from __filemap_get_folio

[Dan Carpenter <error27@gmail.com>]

Hello Christoph Hellwig,

The patch 480c454ff64b: "mm: return an ERR_PTR from
__filemap_get_folio" from Mar 7, 2023, leads to the following Smatch
static checker warning:

	mm/filemap.c:3381 filemap_fault()
	warn: 'folio' is an error pointer or valid

mm/filemap.c
  3262          folio = filemap_get_folio(mapping, index);
  3263          if (likely(!IS_ERR(folio))) {
  3264                  /*
  3265                   * We found the page, so try async readahead before waiting for
  3266                   * the lock.
  3267                   */
  3268                  if (!(vmf->flags & FAULT_FLAG_TRIED))
  3269                          fpin = do_async_mmap_readahead(vmf, folio);
  3270                  if (unlikely(!folio_test_uptodate(folio))) {
  3271                          filemap_invalidate_lock_shared(mapping);
  3272                          mapping_locked = true;
  3273                  }
  3274          } else {
  3275                  /* No page in the page cache at all */
  3276                  count_vm_event(PGMAJFAULT);
  3277                  count_memcg_event_mm(vmf->vma->vm_mm, PGMAJFAULT);
  3278                  ret = VM_FAULT_MAJOR;
  3279                  fpin = do_sync_mmap_readahead(vmf);
  3280  retry_find:
  3281                  /*
  3282                   * See comment in filemap_create_folio() why we need
  3283                   * invalidate_lock
  3284                   */
  3285                  if (!mapping_locked) {
  3286                          filemap_invalidate_lock_shared(mapping);
  3287                          mapping_locked = true;
  3288                  }
  3289                  folio = __filemap_get_folio(mapping, index,
  3290                                            FGP_CREAT|FGP_FOR_MMAP,
  3291                                            vmf->gfp_mask);

This function was changed to return error pointers instead of NULL.

  3292                  if (IS_ERR(folio)) {
  3293                          if (fpin)
  3294                                  goto out_retry;
  3295                          filemap_invalidate_unlock_shared(mapping);
  3296                          return VM_FAULT_OOM;
  3297                  }
  3298          }
  3299  
  3300          if (!lock_folio_maybe_drop_mmap(vmf, folio, &fpin))
  3301                  goto out_retry;
  3302
  3303          /* Did it get truncated? */
  3304          if (unlikely(folio->mapping != mapping)) {
  3305                  folio_unlock(folio);
  3306                  folio_put(folio);
  3307                  goto retry_find;
  3308          }
  3309          VM_BUG_ON_FOLIO(!folio_contains(folio, index), folio);
  3310  
  3311          /*
  3312           * We have a locked page in the page cache, now we need to check
  3313           * that it's up-to-date. If not, it is going to be due to an error.
  3314           */
  3315          if (unlikely(!folio_test_uptodate(folio))) {
  3316                  /*
  3317                   * The page was in cache and uptodate and now it is not.
  3318                   * Strange but possible since we didn't hold the page lock all
  3319                   * the time. Let's drop everything get the invalidate lock and
  3320                   * try again.
  3321                   */
  3322                  if (!mapping_locked) {
  3323                          folio_unlock(folio);
  3324                          folio_put(folio);
  3325                          goto retry_find;
  3326                  }
  3327                  goto page_not_uptodate;
  3328          }
  3329  
  3330          /*
  3331           * We've made it this far and we had to drop our mmap_lock, now is the
  3332           * time to return to the upper layer and have it re-find the vma and
  3333           * redo the fault.
  3334           */
  3335          if (fpin) {
  3336                  folio_unlock(folio);
  3337                  goto out_retry;
  3338          }
  3339          if (mapping_locked)
  3340                  filemap_invalidate_unlock_shared(mapping);
  3341  
  3342          /*
  3343           * Found the page and have a reference on it.
  3344           * We must recheck i_size under page lock.
  3345           */
  3346          max_idx = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE);
  3347          if (unlikely(index >= max_idx)) {
  3348                  folio_unlock(folio);
  3349                  folio_put(folio);
  3350                  return VM_FAULT_SIGBUS;
  3351          }
  3352  
  3353          vmf->page = folio_file_page(folio, index);
  3354          return ret | VM_FAULT_LOCKED;
  3355  
  3356  page_not_uptodate:
  3357          /*
  3358           * Umm, take care of errors if the page isn't up-to-date.
  3359           * Try to re-read it _once_. We do this synchronously,
  3360           * because there really aren't any performance issues here
  3361           * and we need to check for errors.
  3362           */
  3363          fpin = maybe_unlock_mmap_for_io(vmf, fpin);
  3364          error = filemap_read_folio(file, mapping->a_ops->read_folio, folio);
  3365          if (fpin)
  3366                  goto out_retry;
  3367          folio_put(folio);
  3368  
  3369          if (!error || error == AOP_TRUNCATED_PAGE)
  3370                  goto retry_find;
  3371          filemap_invalidate_unlock_shared(mapping);
  3372  
  3373          return VM_FAULT_SIGBUS;
  3374  
  3375  out_retry:
  3376          /*
  3377           * We dropped the mmap_lock, we need to return to the fault handler to
  3378           * re-find the vma and come back and find our hopefully still populated
  3379           * page.
  3380           */
  3381          if (folio)
                    ^^^^^
Should this be !IS_ERR() now?

  3382                  folio_put(folio);
  3383          if (mapping_locked)
  3384                  filemap_invalidate_unlock_shared(mapping);
  3385          if (fpin)
  3386                  fput(fpin);
  3387          return ret | VM_FAULT_RETRY;
  3388  }



regards,
dan carpenter