2021-07-01 03:09:14

by Desmond Cheong Zhi Xi

[permalink] [raw]
Subject: [PATCH v2 0/3] hfs: fix various errors

Hi,

This series ultimately aims to address a lockdep warning in hfs_find_init reported by Syzbot:
https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db

The work done for this led to the discovery of another bug, and the Syzkaller repro test also reveals an invalid memory access error after clearing the lockdep warning. Hence, this series is broken up into three patches:

1. Add a missing call to hfs_find_exit for an error path in hfs_fill_super

2. Fix memory mapping in hfs_bnode_read by fixing calls to kmap

3. Add lock nesting notation to tell lockdep that the observed locking hierarchy is safe

v1 -> v2:
Patch 1: Consolidated calls to hfs_find_exit on error paths in hfs_fill_super, as suggested by Viacheslav Dubeyko.
Patch 2: Added safety checks, clarified code, and switched from kmap/kunmap to kmap_atomic/kunmap_atomic, as suggested by Viacheslav Dubeyko.

Desmond Cheong Zhi Xi (3):
hfs: add missing clean-up in hfs_fill_super
hfs: fix high memory mapping in hfs_bnode_read
hfs: add lock nesting notation to hfs_find_init

fs/hfs/bfind.c | 14 +++++++++++++-
fs/hfs/bnode.c | 25 ++++++++++++++++++++-----
fs/hfs/btree.h | 7 +++++++
fs/hfs/super.c | 10 +++++-----
4 files changed, 45 insertions(+), 11 deletions(-)

--
2.25.1


2021-07-01 03:09:37

by Desmond Cheong Zhi Xi

[permalink] [raw]
Subject: [PATCH v2 1/3] hfs: add missing clean-up in hfs_fill_super

Before exiting hfs_fill_super, the struct hfs_find_data used in
hfs_find_init should be passed to hfs_find_exit to be cleaned up, and
to release the lock held on the btree.

The call to hfs_find_exit is missing from an error path. We add it
back in by consolidating calls to hfs_find_exit for error paths.

Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
fs/hfs/super.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 44d07c9e3a7f..12d9bae39363 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -420,14 +420,12 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
if (!res) {
if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
res = -EIO;
- goto bail;
+ goto bail_hfs_find;
}
hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
}
- if (res) {
- hfs_find_exit(&fd);
- goto bail_no_root;
- }
+ if (res)
+ goto bail_hfs_find;
res = -EINVAL;
root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
hfs_find_exit(&fd);
@@ -443,6 +441,8 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
/* everything's okay */
return 0;

+bail_hfs_find:
+ hfs_find_exit(&fd);
bail_no_root:
pr_err("get root inode failed\n");
bail:
--
2.25.1

2021-07-01 03:09:51

by Desmond Cheong Zhi Xi

[permalink] [raw]
Subject: [PATCH v2 2/3] hfs: fix high memory mapping in hfs_bnode_read

Pages that we read in hfs_bnode_read need to be kmapped into kernel
address space. However, currently only the 0th page is kmapped. If the
given offset + length exceeds this 0th page, then we have an invalid
memory access.

To fix this, we kmap relevant pages one by one and copy their relevant
portions of data.

An example of invalid memory access occurring without this fix can be
seen in the following crash report:

==================================================================
BUG: KASAN: use-after-free in memcpy include/linux/fortify-string.h:191 [inline]
BUG: KASAN: use-after-free in hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
Read of size 2 at addr ffff888125fdcffe by task syz-executor5/4634

CPU: 0 PID: 4634 Comm: syz-executor5 Not tainted 5.13.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:79 [inline]
dump_stack+0x195/0x1f8 lib/dump_stack.c:120
print_address_description.constprop.0+0x1d/0x110 mm/kasan/report.c:233
__kasan_report mm/kasan/report.c:419 [inline]
kasan_report.cold+0x7b/0xd4 mm/kasan/report.c:436
check_region_inline mm/kasan/generic.c:180 [inline]
kasan_check_range+0x154/0x1b0 mm/kasan/generic.c:186
memcpy+0x24/0x60 mm/kasan/shadow.c:65
memcpy include/linux/fortify-string.h:191 [inline]
hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
hfs_bnode_read_u16 fs/hfs/bnode.c:34 [inline]
hfs_bnode_find+0x880/0xcc0 fs/hfs/bnode.c:365
hfs_brec_find+0x2d8/0x540 fs/hfs/bfind.c:126
hfs_brec_read+0x27/0x120 fs/hfs/bfind.c:165
hfs_cat_find_brec+0x19a/0x3b0 fs/hfs/catalog.c:194
hfs_fill_super+0xc13/0x1460 fs/hfs/super.c:419
mount_bdev+0x331/0x3f0 fs/super.c:1368
hfs_mount+0x35/0x40 fs/hfs/super.c:457
legacy_get_tree+0x10c/0x220 fs/fs_context.c:592
vfs_get_tree+0x93/0x300 fs/super.c:1498
do_new_mount fs/namespace.c:2905 [inline]
path_mount+0x13f5/0x20e0 fs/namespace.c:3235
do_mount fs/namespace.c:3248 [inline]
__do_sys_mount fs/namespace.c:3456 [inline]
__se_sys_mount fs/namespace.c:3433 [inline]
__x64_sys_mount+0x2b8/0x340 fs/namespace.c:3433
do_syscall_64+0x37/0xc0 arch/x86/entry/common.c:47
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x45e63a
Code: 48 c7 c2 bc ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 88 04 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f9404d410d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000000020000248 RCX: 000000000045e63a
RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f9404d41120
RBP: 00007f9404d41120 R08: 00000000200002c0 R09: 0000000020000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
R13: 0000000000000003 R14: 00000000004ad5d8 R15: 0000000000000000

The buggy address belongs to the page:
page:00000000dadbcf3e refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x125fdc
flags: 0x2fffc0000000000(node=0|zone=2|lastcpupid=0x3fff)
raw: 02fffc0000000000 ffffea000497f748 ffffea000497f6c8 0000000000000000
raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff888125fdce80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff888125fdcf00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>ffff888125fdcf80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff888125fdd000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff888125fdd080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================

Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
fs/hfs/bnode.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index b63a4df7327b..c0a73a6ffb28 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -15,16 +15,31 @@

#include "btree.h"

-void hfs_bnode_read(struct hfs_bnode *node, void *buf,
- int off, int len)
+void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
{
struct page *page;
+ int pagenum;
+ int bytes_read;
+ int bytes_to_read;
+ void *vaddr;

off += node->page_offset;
- page = node->page[0];
+ pagenum = off >> PAGE_SHIFT;
+ off &= ~PAGE_MASK; /* compute page offset for the first page */

- memcpy(buf, kmap(page) + off, len);
- kunmap(page);
+ for (bytes_read = 0; bytes_read < len; bytes_read += bytes_to_read) {
+ if (pagenum >= node->tree->pages_per_bnode)
+ break;
+ page = node->page[pagenum];
+ bytes_to_read = min_t(int, len - bytes_read, PAGE_SIZE - off);
+
+ vaddr = kmap_atomic(page);
+ memcpy(buf + bytes_read, vaddr + off, bytes_to_read);
+ kunmap_atomic(vaddr);
+
+ pagenum++;
+ off = 0; /* page offset only applies to the first page */
+ }
}

u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
--
2.25.1

2021-07-01 03:11:31

by Desmond Cheong Zhi Xi

[permalink] [raw]
Subject: [PATCH v2 3/3] hfs: add lock nesting notation to hfs_find_init

Syzbot reports a possible recursive lock:
https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db

This happens due to missing lock nesting information. From the logs,
we see that a call to hfs_fill_super is made to mount the hfs
filesystem. While searching for the root inode, the lock on the
catalog btree is grabbed. Then, when the parent of the root isn't
found, a call to __hfs_bnode_create is made to create the parent of
the root. This eventually leads to a call to hfs_ext_read_extent which
grabs a lock on the extents btree.

Since the order of locking is catalog btree -> extents btree, this
lock hierarchy does not lead to a deadlock.

To tell lockdep that this locking is safe, we add nesting notation to
distinguish between catalog btrees, extents btrees, and attributes
btrees (for HFS+). This has already been done in hfsplus.

Reported-and-tested-by: [email protected]
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
Reviewed-by: Viacheslav Dubeyko <[email protected]>
---
fs/hfs/bfind.c | 14 +++++++++++++-
fs/hfs/btree.h | 7 +++++++
2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
index 4af318fbda77..ef9498a6e88a 100644
--- a/fs/hfs/bfind.c
+++ b/fs/hfs/bfind.c
@@ -25,7 +25,19 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
fd->key = ptr + tree->max_key_len + 2;
hfs_dbg(BNODE_REFS, "find_init: %d (%p)\n",
tree->cnid, __builtin_return_address(0));
- mutex_lock(&tree->tree_lock);
+ switch (tree->cnid) {
+ case HFS_CAT_CNID:
+ mutex_lock_nested(&tree->tree_lock, CATALOG_BTREE_MUTEX);
+ break;
+ case HFS_EXT_CNID:
+ mutex_lock_nested(&tree->tree_lock, EXTENTS_BTREE_MUTEX);
+ break;
+ case HFS_ATTR_CNID:
+ mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
+ break;
+ default:
+ return -EINVAL;
+ }
return 0;
}

diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
index 4ba45caf5939..0e6baee93245 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -13,6 +13,13 @@ typedef int (*btree_keycmp)(const btree_key *, const btree_key *);

#define NODE_HASH_SIZE 256

+/* B-tree mutex nested subclasses */
+enum hfs_btree_mutex_classes {
+ CATALOG_BTREE_MUTEX,
+ EXTENTS_BTREE_MUTEX,
+ ATTR_BTREE_MUTEX,
+};
+
/* A HFS BTree held in memory */
struct hfs_btree {
struct super_block *sb;
--
2.25.1

2021-07-01 17:05:00

by Viacheslav Dubeyko

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] hfs: add missing clean-up in hfs_fill_super



> On Jun 30, 2021, at 8:07 PM, Desmond Cheong Zhi Xi <[email protected]> wrote:
>
> Before exiting hfs_fill_super, the struct hfs_find_data used in
> hfs_find_init should be passed to hfs_find_exit to be cleaned up, and
> to release the lock held on the btree.
>
> The call to hfs_find_exit is missing from an error path. We add it
> back in by consolidating calls to hfs_find_exit for error paths.
>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> fs/hfs/super.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/hfs/super.c b/fs/hfs/super.c
> index 44d07c9e3a7f..12d9bae39363 100644
> --- a/fs/hfs/super.c
> +++ b/fs/hfs/super.c
> @@ -420,14 +420,12 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
> if (!res) {
> if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) {
> res = -EIO;
> - goto bail;
> + goto bail_hfs_find;
> }
> hfs_bnode_read(fd.bnode, &rec, fd.entryoffset, fd.entrylength);
> }
> - if (res) {
> - hfs_find_exit(&fd);
> - goto bail_no_root;
> - }
> + if (res)
> + goto bail_hfs_find;
> res = -EINVAL;
> root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
> hfs_find_exit(&fd);
> @@ -443,6 +441,8 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent)
> /* everything's okay */
> return 0;
>
> +bail_hfs_find:
> + hfs_find_exit(&fd);
> bail_no_root:
> pr_err("get root inode failed\n");
> bail:
> --
> 2.25.1
>

Looks good.

Reviewed-by: Viacheslav Dubeyko <[email protected]>

Thanks,
Slava.

2021-07-01 17:07:07

by Viacheslav Dubeyko

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] hfs: fix high memory mapping in hfs_bnode_read



> On Jun 30, 2021, at 8:07 PM, Desmond Cheong Zhi Xi <[email protected]> wrote:
>
> Pages that we read in hfs_bnode_read need to be kmapped into kernel
> address space. However, currently only the 0th page is kmapped. If the
> given offset + length exceeds this 0th page, then we have an invalid
> memory access.
>
> To fix this, we kmap relevant pages one by one and copy their relevant
> portions of data.
>
> An example of invalid memory access occurring without this fix can be
> seen in the following crash report:
>
> ==================================================================
> BUG: KASAN: use-after-free in memcpy include/linux/fortify-string.h:191 [inline]
> BUG: KASAN: use-after-free in hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
> Read of size 2 at addr ffff888125fdcffe by task syz-executor5/4634
>
> CPU: 0 PID: 4634 Comm: syz-executor5 Not tainted 5.13.0-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:79 [inline]
> dump_stack+0x195/0x1f8 lib/dump_stack.c:120
> print_address_description.constprop.0+0x1d/0x110 mm/kasan/report.c:233
> __kasan_report mm/kasan/report.c:419 [inline]
> kasan_report.cold+0x7b/0xd4 mm/kasan/report.c:436
> check_region_inline mm/kasan/generic.c:180 [inline]
> kasan_check_range+0x154/0x1b0 mm/kasan/generic.c:186
> memcpy+0x24/0x60 mm/kasan/shadow.c:65
> memcpy include/linux/fortify-string.h:191 [inline]
> hfs_bnode_read+0xc4/0xe0 fs/hfs/bnode.c:26
> hfs_bnode_read_u16 fs/hfs/bnode.c:34 [inline]
> hfs_bnode_find+0x880/0xcc0 fs/hfs/bnode.c:365
> hfs_brec_find+0x2d8/0x540 fs/hfs/bfind.c:126
> hfs_brec_read+0x27/0x120 fs/hfs/bfind.c:165
> hfs_cat_find_brec+0x19a/0x3b0 fs/hfs/catalog.c:194
> hfs_fill_super+0xc13/0x1460 fs/hfs/super.c:419
> mount_bdev+0x331/0x3f0 fs/super.c:1368
> hfs_mount+0x35/0x40 fs/hfs/super.c:457
> legacy_get_tree+0x10c/0x220 fs/fs_context.c:592
> vfs_get_tree+0x93/0x300 fs/super.c:1498
> do_new_mount fs/namespace.c:2905 [inline]
> path_mount+0x13f5/0x20e0 fs/namespace.c:3235
> do_mount fs/namespace.c:3248 [inline]
> __do_sys_mount fs/namespace.c:3456 [inline]
> __se_sys_mount fs/namespace.c:3433 [inline]
> __x64_sys_mount+0x2b8/0x340 fs/namespace.c:3433
> do_syscall_64+0x37/0xc0 arch/x86/entry/common.c:47
> entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x45e63a
> Code: 48 c7 c2 bc ff ff ff f7 d8 64 89 02 b8 ff ff ff ff eb d2 e8 88 04 00 00 0f 1f 84 00 00 00 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f9404d410d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> RAX: ffffffffffffffda RBX: 0000000020000248 RCX: 000000000045e63a
> RDX: 0000000020000000 RSI: 0000000020000100 RDI: 00007f9404d41120
> RBP: 00007f9404d41120 R08: 00000000200002c0 R09: 0000000020000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000003
> R13: 0000000000000003 R14: 00000000004ad5d8 R15: 0000000000000000
>
> The buggy address belongs to the page:
> page:00000000dadbcf3e refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x125fdc
> flags: 0x2fffc0000000000(node=0|zone=2|lastcpupid=0x3fff)
> raw: 02fffc0000000000 ffffea000497f748 ffffea000497f6c8 0000000000000000
> raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888125fdce80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff888125fdcf00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
>> ffff888125fdcf80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ^
> ffff888125fdd000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff888125fdd080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ==================================================================
>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> fs/hfs/bnode.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index b63a4df7327b..c0a73a6ffb28 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -15,16 +15,31 @@
>
> #include "btree.h"
>
> -void hfs_bnode_read(struct hfs_bnode *node, void *buf,
> - int off, int len)
> +void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int len)
> {
> struct page *page;
> + int pagenum;
> + int bytes_read;
> + int bytes_to_read;
> + void *vaddr;
>
> off += node->page_offset;
> - page = node->page[0];
> + pagenum = off >> PAGE_SHIFT;
> + off &= ~PAGE_MASK; /* compute page offset for the first page */
>
> - memcpy(buf, kmap(page) + off, len);
> - kunmap(page);
> + for (bytes_read = 0; bytes_read < len; bytes_read += bytes_to_read) {
> + if (pagenum >= node->tree->pages_per_bnode)
> + break;
> + page = node->page[pagenum];
> + bytes_to_read = min_t(int, len - bytes_read, PAGE_SIZE - off);
> +
> + vaddr = kmap_atomic(page);
> + memcpy(buf + bytes_read, vaddr + off, bytes_to_read);
> + kunmap_atomic(vaddr);
> +
> + pagenum++;
> + off = 0; /* page offset only applies to the first page */
> + }
> }
>
> u16 hfs_bnode_read_u16(struct hfs_bnode *node, int off)
> --
> 2.25.1
>

Looks good.

Reviewed-by: Viacheslav Dubeyko <[email protected]>

Thanks,
Slava.

2021-07-01 17:09:44

by Viacheslav Dubeyko

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] hfs: add lock nesting notation to hfs_find_init



> On Jun 30, 2021, at 8:07 PM, Desmond Cheong Zhi Xi <[email protected]> wrote:
>
> Syzbot reports a possible recursive lock:
> https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db
>
> This happens due to missing lock nesting information. From the logs,
> we see that a call to hfs_fill_super is made to mount the hfs
> filesystem. While searching for the root inode, the lock on the
> catalog btree is grabbed. Then, when the parent of the root isn't
> found, a call to __hfs_bnode_create is made to create the parent of
> the root. This eventually leads to a call to hfs_ext_read_extent which
> grabs a lock on the extents btree.
>
> Since the order of locking is catalog btree -> extents btree, this
> lock hierarchy does not lead to a deadlock.
>
> To tell lockdep that this locking is safe, we add nesting notation to
> distinguish between catalog btrees, extents btrees, and attributes
> btrees (for HFS+). This has already been done in hfsplus.
>
> Reported-and-tested-by: [email protected]
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> Reviewed-by: Viacheslav Dubeyko <[email protected]>
> ---
> fs/hfs/bfind.c | 14 +++++++++++++-
> fs/hfs/btree.h | 7 +++++++
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/fs/hfs/bfind.c b/fs/hfs/bfind.c
> index 4af318fbda77..ef9498a6e88a 100644
> --- a/fs/hfs/bfind.c
> +++ b/fs/hfs/bfind.c
> @@ -25,7 +25,19 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
> fd->key = ptr + tree->max_key_len + 2;
> hfs_dbg(BNODE_REFS, "find_init: %d (%p)\n",
> tree->cnid, __builtin_return_address(0));
> - mutex_lock(&tree->tree_lock);
> + switch (tree->cnid) {
> + case HFS_CAT_CNID:
> + mutex_lock_nested(&tree->tree_lock, CATALOG_BTREE_MUTEX);
> + break;
> + case HFS_EXT_CNID:
> + mutex_lock_nested(&tree->tree_lock, EXTENTS_BTREE_MUTEX);
> + break;
> + case HFS_ATTR_CNID:
> + mutex_lock_nested(&tree->tree_lock, ATTR_BTREE_MUTEX);
> + break;
> + default:
> + return -EINVAL;
> + }
> return 0;
> }
>
> diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
> index 4ba45caf5939..0e6baee93245 100644
> --- a/fs/hfs/btree.h
> +++ b/fs/hfs/btree.h
> @@ -13,6 +13,13 @@ typedef int (*btree_keycmp)(const btree_key *, const btree_key *);
>
> #define NODE_HASH_SIZE 256
>
> +/* B-tree mutex nested subclasses */
> +enum hfs_btree_mutex_classes {
> + CATALOG_BTREE_MUTEX,
> + EXTENTS_BTREE_MUTEX,
> + ATTR_BTREE_MUTEX,
> +};
> +
> /* A HFS BTree held in memory */
> struct hfs_btree {
> struct super_block *sb;
> --
> 2.25.1
>

Looks good.

Reviewed-by: Viacheslav Dubeyko <[email protected]>

Thanks,
Slava.


2021-07-01 17:46:16

by Viacheslav Dubeyko

[permalink] [raw]
Subject: Re: [PATCH v2 0/3] hfs: fix various errors



> On Jun 30, 2021, at 8:07 PM, Desmond Cheong Zhi Xi <[email protected]> wrote:
>
> Hi,
>
> This series ultimately aims to address a lockdep warning in hfs_find_init reported by Syzbot:
> https://syzkaller.appspot.com/bug?id=f007ef1d7a31a469e3be7aeb0fde0769b18585db
>
> The work done for this led to the discovery of another bug, and the Syzkaller repro test also reveals an invalid memory access error after clearing the lockdep warning. Hence, this series is broken up into three patches:
>
> 1. Add a missing call to hfs_find_exit for an error path in hfs_fill_super
>
> 2. Fix memory mapping in hfs_bnode_read by fixing calls to kmap
>
> 3. Add lock nesting notation to tell lockdep that the observed locking hierarchy is safe
>
> v1 -> v2:
> Patch 1: Consolidated calls to hfs_find_exit on error paths in hfs_fill_super, as suggested by Viacheslav Dubeyko.
> Patch 2: Added safety checks, clarified code, and switched from kmap/kunmap to kmap_atomic/kunmap_atomic, as suggested by Viacheslav Dubeyko.
>
> Desmond Cheong Zhi Xi (3):
> hfs: add missing clean-up in hfs_fill_super
> hfs: fix high memory mapping in hfs_bnode_read
> hfs: add lock nesting notation to hfs_find_init
>
> fs/hfs/bfind.c | 14 +++++++++++++-
> fs/hfs/bnode.c | 25 ++++++++++++++++++++-----
> fs/hfs/btree.h | 7 +++++++
> fs/hfs/super.c | 10 +++++-----
> 4 files changed, 45 insertions(+), 11 deletions(-)
>
> --
> 2.25.1
>

Looks good.

Reviewed-by: Viacheslav Dubeyko <[email protected]>

Thanks,
Slava.