[PATCH v4 7/7] configfs: Fix LOCKDEP nesting issues with fragment semaphores

[PATCH v4 7/7] configfs: Fix LOCKDEP nesting issues with fragment semaphores

[Mike Leach]

CoreSight uses configfs to represent the user interface to programmed
configurations, which can be loaded and unloaded dynamically via configfs.

These add and remove configurations using register/unregister group
calls.

It has been found that if CONFIG_LOCKDEP is enabled, then it appears to
be confused by the nesting inherent in the fragment semaphores used
by groups and the underlying subsystem.

This patch sets up a mechanism to use separate classes for the fragment
semaphores, in a similar way to that already in place to fix nesting
issues with the i_mutexes.

Cc: Joel Becker <jlbec@evilplan.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 fs/configfs/configfs_internal.h |  3 +++
 fs/configfs/dir.c               | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index c0395363eab9..736c74ec4b7a 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -22,6 +22,9 @@ struct configfs_fragment {
 	atomic_t frag_count;
 	struct rw_semaphore frag_sem;
 	bool frag_dead;
+#ifdef CONFIG_LOCKDEP
+	int frag_depth;
+#endif
 };
 
 void put_fragment(struct configfs_fragment *);
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index d1f9d2632202..6ecd8961afc3 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -133,6 +133,41 @@ configfs_adjust_dir_dirent_depth_after_populate(struct configfs_dirent *sd)
 	sd->s_depth = -1;
 }
 
+/* fragment semaphore needs some lockdep handling */
+static struct lock_class_key default_frag_class[MAX_LOCK_DEPTH];
+
+/*
+ * Set the lockdep depth for a new fragment based on the parent frag depth.
+ * Called from register_subsystem() with NULL parent group to set root subsystem
+ * depth which defaults to 0 in a new fragment, and from register_group() with the
+ * parent group to set a new group fragment based on the parent fragment depth.
+ *
+ * Prevents lockdep getting upset on the unregister_group() call if it cannot
+ * understand the hierarchy of fragments.
+ */
+static void configfs_adjust_frag_depth(struct configfs_fragment *frag,
+				       struct config_group *parent_group)
+{
+	struct configfs_dirent *parent_dirent;
+
+	if (parent_group) {
+		// find parent frag
+		parent_dirent = parent_group->cg_item.ci_dentry->d_fsdata;
+		frag->frag_depth = parent_dirent->s_frag->frag_depth + 1;
+	}
+
+	if (frag->frag_depth < ARRAY_SIZE(default_frag_class)) {
+		lockdep_set_class(&frag->frag_sem,
+				  &default_frag_class[frag->frag_depth]);
+	} else {
+		/*
+		 * In practice the maximum level of locking depth is
+		 * already reached. Just inform about possible reasons.
+		 */
+		pr_info("Too many levels of fragments for the locking correctness validator.\n");
+	}
+}
+
 #else /* CONFIG_LOCKDEP */
 
 static void configfs_init_dirent_depth(struct configfs_dirent *sd)
@@ -154,6 +189,11 @@ configfs_adjust_dir_dirent_depth_after_populate(struct configfs_dirent *sd)
 {
 }
 
+static void configfs_adjust_frag_depth(struct configfs_fragment *frag,
+				       struct config_group *parent_group)
+{
+}
+
 #endif /* CONFIG_LOCKDEP */
 
 static struct configfs_fragment *new_fragment(void)
@@ -165,6 +205,9 @@ static struct configfs_fragment *new_fragment(void)
 		atomic_set(&p->frag_count, 1);
 		init_rwsem(&p->frag_sem);
 		p->frag_dead = false;
+#ifdef CONFIG_LOCKDEP
+		p->frag_depth = 0;
+#endif
 	}
 	return p;
 }
@@ -1742,6 +1785,7 @@ int configfs_register_group(struct config_group *parent_group,
 	parent = parent_group->cg_item.ci_dentry;
 
 	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
+	configfs_adjust_frag_depth(frag, parent_group);
 	ret = create_default_group(parent_group, group, frag);
 	if (ret)
 		goto err_out;
@@ -1872,6 +1916,7 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 	mutex_unlock(&configfs_subsystem_mutex);
 
 	inode_lock_nested(d_inode(root), I_MUTEX_PARENT);
+	configfs_adjust_frag_depth(frag, NULL);
 
 	err = -ENOMEM;
 	dentry = d_alloc_name(root, group->cg_item.ci_name);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/7] configfs: Fix LOCKDEP nesting issues with fragment semaphores

[Christoph Hellwig]

Can you send me the whole series?  Reviewing a patch 7 out of 7 without
context is basically impossible.

On Mon, Jul 04, 2022 at 04:42:49PM +0100, Mike Leach wrote:
> CoreSight uses configfs to represent the user interface to programmed
> configurations, which can be loaded and unloaded dynamically via configfs.
> 
> These add and remove configurations using register/unregister group
> calls.
> 
> It has been found that if CONFIG_LOCKDEP is enabled, then it appears to
> be confused by the nesting inherent in the fragment semaphores used
> by groups and the underlying subsystem.
> 
> This patch sets up a mechanism to use separate classes for the fragment
> semaphores, in a similar way to that already in place to fix nesting
> issues with the i_mutexes.
> 
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  fs/configfs/configfs_internal.h |  3 +++
>  fs/configfs/dir.c               | 45 +++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
> index c0395363eab9..736c74ec4b7a 100644
> --- a/fs/configfs/configfs_internal.h
> +++ b/fs/configfs/configfs_internal.h
> @@ -22,6 +22,9 @@ struct configfs_fragment {
>  	atomic_t frag_count;
>  	struct rw_semaphore frag_sem;
>  	bool frag_dead;
> +#ifdef CONFIG_LOCKDEP
> +	int frag_depth;
> +#endif
>  };
>  
>  void put_fragment(struct configfs_fragment *);
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index d1f9d2632202..6ecd8961afc3 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -133,6 +133,41 @@ configfs_adjust_dir_dirent_depth_after_populate(struct configfs_dirent *sd)
>  	sd->s_depth = -1;
>  }
>  
> +/* fragment semaphore needs some lockdep handling */
> +static struct lock_class_key default_frag_class[MAX_LOCK_DEPTH];
> +
> +/*
> + * Set the lockdep depth for a new fragment based on the parent frag depth.
> + * Called from register_subsystem() with NULL parent group to set root subsystem
> + * depth which defaults to 0 in a new fragment, and from register_group() with the
> + * parent group to set a new group fragment based on the parent fragment depth.
> + *
> + * Prevents lockdep getting upset on the unregister_group() call if it cannot
> + * understand the hierarchy of fragments.
> + */
> +static void configfs_adjust_frag_depth(struct configfs_fragment *frag,
> +				       struct config_group *parent_group)
> +{
> +	struct configfs_dirent *parent_dirent;
> +
> +	if (parent_group) {
> +		// find parent frag
> +		parent_dirent = parent_group->cg_item.ci_dentry->d_fsdata;
> +		frag->frag_depth = parent_dirent->s_frag->frag_depth + 1;
> +	}
> +
> +	if (frag->frag_depth < ARRAY_SIZE(default_frag_class)) {
> +		lockdep_set_class(&frag->frag_sem,
> +				  &default_frag_class[frag->frag_depth]);
> +	} else {
> +		/*
> +		 * In practice the maximum level of locking depth is
> +		 * already reached. Just inform about possible reasons.
> +		 */
> +		pr_info("Too many levels of fragments for the locking correctness validator.\n");
> +	}
> +}
> +
>  #else /* CONFIG_LOCKDEP */
>  
>  static void configfs_init_dirent_depth(struct configfs_dirent *sd)
> @@ -154,6 +189,11 @@ configfs_adjust_dir_dirent_depth_after_populate(struct configfs_dirent *sd)
>  {
>  }
>  
> +static void configfs_adjust_frag_depth(struct configfs_fragment *frag,
> +				       struct config_group *parent_group)
> +{
> +}
> +
>  #endif /* CONFIG_LOCKDEP */
>  
>  static struct configfs_fragment *new_fragment(void)
> @@ -165,6 +205,9 @@ static struct configfs_fragment *new_fragment(void)
>  		atomic_set(&p->frag_count, 1);
>  		init_rwsem(&p->frag_sem);
>  		p->frag_dead = false;
> +#ifdef CONFIG_LOCKDEP
> +		p->frag_depth = 0;
> +#endif
>  	}
>  	return p;
>  }
> @@ -1742,6 +1785,7 @@ int configfs_register_group(struct config_group *parent_group,
>  	parent = parent_group->cg_item.ci_dentry;
>  
>  	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
> +	configfs_adjust_frag_depth(frag, parent_group);
>  	ret = create_default_group(parent_group, group, frag);
>  	if (ret)
>  		goto err_out;
> @@ -1872,6 +1916,7 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
>  	mutex_unlock(&configfs_subsystem_mutex);
>  
>  	inode_lock_nested(d_inode(root), I_MUTEX_PARENT);
> +	configfs_adjust_frag_depth(frag, NULL);
>  
>  	err = -ENOMEM;
>  	dentry = d_alloc_name(root, group->cg_item.ci_name);
> -- 
> 2.17.1
---end quoted text---

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/7] configfs: Fix LOCKDEP nesting issues with fragment semaphores

[Mike Leach]

Hi Christoph.

[adding in Suzuki and Mathieu who maintain the coresight subsystem & lists]

You are correct - in this patchset we are adding the use of a binary
attribute to load and unload Coresight configurations and features -
which action has a side effect of adding and removing entries in
particular sub-directories in our configfs sub-system.

Our use case for configfs is somewhat more complex than the other
existing usages - such as ACPI, where the format of the directory
structure and attributes is static, and known ahead of time.

The CoreSight configurations have variable numbers of attributes
appearing in the configfs directories  - so the attribute arrays are
built dynamically at load time. The load process and also result in
elements appearing in both the cs-syscfg/configurations and the
cs-syscfg/features sub directories. This dynamic nature and split
elements means that the traditional mkdir/rmdir configfs paradigm
cannot be made to work.

We currently have two methods upstreamed for loading configurations, 1
is a configuration directly built into the coresight core code, and
the other is to allow configurations to be loaded as kernel loadable
modules.
However these 2 are dependent on compile time operations, and have
kernel dependencies.

Hence we have introduced the direct load via configfs binary attribute
- which is more portable, flexible and convenient for the end user.

I appreciate that this may not be a usage you anticipated for
configfs, but it does serve our purposes very well, and as far a I can
tell from examination of configfs code and considerable testing, works
fine and does not have any operational issues, other than the lockdep
warnings on unload, which are caused be the fragment locks (lockdep
being confused by holding the fragment for the initial cs-syscfg root
system, and the one for the sub-directory we want to unload).

I am open to suggestions for a different way of doing this within the
established directory structure we need to maintain.

Thanks and Regards

Mike









On Fri, 29 Jul 2022 at 14:24, Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Jul 29, 2022 at 07:41:00AM +0100, Mike Leach wrote:
> > Sure - see below
> >
> > Happens  during the unload process of the coresight configuration.
>
> Hmm.  It seems like youre bin_attr ->write handler (which gets called
> from configfs_release_bin_file) tries to unregister a group.  That's
> not really how the configfs API is supposed to be used.



--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/7] configfs: Fix LOCKDEP nesting issues with fragment semaphores

[Christoph Hellwig]

On Fri, Aug 05, 2022 at 03:15:53PM +0100, Mike Leach wrote:
> You are correct - in this patchset we are adding the use of a binary
> attribute to load and unload Coresight configurations and features -
> which action has a side effect of adding and removing entries in
> particular sub-directories in our configfs sub-system.

I don't think configfs was designed to be that dynamic.  Especially
the fact that a write to a binary attribute, which is intended to
be just for passthrough to hardware is a bit concerning.

From your further description it sounds like this binary attribute
is loading structured data interpreted by the kernel, which really
isn't how binary attributes in configfs or sysfs are intended to
be used.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/7] configfs: Fix LOCKDEP nesting issues with fragment semaphores

[Mike Leach]

Hi Christoph

On Thu, 11 Aug 2022 at 16:09, Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Aug 05, 2022 at 03:15:53PM +0100, Mike Leach wrote:
> > You are correct - in this patchset we are adding the use of a binary
> > attribute to load and unload Coresight configurations and features -
> > which action has a side effect of adding and removing entries in
> > particular sub-directories in our configfs sub-system.
>
> I don't think configfs was designed to be that dynamic.  Especially
> the fact that a write to a binary attribute, which is intended to
> be just for passthrough to hardware is a bit concerning.
>

configfs may not  have originally been intended to be used in quite
this way, but the system is robust and well designed, and the APIs
work perfectly well in the this way.
The model of user programming a configuration item for kernel backed
elements that have a userspace lifetime (as described in configfs.rst)
is precisely the model we need to successfully leverage the coresight
subsystem. Our requirements are more complex than the ACPI tables or
USB gadget drivers that use configfs, as we are potentially
configuring multiple coresight devices. What the user sees in the
configfs directories for the configurations are only the user
adjustable features of these configurations. The rest of the
programming is set on the relevant devices.

> From your further description it sounds like this binary attribute
> is loading structured data interpreted by the kernel, which really
> isn't how binary attributes in configfs or sysfs are intended to
> be used.

If you look at the ACPI configfs, then this is in fact loading an ACPI
table structure into the kernel, through a binary attribute, that is
validated and added to a list of tables, if it passes the validation.
This is also an example of using a binary attribute to pass structured
data into the kernel for subsequent interpretation - and this example
is why I chose to use binary attributes in this way.

The data passed through the coresight 'load' binary attribute is
validated, and then only if valid loaded into the coresight subsystem
for use when coresight trace is activated and the user decides to use
the configuration. (coresight configurations are loaded, but are only
used if specifically selected during a trace session).

Regards

Mike


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel