| Message ID | 20220622101621.4312-2-mike.leach@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix lockdep issues seen in CoreSight configfs interface | expand |
On Wed, Jun 22, 2022 at 11:16:20AM +0100, Mike Leach wrote: > Any loaded configurations must be correctly unloaded on coresight module > exit, or issues can arise with nested locking in the configfs directory > code if built with CONFIG_LOCKDEP. > > Prior to this patch, the preloaded configuration configfs directory entries > were being unloaded by the recursive code in > configfs_unregister_subsystem(). > > However, when built with CONFIG_LOCKDEP, this caused a nested lock warning, > which was not mitigated by the LOCKDEP dependent code in fs/configfs/dir.c > designed to prevent this, due to the different directory levels for the > root of the directory being removed. > > As the preloaded (and all other) configurations are registered after > configfs_register_subsystem(), we now explicitly unload them before the > call to configfs_unregister_subsystem(). > > The new routine cscfg_unload_cfgs_on_exit() iterates through the load > owner list to unload any remaining configurations that were not unloaded > by the user before the module exits. This covers both the > CSCFG_OWNER_PRELOAD and CSCFG_OWNER_MODULE owner types, and will be > extended to cover future load owner types for CoreSight configurations. > > Applies to coresight/next The above sentence will end up in the kernel log forever, which is not what we want. > > Fixes: eb2ec49606c2 ("coresight: syscfg: Update load API for config loadable modules") > Reported-by: Suzuki Poulose <suzuki.poulose@arm.com> > Signed-off-by: Mike Leach <mike.leach@linaro.org> > --- > .../hwtracing/coresight/coresight-syscfg.c | 106 ++++++++++++++++-- > 1 file changed, 95 insertions(+), 11 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > index 11850fd8c3b5..9cd7d3c91d8e 100644 > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > @@ -414,6 +414,27 @@ static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, vo > } > } > > +/* > + * Unregister all configuration and features from configfs owned by load_owner. > + * Although this is called without the list mutex being held, it is in the > + * context of an unload operation which are strictly serialised, > + * so the lists cannot change during this call. > + */ > +static void cscfg_fs_unregister_cfgs_feats(void *load_owner) > +{ > + struct cscfg_config_desc *config_desc; > + struct cscfg_feature_desc *feat_desc; > + > + list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) { > + if (config_desc->load_owner == load_owner) > + cscfg_configfs_del_config(config_desc); > + } > + list_for_each_entry(feat_desc, &cscfg_mgr->feat_desc_list, item) { > + if (feat_desc->load_owner == load_owner) > + cscfg_configfs_del_feature(feat_desc); > + } > +} > + > /* > * removal is relatively easy - just remove from all lists, anything that > * matches the owner. Memory for the descriptors will be managed by the owner, > @@ -1022,10 +1043,13 @@ struct device *cscfg_device(void) > /* Must have a release function or the kernel will complain on module unload */ > static void cscfg_dev_release(struct device *dev) > { > + mutex_lock(&cscfg_mutex); > kfree(cscfg_mgr); > cscfg_mgr = NULL; > + mutex_unlock(&cscfg_mutex); > } > > + > /* a device is needed to "own" some kernel elements such as sysfs entries. */ > static int cscfg_create_device(void) > { > @@ -1042,6 +1066,13 @@ static int cscfg_create_device(void) > if (!cscfg_mgr) > goto create_dev_exit_unlock; > > + /* initialise the cscfg_mgr structure */ > + INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); > + INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); > + INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > + INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > + atomic_set(&cscfg_mgr->sys_active_cnt, 0); > + > /* setup the device */ > dev = cscfg_device(); > dev->release = cscfg_dev_release; > @@ -1056,17 +1087,74 @@ static int cscfg_create_device(void) > return err; > } > > -static void cscfg_clear_device(void) > +/* > + * Loading and unloading is generally on user discretion. > + * If exiting due to coresight module unload, we need to unload any configurations that remain, > + * before we unregister the configfs intrastructure. > + * > + * Do this by walking the load_owner list and taking appropriate action, depending on the load > + * owner type. > + */ > +static void cscfg_unload_cfgs_on_exit(void) > { > - struct cscfg_config_desc *cfg_desc; > + struct cscfg_load_owner_info *owner_info = NULL; > > + /* > + * grab the mutex - even though we are exiting, some configfs files > + * may still be live till we dump them, so ensure list data is > + * protected from a race condition. > + */ > mutex_lock(&cscfg_mutex); > - list_for_each_entry(cfg_desc, &cscfg_mgr->config_desc_list, item) { > - etm_perf_del_symlink_cscfg(cfg_desc); > + while (!list_empty(&cscfg_mgr->load_order_list)) { > + > + /* remove in reverse order of loading */ > + owner_info = list_last_entry(&cscfg_mgr->load_order_list, > + struct cscfg_load_owner_info, item); > + > + /* action according to type */ > + switch (owner_info->type) { > + case CSCFG_OWNER_PRELOAD: > + /* > + * preloaded descriptors are statically allocated in > + * this module - just need to unload dynamic items from > + * csdev lists, and remove from configfs directories. > + */ > + pr_info("cscfg: unloading preloaded configurations\n"); > + break; > + > + case CSCFG_OWNER_MODULE: > + /* > + * this is an error - the loadable module must have been unloaded prior > + * to the coresight module unload. Therefore that module has not > + * correctly unloaded configs in its own exit code. > + * Nothing to do other than emit an error string as the static descriptor > + * references we need to unload will have disappeared with the module. > + */ > + pr_err("cscfg: ERROR - a loadable module failed to " > + "unload configs on exit\n"); This gives me a checkpatch warning. Please correct and resend a new revision. Thanks, Mathieu > + goto list_remove; > + } > + > + /* remove from configfs - outside the scope of the list mutex */ > + mutex_unlock(&cscfg_mutex); > + cscfg_fs_unregister_cfgs_feats(owner_info); > + mutex_lock(&cscfg_mutex); > + > + /* Next unload from csdev lists. */ > + cscfg_unload_owned_cfgs_feats(owner_info); > + > +list_remove: > + /* remove from load order list */ > + list_del(&owner_info->item); > } > + mutex_unlock(&cscfg_mutex); > +} > + > +static void cscfg_clear_device(void) > +{ > + cscfg_unload_cfgs_on_exit(); > cscfg_configfs_release(cscfg_mgr); > device_unregister(cscfg_device()); > - mutex_unlock(&cscfg_mutex); > } > > /* Initialise system config management API device */ > @@ -1074,20 +1162,16 @@ int __init cscfg_init(void) > { > int err = 0; > > + /* create the device and init cscfg_mgr */ > err = cscfg_create_device(); > if (err) > return err; > > + /* initialise configfs subsystem */ > err = cscfg_configfs_init(cscfg_mgr); > if (err) > goto exit_err; > > - INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); > - INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); > - INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > - INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > - atomic_set(&cscfg_mgr->sys_active_cnt, 0); > - > /* preload built-in configurations */ > err = cscfg_preload(THIS_MODULE); > if (err) > -- > 2.17.1 >
Hi, On Thu, 23 Jun 2022 at 20:34, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > On Wed, Jun 22, 2022 at 11:16:20AM +0100, Mike Leach wrote: > > Any loaded configurations must be correctly unloaded on coresight module > > exit, or issues can arise with nested locking in the configfs directory > > code if built with CONFIG_LOCKDEP. > > > > Prior to this patch, the preloaded configuration configfs directory entries > > were being unloaded by the recursive code in > > configfs_unregister_subsystem(). > > > > However, when built with CONFIG_LOCKDEP, this caused a nested lock warning, > > which was not mitigated by the LOCKDEP dependent code in fs/configfs/dir.c > > designed to prevent this, due to the different directory levels for the > > root of the directory being removed. > > > > As the preloaded (and all other) configurations are registered after > > configfs_register_subsystem(), we now explicitly unload them before the > > call to configfs_unregister_subsystem(). > > > > The new routine cscfg_unload_cfgs_on_exit() iterates through the load > > owner list to unload any remaining configurations that were not unloaded > > by the user before the module exits. This covers both the > > CSCFG_OWNER_PRELOAD and CSCFG_OWNER_MODULE owner types, and will be > > extended to cover future load owner types for CoreSight configurations. > > > > Applies to coresight/next > Will remove > The above sentence will end up in the kernel log forever, which is not what we > want. > > > > > Fixes: eb2ec49606c2 ("coresight: syscfg: Update load API for config loadable modules") > > Reported-by: Suzuki Poulose <suzuki.poulose@arm.com> > > Signed-off-by: Mike Leach <mike.leach@linaro.org> > > --- > > .../hwtracing/coresight/coresight-syscfg.c | 106 ++++++++++++++++-- > > 1 file changed, 95 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c > > index 11850fd8c3b5..9cd7d3c91d8e 100644 > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c > > @@ -414,6 +414,27 @@ static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, vo > > } > > } > > > > +/* > > + * Unregister all configuration and features from configfs owned by load_owner. > > + * Although this is called without the list mutex being held, it is in the > > + * context of an unload operation which are strictly serialised, > > + * so the lists cannot change during this call. > > + */ > > +static void cscfg_fs_unregister_cfgs_feats(void *load_owner) > > +{ > > + struct cscfg_config_desc *config_desc; > > + struct cscfg_feature_desc *feat_desc; > > + > > + list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) { > > + if (config_desc->load_owner == load_owner) > > + cscfg_configfs_del_config(config_desc); > > + } > > + list_for_each_entry(feat_desc, &cscfg_mgr->feat_desc_list, item) { > > + if (feat_desc->load_owner == load_owner) > > + cscfg_configfs_del_feature(feat_desc); > > + } > > +} > > + > > /* > > * removal is relatively easy - just remove from all lists, anything that > > * matches the owner. Memory for the descriptors will be managed by the owner, > > @@ -1022,10 +1043,13 @@ struct device *cscfg_device(void) > > /* Must have a release function or the kernel will complain on module unload */ > > static void cscfg_dev_release(struct device *dev) > > { > > + mutex_lock(&cscfg_mutex); > > kfree(cscfg_mgr); > > cscfg_mgr = NULL; > > + mutex_unlock(&cscfg_mutex); > > } > > > > + > > /* a device is needed to "own" some kernel elements such as sysfs entries. */ > > static int cscfg_create_device(void) > > { > > @@ -1042,6 +1066,13 @@ static int cscfg_create_device(void) > > if (!cscfg_mgr) > > goto create_dev_exit_unlock; > > > > + /* initialise the cscfg_mgr structure */ > > + INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); > > + INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); > > + INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > > + INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > > + atomic_set(&cscfg_mgr->sys_active_cnt, 0); > > + > > /* setup the device */ > > dev = cscfg_device(); > > dev->release = cscfg_dev_release; > > @@ -1056,17 +1087,74 @@ static int cscfg_create_device(void) > > return err; > > } > > > > -static void cscfg_clear_device(void) > > +/* > > + * Loading and unloading is generally on user discretion. > > + * If exiting due to coresight module unload, we need to unload any configurations that remain, > > + * before we unregister the configfs intrastructure. > > + * > > + * Do this by walking the load_owner list and taking appropriate action, depending on the load > > + * owner type. > > + */ > > +static void cscfg_unload_cfgs_on_exit(void) > > { > > - struct cscfg_config_desc *cfg_desc; > > + struct cscfg_load_owner_info *owner_info = NULL; > > > > + /* > > + * grab the mutex - even though we are exiting, some configfs files > > + * may still be live till we dump them, so ensure list data is > > + * protected from a race condition. > > + */ > > mutex_lock(&cscfg_mutex); > > - list_for_each_entry(cfg_desc, &cscfg_mgr->config_desc_list, item) { > > - etm_perf_del_symlink_cscfg(cfg_desc); > > + while (!list_empty(&cscfg_mgr->load_order_list)) { > > + > > + /* remove in reverse order of loading */ > > + owner_info = list_last_entry(&cscfg_mgr->load_order_list, > > + struct cscfg_load_owner_info, item); > > + > > + /* action according to type */ > > + switch (owner_info->type) { > > + case CSCFG_OWNER_PRELOAD: > > + /* > > + * preloaded descriptors are statically allocated in > > + * this module - just need to unload dynamic items from > > + * csdev lists, and remove from configfs directories. > > + */ > > + pr_info("cscfg: unloading preloaded configurations\n"); > > + break; > > + > > + case CSCFG_OWNER_MODULE: > > + /* > > + * this is an error - the loadable module must have been unloaded prior > > + * to the coresight module unload. Therefore that module has not > > + * correctly unloaded configs in its own exit code. > > + * Nothing to do other than emit an error string as the static descriptor > > + * references we need to unload will have disappeared with the module. > > + */ > > + pr_err("cscfg: ERROR - a loadable module failed to " > > + "unload configs on exit\n"); > > This gives me a checkpatch warning. Please correct and resend a new revision. > OK - this was done at the explicit request of Suzuki, but will re-do for new version. Thanks Mike > Thanks, > Mathieu > > > + goto list_remove; > > + } > > + > > + /* remove from configfs - outside the scope of the list mutex */ > > + mutex_unlock(&cscfg_mutex); > > + cscfg_fs_unregister_cfgs_feats(owner_info); > > + mutex_lock(&cscfg_mutex); > > + > > + /* Next unload from csdev lists. */ > > + cscfg_unload_owned_cfgs_feats(owner_info); > > + > > +list_remove: > > + /* remove from load order list */ > > + list_del(&owner_info->item); > > } > > + mutex_unlock(&cscfg_mutex); > > +} > > + > > +static void cscfg_clear_device(void) > > +{ > > + cscfg_unload_cfgs_on_exit(); > > cscfg_configfs_release(cscfg_mgr); > > device_unregister(cscfg_device()); > > - mutex_unlock(&cscfg_mutex); > > } > > > > /* Initialise system config management API device */ > > @@ -1074,20 +1162,16 @@ int __init cscfg_init(void) > > { > > int err = 0; > > > > + /* create the device and init cscfg_mgr */ > > err = cscfg_create_device(); > > if (err) > > return err; > > > > + /* initialise configfs subsystem */ > > err = cscfg_configfs_init(cscfg_mgr); > > if (err) > > goto exit_err; > > > > - INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); > > - INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); > > - INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); > > - INIT_LIST_HEAD(&cscfg_mgr->load_order_list); > > - atomic_set(&cscfg_mgr->sys_active_cnt, 0); > > - > > /* preload built-in configurations */ > > err = cscfg_preload(THIS_MODULE); > > if (err) > > -- > > 2.17.1 > >
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c index 11850fd8c3b5..9cd7d3c91d8e 100644 --- a/drivers/hwtracing/coresight/coresight-syscfg.c +++ b/drivers/hwtracing/coresight/coresight-syscfg.c @@ -414,6 +414,27 @@ static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, vo } } +/* + * Unregister all configuration and features from configfs owned by load_owner. + * Although this is called without the list mutex being held, it is in the + * context of an unload operation which are strictly serialised, + * so the lists cannot change during this call. + */ +static void cscfg_fs_unregister_cfgs_feats(void *load_owner) +{ + struct cscfg_config_desc *config_desc; + struct cscfg_feature_desc *feat_desc; + + list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) { + if (config_desc->load_owner == load_owner) + cscfg_configfs_del_config(config_desc); + } + list_for_each_entry(feat_desc, &cscfg_mgr->feat_desc_list, item) { + if (feat_desc->load_owner == load_owner) + cscfg_configfs_del_feature(feat_desc); + } +} + /* * removal is relatively easy - just remove from all lists, anything that * matches the owner. Memory for the descriptors will be managed by the owner, @@ -1022,10 +1043,13 @@ struct device *cscfg_device(void) /* Must have a release function or the kernel will complain on module unload */ static void cscfg_dev_release(struct device *dev) { + mutex_lock(&cscfg_mutex); kfree(cscfg_mgr); cscfg_mgr = NULL; + mutex_unlock(&cscfg_mutex); } + /* a device is needed to "own" some kernel elements such as sysfs entries. */ static int cscfg_create_device(void) { @@ -1042,6 +1066,13 @@ static int cscfg_create_device(void) if (!cscfg_mgr) goto create_dev_exit_unlock; + /* initialise the cscfg_mgr structure */ + INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); + INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); + INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); + INIT_LIST_HEAD(&cscfg_mgr->load_order_list); + atomic_set(&cscfg_mgr->sys_active_cnt, 0); + /* setup the device */ dev = cscfg_device(); dev->release = cscfg_dev_release; @@ -1056,17 +1087,74 @@ static int cscfg_create_device(void) return err; } -static void cscfg_clear_device(void) +/* + * Loading and unloading is generally on user discretion. + * If exiting due to coresight module unload, we need to unload any configurations that remain, + * before we unregister the configfs intrastructure. + * + * Do this by walking the load_owner list and taking appropriate action, depending on the load + * owner type. + */ +static void cscfg_unload_cfgs_on_exit(void) { - struct cscfg_config_desc *cfg_desc; + struct cscfg_load_owner_info *owner_info = NULL; + /* + * grab the mutex - even though we are exiting, some configfs files + * may still be live till we dump them, so ensure list data is + * protected from a race condition. + */ mutex_lock(&cscfg_mutex); - list_for_each_entry(cfg_desc, &cscfg_mgr->config_desc_list, item) { - etm_perf_del_symlink_cscfg(cfg_desc); + while (!list_empty(&cscfg_mgr->load_order_list)) { + + /* remove in reverse order of loading */ + owner_info = list_last_entry(&cscfg_mgr->load_order_list, + struct cscfg_load_owner_info, item); + + /* action according to type */ + switch (owner_info->type) { + case CSCFG_OWNER_PRELOAD: + /* + * preloaded descriptors are statically allocated in + * this module - just need to unload dynamic items from + * csdev lists, and remove from configfs directories. + */ + pr_info("cscfg: unloading preloaded configurations\n"); + break; + + case CSCFG_OWNER_MODULE: + /* + * this is an error - the loadable module must have been unloaded prior + * to the coresight module unload. Therefore that module has not + * correctly unloaded configs in its own exit code. + * Nothing to do other than emit an error string as the static descriptor + * references we need to unload will have disappeared with the module. + */ + pr_err("cscfg: ERROR - a loadable module failed to " + "unload configs on exit\n"); + goto list_remove; + } + + /* remove from configfs - outside the scope of the list mutex */ + mutex_unlock(&cscfg_mutex); + cscfg_fs_unregister_cfgs_feats(owner_info); + mutex_lock(&cscfg_mutex); + + /* Next unload from csdev lists. */ + cscfg_unload_owned_cfgs_feats(owner_info); + +list_remove: + /* remove from load order list */ + list_del(&owner_info->item); } + mutex_unlock(&cscfg_mutex); +} + +static void cscfg_clear_device(void) +{ + cscfg_unload_cfgs_on_exit(); cscfg_configfs_release(cscfg_mgr); device_unregister(cscfg_device()); - mutex_unlock(&cscfg_mutex); } /* Initialise system config management API device */ @@ -1074,20 +1162,16 @@ int __init cscfg_init(void) { int err = 0; + /* create the device and init cscfg_mgr */ err = cscfg_create_device(); if (err) return err; + /* initialise configfs subsystem */ err = cscfg_configfs_init(cscfg_mgr); if (err) goto exit_err; - INIT_LIST_HEAD(&cscfg_mgr->csdev_desc_list); - INIT_LIST_HEAD(&cscfg_mgr->feat_desc_list); - INIT_LIST_HEAD(&cscfg_mgr->config_desc_list); - INIT_LIST_HEAD(&cscfg_mgr->load_order_list); - atomic_set(&cscfg_mgr->sys_active_cnt, 0); - /* preload built-in configurations */ err = cscfg_preload(THIS_MODULE); if (err)
Any loaded configurations must be correctly unloaded on coresight module exit, or issues can arise with nested locking in the configfs directory code if built with CONFIG_LOCKDEP. Prior to this patch, the preloaded configuration configfs directory entries were being unloaded by the recursive code in configfs_unregister_subsystem(). However, when built with CONFIG_LOCKDEP, this caused a nested lock warning, which was not mitigated by the LOCKDEP dependent code in fs/configfs/dir.c designed to prevent this, due to the different directory levels for the root of the directory being removed. As the preloaded (and all other) configurations are registered after configfs_register_subsystem(), we now explicitly unload them before the call to configfs_unregister_subsystem(). The new routine cscfg_unload_cfgs_on_exit() iterates through the load owner list to unload any remaining configurations that were not unloaded by the user before the module exits. This covers both the CSCFG_OWNER_PRELOAD and CSCFG_OWNER_MODULE owner types, and will be extended to cover future load owner types for CoreSight configurations. Applies to coresight/next Fixes: eb2ec49606c2 ("coresight: syscfg: Update load API for config loadable modules") Reported-by: Suzuki Poulose <suzuki.poulose@arm.com> Signed-off-by: Mike Leach <mike.leach@linaro.org> --- .../hwtracing/coresight/coresight-syscfg.c | 106 ++++++++++++++++-- 1 file changed, 95 insertions(+), 11 deletions(-)