On Tue, May 03, 2022 at 07:42:12AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 29, 2022 at 06:25:20PM +0100, Matthew Wilcox (Oracle) wrote:
> > The ->readpage and ->read_folio operations are always called with the
> > same set of bits; it's only the type which differs. Use a union to
> > help with the transition and convert all the callers to use ->read_folio.
> >
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > ---
> > fs/buffer.c | 2 +-
> > include/linux/fs.h | 5 ++++-
> > mm/filemap.c | 6 +++---
> > mm/readahead.c | 10 +++++-----
> > 4 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 9737e0dbe3ec..5826ef29fe70 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -2824,7 +2824,7 @@ int nobh_truncate_page(struct address_space *mapping,
> >
> > /* Ok, it's mapped. Make sure it's up-to-date */
> > if (!folio_test_uptodate(folio)) {
> > - err = mapping->a_ops->readpage(NULL, &folio->page);
> > + err = mapping->a_ops->read_folio(NULL, folio);
> > if (err) {
> > folio_put(folio);
> > goto out;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 2be852661a29..5ecc4b74204d 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -335,7 +335,10 @@ static inline bool is_sync_kiocb(struct kiocb *kiocb)
> >
> > struct address_space_operations {
> > int (*writepage)(struct page *page, struct writeback_control *wbc);
> > - int (*readpage)(struct file *, struct page *);
> > + union {
> > + int (*readpage)(struct file *, struct page *);
> > + int (*read_folio)(struct file *, struct folio *);
> > + };
>
> I don't think this is going to make CFI very happy.
Good news is all the readpage instances are removed at the end of the
series. :)
But yes, bad news is that bisectability under CFI at this point in the
series CFI would trap every instance of ->read_folio, since the assigned
prototype:
static int ext4_readpage(struct file *file, struct page *page)
...
static const struct address_space_operations ext4_aops = {
.readpage = ext4_readpage,
doesn't match the call prototype:
int (*read_folio)(struct file *, struct folio *);
...
err = mapping->a_ops->read_folio(NULL, folio);
If you want to survive this, you'll need to avoid the union and add a
wrapper to be removed when you remove read_page:
struct address_space_operations {
int (*writepage)(struct page *page, struct writeback_control *wbc);
int (*readpage)(struct file *, struct page *);
int (*read_folio)(struct file *, struct folio *);
#define READ_FOLIO(ops, p, folio_or_page) ({ \
int __rf_err; \
if (ops->read_folio) \
__rf_err = ops->read_folio(p, folio_or_page); \
else \
__rf_err = ops->read_page(p, folio_or_page); \
__rf_err; \
})
...
err = READ_FOLIO(mapping->a_ops, NULL, folio);
Then only those cases that have had an appropriately matching callback
assigned to read_folio will call it.
And then you can drop the macro in "mm,fs: Remove stray references to ->readpage"
-Kees
--
Kees Cook
