[bug report] gfs2: Convert gfs2_jhead_process_page() to use a folio

[bug report] gfs2: Convert gfs2_jhead_process_page() to use a folio

[Dan Carpenter]

Hello Matthew Wilcox (Oracle),

The patch 240159077d00: "gfs2: Convert gfs2_jhead_process_page() to
use a folio" from May 13, 2022 (linux-next), leads to the following
Smatch static checker warning:

	fs/gfs2/lops.c:485 gfs2_jhead_process_page()
	error: 'folio' dereferencing possible ERR_PTR()

fs/gfs2/lops.c
    469 static void gfs2_jhead_process_page(struct gfs2_jdesc *jd, unsigned long index,
    470                                     struct gfs2_log_header_host *head,
    471                                     bool *done)
    472 {
    473         struct folio *folio;
    474 
    475         folio = filemap_get_folio(jd->jd_inode->i_mapping, index);

Does filemap_get_folio() need to be checked for errors?

    476 
    477         folio_wait_locked(folio);
    478         if (folio_test_error(folio))
    479                 *done = true;
    480 
    481         if (!*done)
    482                 *done = gfs2_jhead_pg_srch(jd, head, &folio->page);
    483 
    484         /* filemap_get_folio() and the earlier grab_cache_page() */
--> 485         folio_put_refs(folio, 2);
    486 }

regards,
dan carpenter

Re: [bug report] gfs2: Convert gfs2_jhead_process_page() to use a folio

[Matthew Wilcox]

On Thu, Oct 12, 2023 at 12:43:34PM +0300, Dan Carpenter wrote:
> Hello Matthew Wilcox (Oracle),
> 
> The patch 240159077d00: "gfs2: Convert gfs2_jhead_process_page() to
> use a folio" from May 13, 2022 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	fs/gfs2/lops.c:485 gfs2_jhead_process_page()
> 	error: 'folio' dereferencing possible ERR_PTR()
> 
> fs/gfs2/lops.c
>     469 static void gfs2_jhead_process_page(struct gfs2_jdesc *jd, unsigned long index,
>     470                                     struct gfs2_log_header_host *head,
>     471                                     bool *done)
>     472 {
>     473         struct folio *folio;
>     474 
>     475         folio = filemap_get_folio(jd->jd_inode->i_mapping, index);
> 
> Does filemap_get_folio() need to be checked for errors?

In this specific case, we know that the folio is there and will always
be returned.  See the comment on line 484.  Can I annotate this call so
that the tool knows this is a special case and doesn't warn about it?

>     476 
>     477         folio_wait_locked(folio);
>     478         if (folio_test_error(folio))
>     479                 *done = true;
>     480 
>     481         if (!*done)
>     482                 *done = gfs2_jhead_pg_srch(jd, head, &folio->page);
>     483 
>     484         /* filemap_get_folio() and the earlier grab_cache_page() */
> --> 485         folio_put_refs(folio, 2);
>     486 }
> 
> regards,
> dan carpenter

Re: [bug report] gfs2: Convert gfs2_jhead_process_page() to use a folio

[Dan Carpenter]

On Thu, Oct 12, 2023 at 04:27:55PM +0100, Matthew Wilcox wrote:
> On Thu, Oct 12, 2023 at 12:43:34PM +0300, Dan Carpenter wrote:
> > Hello Matthew Wilcox (Oracle),
> > 
> > The patch 240159077d00: "gfs2: Convert gfs2_jhead_process_page() to
> > use a folio" from May 13, 2022 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > 	fs/gfs2/lops.c:485 gfs2_jhead_process_page()
> > 	error: 'folio' dereferencing possible ERR_PTR()
> > 
> > fs/gfs2/lops.c
> >     469 static void gfs2_jhead_process_page(struct gfs2_jdesc *jd, unsigned long index,
> >     470                                     struct gfs2_log_header_host *head,
> >     471                                     bool *done)
> >     472 {
> >     473         struct folio *folio;
> >     474 
> >     475         folio = filemap_get_folio(jd->jd_inode->i_mapping, index);
> > 
> > Does filemap_get_folio() need to be checked for errors?
> 
> In this specific case, we know that the folio is there and will always
> be returned.  See the comment on line 484.  Can I annotate this call so
> that the tool knows this is a special case and doesn't warn about it?
> 

Thanks for looking at this.  There is no annotation.  In the kernel we
are very good at fixing static checker warnings so everything old is a
false positive.

This one is from May but I recently changed how dereferences were
handled so it's the first time I had seen the warning.

regards,
dan carpenter