On Wed, Sep 21, 2022 at 11:10:10AM +0200, Alexander Larsson wrote:
> When do_read_cache_folio() returns an error pointer the code
> was dereferencing it rather than forwarding the error via
> ERR_CAST().
>
> Found during code review.
>
> Fixes: 539a3322f208 ("filemap: Add read_cache_folio and read_mapping_folio")
> Signed-off-by: Alexander Larsson <alexl@xxxxxxxxxx>
> ---
> mm/filemap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 15800334147b..6bc55506f7a8 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3560,7 +3560,7 @@ static struct page *do_read_cache_page(struct address_space *mapping,
>
> folio = do_read_cache_folio(mapping, index, filler, file, gfp);
> if (IS_ERR(folio))
> - return &folio->page;
> + return ERR_CAST(folio);
Where do you see a dereference? I agree that your variant is cleaner,
but &folio->page does *NOT* dereference anything - it's an equivalent of
(struct page *)((unsigned long)folio + offsetof(struct folio, page))
and the reason it happens to work is that page is the first member in
struct folio, so the offsetof ends up being 0 and we are left with a cast
from struct folio * to struct page *, i.e. the same thing ERR_CAST()
variant end up with (it casts to void *, which is converted to struct
page * since return acts as assignment wrt type conversions).
It *is* brittle and misguiding, and your patch is a much more clear
way to spell that thing, no arguments about it; just that your patch
is not changing behaviour.
