[RFC PATCH 0/4] ALSA: hda: potential hdac_stream locking issues?

[RFC PATCH 0/4] ALSA: hda: potential hdac_stream locking issues?

[Pierre-Louis Bossart]

While reviewing the HDAudio DMA handling, I found a number of
inconsistencies in how spin_locks are used. It's not clear what the
HDaudio bus->reg_lock is supposed to protect. In most cases only the
writes to specific boolean status flags are protected, and there are
multiple cases of taking the lock after testing a status flag.

This patchset suggests a more consistent locking pattern, but it's
entirely possible that the bus->reg_lock is only intented to protect
register read/write access on the HDaudio bus, and not the status
flags, and that this entire piece of code is completely
over-engineered.

On the Intel side no one knows why this spinlock was used, the reasons
are lost to history. I set the 'RFC' status on purpose in the hope
that Takashi might recall what this lock is supposed to protect. The
diff format makes this patchset difficult to review, it's recommended
to apply the patches and look at entire functions with changes to get
a better idea of the suggested changes.

Pierre-Louis Bossart (4):
  ALSA: hda: hdac_stream: fix potential locking issue in
    snd_hdac_stream_assign()
  ALSA: hda: hdac_stream: reset assigned_key in stream_release()
  ALSA: hda: hdac_ext_stream: fix potential locking issues
  ASoC: SOF: Intel: hda-dai: fix potential locking issue

 include/sound/hdaudio_ext.h     |  2 ++
 sound/hda/ext/hdac_ext_stream.c | 46 ++++++++++++++++++++-------------
 sound/hda/hdac_stream.c         |  5 ++--
 sound/soc/sof/intel/hda-dai.c   |  7 ++---
 4 files changed, 37 insertions(+), 23 deletions(-)

-- 
2.25.1

[RFC PATCH 1/4] ALSA: hda: hdac_stream: fix potential locking issue in snd_hdac_stream_assign()

[Pierre-Louis Bossart]

The fields 'opened', 'running', 'assigned_key' are all protected by a
spinlock, but the spinlock is not taken when looking for a
stream. This can result in a possible race between assign() and
release().

Fix by taking the spinlock before walking through the bus stream list.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/hda/hdac_stream.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index 1eb8563db2df..9867555883c3 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -296,6 +296,7 @@ struct hdac_stream *snd_hdac_stream_assign(struct hdac_bus *bus,
 	int key = (substream->pcm->device << 16) | (substream->number << 2) |
 		(substream->stream + 1);
 
+	spin_lock_irq(&bus->reg_lock);
 	list_for_each_entry(azx_dev, &bus->stream_list, list) {
 		if (azx_dev->direction != substream->stream)
 			continue;
@@ -309,13 +310,12 @@ struct hdac_stream *snd_hdac_stream_assign(struct hdac_bus *bus,
 			res = azx_dev;
 	}
 	if (res) {
-		spin_lock_irq(&bus->reg_lock);
 		res->opened = 1;
 		res->running = 0;
 		res->assigned_key = key;
 		res->substream = substream;
-		spin_unlock_irq(&bus->reg_lock);
 	}
+	spin_unlock_irq(&bus->reg_lock);
 	return res;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_stream_assign);
-- 
2.25.1

[RFC PATCH 2/4] ALSA: hda: hdac_stream: reset assigned_key in stream_release()

[Pierre-Louis Bossart]

The 'assigned_key' field is set in assign() and never reset. For
symmetry set to zero in release().

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/hda/hdac_stream.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index 9867555883c3..3ae23c50d505 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -333,6 +333,7 @@ void snd_hdac_stream_release(struct hdac_stream *azx_dev)
 	spin_lock_irq(&bus->reg_lock);
 	azx_dev->opened = 0;
 	azx_dev->running = 0;
+	azx_dev->assigned_key = 0;
 	azx_dev->substream = NULL;
 	spin_unlock_irq(&bus->reg_lock);
 }
-- 
2.25.1

Re: [RFC PATCH 2/4] ALSA: hda: hdac_stream: reset assigned_key in stream_release()

[Takashi Iwai]

On Fri, 24 Sep 2021 21:24:15 +0200,
Pierre-Louis Bossart wrote:
> 
> The 'assigned_key' field is set in assign() and never reset. For
> symmetry set to zero in release().

This is intentional behavior.  We want to assign to the same stream
persistently unless it has to be reassigned to another.


thanks,

Takashi

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/hda/hdac_stream.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index 9867555883c3..3ae23c50d505 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -333,6 +333,7 @@ void snd_hdac_stream_release(struct hdac_stream *azx_dev)
>  	spin_lock_irq(&bus->reg_lock);
>  	azx_dev->opened = 0;
>  	azx_dev->running = 0;
> +	azx_dev->assigned_key = 0;
>  	azx_dev->substream = NULL;
>  	spin_unlock_irq(&bus->reg_lock);
>  }
> -- 
> 2.25.1
> 

[RFC PATCH 3/4] ALSA: hda: hdac_ext_stream: fix potential locking issues

[Pierre-Louis Bossart]

The code for hdac_ext_stream seems inherited from hdac_stream, and
similar locking issues are present: the use of the bus->reg_lock
spinlock is inconsistent, with only writes to specific fields being
protected.

Apply similar fix as in hdac_stream by protecting all accesses to
'link_locked' and 'decoupled' fields, with a new helper
snd_hdac_ext_stream_decouple_locked() added to simplify code
changes.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/hdaudio_ext.h     |  2 ++
 sound/hda/ext/hdac_ext_stream.c | 46 ++++++++++++++++++++-------------
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index 375581634143..d4e31ea16aba 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -88,6 +88,8 @@ struct hdac_ext_stream *snd_hdac_ext_stream_assign(struct hdac_bus *bus,
 					   struct snd_pcm_substream *substream,
 					   int type);
 void snd_hdac_ext_stream_release(struct hdac_ext_stream *azx_dev, int type);
+void snd_hdac_ext_stream_decouple_locked(struct hdac_bus *bus,
+				  struct hdac_ext_stream *azx_dev, bool decouple);
 void snd_hdac_ext_stream_decouple(struct hdac_bus *bus,
 				struct hdac_ext_stream *azx_dev, bool decouple);
 void snd_hdac_ext_stop_streams(struct hdac_bus *bus);
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
index 0c005d67fa89..37154ed43bd5 100644
--- a/sound/hda/ext/hdac_ext_stream.c
+++ b/sound/hda/ext/hdac_ext_stream.c
@@ -106,20 +106,14 @@ void snd_hdac_stream_free_all(struct hdac_bus *bus)
 }
 EXPORT_SYMBOL_GPL(snd_hdac_stream_free_all);
 
-/**
- * snd_hdac_ext_stream_decouple - decouple the hdac stream
- * @bus: HD-audio core bus
- * @stream: HD-audio ext core stream object to initialize
- * @decouple: flag to decouple
- */
-void snd_hdac_ext_stream_decouple(struct hdac_bus *bus,
-				struct hdac_ext_stream *stream, bool decouple)
+void snd_hdac_ext_stream_decouple_locked(struct hdac_bus *bus,
+					 struct hdac_ext_stream *stream,
+					 bool decouple)
 {
 	struct hdac_stream *hstream = &stream->hstream;
 	u32 val;
 	int mask = AZX_PPCTL_PROCEN(hstream->index);
 
-	spin_lock_irq(&bus->reg_lock);
 	val = readw(bus->ppcap + AZX_REG_PP_PPCTL) & mask;
 
 	if (decouple && !val)
@@ -128,6 +122,20 @@ void snd_hdac_ext_stream_decouple(struct hdac_bus *bus,
 		snd_hdac_updatel(bus->ppcap, AZX_REG_PP_PPCTL, mask, 0);
 
 	stream->decoupled = decouple;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_decouple_locked);
+
+/**
+ * snd_hdac_ext_stream_decouple - decouple the hdac stream
+ * @bus: HD-audio core bus
+ * @stream: HD-audio ext core stream object to initialize
+ * @decouple: flag to decouple
+ */
+void snd_hdac_ext_stream_decouple(struct hdac_bus *bus,
+				  struct hdac_ext_stream *stream, bool decouple)
+{
+	spin_lock_irq(&bus->reg_lock);
+	snd_hdac_ext_stream_decouple_locked(bus, stream, decouple);
 	spin_unlock_irq(&bus->reg_lock);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_decouple);
@@ -252,6 +260,7 @@ hdac_ext_link_stream_assign(struct hdac_bus *bus,
 		return NULL;
 	}
 
+	spin_lock_irq(&bus->reg_lock);
 	list_for_each_entry(stream, &bus->stream_list, list) {
 		struct hdac_ext_stream *hstream = container_of(stream,
 						struct hdac_ext_stream,
@@ -266,17 +275,16 @@ hdac_ext_link_stream_assign(struct hdac_bus *bus,
 		}
 
 		if (!hstream->link_locked) {
-			snd_hdac_ext_stream_decouple(bus, hstream, true);
+			snd_hdac_ext_stream_decouple_locked(bus, hstream, true);
 			res = hstream;
 			break;
 		}
 	}
 	if (res) {
-		spin_lock_irq(&bus->reg_lock);
 		res->link_locked = 1;
 		res->link_substream = substream;
-		spin_unlock_irq(&bus->reg_lock);
 	}
+	spin_unlock_irq(&bus->reg_lock);
 	return res;
 }
 
@@ -292,6 +300,7 @@ hdac_ext_host_stream_assign(struct hdac_bus *bus,
 		return NULL;
 	}
 
+	spin_lock_irq(&bus->reg_lock);
 	list_for_each_entry(stream, &bus->stream_list, list) {
 		struct hdac_ext_stream *hstream = container_of(stream,
 						struct hdac_ext_stream,
@@ -301,18 +310,17 @@ hdac_ext_host_stream_assign(struct hdac_bus *bus,
 
 		if (!stream->opened) {
 			if (!hstream->decoupled)
-				snd_hdac_ext_stream_decouple(bus, hstream, true);
+				snd_hdac_ext_stream_decouple_locked(bus, hstream, true);
 			res = hstream;
 			break;
 		}
 	}
 	if (res) {
-		spin_lock_irq(&bus->reg_lock);
 		res->hstream.opened = 1;
 		res->hstream.running = 0;
 		res->hstream.substream = substream;
-		spin_unlock_irq(&bus->reg_lock);
 	}
+	spin_unlock_irq(&bus->reg_lock);
 
 	return res;
 }
@@ -378,15 +386,17 @@ void snd_hdac_ext_stream_release(struct hdac_ext_stream *stream, int type)
 		break;
 
 	case HDAC_EXT_STREAM_TYPE_HOST:
+		spin_lock_irq(&bus->reg_lock);
 		if (stream->decoupled && !stream->link_locked)
-			snd_hdac_ext_stream_decouple(bus, stream, false);
+			snd_hdac_ext_stream_decouple_locked(bus, stream, false);
+		spin_unlock_irq(&bus->reg_lock);
 		snd_hdac_stream_release(&stream->hstream);
 		break;
 
 	case HDAC_EXT_STREAM_TYPE_LINK:
-		if (stream->decoupled && !stream->hstream.opened)
-			snd_hdac_ext_stream_decouple(bus, stream, false);
 		spin_lock_irq(&bus->reg_lock);
+		if (stream->decoupled && !stream->hstream.opened)
+			snd_hdac_ext_stream_decouple_locked(bus, stream, false);
 		stream->link_locked = 0;
 		stream->link_substream = NULL;
 		spin_unlock_irq(&bus->reg_lock);
-- 
2.25.1

[RFC PATCH 4/4] ASoC: SOF: Intel: hda-dai: fix potential locking issue

[Pierre-Louis Bossart]

The initial hdac_stream code was adapted a third time with the same
locking issues. Move the spin_lock outside the loops and make sure the
fields are protected on read/write.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/intel/hda-dai.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index d71165fb6fad..e349f06c49fa 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -72,6 +72,7 @@ static struct hdac_ext_stream *
 		return NULL;
 	}
 
+	spin_lock_irq(&bus->reg_lock);
 	list_for_each_entry(stream, &bus->stream_list, list) {
 		struct hdac_ext_stream *hstream =
 			stream_to_hdac_ext_stream(stream);
@@ -111,12 +112,12 @@ static struct hdac_ext_stream *
 		 * is updated in snd_hdac_ext_stream_decouple().
 		 */
 		if (!res->decoupled)
-			snd_hdac_ext_stream_decouple(bus, res, true);
-		spin_lock_irq(&bus->reg_lock);
+			snd_hdac_ext_stream_decouple_locked(bus, res, true);
+
 		res->link_locked = 1;
 		res->link_substream = substream;
-		spin_unlock_irq(&bus->reg_lock);
 	}
+	spin_unlock_irq(&bus->reg_lock);
 
 	return res;
 }
-- 
2.25.1

Re: [RFC PATCH 4/4] ASoC: SOF: Intel: hda-dai: fix potential locking issue

[Mark Brown]

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

On Fri, Sep 24, 2021 at 02:24:17PM -0500, Pierre-Louis Bossart wrote:
> The initial hdac_stream code was adapted a third time with the same
> locking issues. Move the spin_lock outside the loops and make sure the
> fields are protected on read/write.

Acked-by: Mark Brown <broonie@kernel.org>

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

Re: [RFC PATCH 0/4] ALSA: hda: potential hdac_stream locking issues?

[Takashi Iwai]

On Fri, 24 Sep 2021 21:24:13 +0200,
Pierre-Louis Bossart wrote:
> 
> While reviewing the HDAudio DMA handling, I found a number of
> inconsistencies in how spin_locks are used. It's not clear what the
> HDaudio bus->reg_lock is supposed to protect. In most cases only the
> writes to specific boolean status flags are protected, and there are
> multiple cases of taking the lock after testing a status flag.
> 
> This patchset suggests a more consistent locking pattern, but it's
> entirely possible that the bus->reg_lock is only intented to protect
> register read/write access on the HDaudio bus, and not the status
> flags, and that this entire piece of code is completely
> over-engineered.
> 
> On the Intel side no one knows why this spinlock was used, the reasons
> are lost to history. I set the 'RFC' status on purpose in the hope
> that Takashi might recall what this lock is supposed to protect. The
> diff format makes this patchset difficult to review, it's recommended
> to apply the patches and look at entire functions with changes to get
> a better idea of the suggested changes.

Oh well, the missing piece was the lock inside the loop in
*_stream_assign().  It was lost while factoring out and converting to
the HD-audio core code.  (The lock wasn't taken on the whole loop at
that time because the list itself was supposed to be static.)

In anyway, I applied all patches except for patch#2, as the fixes for
spinlock look correct.


thanks,

Takashi