* [PATCH] filemap: Fix error propagation in do_read_cache_page()
@ 2022-09-21 9:10 Alexander Larsson
2022-09-21 17:16 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Larsson @ 2022-09-21 9:10 UTC (permalink / raw)
To: willy; +Cc: linux-fsdevel, Alexander Larsson
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@redhat.com>
---
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);
return folio_file_page(folio, index);
}
--
2.37.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] filemap: Fix error propagation in do_read_cache_page()
2022-09-21 9:10 [PATCH] filemap: Fix error propagation in do_read_cache_page() Alexander Larsson
@ 2022-09-21 17:16 ` Al Viro
2022-09-22 8:06 ` Alexander Larsson
2022-09-24 21:43 ` Matthew Wilcox
0 siblings, 2 replies; 4+ messages in thread
From: Al Viro @ 2022-09-21 17:16 UTC (permalink / raw)
To: Alexander Larsson; +Cc: willy, linux-fsdevel
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@redhat.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] filemap: Fix error propagation in do_read_cache_page()
2022-09-21 17:16 ` Al Viro
@ 2022-09-22 8:06 ` Alexander Larsson
2022-09-24 21:43 ` Matthew Wilcox
1 sibling, 0 replies; 4+ messages in thread
From: Alexander Larsson @ 2022-09-22 8:06 UTC (permalink / raw)
To: Al Viro; +Cc: willy, linux-fsdevel
On Wed, Sep 21, 2022 at 7:32 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> 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@redhat.com>
> > ---
> > 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.
Yeah, it doesn't actually dereference, but what I was thinking is that
the caller could dereference it, if the addition made it not an error.
However, I didn't look at the actual offset of page in folio, so
you're right, this is actually fine.
Still, better change this to avoid confusing people.
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Alexander Larsson Red Hat, Inc
alexl@redhat.com alexander.larsson@gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] filemap: Fix error propagation in do_read_cache_page()
2022-09-21 17:16 ` Al Viro
2022-09-22 8:06 ` Alexander Larsson
@ 2022-09-24 21:43 ` Matthew Wilcox
1 sibling, 0 replies; 4+ messages in thread
From: Matthew Wilcox @ 2022-09-24 21:43 UTC (permalink / raw)
To: Al Viro; +Cc: Alexander Larsson, linux-fsdevel
On Wed, Sep 21, 2022 at 06:16:18PM +0100, Al Viro wrote:
> 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@redhat.com>
> > ---
> > 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.
I don't see it that way. &folio->page is the idiomatic way to do this.
What it really is, is an indicator that code needs to be converted from
calling do_read_cache_page() and friends to calling the folio equivalents.
Also, I should have moved this code to folio-compat.c where it would be
with all the other code that uses this idiom.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-24 21:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 9:10 [PATCH] filemap: Fix error propagation in do_read_cache_page() Alexander Larsson
2022-09-21 17:16 ` Al Viro
2022-09-22 8:06 ` Alexander Larsson
2022-09-24 21:43 ` Matthew Wilcox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).