[RFC PATCH 0/4] ALSA: hda: potential hdac_stream locking issues?
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(-)
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);
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); }
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
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);
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; }
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
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
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai