Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 14:55:33 +0200
From:   Jan Kara <jack@suse.cz>
To:     Matthew Wilcox <willy6545@gmail.com>
Cc:     Hannes Reinecke <hare@suse.de>, Matthew Wilcox <matthew@wil.cx>,
        Mel Gorman <mgorman@suse.de>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Jan Kara <jack@suse.cz>
Subject: Re: Oddities in do_read_cache_page()
Message-ID: <20220627125533.v3o547uuvfo7r3pz@quack3.lan>
References: <f69f7f28-cf24-ebca-41f2-f6cf76435839@suse.de>
 <CAFhKne_hYFU0g5_68R=FA_QRWQc8ZRM8eCTNFwhP+4p4YHhZ8Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAFhKne_hYFU0g5_68R=FA_QRWQc8ZRM8eCTNFwhP+4p4YHhZ8Q@mail.gmail.com>
Precedence: bulk

Hi Matthew!

On Mon 27-06-22 07:17:35, Matthew Wilcox wrote:
> &folio->page does not dereference the pointer, it simply performs
> arithmetic on it. In this case, adding zero. It also changes the type from
> folio to page. There's no bug here.

I agree there's no functional bug there (at least yet). But arguably this
would be much more readable and definitely more future-proof as:

	return (struct page *)ERR_CAST(folio);

Because even doing arithmetics on error pointer is fishy...

								Honza

> 
> On Mon., Jun. 27, 2022, 04:12 Hannes Reinecke, <hare@suse.de> wrote:
> 
> > Hey Matt,
> >
> > I've stumbled across this code in do_read_cache_page():
> >
> >          struct folio *folio;
> >
> >          folio = do_read_cache_folio(mapping, index, filler, file, gfp);
> >          if (IS_ERR(folio))
> >                  return &folio->page;
> >          return folio_file_page(folio, index);
> >
> > Following 'do_read_cache_folio()' I see that it does things like
> >
> >                  folio = filemap_alloc_folio(gfp, 0);
> >                  if (!folio)
> >                          return ERR_PTR(-ENOMEM);
> >
> > Now I freely admit that my knowledge of folios is hazy at best, but
> > dereferencing an error pointer is something I would seriously frown upon
> >   if I were to review the code.
> > Care to explain?
> > Or is it, indeed, simply a bug?
> >
> > Cheers,
> >
> > Hannes
> > --
> > Dr. Hannes Reinecke                        Kernel Storage Architect
> > hare@suse.de                                      +49 911 74053 688
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 N?rnberg
> > HRB 36809 (AG N?rnberg), GF: Felix Imend?rffer
> >
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR