Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/3] hfs: add missing clean-up in hfs_fill_super | From | Desmond Cheong Zhi Xi <> | Date | Wed, 30 Jun 2021 12:50:24 +0800 |
| |
On 30/6/21 3:13 am, Viacheslav Dubeyko wrote: > > >> On Jun 29, 2021, at 7:48 AM, Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote: >> >> On exiting hfs_fill_super, the file descriptor 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 this error path, so we add >> it in to release resources. >> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> --- >> fs/hfs/super.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/hfs/super.c b/fs/hfs/super.c >> index 44d07c9e3a7f..48340b77eb36 100644 >> --- a/fs/hfs/super.c >> +++ b/fs/hfs/super.c >> @@ -419,6 +419,7 @@ static int hfs_fill_super(struct super_block *sb, void *data, int silent) >> res = hfs_cat_find_brec(sb, HFS_ROOT_CNID, &fd); >> if (!res) { >> if (fd.entrylength > sizeof(rec) || fd.entrylength < 0) { >> + hfs_find_exit(&fd); > > I see that there are several places of hfs_find_exit() calls in hfs_fill_super(). Maybe, it makes sense to move the hfs_find_exit() call to the end of the hfs_fill_super()? In this case we could process this activity of resources freeing into one place. I mean line 449 in the source code (failure case). > > Thanks, > Slava. > >> res = -EIO; >> goto bail; >> } >> -- >> 2.25.1 >> >
Thanks for the suggestion. Since the bail and bail_no_root error paths are used before hfs_find_init and after hfs_find_exit are called in the normal execution case, moving hfs_find_exit under the bail label wouldn't work.
Perhaps this can be done by introducing another goto label. Any thoughts on the following?
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:
| |