From ad538dba7f5259df31fc45e90835edc12756db40 Mon Sep 17 00:00:00 2001 From: Lai Siyao Date: Sun, 12 Jun 2011 20:56:03 -0700 Subject: [PATCH] LU-106 procfs: many proc entries are not accessed safely Some in memory data may be released/uninitialized at the time of proc entry creation/removal, this patch includes the following fixes: * initialize data before proc entry creation * free data after proc entry removal * free proc entries in obd_precleanup() because obd_uuid/nid/nid_stats_hash are released in class_cleanup(). * free proc entries after obd_zombie_barrier() because obd_export hold one refcound of nid_stat. * check osd->od_mount before accessing osd proc entries because the osd proc entries are created before mount. Signed-off-by: Lai Siyao Change-Id: I03cb977e1be0747032a70f6a39fec804f81d70cc Reviewed-on: http://review.whamcloud.com/326 Tested-by: Hudson Reviewed-by: Johann Lombardi Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/cmm/cmm_device.c | 1 + lustre/fld/fld_request.c | 10 ++++----- lustre/include/lprocfs_status.h | 1 - lustre/include/lustre_fld.h | 2 ++ lustre/lmv/lmv_obd.c | 6 ++++-- lustre/lov/lov_obd.c | 6 ++---- lustre/mdc/mdc_request.c | 4 ++-- lustre/mdt/mdt_handler.c | 28 ++++++++++++------------- lustre/mdt/mdt_lproc.c | 15 +++++++------- lustre/mgs/mgs_handler.c | 12 +++++------ lustre/obdclass/lprocfs_status.c | 9 ++++---- lustre/obdclass/obd_config.c | 12 ++++++----- lustre/obdfilter/filter.c | 18 ++++++++-------- lustre/osc/osc_request.c | 4 ++-- lustre/osd-ldiskfs/osd_lproc.c | 45 ++++++++++++++++++++++++++++++++++------ 15 files changed, 104 insertions(+), 69 deletions(-) diff --git a/lustre/cmm/cmm_device.c b/lustre/cmm/cmm_device.c index 2105222..e17358d 100644 --- a/lustre/cmm/cmm_device.c +++ b/lustre/cmm/cmm_device.c @@ -837,6 +837,7 @@ static struct lu_device *cmm_device_fini(const struct lu_env *env, } cfs_spin_unlock(&cm->cmm_tgt_guard); + fld_client_proc_fini(cm->cmm_fld); fld_client_fini(cm->cmm_fld); ls = cmm2lu_dev(cm)->ld_site; lu_site2md(ls)->ms_client_fld = NULL; diff --git a/lustre/fld/fld_request.c b/lustre/fld/fld_request.c index 2ea527a..af34ec4 100644 --- a/lustre/fld/fld_request.c +++ b/lustre/fld/fld_request.c @@ -280,8 +280,6 @@ int fld_client_del_target(struct lu_client_fld *fld, } EXPORT_SYMBOL(fld_client_del_target); -static void fld_client_proc_fini(struct lu_client_fld *fld); - #ifdef LPROCFS static int fld_client_proc_init(struct lu_client_fld *fld) { @@ -314,7 +312,7 @@ out_cleanup: return rc; } -static void fld_client_proc_fini(struct lu_client_fld *fld) +void fld_client_proc_fini(struct lu_client_fld *fld) { ENTRY; if (fld->lcf_proc_dir) { @@ -330,12 +328,14 @@ static int fld_client_proc_init(struct lu_client_fld *fld) return 0; } -static void fld_client_proc_fini(struct lu_client_fld *fld) +void fld_client_proc_fini(struct lu_client_fld *fld) { return; } #endif +EXPORT_SYMBOL(fld_client_proc_fini); + static inline int hash_is_sane(int hash) { return (hash >= 0 && hash < ARRAY_SIZE(fld_hash)); @@ -398,8 +398,6 @@ void fld_client_fini(struct lu_client_fld *fld) struct lu_fld_target *target, *tmp; ENTRY; - fld_client_proc_fini(fld); - cfs_spin_lock(&fld->lcf_lock); cfs_list_for_each_entry_safe(target, tmp, &fld->lcf_targets, ft_chain) { diff --git a/lustre/include/lprocfs_status.h b/lustre/include/lprocfs_status.h index aeed486..09bf178 100644 --- a/lustre/include/lprocfs_status.h +++ b/lustre/include/lprocfs_status.h @@ -486,7 +486,6 @@ extern cfs_proc_dir_entry_t *lprocfs_srch(cfs_proc_dir_entry_t *root, extern int lprocfs_obd_setup(struct obd_device *obd, struct lprocfs_vars *list); extern int lprocfs_obd_cleanup(struct obd_device *obd); -extern void lprocfs_free_per_client_stats(struct obd_device *obd); extern struct file_operations lprocfs_evict_client_fops; extern int lprocfs_seq_create(cfs_proc_dir_entry_t *parent, char *name, diff --git a/lustre/include/lustre_fld.h b/lustre/include/lustre_fld.h index 428b352..a0328d1 100644 --- a/lustre/include/lustre_fld.h +++ b/lustre/include/lustre_fld.h @@ -193,6 +193,8 @@ int fld_client_add_target(struct lu_client_fld *fld, int fld_client_del_target(struct lu_client_fld *fld, __u64 idx); +void fld_client_proc_fini(struct lu_client_fld *fld); + /** @} fld */ #endif diff --git a/lustre/lmv/lmv_obd.c b/lustre/lmv/lmv_obd.c index 4aef69a..3e765ba 100644 --- a/lustre/lmv/lmv_obd.c +++ b/lustre/lmv/lmv_obd.c @@ -1140,7 +1140,6 @@ static int lmv_cleanup(struct obd_device *obd) ENTRY; fld_client_fini(&lmv->lmv_fld); - lprocfs_obd_cleanup(obd); lmv_object_cleanup(obd); OBD_FREE(lmv->datas, lmv->datas_size); OBD_FREE(lmv->tgts, lmv->tgts_size); @@ -2631,7 +2630,8 @@ repeat: static int lmv_precleanup(struct obd_device *obd, enum obd_cleanup_stage stage) { - int rc = 0; + struct lmv_obd *lmv = &obd->u.lmv; + int rc = 0; switch (stage) { case OBD_CLEANUP_EARLY: @@ -2639,6 +2639,8 @@ static int lmv_precleanup(struct obd_device *obd, enum obd_cleanup_stage stage) * stack. */ break; case OBD_CLEANUP_EXPORTS: + fld_client_proc_fini(&lmv->lmv_fld); + lprocfs_obd_cleanup(obd); rc = obd_llog_finish(obd, 0); if (rc != 0) CERROR("failed to cleanup llogging subsystems\n"); diff --git a/lustre/lov/lov_obd.c b/lustre/lov/lov_obd.c index 0497cb2..a70e64a 100644 --- a/lustre/lov/lov_obd.c +++ b/lustre/lov/lov_obd.c @@ -884,6 +884,7 @@ static int lov_precleanup(struct obd_device *obd, enum obd_cleanup_stage stage) break; } case OBD_CLEANUP_EXPORTS: + lprocfs_obd_cleanup(obd); rc = obd_llog_finish(obd, 0); if (rc != 0) CERROR("failed to cleanup llogging subsystems\n"); @@ -897,6 +898,7 @@ static int lov_cleanup(struct obd_device *obd) struct lov_obd *lov = &obd->u.lov; cfs_list_t *pos, *tmp; struct pool_desc *pool; + ENTRY; cfs_list_for_each_safe(pos, tmp, &lov->lov_pool_list) { pool = cfs_list_entry(pos, struct pool_desc, pool_list); @@ -932,10 +934,6 @@ static int lov_cleanup(struct obd_device *obd) lov->lov_tgt_size); lov->lov_tgt_size = 0; } - - /* clear pools parent proc entry only after all pools is killed */ - lprocfs_obd_cleanup(obd); - OBD_FREE_PTR(lov->lov_qos.lq_statfs_data); RETURN(0); } diff --git a/lustre/mdc/mdc_request.c b/lustre/mdc/mdc_request.c index 9b63d42..2f05091 100644 --- a/lustre/mdc/mdc_request.c +++ b/lustre/mdc/mdc_request.c @@ -2082,6 +2082,8 @@ static int mdc_precleanup(struct obd_device *obd, enum obd_cleanup_stage stage) libcfs_kkuc_group_rem(0, KUC_GRP_HSM); obd_cleanup_client_import(obd); + ptlrpc_lprocfs_unregister_obd(obd); + lprocfs_obd_cleanup(obd); rc = obd_llog_finish(obd, 0); if (rc != 0) @@ -2099,8 +2101,6 @@ static int mdc_cleanup(struct obd_device *obd) OBD_FREE(cli->cl_setattr_lock, sizeof (*cli->cl_setattr_lock)); OBD_FREE(cli->cl_close_lock, sizeof (*cli->cl_close_lock)); - ptlrpc_lprocfs_unregister_obd(obd); - lprocfs_obd_cleanup(obd); ptlrpcd_decref(); return client_obd_cleanup(obd); diff --git a/lustre/mdt/mdt_handler.c b/lustre/mdt/mdt_handler.c index 001ef1d..8a57ab0 100644 --- a/lustre/mdt/mdt_handler.c +++ b/lustre/mdt/mdt_handler.c @@ -4319,6 +4319,9 @@ static void mdt_fini(const struct lu_env *env, struct mdt_device *m) mdt_obd_llog_cleanup(obd); obd_exports_barrier(obd); obd_zombie_barrier(); + + mdt_procfs_fini(m); + #ifdef HAVE_QUOTA_SUPPORT next->md_ops->mdo_quota.mqo_cleanup(env, next); #endif @@ -4354,10 +4357,6 @@ static void mdt_fini(const struct lu_env *env, struct mdt_device *m) */ mdt_stack_fini(env, m, md2lu_dev(m->mdt_child)); - lprocfs_free_per_client_stats(obd); - lprocfs_free_obd_stats(obd); - mdt_procfs_fini(m); - if (ls) { struct md_site *mite; @@ -4492,12 +4491,6 @@ static int mdt_init0(const struct lu_env *env, struct mdt_device *m, GOTO(err_free_site, rc); } - rc = mdt_procfs_init(m, dev); - if (rc) { - CERROR("Can't init MDT lprocfs, rc %d\n", rc); - GOTO(err_fini_proc, rc); - } - /* set server index */ lu_site2md(s)->ms_node_id = node_id; @@ -4521,7 +4514,7 @@ static int mdt_init0(const struct lu_env *env, struct mdt_device *m, rc = mdt_stack_init((struct lu_env *)env, m, cfg, lmi); if (rc) { CERROR("Can't init device stack, rc %d\n", rc); - GOTO(err_fini_proc, rc); + GOTO(err_lu_site, rc); } rc = lut_init(env, &m->mdt_lut, obd, m->mdt_bottom); @@ -4609,9 +4602,15 @@ static int mdt_init0(const struct lu_env *env, struct mdt_device *m, target_recovery_init(&m->mdt_lut, mdt_recovery_handle); + rc = mdt_procfs_init(m, dev); + if (rc) { + CERROR("Can't init MDT lprocfs, rc %d\n", rc); + GOTO(err_recovery, rc); + } + rc = mdt_start_ptlrpc_service(m); if (rc) - GOTO(err_recovery, rc); + GOTO(err_procfs, rc); ping_evictor_start(); @@ -4635,6 +4634,8 @@ static int mdt_init0(const struct lu_env *env, struct mdt_device *m, err_stop_service: ping_evictor_stop(); mdt_stop_ptlrpc_service(m); +err_procfs: + mdt_procfs_fini(m); err_recovery: target_recovery_fini(obd); upcall_cache_cleanup(m->mdt_identity_cache); @@ -4662,8 +4663,7 @@ err_lut: lut_fini(env, &m->mdt_lut); err_fini_stack: mdt_stack_fini(env, m, md2lu_dev(m->mdt_child)); -err_fini_proc: - mdt_procfs_fini(m); +err_lu_site: lu_site_fini(s); err_free_site: OBD_FREE_PTR(mite); diff --git a/lustre/mdt/mdt_lproc.c b/lustre/mdt/mdt_lproc.c index 053f626..f141398 100644 --- a/lustre/mdt/mdt_lproc.c +++ b/lustre/mdt/mdt_lproc.c @@ -126,18 +126,20 @@ int mdt_procfs_fini(struct mdt_device *mdt) struct lu_device *ld = &mdt->mdt_md_dev.md_lu_dev; struct obd_device *obd = ld->ld_obd; - if (mdt->mdt_proc_entry) { - lu_time_fini(&ld->ld_site->ls_time_stats); - lu_time_fini(&mdt->mdt_stats); - mdt->mdt_proc_entry = NULL; - } if (obd->obd_proc_exports_entry) { lprocfs_remove_proc_entry("clear", obd->obd_proc_exports_entry); obd->obd_proc_exports_entry = NULL; } + lprocfs_free_per_client_stats(obd); + lprocfs_obd_cleanup(obd); ptlrpc_lprocfs_unregister_obd(obd); + if (mdt->mdt_proc_entry) { + lu_time_fini(&ld->ld_site->ls_time_stats); + lu_time_fini(&mdt->mdt_stats); + mdt->mdt_proc_entry = NULL; + } lprocfs_free_md_stats(obd); - lprocfs_obd_cleanup(obd); + lprocfs_free_obd_stats(obd); RETURN(0); } @@ -536,7 +538,6 @@ static int lprocfs_rd_root_squash(char *page, char **start, off_t off, { struct obd_device *obd = data; struct mdt_device *mdt = mdt_dev(obd->obd_lu_dev); - ENTRY; return snprintf(page, count, "%u:%u\n", mdt->mdt_squash_uid, mdt->mdt_squash_gid); diff --git a/lustre/mgs/mgs_handler.c b/lustre/mgs/mgs_handler.c index bb2705e..e8b1d38 100644 --- a/lustre/mgs/mgs_handler.c +++ b/lustre/mgs/mgs_handler.c @@ -286,13 +286,19 @@ err_put: static int mgs_precleanup(struct obd_device *obd, enum obd_cleanup_stage stage) { + struct mgs_obd *mgs = &obd->u.mgs; int rc = 0; ENTRY; switch (stage) { case OBD_CLEANUP_EARLY: + break; case OBD_CLEANUP_EXPORTS: + ping_evictor_stop(); + ptlrpc_unregister_service(mgs->mgs_service); + mgs_cleanup_fsdb_list(obd); rc = obd_llog_finish(obd, 0); + lproc_mgs_cleanup(obd); break; } RETURN(rc); @@ -309,12 +315,6 @@ static int mgs_cleanup(struct obd_device *obd) if (mgs->mgs_sb == NULL) RETURN(0); - ping_evictor_stop(); - - ptlrpc_unregister_service(mgs->mgs_service); - - mgs_cleanup_fsdb_list(obd); - lproc_mgs_cleanup(obd); mgs_fs_cleanup(obd); server_put_mount(obd->obd_name, mgs->mgs_vfsmnt); diff --git a/lustre/obdclass/lprocfs_status.c b/lustre/obdclass/lprocfs_status.c index 1610794..d85954b 100644 --- a/lustre/obdclass/lprocfs_status.c +++ b/lustre/obdclass/lprocfs_status.c @@ -1146,8 +1146,6 @@ static void lprocfs_free_client_stats(struct nid_stat *client_stat) "nid %s:count %d\n", libcfs_nid2str(client_stat->nid), atomic_read(&client_stat->nid_exp_ref_count)); - cfs_hlist_del_init(&client_stat->nid_hash); - if (client_stat->nid_proc) lprocfs_remove(&client_stat->nid_proc); @@ -1167,18 +1165,19 @@ static void lprocfs_free_client_stats(struct nid_stat *client_stat) void lprocfs_free_per_client_stats(struct obd_device *obd) { + cfs_hash_t *hash = obd->obd_nid_stats_hash; struct nid_stat *stat; ENTRY; /* we need extra list - because hash_exit called to early */ /* not need locking because all clients is died */ - while(!cfs_list_empty(&obd->obd_nid_stats)) { + while (!cfs_list_empty(&obd->obd_nid_stats)) { stat = cfs_list_entry(obd->obd_nid_stats.next, struct nid_stat, nid_list); cfs_list_del_init(&stat->nid_list); + cfs_hash_del(hash, &stat->nid, &stat->nid_hash); lprocfs_free_client_stats(stat); } - EXIT; } @@ -1778,7 +1777,7 @@ int lprocfs_nid_stats_clear_read(char *page, char **start, off_t off, } EXPORT_SYMBOL(lprocfs_nid_stats_clear_read); -int lprocfs_nid_stats_clear_write_cb(void *obj, void *data) +static int lprocfs_nid_stats_clear_write_cb(void *obj, void *data) { struct nid_stat *stat = obj; int i; diff --git a/lustre/obdclass/obd_config.c b/lustre/obdclass/obd_config.c index f9fa331..4c2cf37 100644 --- a/lustre/obdclass/obd_config.c +++ b/lustre/obdclass/obd_config.c @@ -615,6 +615,12 @@ int class_cleanup(struct obd_device *obd, struct lustre_cfg *lcfg) class_disconnect_exports(obd); } + /* Precleanup, we must make sure all exports get destroyed. */ + err = obd_precleanup(obd, OBD_CLEANUP_EXPORTS); + if (err) + CERROR("Precleanup %s returned %d\n", + obd->obd_name, err); + /* destroy an uuid-export hash body */ if (obd->obd_uuid_hash) { cfs_hash_putref(obd->obd_uuid_hash); @@ -633,13 +639,9 @@ int class_cleanup(struct obd_device *obd, struct lustre_cfg *lcfg) obd->obd_nid_stats_hash = NULL; } - /* Precleanup, we must make sure all exports get destroyed. */ - err = obd_precleanup(obd, OBD_CLEANUP_EXPORTS); - if (err) - CERROR("Precleanup %s returned %d\n", - obd->obd_name, err); class_decref(obd, "setup", obd); obd->obd_set_up = 0; + RETURN(0); } diff --git a/lustre/obdfilter/filter.c b/lustre/obdfilter/filter.c index 31f4cbe..a77ca65 100644 --- a/lustre/obdfilter/filter.c +++ b/lustre/obdfilter/filter.c @@ -2611,7 +2611,16 @@ static int filter_precleanup(struct obd_device *obd, case OBD_CLEANUP_EXPORTS: /* Stop recovery before namespace cleanup. */ target_recovery_fini(obd); + + obd_exports_barrier(obd); + obd_zombie_barrier(); + rc = filter_llog_preclean(obd); + lprocfs_remove_proc_entry("clear", obd->obd_proc_exports_entry); + lprocfs_free_per_client_stats(obd); + lprocfs_obd_cleanup(obd); + lprocfs_free_obd_stats(obd); + lquota_cleanup(filter_quota_interface_ref, obd); break; } RETURN(rc); @@ -2626,15 +2635,6 @@ static int filter_cleanup(struct obd_device *obd) LCONSOLE_WARN("%s: shutting down for failover; client state " "will be preserved.\n", obd->obd_name); - obd_exports_barrier(obd); - obd_zombie_barrier(); - - lprocfs_remove_proc_entry("clear", obd->obd_proc_exports_entry); - lprocfs_free_per_client_stats(obd); - lprocfs_free_obd_stats(obd); - lprocfs_obd_cleanup(obd); - lquota_cleanup(filter_quota_interface_ref, obd); - ldlm_namespace_free(obd->obd_namespace, NULL, obd->obd_force); obd->obd_namespace = NULL; diff --git a/lustre/osc/osc_request.c b/lustre/osc/osc_request.c index a0d45b5..c5e4bd0 100644 --- a/lustre/osc/osc_request.c +++ b/lustre/osc/osc_request.c @@ -4533,6 +4533,8 @@ static int osc_precleanup(struct obd_device *obd, enum obd_cleanup_stage stage) */ obd_zombie_barrier(); obd_cleanup_client_import(obd); + ptlrpc_lprocfs_unregister_obd(obd); + lprocfs_obd_cleanup(obd); rc = obd_llog_finish(obd, 0); if (rc != 0) CERROR("failed to cleanup llogging subsystems\n"); @@ -4547,8 +4549,6 @@ int osc_cleanup(struct obd_device *obd) int rc; ENTRY; - ptlrpc_lprocfs_unregister_obd(obd); - lprocfs_obd_cleanup(obd); /* free memory of osc quota cache */ lquota_cleanup(quota_interface, obd); diff --git a/lustre/osd-ldiskfs/osd_lproc.c b/lustre/osd-ldiskfs/osd_lproc.c index df25006..a8862e6 100644 --- a/lustre/osd-ldiskfs/osd_lproc.c +++ b/lustre/osd-ldiskfs/osd_lproc.c @@ -121,7 +121,12 @@ int lprocfs_osd_rd_blksize(char *page, char **start, off_t off, int count, int *eof, void *data) { struct osd_device *osd = data; - int rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); + int rc; + + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + + rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); if (!rc) { *eof = 1; rc = snprintf(page, count, "%ld\n", osd->od_kstatfs.f_bsize); @@ -133,7 +138,12 @@ int lprocfs_osd_rd_kbytestotal(char *page, char **start, off_t off, int count, int *eof, void *data) { struct osd_device *osd = data; - int rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); + int rc; + + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + + rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); if (!rc) { __u32 blk_size = osd->od_kstatfs.f_bsize >> 10; __u64 result = osd->od_kstatfs.f_blocks; @@ -151,7 +161,12 @@ int lprocfs_osd_rd_kbytesfree(char *page, char **start, off_t off, int count, int *eof, void *data) { struct osd_device *osd = data; - int rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); + int rc; + + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + + rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); if (!rc) { __u32 blk_size = osd->od_kstatfs.f_bsize >> 10; __u64 result = osd->od_kstatfs.f_bfree; @@ -169,7 +184,12 @@ int lprocfs_osd_rd_kbytesavail(char *page, char **start, off_t off, int count, int *eof, void *data) { struct osd_device *osd = data; - int rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); + int rc; + + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + + rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); if (!rc) { __u32 blk_size = osd->od_kstatfs.f_bsize >> 10; __u64 result = osd->od_kstatfs.f_bavail; @@ -187,7 +207,12 @@ int lprocfs_osd_rd_filestotal(char *page, char **start, off_t off, int count, int *eof, void *data) { struct osd_device *osd = data; - int rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); + int rc; + + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + + rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); if (!rc) { *eof = 1; rc = snprintf(page, count, LPU64"\n", osd->od_kstatfs.f_files); @@ -200,7 +225,12 @@ int lprocfs_osd_rd_filesfree(char *page, char **start, off_t off, int count, int *eof, void *data) { struct osd_device *osd = data; - int rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); + int rc; + + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + + rc = osd_statfs(NULL, &osd->od_dt_dev, &osd->od_kstatfs); if (!rc) { *eof = 1; rc = snprintf(page, count, LPU64"\n", osd->od_kstatfs.f_ffree); @@ -223,6 +253,9 @@ static int lprocfs_osd_rd_mntdev(char *page, char **start, off_t off, int count, struct osd_device *osd = data; LASSERT(osd != NULL); + if (unlikely(osd->od_mount == NULL)) + return -EINPROGRESS; + LASSERT(osd->od_mount->lmi_mnt->mnt_devname); *eof = 1; -- 1.8.3.1