Re: [PATCH 1/1] fs/ext2: Avoid page_address on pages returned by ext2_get_page
From: Jan Kara <jack@suse.cz>
Date: 2021-07-13 16:30:22
Also in:
lkml
On Tue 13-07-21 16:59:18, Javier Pello wrote:
From: Javier Pello <redacted>
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? I have just a few nits below:
Signed-off-by: Javier Pello <redacted> Fixes: 782b76d7abdf fs/ext2: Replace kmap() with kmap_local_page()
Please wrap subject in Fixes tag into ("...").
quoted hunk ↗ jump to hunk
@@ -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...
quoted hunk ↗ jump to hunk
+ 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 [off-list ref] SUSE Labs, CR