Re: [PATCH 1/1] fs/ext2: Avoid page_address on pages returned by ext2_get_page

[Ira Weiny]

On Tue, Jul 13, 2021 at 06:30:18PM +0200, Jan Kara wrote:
> On Tue 13-07-21 16:59:18, Javier Pello wrote:
> > From: Javier Pello <javier.pello@xxxxxxx>
> > 
> > Commit 782b76d7abdf02b12c46ed6f1e9bf715569027f7 ("fs/ext2: Replace
> > kmap() with kmap_local_page()") replaced the kmap/kunmap calls in
> > ext2_get_page/ext2_put_page with kmap_local_page/kunmap_local for
> > efficiency reasons. As a necessary side change, the commit also
> > made ext2_get_page (and ext2_find_entry and ext2_dotdot) return
> > the mapping address along with the page itself, as it is required
> > for kunmap_local, and converted uses of page_address on such pages
> > to use the newly returned address instead. However, uses of
> > page_address on such pages were missed in ext2_check_page and
> > ext2_delete_entry, which triggers oopses if kmap_local_page happens
> > to return an address from high memory. Fix this now by converting
> > the remaining uses of page_address to use the right address, as
> > returned by kmap_local_page.
> 
> Good catch. Thanks for the patch. Ira, can you please check the patch as
> well?

Yes thanks...  I totally missed those uses.  Sorry.

I don't see any issues with the patch but I think Jan's suggestions below are
good.

On a deeper note I wonder if adding kmap local support to page_address is
appropriate.  But for now this is the correct fix.

Please CC me on V2.  And sorry for the problem,
Ira

> 
> I have just a few nits below:
> 
> > Signed-off-by: Javier Pello <javier.pello@xxxxxxx>
> > Fixes: 782b76d7abdf fs/ext2: Replace kmap() with kmap_local_page()
> 
> Please wrap subject in Fixes tag into ("...").
> 
> > @@ -584,16 +584,16 @@ int ext2_add_link (struct dentry *dentry, struct inode *inode)
> >   * ext2_delete_entry deletes a directory entry by merging it with the
> >   * previous entry. Page is up-to-date.
> >   */
> > -int ext2_delete_entry (struct ext2_dir_entry_2 * dir, struct page * page )
> > +int ext2_delete_entry (struct ext2_dir_entry_2 *dir, struct page *page,
> > +			void *kaddr)
> 
> Why not have 'kaddr' as char *. We type it to char * basically everywhere
> anyway.
> 
> >  {
> >  	struct inode *inode = page->mapping->host;
> > -	char *kaddr = page_address(page);
> > -	unsigned from = ((char*)dir - kaddr) & ~(ext2_chunk_size(inode)-1);
> > -	unsigned to = ((char *)dir - kaddr) +
> > -				ext2_rec_len_from_disk(dir->rec_len);
> > +	unsigned int delta = (char *)dir - (char *)kaddr;
> 
> Maybe I'd call this 'offset' rather than 'delta'. Also if kaddr will stay
> char *, you maybe just don't need to touch this...
> 
> > +	unsigned int from = delta & ~(ext2_chunk_size(inode)-1);
> > +	unsigned int to = delta + ext2_rec_len_from_disk(dir->rec_len);
> >  	loff_t pos;
> >  	ext2_dirent * pde = NULL;
> > -	ext2_dirent * de = (ext2_dirent *) (kaddr + from);
> > +	ext2_dirent *de = (ext2_dirent *) ((char *)kaddr + from);
> >  	int err;
> >  
> >  	while ((char*)de < (char*)dir) {
> > @@ -607,7 +607,7 @@ int ext2_delete_entry (struct ext2_dir_entry_2 * dir, struct page * page )
> >  		de = ext2_next_entry(de);
> >  	}
> >  	if (pde)
> > -		from = (char*)pde - (char*)page_address(page);
> > +		from = (char *)pde - (char *)kaddr;
> >  	pos = page_offset(page) + from;
> >  	lock_page(page);
> >  	err = ext2_prepare_chunk(page, pos, to - from);
> 
> 									Honza
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR