From: Javier Pello <[email protected]>
The current mainline kernel oopses when handling ext2 filesystems,
and the filesystem is not usable, sometimes leading to a panic.
I recently tried to upgrade the kernel on my system from 5.10.7 to
5.13.1 but, when I booted the new kernel, I started getting oopses
consistently during the boot process as the init script tried to
mount an ext2 filesystem, and other weird behaviour (like tasks
stuck in uninterruptible sleep) if I accessed the filesystem later
(which I did not do for long for fear of data loss). I managed to
set up a QEMU virtual machine with as small a reproducer as I could
get (kernel stripped of as much stuff as possible) and this is the
oops message that I got (I set the kernel to panic on oops to that
I could get the first report; otherwise, the kernel tries to go on
but ends up attempting to kill init):
BUG: kernel NULL pointer dereference, address: 00000004
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
*pde = 00000000
Oops: 0000 [#1]
CPU: 0 PID: 41 Comm: init.boot Not tainted 5.12.0-rc3+ #23
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014
EIP: ext2_get_page.isra.0+0xe2/0x2b0
Code: 8b 45 dc 89 5d cc 8b 80 cc 01 00 00 8b 40 34 8b 00 89 45 d4 8b 45 e0 f7 d8 89 45 e0 8b 45 e8 83 e8 0c 89 45 d0 8b 45 f0 01 f8 <0f> b7 48 04 89 ca 66 83 f9 0b 76 4a 83 e2 03 0f 85 09 01 00 00 0f
EAX: 00000000 EBX: f73fa700 ECX: 00000000 EDX: 00000001
ESI: 00000000 EDI: 00000000 EBP: c2509ce8 ESP: c2509cb0
DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 EFLAGS: 00010246
CR0: 80050033 CR2: 00000004 CR3: 024dd000 CR4: 00150e90
Call Trace:
ext2_find_entry+0x79/0x240
ext2_inode_by_name+0x16/0x70
ext2_lookup+0x27/0x70
__lookup_slow+0x4f/0xe0
walk_component+0xf7/0x160
link_path_walk.part.0+0x24d/0x350
? terminate_walk+0x7d/0xf0
path_lookupat+0x39/0x180
filename_lookup+0x78/0x130
? kmem_cache_alloc+0x21/0x130
? getname_flags+0x1f/0x160
? getname_flags+0x36/0x160
user_path_at_empty+0x25/0x30
vfs_statx+0x53/0xd0
__do_sys_stat64+0x27/0x50
__ia32_sys_stat64+0xd/0x10
__do_fast_syscall_32+0x40/0x70
do_fast_syscall_32+0x28/0x60
do_SYSENTER_32+0x15/0x20
entry_SYSENTER_32+0x98/0xe6
EIP: 0xb7ee8549
Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d 76 00 58 b8 77 00 00 00 cd 80 90 8d 76
EAX: ffffffda EBX: 0a0dd360 ECX: bf993cc0 EDX: b7e64000
ESI: 0a0dd360 EDI: 0a0dd000 EBP: 0a0dd340 ESP: bf993c98
DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000296
CR2: 0000000000000004
---[ end trace 865a3cf144c3889e ]---
I am running a 32-bit kernel on an x86 machine, in case this is
relevant. Also, I noticed that the high memory support config
option must be set to 4GB to trigger the bug; I could not
reproduce the bug if I set high memory to off.
As the text says, the oops is triggered within ext2_get_page.
I managed to track it down to line 130 in fs/ext2/dir.c, within
ext2_check_page,
rec_len = ext2_rec_len_from_disk(p->rec_len);
Adding a printk in the function I confirmed that, while variable
page has a non-null value on function entry, the assignment
char *kaddr = page_address(page);
in line 114 sets kaddr to a null value indeed, and this triggers
the bug when dereferenced later.
I bisected the problem to commit
782b76d7abdf02b12c46ed6f1e9bf715569027f7
fs/ext2: Replace kmap() with kmap_local_page()
The oops triggers consistently on this commit but its parent
commit works fine. Analysing the commit, I think that it may be
incomplete, as ext2_check_page and ext2_delete_entry are still
using page_address to get at the page address, but this no longer
works because those pages are now mapped with kmap_local_page,
not kmap. It seems to me that ext2_check_page and ext2_delete_entry
should be provided with the page address that their callers already
have for the page, as with all other functions in fs/ext2/dir.c
now. The proposed patch does precisely this.
Javier Pello (1):
fs/ext2: Avoid page_address on pages returned by ext2_get_page
fs/ext2/dir.c | 20 ++++++++++----------
fs/ext2/ext2.h | 3 ++-
fs/ext2/namei.c | 4 ++--
3 files changed, 14 insertions(+), 13 deletions(-)
--
2.30.1
From: Javier Pello <[email protected]>
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.
Signed-off-by: Javier Pello <[email protected]>
Fixes: 782b76d7abdf fs/ext2: Replace kmap() with kmap_local_page()
---
fs/ext2/dir.c | 20 ++++++++++----------
fs/ext2/ext2.h | 3 ++-
fs/ext2/namei.c | 4 ++--
3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 14292dba..0c59b4d3 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -106,12 +106,11 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
return err;
}
-static bool ext2_check_page(struct page *page, int quiet)
+static bool ext2_check_page(struct page *page, int quiet, char *kaddr)
{
struct inode *dir = page->mapping->host;
struct super_block *sb = dir->i_sb;
unsigned chunk_size = ext2_chunk_size(dir);
- char *kaddr = page_address(page);
u32 max_inumber = le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count);
unsigned offs, rec_len;
unsigned limit = PAGE_SIZE;
@@ -205,7 +204,8 @@ static struct page * ext2_get_page(struct inode *dir, unsigned long n,
if (!IS_ERR(page)) {
*page_addr = kmap_local_page(page);
if (unlikely(!PageChecked(page))) {
- if (PageError(page) || !ext2_check_page(page, quiet))
+ if (PageError(page) || !ext2_check_page(page, quiet,
+ *page_addr))
goto fail;
}
}
@@ -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)
{
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;
+ 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);
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index b0a69482..7bda9379 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -740,7 +740,8 @@ extern int ext2_inode_by_name(struct inode *dir,
extern int ext2_make_empty(struct inode *, struct inode *);
extern struct ext2_dir_entry_2 *ext2_find_entry(struct inode *, const struct qstr *,
struct page **, void **res_page_addr);
-extern int ext2_delete_entry (struct ext2_dir_entry_2 *, struct page *);
+extern int ext2_delete_entry(struct ext2_dir_entry_2 *dir, struct page *page,
+ void *kaddr);
extern int ext2_empty_dir (struct inode *);
extern struct ext2_dir_entry_2 *ext2_dotdot(struct inode *dir, struct page **p, void **pa);
extern void ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 1f69b816..5f6b7560 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -293,7 +293,7 @@ static int ext2_unlink(struct inode * dir, struct dentry *dentry)
goto out;
}
- err = ext2_delete_entry (de, page);
+ err = ext2_delete_entry (de, page, page_addr);
ext2_put_page(page, page_addr);
if (err)
goto out;
@@ -397,7 +397,7 @@ static int ext2_rename (struct user_namespace * mnt_userns,
old_inode->i_ctime = current_time(old_inode);
mark_inode_dirty(old_inode);
- ext2_delete_entry(old_de, old_page);
+ ext2_delete_entry(old_de, old_page, old_page_addr);
if (dir_de) {
if (old_dir != new_dir)
--
2.30.1
On Tue 13-07-21 16:59:18, Javier Pello wrote:
> From: Javier Pello <[email protected]>
>
> 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 <[email protected]>
> 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 <[email protected]>
SUSE Labs, CR
On Tue, 13 Jul 2021 18:30:18, Jan Kara wrote:
> On Tue 13-07-21 16:59:18, Javier Pello wrote:
> > 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 <[email protected]>
> > Fixes: 782b76d7abdf fs/ext2: Replace kmap() with kmap_local_page()
>
> Please wrap subject in Fixes tag into ("...").
Will do.
> > @@ -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.
I thought about that, as well, but in the end I leaned towards void *
because it is a generic pointer, conceptually. Would you rather have it
be char *?
> > {
> > 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'.
Fair enough.
> Also if kaddr will stay
> char *, you maybe just don't need to touch this...
Yes, if kaddr becomes char * then there is no need to touch these lines.
Javier
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 <[email protected]>
> >
> > 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 <[email protected]>
> > 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 <[email protected]>
> SUSE Labs, CR
On Tue 13-07-21 19:33:19, Javier Pello wrote:
> On Tue, 13 Jul 2021 18:30:18, Jan Kara wrote:
> > > @@ -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.
>
> I thought about that, as well, but in the end I leaned towards void *
> because it is a generic pointer, conceptually. Would you rather have it
> be char *?
Well, it depends on how you look at it. We can also think of kaddr as a
start of buffer 'dir' is pointing to. Anyway given this is not some generic
function but a very targetted one with only a few call sites I'd just lean
towards making our life easy and making kaddr char *.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
From: Javier Pello <[email protected]>
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.
Signed-off-by: Javier Pello <[email protected]>
Fixes: 782b76d7abdf ("fs/ext2: Replace kmap() with kmap_local_page()")
---
v2: Use char * for the new parameter to ext2_delete_entry.
fs/ext2/dir.c | 12 ++++++------
fs/ext2/ext2.h | 3 ++-
fs/ext2/namei.c | 4 ++--
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index 14292dba..2c2f179b 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -106,12 +106,11 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
return err;
}
-static bool ext2_check_page(struct page *page, int quiet)
+static bool ext2_check_page(struct page *page, int quiet, char *kaddr)
{
struct inode *dir = page->mapping->host;
struct super_block *sb = dir->i_sb;
unsigned chunk_size = ext2_chunk_size(dir);
- char *kaddr = page_address(page);
u32 max_inumber = le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count);
unsigned offs, rec_len;
unsigned limit = PAGE_SIZE;
@@ -205,7 +204,8 @@ static struct page * ext2_get_page(struct inode *dir, unsigned long n,
if (!IS_ERR(page)) {
*page_addr = kmap_local_page(page);
if (unlikely(!PageChecked(page))) {
- if (PageError(page) || !ext2_check_page(page, quiet))
+ if (PageError(page) || !ext2_check_page(page, quiet,
+ *page_addr))
goto fail;
}
}
@@ -584,10 +584,10 @@ 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,
+ char *kaddr)
{
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);
@@ -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 - kaddr;
pos = page_offset(page) + from;
lock_page(page);
err = ext2_prepare_chunk(page, pos, to - from);
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index b0a69482..e512630c 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -740,7 +740,8 @@ extern int ext2_inode_by_name(struct inode *dir,
extern int ext2_make_empty(struct inode *, struct inode *);
extern struct ext2_dir_entry_2 *ext2_find_entry(struct inode *, const struct qstr *,
struct page **, void **res_page_addr);
-extern int ext2_delete_entry (struct ext2_dir_entry_2 *, struct page *);
+extern int ext2_delete_entry(struct ext2_dir_entry_2 *dir, struct page *page,
+ char *kaddr);
extern int ext2_empty_dir (struct inode *);
extern struct ext2_dir_entry_2 *ext2_dotdot(struct inode *dir, struct page **p, void **pa);
extern void ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index 1f69b816..5f6b7560 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -293,7 +293,7 @@ static int ext2_unlink(struct inode * dir, struct dentry *dentry)
goto out;
}
- err = ext2_delete_entry (de, page);
+ err = ext2_delete_entry (de, page, page_addr);
ext2_put_page(page, page_addr);
if (err)
goto out;
@@ -397,7 +397,7 @@ static int ext2_rename (struct user_namespace * mnt_userns,
old_inode->i_ctime = current_time(old_inode);
mark_inode_dirty(old_inode);
- ext2_delete_entry(old_de, old_page);
+ ext2_delete_entry(old_de, old_page, old_page_addr);
if (dir_de) {
if (old_dir != new_dir)
--
2.30.1
On Wed, Jul 14, 2021 at 06:54:48PM +0200, Javier Pello wrote:
> From: Javier Pello <[email protected]>
>
> 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.
>
Again thanks for catching this!
Reviewed-by: Ira Weiny <[email protected]>
> Signed-off-by: Javier Pello <[email protected]>
> Fixes: 782b76d7abdf ("fs/ext2: Replace kmap() with kmap_local_page()")
> ---
>
> v2: Use char * for the new parameter to ext2_delete_entry.
>
> fs/ext2/dir.c | 12 ++++++------
> fs/ext2/ext2.h | 3 ++-
> fs/ext2/namei.c | 4 ++--
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
> index 14292dba..2c2f179b 100644
> --- a/fs/ext2/dir.c
> +++ b/fs/ext2/dir.c
> @@ -106,12 +106,11 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
> return err;
> }
>
> -static bool ext2_check_page(struct page *page, int quiet)
> +static bool ext2_check_page(struct page *page, int quiet, char *kaddr)
> {
> struct inode *dir = page->mapping->host;
> struct super_block *sb = dir->i_sb;
> unsigned chunk_size = ext2_chunk_size(dir);
> - char *kaddr = page_address(page);
> u32 max_inumber = le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count);
> unsigned offs, rec_len;
> unsigned limit = PAGE_SIZE;
> @@ -205,7 +204,8 @@ static struct page * ext2_get_page(struct inode *dir, unsigned long n,
> if (!IS_ERR(page)) {
> *page_addr = kmap_local_page(page);
> if (unlikely(!PageChecked(page))) {
> - if (PageError(page) || !ext2_check_page(page, quiet))
> + if (PageError(page) || !ext2_check_page(page, quiet,
> + *page_addr))
> goto fail;
> }
> }
> @@ -584,10 +584,10 @@ 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,
> + char *kaddr)
> {
> 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);
> @@ -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 - kaddr;
> pos = page_offset(page) + from;
> lock_page(page);
> err = ext2_prepare_chunk(page, pos, to - from);
> diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> index b0a69482..e512630c 100644
> --- a/fs/ext2/ext2.h
> +++ b/fs/ext2/ext2.h
> @@ -740,7 +740,8 @@ extern int ext2_inode_by_name(struct inode *dir,
> extern int ext2_make_empty(struct inode *, struct inode *);
> extern struct ext2_dir_entry_2 *ext2_find_entry(struct inode *, const struct qstr *,
> struct page **, void **res_page_addr);
> -extern int ext2_delete_entry (struct ext2_dir_entry_2 *, struct page *);
> +extern int ext2_delete_entry(struct ext2_dir_entry_2 *dir, struct page *page,
> + char *kaddr);
> extern int ext2_empty_dir (struct inode *);
> extern struct ext2_dir_entry_2 *ext2_dotdot(struct inode *dir, struct page **p, void **pa);
> extern void ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
> diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
> index 1f69b816..5f6b7560 100644
> --- a/fs/ext2/namei.c
> +++ b/fs/ext2/namei.c
> @@ -293,7 +293,7 @@ static int ext2_unlink(struct inode * dir, struct dentry *dentry)
> goto out;
> }
>
> - err = ext2_delete_entry (de, page);
> + err = ext2_delete_entry (de, page, page_addr);
> ext2_put_page(page, page_addr);
> if (err)
> goto out;
> @@ -397,7 +397,7 @@ static int ext2_rename (struct user_namespace * mnt_userns,
> old_inode->i_ctime = current_time(old_inode);
> mark_inode_dirty(old_inode);
>
> - ext2_delete_entry(old_de, old_page);
> + ext2_delete_entry(old_de, old_page, old_page_addr);
>
> if (dir_de) {
> if (old_dir != new_dir)
> --
> 2.30.1
On Wed 14-07-21 10:07:46, Ira Weiny wrote:
> On Wed, Jul 14, 2021 at 06:54:48PM +0200, Javier Pello wrote:
> > From: Javier Pello <[email protected]>
> >
> > 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.
> >
>
> Again thanks for catching this!
>
> Reviewed-by: Ira Weiny <[email protected]>
Thanks for the patch and the review. I'de added the patch to my tree.
Honza
>
> > Signed-off-by: Javier Pello <[email protected]>
> > Fixes: 782b76d7abdf ("fs/ext2: Replace kmap() with kmap_local_page()")
> > ---
> >
> > v2: Use char * for the new parameter to ext2_delete_entry.
> >
> > fs/ext2/dir.c | 12 ++++++------
> > fs/ext2/ext2.h | 3 ++-
> > fs/ext2/namei.c | 4 ++--
> > 3 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
> > index 14292dba..2c2f179b 100644
> > --- a/fs/ext2/dir.c
> > +++ b/fs/ext2/dir.c
> > @@ -106,12 +106,11 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
> > return err;
> > }
> >
> > -static bool ext2_check_page(struct page *page, int quiet)
> > +static bool ext2_check_page(struct page *page, int quiet, char *kaddr)
> > {
> > struct inode *dir = page->mapping->host;
> > struct super_block *sb = dir->i_sb;
> > unsigned chunk_size = ext2_chunk_size(dir);
> > - char *kaddr = page_address(page);
> > u32 max_inumber = le32_to_cpu(EXT2_SB(sb)->s_es->s_inodes_count);
> > unsigned offs, rec_len;
> > unsigned limit = PAGE_SIZE;
> > @@ -205,7 +204,8 @@ static struct page * ext2_get_page(struct inode *dir, unsigned long n,
> > if (!IS_ERR(page)) {
> > *page_addr = kmap_local_page(page);
> > if (unlikely(!PageChecked(page))) {
> > - if (PageError(page) || !ext2_check_page(page, quiet))
> > + if (PageError(page) || !ext2_check_page(page, quiet,
> > + *page_addr))
> > goto fail;
> > }
> > }
> > @@ -584,10 +584,10 @@ 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,
> > + char *kaddr)
> > {
> > 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);
> > @@ -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 - kaddr;
> > pos = page_offset(page) + from;
> > lock_page(page);
> > err = ext2_prepare_chunk(page, pos, to - from);
> > diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> > index b0a69482..e512630c 100644
> > --- a/fs/ext2/ext2.h
> > +++ b/fs/ext2/ext2.h
> > @@ -740,7 +740,8 @@ extern int ext2_inode_by_name(struct inode *dir,
> > extern int ext2_make_empty(struct inode *, struct inode *);
> > extern struct ext2_dir_entry_2 *ext2_find_entry(struct inode *, const struct qstr *,
> > struct page **, void **res_page_addr);
> > -extern int ext2_delete_entry (struct ext2_dir_entry_2 *, struct page *);
> > +extern int ext2_delete_entry(struct ext2_dir_entry_2 *dir, struct page *page,
> > + char *kaddr);
> > extern int ext2_empty_dir (struct inode *);
> > extern struct ext2_dir_entry_2 *ext2_dotdot(struct inode *dir, struct page **p, void **pa);
> > extern void ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
> > diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
> > index 1f69b816..5f6b7560 100644
> > --- a/fs/ext2/namei.c
> > +++ b/fs/ext2/namei.c
> > @@ -293,7 +293,7 @@ static int ext2_unlink(struct inode * dir, struct dentry *dentry)
> > goto out;
> > }
> >
> > - err = ext2_delete_entry (de, page);
> > + err = ext2_delete_entry (de, page, page_addr);
> > ext2_put_page(page, page_addr);
> > if (err)
> > goto out;
> > @@ -397,7 +397,7 @@ static int ext2_rename (struct user_namespace * mnt_userns,
> > old_inode->i_ctime = current_time(old_inode);
> > mark_inode_dirty(old_inode);
> >
> > - ext2_delete_entry(old_de, old_page);
> > + ext2_delete_entry(old_de, old_page, old_page_addr);
> >
> > if (dir_de) {
> > if (old_dir != new_dir)
> > --
> > 2.30.1
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, 15 Jul 2021 14:17:30 +0200, Jan Kara wrote:
> On Wed 14-07-21 10:07:46, Ira Weiny wrote:
> > On Wed, Jul 14, 2021 at 06:54:48PM +0200, Javier Pello wrote:
> > > From: Javier Pello <[email protected]>
> > >
> > > 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.
> >
> > Again thanks for catching this!
> >
> > Reviewed-by: Ira Weiny <[email protected]>
>
> Thanks for the patch and the review. I'de added the patch to my tree.
>
> Honza
Thank you both for dealing with this issue so quickly.
Javier