[PATCH v4 1/2] coresight: configfs: Fix unload of configurations on module exit
Mike Leach
mike.leach at linaro.org
Tue Jun 28 10:25:57 PDT 2022
Hi,
On Thu, 23 Jun 2022 at 20:34, Mathieu Poirier
<mathieu.poirier at 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 at arm.com>
> > Signed-off-by: Mike Leach <mike.leach at 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
> >
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
More information about the linux-arm-kernel
mailing list