| Message ID | 20220921091010.1309093-1-alexl@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | filemap: Fix error propagation in do_read_cache_page() | expand |
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.
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.
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.
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); }
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(-)