| Message ID | 20200218141013.7290-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: SOF: Intel: Fix stream cleanup on pcm close | expand |
On 2/18/20 8:10 AM, Cezary Rojewski wrote: > Field "substream" gets assigned during stream setup in > hda_dsp_pcm_hw_params() but it is never cleared afterwards during > hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can > mistakenly make use of that pointer as it's bypassing all > "if (s->substream)" checks. > > Nulling the pointer during close operation ensures no wild pointers are > left behind. > > Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM operations") > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/sof/intel/hda-pcm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c > index a46a6baa1c3f..4b3a89cf20e7 100644 > --- a/sound/soc/sof/intel/hda-pcm.c > +++ b/sound/soc/sof/intel/hda-pcm.c > @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev, > > /* unbinding pcm substream to hda stream */ > substream->runtime->private_data = NULL; > + hstream->substream = NULL; > return 0; > } Humm, yes we should clean this, but wondering if the close() operation is the right place. Doing this is hda_dsp_stream_hw_free() sounds more logical to me?
On 2020-02-18 17:45, Pierre-Louis Bossart wrote: > On 2/18/20 8:10 AM, Cezary Rojewski wrote: >> Field "substream" gets assigned during stream setup in >> hda_dsp_pcm_hw_params() but it is never cleared afterwards during >> hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can >> mistakenly make use of that pointer as it's bypassing all >> "if (s->substream)" checks. >> >> Nulling the pointer during close operation ensures no wild pointers are >> left behind. >> >> Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM >> operations") >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> sound/soc/sof/intel/hda-pcm.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/sound/soc/sof/intel/hda-pcm.c >> b/sound/soc/sof/intel/hda-pcm.c >> index a46a6baa1c3f..4b3a89cf20e7 100644 >> --- a/sound/soc/sof/intel/hda-pcm.c >> +++ b/sound/soc/sof/intel/hda-pcm.c >> @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev, >> /* unbinding pcm substream to hda stream */ >> substream->runtime->private_data = NULL; >> + hstream->substream = NULL; >> return 0; >> } > > > Humm, yes we should clean this, but wondering if the close() operation > is the right place. Doing this is hda_dsp_stream_hw_free() sounds more > logical to me? Ain't hda-pcm.c the best place for it as "hstream->substream = substream" happens there too? If the cleanup is to be done in _hw_free(), then I'd expect the same to happen to the original assignments. Doubt we want to do the later so.. _close() for the win? In general the existing hstream->substream initialization looks kinda disconnected from the actual stream assignment code - _stream_get() - as if the duties of the state machine were shared. Czarek
Hi Cezary, >>> diff --git a/sound/soc/sof/intel/hda-pcm.c >>> b/sound/soc/sof/intel/hda-pcm.c >>> index a46a6baa1c3f..4b3a89cf20e7 100644 >>> --- a/sound/soc/sof/intel/hda-pcm.c >>> +++ b/sound/soc/sof/intel/hda-pcm.c >>> @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev, >>> /* unbinding pcm substream to hda stream */ >>> substream->runtime->private_data = NULL; >>> + hstream->substream = NULL; >>> return 0; >>> } >> >> >> Humm, yes we should clean this, but wondering if the close() operation >> is the right place. Doing this is hda_dsp_stream_hw_free() sounds more >> logical to me? > > Ain't hda-pcm.c the best place for it as "hstream->substream = > substream" happens there too? If the cleanup is to be done in > _hw_free(), then I'd expect the same to happen to the original > assignments. Doubt we want to do the later so.. _close() for the win? > > In general the existing hstream->substream initialization looks kinda > disconnected from the actual stream assignment code - _stream_get() - as > if the duties of the state machine were shared. I am having difficulties interpreting your answer, i.e. I don't know what the last sentence refers to. Currently open() and close() are perfectly symmetrical, I don't really see why you'd want to change and add an imbalanced set of operations, unless you moved hstream->substream = substream; to the open() instead of hw_params(). Or alternatively add a hw_free() in hda-pcm.c to mirror what's done in hw_params.
On 2020-02-18 20:05, Pierre-Louis Bossart wrote: >> >> Ain't hda-pcm.c the best place for it as "hstream->substream = >> substream" happens there too? If the cleanup is to be done in >> _hw_free(), then I'd expect the same to happen to the original >> assignments. Doubt we want to do the later so.. _close() for the win? >> >> In general the existing hstream->substream initialization looks kinda >> disconnected from the actual stream assignment code - _stream_get() - >> as if the duties of the state machine were shared. > > I am having difficulties interpreting your answer, i.e. I don't know > what the last sentence refers to. It's just safer to tie substream assignment directly with _stream_get() so problems such as this don't stay hidden and arise later during the development process. _stream_assign() in /hda/ext/hdac_ext_stream/ & _stream_release in /hda/hdac_stream do exactly that. > Currently open() and close() are perfectly symmetrical, I don't really > see why you'd want to change and add an imbalanced set of operations, > unless you moved > > hstream->substream = substream; > > to the open() instead of hw_params(). > > Or alternatively add a hw_free() in hda-pcm.c to mirror what's done in > hw_params. Relocated to _hw_free(). Thanks.
diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c index a46a6baa1c3f..4b3a89cf20e7 100644 --- a/sound/soc/sof/intel/hda-pcm.c +++ b/sound/soc/sof/intel/hda-pcm.c @@ -246,5 +246,6 @@ int hda_dsp_pcm_close(struct snd_sof_dev *sdev, /* unbinding pcm substream to hda stream */ substream->runtime->private_data = NULL; + hstream->substream = NULL; return 0; }
Field "substream" gets assigned during stream setup in hda_dsp_pcm_hw_params() but it is never cleared afterwards during hda_dsp_pcm_close(). Now, any non-pcm operation e.g.: compress can mistakenly make use of that pointer as it's bypassing all "if (s->substream)" checks. Nulling the pointer during close operation ensures no wild pointers are left behind. Fixes: cdae3b9a47aa ("ASoC: SOF: Intel: Add Intel specific HDA PCM operations") Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/sof/intel/hda-pcm.c | 1 + 1 file changed, 1 insertion(+)