| Message ID | 20220408103012.1419-6-heming.zhao@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rewrite error handling during mounting stage | expand |
On 4/8/22 6:30 PM, Heming Zhao wrote: > current ocfs2_fill_super error handling is very simple, and includes > kernel crash possibility. > > e.g. > > ocfs2_fill_super > ocfs2_initialize_super // fails in check max_slots & return > > Then osb->osb_mount_event is not properly initialized, and then will > crash at wake_up(&osb->osb_mount_event) in goto label read_super_error. > With patch 3, this case won't exist any more. > For fixing the issue and enhancing error handling, we rewrite error > handling of ocfs2_fill_super. > > Signed-off-by: Heming Zhao <heming.zhao@suse.com> > --- > fs/ocfs2/super.c | 67 ++++++++++++++++++++++++------------------------ > 1 file changed, 33 insertions(+), 34 deletions(-) > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > index d4b7a29cb364..c3efd9ad49ac 100644 > --- a/fs/ocfs2/super.c > +++ b/fs/ocfs2/super.c > @@ -989,28 +989,27 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { > status = -EINVAL; > - goto read_super_error; > + goto out; > } > > /* probe for superblock */ > status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); > if (status < 0) { > mlog(ML_ERROR, "superblock probe failed!\n"); > - goto read_super_error; > + goto out; > } > > status = ocfs2_initialize_super(sb, bh, sector_size, &stats); > - osb = OCFS2_SB(sb); > - if (status < 0) { > - mlog_errno(status); > - goto read_super_error; > - } > brelse(bh); > bh = NULL; > + if (status < 0) > + goto out; > + > + osb = OCFS2_SB(sb); > > if (!ocfs2_check_set_options(sb, &parsed_options)) { > status = -EINVAL; > - goto read_super_error; > + goto out_super; > } > osb->s_mount_opt = parsed_options.mount_opt; > osb->s_atime_quantum = parsed_options.atime_quantum; > @@ -1027,7 +1026,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = ocfs2_verify_userspace_stack(osb, &parsed_options); > if (status) > - goto read_super_error; > + goto out_super; > > sb->s_magic = OCFS2_SUPER_MAGIC; > > @@ -1041,7 +1040,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > status = -EACCES; > mlog(ML_ERROR, "Readonly device detected but readonly " > "mount was not specified.\n"); > - goto read_super_error; > + goto out_super; > } > > /* You should not be able to start a local heartbeat > @@ -1050,7 +1049,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > status = -EROFS; > mlog(ML_ERROR, "Local heartbeat specified on readonly " > "device.\n"); > - goto read_super_error; > + goto out_super; > } > > status = ocfs2_check_journals_nolocks(osb); > @@ -1059,9 +1058,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > mlog(ML_ERROR, "Recovery required on readonly " > "file system, but write access is " > "unavailable.\n"); > - else > - mlog_errno(status); > - goto read_super_error; > + goto out_super; > } > > ocfs2_set_ro_flag(osb, 1); > @@ -1077,10 +1074,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > } > > status = ocfs2_verify_heartbeat(osb); > - if (status < 0) { > - mlog_errno(status); > - goto read_super_error; > - } > + if (status < 0) > + goto out_super; > > osb->osb_debug_root = debugfs_create_dir(osb->uuid_str, > ocfs2_debugfs_root); > @@ -1094,15 +1089,14 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = ocfs2_mount_volume(sb); > if (status < 0) > - goto read_super_error; > + goto out_debugfs; > > if (osb->root_inode) > inode = igrab(osb->root_inode); > > if (!inode) { > status = -EIO; > - mlog_errno(status); > - goto read_super_error; > + goto out_journal; > } > > osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL, > @@ -1110,7 +1104,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > if (!osb->osb_dev_kset) { > status = -ENOMEM; > mlog(ML_ERROR, "Unable to create device kset %s.\n", sb->s_id); > - goto read_super_error; > + goto out_journal; > } > > /* Create filecheck sysfs related directories/files at > @@ -1119,14 +1113,13 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > status = -ENOMEM; > mlog(ML_ERROR, "Unable to create filecheck sysfs directory at " > "/sys/fs/ocfs2/%s/filecheck.\n", sb->s_id); > - goto read_super_error; > + goto out_journal; > } > > root = d_make_root(inode); > if (!root) { > status = -ENOMEM; > - mlog_errno(status); > - goto read_super_error; > + goto out_journal; > } > > sb->s_root = root; > @@ -1178,17 +1171,23 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > return status; > > -read_super_error: > - brelse(bh); > +out_journal: rename it to out_dismount? > + mlog_errno(status); > + atomic_set(&osb->vol_state, VOLUME_DISABLED); > + wake_up(&osb->osb_mount_event); > + ocfs2_dismount_volume(sb, 1); > > - if (status) > - mlog_errno(status); > + return status; Or remove mlog_errno() before and then just goto out? > > - if (osb) { > - atomic_set(&osb->vol_state, VOLUME_DISABLED); > - wake_up(&osb->osb_mount_event); > - ocfs2_dismount_volume(sb, 1); > - } > +out_debugfs: > + debugfs_remove_recursive(osb->osb_debug_root); > +out_super: > + ocfs2_release_system_inodes(osb); > + kfree(osb->recovery_map); > + ocfs2_delete_osb(osb); > + kfree(osb); Seems we have to take care all resources in ocfs2_initialize_super()? > +out: > + mlog_errno(status); > As I described in your previous mail, ocfs2_sb_probe() may return error with non-NULL bh, so we should brelse here. Thanks, Joseph > return status; > }
On 4/9/22 22:02, Joseph Qi wrote: > > > On 4/8/22 6:30 PM, Heming Zhao wrote: >> current ocfs2_fill_super error handling is very simple, and includes >> kernel crash possibility. >> >> e.g. >> >> ocfs2_fill_super >> ocfs2_initialize_super // fails in check max_slots & return >> >> Then osb->osb_mount_event is not properly initialized, and then will >> crash at wake_up(&osb->osb_mount_event) in goto label read_super_error. >> > With patch 3, this case won't exist any more. Agree. After patch 3, ocfs2_initialize_super() will free osb before returning failure, and this issue won't be triggered. I will change it in next version. > >> For fixing the issue and enhancing error handling, we rewrite error >> handling of ocfs2_fill_super. >> >> Signed-off-by: Heming Zhao <heming.zhao@suse.com> >> --- >> fs/ocfs2/super.c | 67 ++++++++++++++++++++++++------------------------ >> 1 file changed, 33 insertions(+), 34 deletions(-) >> >> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c >> index d4b7a29cb364..c3efd9ad49ac 100644 >> --- a/fs/ocfs2/super.c >> +++ b/fs/ocfs2/super.c >> @@ -989,28 +989,27 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> >> if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { >> status = -EINVAL; >> - goto read_super_error; >> + goto out; >> } >> >> /* probe for superblock */ >> status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); >> if (status < 0) { >> mlog(ML_ERROR, "superblock probe failed!\n"); >> - goto read_super_error; >> + goto out; >> } >> >> status = ocfs2_initialize_super(sb, bh, sector_size, &stats); >> - osb = OCFS2_SB(sb); >> - if (status < 0) { >> - mlog_errno(status); >> - goto read_super_error; >> - } >> brelse(bh); >> bh = NULL; >> + if (status < 0) >> + goto out; >> + >> + osb = OCFS2_SB(sb); >> >> if (!ocfs2_check_set_options(sb, &parsed_options)) { >> status = -EINVAL; >> - goto read_super_error; >> + goto out_super; >> } >> osb->s_mount_opt = parsed_options.mount_opt; >> osb->s_atime_quantum = parsed_options.atime_quantum; >> @@ -1027,7 +1026,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> >> status = ocfs2_verify_userspace_stack(osb, &parsed_options); >> if (status) >> - goto read_super_error; >> + goto out_super; >> >> sb->s_magic = OCFS2_SUPER_MAGIC; >> >> @@ -1041,7 +1040,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> status = -EACCES; >> mlog(ML_ERROR, "Readonly device detected but readonly " >> "mount was not specified.\n"); >> - goto read_super_error; >> + goto out_super; >> } >> >> /* You should not be able to start a local heartbeat >> @@ -1050,7 +1049,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> status = -EROFS; >> mlog(ML_ERROR, "Local heartbeat specified on readonly " >> "device.\n"); >> - goto read_super_error; >> + goto out_super; >> } >> >> status = ocfs2_check_journals_nolocks(osb); >> @@ -1059,9 +1058,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> mlog(ML_ERROR, "Recovery required on readonly " >> "file system, but write access is " >> "unavailable.\n"); >> - else >> - mlog_errno(status); >> - goto read_super_error; >> + goto out_super; >> } >> >> ocfs2_set_ro_flag(osb, 1); >> @@ -1077,10 +1074,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> } >> >> status = ocfs2_verify_heartbeat(osb); >> - if (status < 0) { >> - mlog_errno(status); >> - goto read_super_error; >> - } >> + if (status < 0) >> + goto out_super; >> >> osb->osb_debug_root = debugfs_create_dir(osb->uuid_str, >> ocfs2_debugfs_root); >> @@ -1094,15 +1089,14 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> >> status = ocfs2_mount_volume(sb); >> if (status < 0) >> - goto read_super_error; >> + goto out_debugfs; >> >> if (osb->root_inode) >> inode = igrab(osb->root_inode); >> >> if (!inode) { >> status = -EIO; >> - mlog_errno(status); >> - goto read_super_error; >> + goto out_journal; >> } >> >> osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL, >> @@ -1110,7 +1104,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> if (!osb->osb_dev_kset) { >> status = -ENOMEM; >> mlog(ML_ERROR, "Unable to create device kset %s.\n", sb->s_id); >> - goto read_super_error; >> + goto out_journal; >> } >> >> /* Create filecheck sysfs related directories/files at >> @@ -1119,14 +1113,13 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> status = -ENOMEM; >> mlog(ML_ERROR, "Unable to create filecheck sysfs directory at " >> "/sys/fs/ocfs2/%s/filecheck.\n", sb->s_id); >> - goto read_super_error; >> + goto out_journal; >> } >> >> root = d_make_root(inode); >> if (!root) { >> status = -ENOMEM; >> - mlog_errno(status); >> - goto read_super_error; >> + goto out_journal; >> } >> >> sb->s_root = root; >> @@ -1178,17 +1171,23 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) >> >> return status; >> >> -read_super_error: >> - brelse(bh); >> +out_journal: > > rename it to out_dismount? agree. > >> + mlog_errno(status); >> + atomic_set(&osb->vol_state, VOLUME_DISABLED); >> + wake_up(&osb->osb_mount_event); >> + ocfs2_dismount_volume(sb, 1); >> >> - if (status) >> - mlog_errno(status); >> + return status; > > Or remove mlog_errno() before and then just goto out? your mean: at the end of lable "out_journal", call "goto out"? if yes, no problem for me. > >> >> - if (osb) { >> - atomic_set(&osb->vol_state, VOLUME_DISABLED); >> - wake_up(&osb->osb_mount_event); >> - ocfs2_dismount_volume(sb, 1); >> - } >> +out_debugfs: >> + debugfs_remove_recursive(osb->osb_debug_root); >> +out_super: >> + ocfs2_release_system_inodes(osb); >> + kfree(osb->recovery_map); >> + ocfs2_delete_osb(osb); >> + kfree(osb); > > Seems we have to take care all resources in ocfs2_initialize_super()? Yes, I checked every lines. for ocfs2_initialize_super(), I lists all my found resources: (list in time order): osb, osb->recovery_map, osb->vol_label, osb->slot_recovery_generations, osb->osb_orphan_wipes, osb->journal, osb->uuid_str - all above have itselves out_xx label. osb->osb_dlm_debug & dlm sysfs stuffs. - release by out_dlm_out system inodes (by ocfs2_init_global_system_inodes) - release by out_system_inodes osb->slot_info, osb->slot_info->si_bh - release by out_slot_info workqueue:osb->ocfs2_wq - will be freed in ocfs2_fill_super label "out_super": ocfs2_delete_osb(). or be freed in ocfs2_dismount_volume => ocfs2_delete_osb(). > >> +out: >> + mlog_errno(status); >> > As I described in your previous mail, ocfs2_sb_probe() may return error > with non-NULL bh, so we should brelse here. > I also explained why I dropped the brelse(bh), maybe anti-spam system ate my mail. (I found my emails also disappeared in other mailling lists.) I re-explain here. below code based on patch 5/5. please note <2> & <4>. ``` static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) { ... ... if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { status = -EINVAL; goto out; } //hm: <1> bh is alloced/created from here. /* probe for superblock */ status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); if (status < 0) { mlog(ML_ERROR, "superblock probe failed!\n"); //hm: <2> if error, ocfs2_sb_probe() will release bh. So "out" label //doesn't need to call brelse(bh). goto out; } status = ocfs2_initialize_super(sb, bh, sector_size, &stats); brelse(bh); //hm: <3> release bh before assessment status. bh = NULL; if (status < 0) goto out; //hm: <4> if error, bh is already released before "if()", // So "out" label does't need to call brelse(bh) osb = OCFS2_SB(sb); ... ... out: mlog_errno(status); return status; } ``` Thanks, Heming
Resend my reply for easy review. On Sat, Apr 09, 2022 at 10:02:16PM +0800, Joseph Qi wrote: > > > On 4/8/22 6:30 PM, Heming Zhao wrote: > > current ocfs2_fill_super error handling is very simple, and includes > > kernel crash possibility. > > > > e.g. > > > > ocfs2_fill_super > > ocfs2_initialize_super // fails in check max_slots & return > > > > Then osb->osb_mount_event is not properly initialized, and then will > > crash at wake_up(&osb->osb_mount_event) in goto label read_super_error. > > > With patch 3, this case won't exist any more. Agree. After patch 3, ocfs2_initialize_super() will free osb before returning failure, and this issue won't be triggered. I will change the commit log in next version. > > > For fixing the issue and enhancing error handling, we rewrite error > > handling of ocfs2_fill_super. > > > > Signed-off-by: Heming Zhao <heming.zhao@suse.com> > > --- > > fs/ocfs2/super.c | 67 ++++++++++++++++++++++++------------------------ > > 1 file changed, 33 insertions(+), 34 deletions(-) > > > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > > index d4b7a29cb364..c3efd9ad49ac 100644 > > --- a/fs/ocfs2/super.c > > +++ b/fs/ocfs2/super.c > > @@ -989,28 +989,27 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { > > status = -EINVAL; > > - goto read_super_error; > > + goto out; > > } > > > > /* probe for superblock */ > > status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); > > if (status < 0) { > > mlog(ML_ERROR, "superblock probe failed!\n"); > > - goto read_super_error; > > + goto out; > > } > > > > status = ocfs2_initialize_super(sb, bh, sector_size, &stats); > > - osb = OCFS2_SB(sb); > > - if (status < 0) { > > - mlog_errno(status); > > - goto read_super_error; > > - } > > brelse(bh); > > bh = NULL; > > + if (status < 0) > > + goto out; > > + > > + osb = OCFS2_SB(sb); > > > > if (!ocfs2_check_set_options(sb, &parsed_options)) { > > status = -EINVAL; > > - goto read_super_error; > > + goto out_super; > > } > > osb->s_mount_opt = parsed_options.mount_opt; > > osb->s_atime_quantum = parsed_options.atime_quantum; > > @@ -1027,7 +1026,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > status = ocfs2_verify_userspace_stack(osb, &parsed_options); > > if (status) > > - goto read_super_error; > > + goto out_super; > > > > sb->s_magic = OCFS2_SUPER_MAGIC; > > > > @@ -1041,7 +1040,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = -EACCES; > > mlog(ML_ERROR, "Readonly device detected but readonly " > > "mount was not specified.\n"); > > - goto read_super_error; > > + goto out_super; > > } > > > > /* You should not be able to start a local heartbeat > > @@ -1050,7 +1049,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = -EROFS; > > mlog(ML_ERROR, "Local heartbeat specified on readonly " > > "device.\n"); > > - goto read_super_error; > > + goto out_super; > > } > > > > status = ocfs2_check_journals_nolocks(osb); > > @@ -1059,9 +1058,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > mlog(ML_ERROR, "Recovery required on readonly " > > "file system, but write access is " > > "unavailable.\n"); > > - else > > - mlog_errno(status); > > - goto read_super_error; > > + goto out_super; > > } > > > > ocfs2_set_ro_flag(osb, 1); > > @@ -1077,10 +1074,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > } > > > > status = ocfs2_verify_heartbeat(osb); > > - if (status < 0) { > > - mlog_errno(status); > > - goto read_super_error; > > - } > > + if (status < 0) > > + goto out_super; > > > > osb->osb_debug_root = debugfs_create_dir(osb->uuid_str, > > ocfs2_debugfs_root); > > @@ -1094,15 +1089,14 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > status = ocfs2_mount_volume(sb); > > if (status < 0) > > - goto read_super_error; > > + goto out_debugfs; > > > > if (osb->root_inode) > > inode = igrab(osb->root_inode); > > > > if (!inode) { > > status = -EIO; > > - mlog_errno(status); > > - goto read_super_error; > > + goto out_journal; > > } > > > > osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL, > > @@ -1110,7 +1104,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > if (!osb->osb_dev_kset) { > > status = -ENOMEM; > > mlog(ML_ERROR, "Unable to create device kset %s.\n", sb->s_id); > > - goto read_super_error; > > + goto out_journal; > > } > > > > /* Create filecheck sysfs related directories/files at > > @@ -1119,14 +1113,13 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > status = -ENOMEM; > > mlog(ML_ERROR, "Unable to create filecheck sysfs directory at " > > "/sys/fs/ocfs2/%s/filecheck.\n", sb->s_id); > > - goto read_super_error; > > + goto out_journal; > > } > > > > root = d_make_root(inode); > > if (!root) { > > status = -ENOMEM; > > - mlog_errno(status); > > - goto read_super_error; > > + goto out_journal; > > } > > > > sb->s_root = root; > > @@ -1178,17 +1171,23 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) > > > > return status; > > > > -read_super_error: > > - brelse(bh); > > +out_journal: > > rename it to out_dismount? agree. > > > + mlog_errno(status); > > + atomic_set(&osb->vol_state, VOLUME_DISABLED); > > + wake_up(&osb->osb_mount_event); > > + ocfs2_dismount_volume(sb, 1); > > > > - if (status) > > - mlog_errno(status); > > + return status; > > Or remove mlog_errno() before and then just goto out? Your mean: at the end of lable "out_journal", call "goto out"? If yes, no problem for me. > > > > > - if (osb) { > > - atomic_set(&osb->vol_state, VOLUME_DISABLED); > > - wake_up(&osb->osb_mount_event); > > - ocfs2_dismount_volume(sb, 1); > > - } > > +out_debugfs: > > + debugfs_remove_recursive(osb->osb_debug_root); > > +out_super: > > + ocfs2_release_system_inodes(osb); > > + kfree(osb->recovery_map); > > + ocfs2_delete_osb(osb); > > + kfree(osb); > > Seems we have to take care all resources in ocfs2_initialize_super()? Yes, I checked every line. for ocfs2_initialize_super(), I lists all my found resources: (list in time order): osb, osb->recovery_map, osb->vol_label, osb->slot_recovery_generations, osb->osb_orphan_wipes, osb->journal, osb->uuid_str - all above have itselves out_xx label. osb->osb_dlm_debug & dlm sysfs stuffs. - release by out_dlm_out label system inodes (by ocfs2_init_global_system_inodes) - release by out_system_inodes label osb->slot_info, osb->slot_info->si_bh - release by out_slot_info label workqueue:osb->ocfs2_wq - will be freed in ocfs2_fill_super label "out_super": ocfs2_delete_osb(). or be freed in ocfs2_dismount_volume => ocfs2_delete_osb(). > > > +out: > > + mlog_errno(status); > > > As I described in your previous mail, ocfs2_sb_probe() may return error > with non-NULL bh, so we should brelse here. > I also explained why I dropped the brelse(bh), maybe anti-spam system ate my mail. (I found my emails also disappeared in other mailling lists.) I re-explain here. Below code based on patch 5/5. Please note <2> & <4>. ``` static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) { ... ... if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { status = -EINVAL; goto out; } //hm: <1> bh is alloced/created from here. /* probe for superblock */ status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); if (status < 0) { mlog(ML_ERROR, "superblock probe failed!\n"); //hm: <2> if error, ocfs2_sb_probe() will release bh. So "out" label // doesn't need to call brelse(bh). goto out; } status = ocfs2_initialize_super(sb, bh, sector_size, &stats); brelse(bh); //hm: <3> release bh before assessment status. bh = NULL; if (status < 0) goto out; //hm: <4> if error, bh is already released before "if()", // So "out" label does't need to call brelse(bh) osb = OCFS2_SB(sb); ... ... out: //hm: no need to call brelse(bh) mlog_errno(status); return status; } ``` Thanks, Heming
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c index d4b7a29cb364..c3efd9ad49ac 100644 --- a/fs/ocfs2/super.c +++ b/fs/ocfs2/super.c @@ -989,28 +989,27 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) if (!ocfs2_parse_options(sb, data, &parsed_options, 0)) { status = -EINVAL; - goto read_super_error; + goto out; } /* probe for superblock */ status = ocfs2_sb_probe(sb, &bh, §or_size, &stats); if (status < 0) { mlog(ML_ERROR, "superblock probe failed!\n"); - goto read_super_error; + goto out; } status = ocfs2_initialize_super(sb, bh, sector_size, &stats); - osb = OCFS2_SB(sb); - if (status < 0) { - mlog_errno(status); - goto read_super_error; - } brelse(bh); bh = NULL; + if (status < 0) + goto out; + + osb = OCFS2_SB(sb); if (!ocfs2_check_set_options(sb, &parsed_options)) { status = -EINVAL; - goto read_super_error; + goto out_super; } osb->s_mount_opt = parsed_options.mount_opt; osb->s_atime_quantum = parsed_options.atime_quantum; @@ -1027,7 +1026,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) status = ocfs2_verify_userspace_stack(osb, &parsed_options); if (status) - goto read_super_error; + goto out_super; sb->s_magic = OCFS2_SUPER_MAGIC; @@ -1041,7 +1040,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) status = -EACCES; mlog(ML_ERROR, "Readonly device detected but readonly " "mount was not specified.\n"); - goto read_super_error; + goto out_super; } /* You should not be able to start a local heartbeat @@ -1050,7 +1049,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) status = -EROFS; mlog(ML_ERROR, "Local heartbeat specified on readonly " "device.\n"); - goto read_super_error; + goto out_super; } status = ocfs2_check_journals_nolocks(osb); @@ -1059,9 +1058,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) mlog(ML_ERROR, "Recovery required on readonly " "file system, but write access is " "unavailable.\n"); - else - mlog_errno(status); - goto read_super_error; + goto out_super; } ocfs2_set_ro_flag(osb, 1); @@ -1077,10 +1074,8 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) } status = ocfs2_verify_heartbeat(osb); - if (status < 0) { - mlog_errno(status); - goto read_super_error; - } + if (status < 0) + goto out_super; osb->osb_debug_root = debugfs_create_dir(osb->uuid_str, ocfs2_debugfs_root); @@ -1094,15 +1089,14 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) status = ocfs2_mount_volume(sb); if (status < 0) - goto read_super_error; + goto out_debugfs; if (osb->root_inode) inode = igrab(osb->root_inode); if (!inode) { status = -EIO; - mlog_errno(status); - goto read_super_error; + goto out_journal; } osb->osb_dev_kset = kset_create_and_add(sb->s_id, NULL, @@ -1110,7 +1104,7 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) if (!osb->osb_dev_kset) { status = -ENOMEM; mlog(ML_ERROR, "Unable to create device kset %s.\n", sb->s_id); - goto read_super_error; + goto out_journal; } /* Create filecheck sysfs related directories/files at @@ -1119,14 +1113,13 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) status = -ENOMEM; mlog(ML_ERROR, "Unable to create filecheck sysfs directory at " "/sys/fs/ocfs2/%s/filecheck.\n", sb->s_id); - goto read_super_error; + goto out_journal; } root = d_make_root(inode); if (!root) { status = -ENOMEM; - mlog_errno(status); - goto read_super_error; + goto out_journal; } sb->s_root = root; @@ -1178,17 +1171,23 @@ static int ocfs2_fill_super(struct super_block *sb, void *data, int silent) return status; -read_super_error: - brelse(bh); +out_journal: + mlog_errno(status); + atomic_set(&osb->vol_state, VOLUME_DISABLED); + wake_up(&osb->osb_mount_event); + ocfs2_dismount_volume(sb, 1); - if (status) - mlog_errno(status); + return status; - if (osb) { - atomic_set(&osb->vol_state, VOLUME_DISABLED); - wake_up(&osb->osb_mount_event); - ocfs2_dismount_volume(sb, 1); - } +out_debugfs: + debugfs_remove_recursive(osb->osb_debug_root); +out_super: + ocfs2_release_system_inodes(osb); + kfree(osb->recovery_map); + ocfs2_delete_osb(osb); + kfree(osb); +out: + mlog_errno(status); return status; }
current ocfs2_fill_super error handling is very simple, and includes kernel crash possibility. e.g. ocfs2_fill_super ocfs2_initialize_super // fails in check max_slots & return Then osb->osb_mount_event is not properly initialized, and then will crash at wake_up(&osb->osb_mount_event) in goto label read_super_error. For fixing the issue and enhancing error handling, we rewrite error handling of ocfs2_fill_super. Signed-off-by: Heming Zhao <heming.zhao@suse.com> --- fs/ocfs2/super.c | 67 ++++++++++++++++++++++++------------------------ 1 file changed, 33 insertions(+), 34 deletions(-)