From: Jan Kara <jack@suse.cz>
To: Javier Pello <javier.pello@urjc.es>
Cc: Jan Kara <jack@suse.com>,
linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
Ira Weiny <ira.weiny@intel.com>
Subject: Re: [PATCH 1/1] fs/ext2: Avoid page_address on pages returned by ext2_get_page
Date: Tue, 13 Jul 2021 18:30:18 +0200 [thread overview]
Message-ID: <20210713163018.GF24271@quack2.suse.cz> (raw)
In-Reply-To: <20210713165918.10da0318af5b9b73e599a517@urjc.es>
On Tue 13-07-21 16:59:18, Javier Pello wrote:
> From: Javier Pello <javier.pello@urjc.es>
>
> 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 <javier.pello@urjc.es>
> 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@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2021-07-13 16:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-13 14:58 [PATCH 0/1] Kernel oopses on ext2 filesystems after 782b76d7abdf Javier Pello
2021-07-13 14:59 ` [PATCH 1/1] fs/ext2: Avoid page_address on pages returned by ext2_get_page Javier Pello
2021-07-13 16:30 ` Jan Kara [this message]
2021-07-13 17:33 ` Javier Pello
2021-07-14 9:00 ` Jan Kara
2021-07-14 16:54 ` [PATCH v2] " Javier Pello
2021-07-14 17:07 ` Ira Weiny
2021-07-15 12:17 ` Jan Kara
2021-07-15 16:18 ` Javier Pello
2021-07-13 18:17 ` [PATCH 1/1] " Ira Weiny
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210713163018.GF24271@quack2.suse.cz \
--to=jack@suse.cz \
--cc=ira.weiny@intel.com \
--cc=jack@suse.com \
--cc=javier.pello@urjc.es \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).