[PATCH] soc: tegra: cbb: walk cbb_list under cbb_lock in the debugfs path

[PATCH] soc: tegra: cbb: walk cbb_list under cbb_lock in the debugfs path

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

tegra194_cbb_probe() and tegra194_cbb_remove() add and remove cbb_list
entries under cbb_lock, and the error interrupt handler walks the list
under the same lock. tegra194_cbb_debugfs_show() walks it holding only
cbb_err_mutex, which the writers never take.

A debugfs read of one CBB instance can therefore run while another
instance is probed or removed. list_add() publishes the node with a
plain store, so the walker can see a node before its links and private
data are visible, or step onto a node that remove is freeing.

Take cbb_lock around the walk. cbb_err_mutex keeps its existing job of
serialising the error log output.

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <jaidevshastri@vt.edu>
---
 drivers/soc/tegra/cbb/tegra194-cbb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/soc/tegra/cbb/tegra194-cbb.c b/drivers/soc/tegra/cbb/tegra194-cbb.c
index 69ef929e0..d1a80b0b5 100644
--- a/drivers/soc/tegra/cbb/tegra194-cbb.c
+++ b/drivers/soc/tegra/cbb/tegra194-cbb.c
@@ -1990,9 +1990,16 @@ static DEFINE_MUTEX(cbb_err_mutex);
 static int tegra194_cbb_debugfs_show(struct tegra_cbb *cbb, struct seq_file *file, void *data)
 {
 	struct tegra_cbb *noc;
+	unsigned long flags;
 
 	mutex_lock(&cbb_err_mutex);
 
+	/*
+	 * cbb_list is modified under cbb_lock by the probe and remove paths of
+	 * the other CBB instances; cbb_err_mutex alone does not exclude them.
+	 */
+	spin_lock_irqsave(&cbb_lock, flags);
+
 	list_for_each_entry(noc, &cbb_list, node) {
 		struct tegra194_cbb *priv = to_tegra194_cbb(noc);
 		u32 status;
@@ -2002,6 +2009,8 @@ static int tegra194_cbb_debugfs_show(struct tegra_cbb *cbb, struct seq_file *fil
 			print_errlog(file, priv, status);
 	}
 
+	spin_unlock_irqrestore(&cbb_lock, flags);
+
 	mutex_unlock(&cbb_err_mutex);
 
 	return 0;

---
base-commit: 93f51579e7df248780214094418f205253383cc5
change-id: 20260921-mb-tegra-cbb-dd2dde85c68f

Best regards,
--  
Jaidev Shastri <jaidevshastri@vt.edu>

Re: [PATCH] soc: tegra: cbb: walk cbb_list under cbb_lock in the debugfs path

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2729 bytes --]

On Mon, Sep 21, 2026 at 09:08:41PM -0400, Jaidev Shastri via B4 Relay wrote:
> From: Jaidev Shastri <jaidevshastri@vt.edu>
> 
> tegra194_cbb_probe() and tegra194_cbb_remove() add and remove cbb_list
> entries under cbb_lock, and the error interrupt handler walks the list
> under the same lock. tegra194_cbb_debugfs_show() walks it holding only
> cbb_err_mutex, which the writers never take.
> 
> A debugfs read of one CBB instance can therefore run while another
> instance is probed or removed. list_add() publishes the node with a
> plain store, so the walker can see a node before its links and private
> data are visible, or step onto a node that remove is freeing.
> 
> Take cbb_lock around the walk. cbb_err_mutex keeps its existing job of
> serialising the error log output.
> 
> Found with MBCheck, a static herd7-based memory consistency checker.
> 
> Signed-off-by: Jaidev Shastri <jaidevshastri@vt.edu>
> ---
>  drivers/soc/tegra/cbb/tegra194-cbb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/soc/tegra/cbb/tegra194-cbb.c b/drivers/soc/tegra/cbb/tegra194-cbb.c
> index 69ef929e0..d1a80b0b5 100644
> --- a/drivers/soc/tegra/cbb/tegra194-cbb.c
> +++ b/drivers/soc/tegra/cbb/tegra194-cbb.c
> @@ -1990,9 +1990,16 @@ static DEFINE_MUTEX(cbb_err_mutex);
>  static int tegra194_cbb_debugfs_show(struct tegra_cbb *cbb, struct seq_file *file, void *data)
>  {
>  	struct tegra_cbb *noc;
> +	unsigned long flags;
>  
>  	mutex_lock(&cbb_err_mutex);
>  
> +	/*
> +	 * cbb_list is modified under cbb_lock by the probe and remove paths of
> +	 * the other CBB instances; cbb_err_mutex alone does not exclude them.
> +	 */
> +	spin_lock_irqsave(&cbb_lock, flags);
> +
>  	list_for_each_entry(noc, &cbb_list, node) {
>  		struct tegra194_cbb *priv = to_tegra194_cbb(noc);
>  		u32 status;
> @@ -2002,6 +2009,8 @@ static int tegra194_cbb_debugfs_show(struct tegra_cbb *cbb, struct seq_file *fil
>  			print_errlog(file, priv, status);
>  	}
>  
> +	spin_unlock_irqrestore(&cbb_lock, flags);
> +
>  	mutex_unlock(&cbb_err_mutex);

cbb_err_mutex no longer serves any purpose (though, honestly, it never
really did), so it can be removed entirely.

There's a slight issue with this patch, though it also previously
existed already (partially). print_errlog() ends up calling the
tegra_cbb_print_err(), which in turn is quite slow when it goes out to
the debug serial. On the other hand, we can't use a mutex for the dumps
during the interrupt handling.

Probably best to get rid of the mutex for now since it's obviously not
needed and maybe revisit this at some point to get rid of the spin lock
around the slow UART operations.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]