Re: [PATCH 33/69] fs: Introduce aops->read_folio

[Kees Cook <keescook@chromium.org>]

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@infradead.org>
> > ---
> >  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