[PATCH 0/1] Kernel oopses on ext2 filesystems after 782b76d7abdf

[PATCH 0/1] Kernel oopses on ext2 filesystems after 782b76d7abdf

[Javier Pello]

From: Javier Pello <javier.pello@urjc.es>

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

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

[Javier Pello]

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.

Signed-off-by: Javier Pello <javier.pello@urjc.es>
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

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

[Jan Kara]

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

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

[Javier Pello]

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 <javier.pello@urjc.es>
> > 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

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

[Jan Kara]

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 <jack@suse.com>
SUSE Labs, CR

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

[Javier Pello]

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.

Signed-off-by: Javier Pello <javier.pello@urjc.es>
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

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

[Ira Weiny]

On Wed, Jul 14, 2021 at 06:54:48PM +0200, 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.
> 

Again thanks for catching this!

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Javier Pello <javier.pello@urjc.es>
> 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

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

[Jan Kara]

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 <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.
> > 
> 
> Again thanks for catching this!
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Thanks for the patch and the review. I'de added the patch to my tree.

								Honza

> 
> > Signed-off-by: Javier Pello <javier.pello@urjc.es>
> > 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 <jack@suse.com>
SUSE Labs, CR

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

[Javier Pello]

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 <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.
> > 
> > Again thanks for catching this!
> > 
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> 
> 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

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@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?

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@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