Message ID | 20220424130952.2436-4-heming.zhao@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | rewrite error handling during mounting stage | expand |
On 4/24/22 9:09 PM, Heming Zhao wrote: > After this patch, when error, ocfs2_fill_super doesn't take care to > release resources which are allocated in ocfs2_initialize_super. > > Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > Signed-off-by: Heming Zhao <heming.zhao@suse.com> > --- > fs/ocfs2/super.c | 59 +++++++++++++++++++++++++++++++++--------------- > 1 file changed, 41 insertions(+), 18 deletions(-) > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > index 8014c690ef72..758ea3313f88 100644 > --- a/fs/ocfs2/super.c > +++ b/fs/ocfs2/super.c > @@ -2022,7 +2022,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out; > } > > sb->s_fs_info = osb; > @@ -2083,7 +2083,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "Invalid number of node slots (%u)\n", > osb->max_slots); > status = -EINVAL; > - goto bail; > + goto out; > } > > ocfs2_orphan_scan_init(osb); > @@ -2092,7 +2092,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (status) { > mlog(ML_ERROR, "Unable to initialize recovery state\n"); > mlog_errno(status); > - goto bail; > + goto out; > } > > init_waitqueue_head(&osb->checkpoint_event); > @@ -2116,7 +2116,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->vol_label) { > mlog(ML_ERROR, "unable to alloc vol label\n"); > status = -ENOMEM; > - goto bail; > + goto out_recovery_map; > } > > osb->slot_recovery_generations = > @@ -2125,7 +2125,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->slot_recovery_generations) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out_vol_label; > } > > init_waitqueue_head(&osb->osb_wipe_event); > @@ -2135,7 +2135,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->osb_orphan_wipes) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out_slot_recovery_gen; > } > > osb->osb_rf_lock_tree = RB_ROOT; > @@ -2151,13 +2151,13 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "couldn't mount because of unsupported " > "optional features (%x).\n", i); > status = -EINVAL; > - goto bail; > + goto out_orphan_wipes; > } > if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) { > mlog(ML_ERROR, "couldn't mount RDWR because of " > "unsupported optional features (%x).\n", i); > status = -EINVAL; > - goto bail; > + goto out_orphan_wipes; > } > > if (ocfs2_clusterinfo_valid(osb)) { > @@ -2178,7 +2178,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > "cluster stack label (%s) \n", > osb->osb_cluster_stack); > status = -EINVAL; > - goto bail; > + goto out_orphan_wipes; > } > memcpy(osb->osb_cluster_name, > OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster, > @@ -2198,7 +2198,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > */ > status = ocfs2_journal_alloc(osb); > if (status < 0) > - goto bail; > + goto out_orphan_wipes; > > INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs); > init_llist_head(&osb->dquot_drop_list); > @@ -2213,7 +2213,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n", > osb->s_clustersize); > status = -EINVAL; > - goto bail; > + goto out_journal; > } > > total_blocks = ocfs2_clusters_to_blocks(osb->sb, > @@ -2225,14 +2225,14 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "Volume too large " > "to mount safely on this system"); > status = -EFBIG; > - goto bail; > + goto out_journal; > } > > if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid, > sizeof(di->id2.i_super.s_uuid))) { > mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n"); > status = -ENOMEM; > - goto bail; > + goto out_journal; > } > > strlcpy(osb->vol_label, di->id2.i_super.s_label, > @@ -2252,7 +2252,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->osb_dlm_debug) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out_uuid_str; > } > > atomic_set(&osb->vol_state, VOLUME_INIT); > @@ -2261,7 +2261,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > status = ocfs2_init_global_system_inodes(osb); > if (status < 0) { > mlog_errno(status); > - goto bail; > + goto out_dlm_out; > } > > /* > @@ -2272,7 +2272,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!inode) { > status = -EINVAL; > mlog_errno(status); > - goto bail; > + goto out_system_inodes; > } > > osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno; > @@ -2285,16 +2285,39 @@ static int ocfs2_initialize_super(struct super_block *sb, > status = ocfs2_init_slot_info(osb); > if (status < 0) { > mlog_errno(status); > - goto bail; > + goto out_system_inodes; > } > > osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM); > if (!osb->ocfs2_wq) { > status = -ENOMEM; > mlog_errno(status); > + goto out_slot_info; > } > > -bail: > + return status; > + > +out_slot_info: > + ocfs2_free_slot_info(osb); > +out_system_inodes: > + ocfs2_release_system_inodes(osb); > +out_dlm_out: > + ocfs2_put_dlm_debug(osb->osb_dlm_debug); > +out_uuid_str: > + kfree(osb->uuid_str); > +out_journal: > + kfree(osb->journal); > +out_orphan_wipes: > + kfree(osb->osb_orphan_wipes); > +out_slot_recovery_gen: > + kfree(osb->slot_recovery_generations); > +out_vol_label: > + kfree(osb->vol_label); > +out_recovery_map: > + kfree(osb->recovery_map); > +out: > + kfree(osb); > + sb->s_fs_info = NULL; Or just set "osb" to NULL is enough to keep code consistency. Other looks good. Once it is resolved, feel free to add: Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > return status; > } >
On 4/28/22 19:37, Joseph Qi wrote: > > > On 4/24/22 9:09 PM, Heming Zhao wrote: >> After this patch, when error, ocfs2_fill_super doesn't take care to >> release resources which are allocated in ocfs2_initialize_super. >> >> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> >> Signed-off-by: Heming Zhao <heming.zhao@suse.com> >> --- >> fs/ocfs2/super.c | 59 +++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 41 insertions(+), 18 deletions(-) >> >> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c >> index 8014c690ef72..758ea3313f88 100644 >> --- a/fs/ocfs2/super.c >> +++ b/fs/ocfs2/super.c >> @@ -2022,7 +2022,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!osb) { >> status = -ENOMEM; >> mlog_errno(status); >> - goto bail; >> + goto out; >> } >> >> sb->s_fs_info = osb; >> @@ -2083,7 +2083,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> mlog(ML_ERROR, "Invalid number of node slots (%u)\n", >> osb->max_slots); >> status = -EINVAL; >> - goto bail; >> + goto out; >> } >> >> ocfs2_orphan_scan_init(osb); >> @@ -2092,7 +2092,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (status) { >> mlog(ML_ERROR, "Unable to initialize recovery state\n"); >> mlog_errno(status); >> - goto bail; >> + goto out; >> } >> >> init_waitqueue_head(&osb->checkpoint_event); >> @@ -2116,7 +2116,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!osb->vol_label) { >> mlog(ML_ERROR, "unable to alloc vol label\n"); >> status = -ENOMEM; >> - goto bail; >> + goto out_recovery_map; >> } >> >> osb->slot_recovery_generations = >> @@ -2125,7 +2125,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!osb->slot_recovery_generations) { >> status = -ENOMEM; >> mlog_errno(status); >> - goto bail; >> + goto out_vol_label; >> } >> >> init_waitqueue_head(&osb->osb_wipe_event); >> @@ -2135,7 +2135,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!osb->osb_orphan_wipes) { >> status = -ENOMEM; >> mlog_errno(status); >> - goto bail; >> + goto out_slot_recovery_gen; >> } >> >> osb->osb_rf_lock_tree = RB_ROOT; >> @@ -2151,13 +2151,13 @@ static int ocfs2_initialize_super(struct super_block *sb, >> mlog(ML_ERROR, "couldn't mount because of unsupported " >> "optional features (%x).\n", i); >> status = -EINVAL; >> - goto bail; >> + goto out_orphan_wipes; >> } >> if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) { >> mlog(ML_ERROR, "couldn't mount RDWR because of " >> "unsupported optional features (%x).\n", i); >> status = -EINVAL; >> - goto bail; >> + goto out_orphan_wipes; >> } >> >> if (ocfs2_clusterinfo_valid(osb)) { >> @@ -2178,7 +2178,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> "cluster stack label (%s) \n", >> osb->osb_cluster_stack); >> status = -EINVAL; >> - goto bail; >> + goto out_orphan_wipes; >> } >> memcpy(osb->osb_cluster_name, >> OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster, >> @@ -2198,7 +2198,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> */ >> status = ocfs2_journal_alloc(osb); >> if (status < 0) >> - goto bail; >> + goto out_orphan_wipes; >> >> INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs); >> init_llist_head(&osb->dquot_drop_list); >> @@ -2213,7 +2213,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n", >> osb->s_clustersize); >> status = -EINVAL; >> - goto bail; >> + goto out_journal; >> } >> >> total_blocks = ocfs2_clusters_to_blocks(osb->sb, >> @@ -2225,14 +2225,14 @@ static int ocfs2_initialize_super(struct super_block *sb, >> mlog(ML_ERROR, "Volume too large " >> "to mount safely on this system"); >> status = -EFBIG; >> - goto bail; >> + goto out_journal; >> } >> >> if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid, >> sizeof(di->id2.i_super.s_uuid))) { >> mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n"); >> status = -ENOMEM; >> - goto bail; >> + goto out_journal; >> } >> >> strlcpy(osb->vol_label, di->id2.i_super.s_label, >> @@ -2252,7 +2252,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!osb->osb_dlm_debug) { >> status = -ENOMEM; >> mlog_errno(status); >> - goto bail; >> + goto out_uuid_str; >> } >> >> atomic_set(&osb->vol_state, VOLUME_INIT); >> @@ -2261,7 +2261,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> status = ocfs2_init_global_system_inodes(osb); >> if (status < 0) { >> mlog_errno(status); >> - goto bail; >> + goto out_dlm_out; >> } >> >> /* >> @@ -2272,7 +2272,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!inode) { >> status = -EINVAL; >> mlog_errno(status); >> - goto bail; >> + goto out_system_inodes; >> } >> >> osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno; >> @@ -2285,16 +2285,39 @@ static int ocfs2_initialize_super(struct super_block *sb, >> status = ocfs2_init_slot_info(osb); >> if (status < 0) { >> mlog_errno(status); >> - goto bail; >> + goto out_system_inodes; >> } >> >> osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM); >> if (!osb->ocfs2_wq) { >> status = -ENOMEM; >> mlog_errno(status); >> + goto out_slot_info; >> } >> >> -bail: >> + return status; >> + >> +out_slot_info: >> + ocfs2_free_slot_info(osb); >> +out_system_inodes: >> + ocfs2_release_system_inodes(osb); >> +out_dlm_out: >> + ocfs2_put_dlm_debug(osb->osb_dlm_debug); >> +out_uuid_str: >> + kfree(osb->uuid_str); >> +out_journal: >> + kfree(osb->journal); >> +out_orphan_wipes: >> + kfree(osb->osb_orphan_wipes); >> +out_slot_recovery_gen: >> + kfree(osb->slot_recovery_generations); >> +out_vol_label: >> + kfree(osb->vol_label); >> +out_recovery_map: >> + kfree(osb->recovery_map); >> +out: >> + kfree(osb); >> + sb->s_fs_info = NULL; > > Or just set "osb" to NULL is enough to keep code consistency. > Other looks good. Once it is resolved, feel free to add: > > Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> In the front of ocfs2_initialize_super, the sb->s_fs_info is set to osb. So set osb to NULL is not enough, sb->s_fs_info still keep wild pointer. The caller (ocfs2_fill_super) will fetch the wild value from OCFS2_SB(sb). Another question: do I need to resend v3 patch with "Reviewed-by: Joseph Qi ..." Thanks, Heming > > >> return status; >> } >> >
On 4/28/22 10:59 PM, heming.zhao@suse.com wrote: > > In the front of ocfs2_initialize_super, the sb->s_fs_info is set to osb. > So set osb to NULL is not enough, sb->s_fs_info still keep wild pointer. > The caller (ocfs2_fill_super) will fetch the wild value from OCFS2_SB(sb). > IC, you are right. > Another question: do I need to resend v3 patch with "Reviewed-by: Joseph Qi ..." Not needed. And Andrew Morton already has added it into -mm tree. Thanks, Joseph
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c index 8014c690ef72..758ea3313f88 100644 --- a/fs/ocfs2/super.c +++ b/fs/ocfs2/super.c @@ -2022,7 +2022,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (!osb) { status = -ENOMEM; mlog_errno(status); - goto bail; + goto out; } sb->s_fs_info = osb; @@ -2083,7 +2083,7 @@ static int ocfs2_initialize_super(struct super_block *sb, mlog(ML_ERROR, "Invalid number of node slots (%u)\n", osb->max_slots); status = -EINVAL; - goto bail; + goto out; } ocfs2_orphan_scan_init(osb); @@ -2092,7 +2092,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (status) { mlog(ML_ERROR, "Unable to initialize recovery state\n"); mlog_errno(status); - goto bail; + goto out; } init_waitqueue_head(&osb->checkpoint_event); @@ -2116,7 +2116,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (!osb->vol_label) { mlog(ML_ERROR, "unable to alloc vol label\n"); status = -ENOMEM; - goto bail; + goto out_recovery_map; } osb->slot_recovery_generations = @@ -2125,7 +2125,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (!osb->slot_recovery_generations) { status = -ENOMEM; mlog_errno(status); - goto bail; + goto out_vol_label; } init_waitqueue_head(&osb->osb_wipe_event); @@ -2135,7 +2135,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (!osb->osb_orphan_wipes) { status = -ENOMEM; mlog_errno(status); - goto bail; + goto out_slot_recovery_gen; } osb->osb_rf_lock_tree = RB_ROOT; @@ -2151,13 +2151,13 @@ static int ocfs2_initialize_super(struct super_block *sb, mlog(ML_ERROR, "couldn't mount because of unsupported " "optional features (%x).\n", i); status = -EINVAL; - goto bail; + goto out_orphan_wipes; } if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) { mlog(ML_ERROR, "couldn't mount RDWR because of " "unsupported optional features (%x).\n", i); status = -EINVAL; - goto bail; + goto out_orphan_wipes; } if (ocfs2_clusterinfo_valid(osb)) { @@ -2178,7 +2178,7 @@ static int ocfs2_initialize_super(struct super_block *sb, "cluster stack label (%s) \n", osb->osb_cluster_stack); status = -EINVAL; - goto bail; + goto out_orphan_wipes; } memcpy(osb->osb_cluster_name, OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster, @@ -2198,7 +2198,7 @@ static int ocfs2_initialize_super(struct super_block *sb, */ status = ocfs2_journal_alloc(osb); if (status < 0) - goto bail; + goto out_orphan_wipes; INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs); init_llist_head(&osb->dquot_drop_list); @@ -2213,7 +2213,7 @@ static int ocfs2_initialize_super(struct super_block *sb, mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n", osb->s_clustersize); status = -EINVAL; - goto bail; + goto out_journal; } total_blocks = ocfs2_clusters_to_blocks(osb->sb, @@ -2225,14 +2225,14 @@ static int ocfs2_initialize_super(struct super_block *sb, mlog(ML_ERROR, "Volume too large " "to mount safely on this system"); status = -EFBIG; - goto bail; + goto out_journal; } if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid, sizeof(di->id2.i_super.s_uuid))) { mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n"); status = -ENOMEM; - goto bail; + goto out_journal; } strlcpy(osb->vol_label, di->id2.i_super.s_label, @@ -2252,7 +2252,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (!osb->osb_dlm_debug) { status = -ENOMEM; mlog_errno(status); - goto bail; + goto out_uuid_str; } atomic_set(&osb->vol_state, VOLUME_INIT); @@ -2261,7 +2261,7 @@ static int ocfs2_initialize_super(struct super_block *sb, status = ocfs2_init_global_system_inodes(osb); if (status < 0) { mlog_errno(status); - goto bail; + goto out_dlm_out; } /* @@ -2272,7 +2272,7 @@ static int ocfs2_initialize_super(struct super_block *sb, if (!inode) { status = -EINVAL; mlog_errno(status); - goto bail; + goto out_system_inodes; } osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno; @@ -2285,16 +2285,39 @@ static int ocfs2_initialize_super(struct super_block *sb, status = ocfs2_init_slot_info(osb); if (status < 0) { mlog_errno(status); - goto bail; + goto out_system_inodes; } osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM); if (!osb->ocfs2_wq) { status = -ENOMEM; mlog_errno(status); + goto out_slot_info; } -bail: + return status; + +out_slot_info: + ocfs2_free_slot_info(osb); +out_system_inodes: + ocfs2_release_system_inodes(osb); +out_dlm_out: + ocfs2_put_dlm_debug(osb->osb_dlm_debug); +out_uuid_str: + kfree(osb->uuid_str); +out_journal: + kfree(osb->journal); +out_orphan_wipes: + kfree(osb->osb_orphan_wipes); +out_slot_recovery_gen: + kfree(osb->slot_recovery_generations); +out_vol_label: + kfree(osb->vol_label); +out_recovery_map: + kfree(osb->recovery_map); +out: + kfree(osb); + sb->s_fs_info = NULL; return status; }