On Tue, Sep 27, 2022 at 08:26:11AM +0200, Takashi Iwai wrote:
> On Tue, 27 Sep 2022 03:22:07 +0200,
> Takashi Sakamoto wrote:
> >
> > Hi,
> >
> > On Mon, Sep 26, 2022 at 03:55:47PM +0200, Takashi Iwai wrote:
> > > Hi,
> > >
> > > this is a patch set for simplifying the reference to the current PCM
> > > state by having the local copy in runtime instead of relying on
> > > runtime->status indirection. This also hardens against the attack by
> > > modifying the mmapped status record.
> >
> > The overall patches looks good to me and I have no objections, while I
> > have some slight opinions to them in a place of sound driver developer.
> >
> > > The first patch does the basic job in the core PCM side,
> >
> > The main concern is indirect accessing to state field via some pointer
> > hops. I think addition of helper macro at first step eases centre of your
> > work, like:
> >
> > ```
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index 8c48a5bce88c..f6a160cb8135 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -669,6 +669,20 @@ void snd_pcm_stream_unlock_irqrestore(struct snd_pcm_substream *substream,
> > stream <= SNDRV_PCM_STREAM_LAST; \
> > stream++)
> >
> > +/**
> > + * snd_pcm_stream_state - Return state in runtime of the PCM substream.
> > + * @substream: substream to check. runtime should be attached.
> > + *
> > + * Return state in runtime of the PCM substream. The substream should exists and
> > + * runtime should be attached to it.
> > + */
> > +static inline snd_pcm_state_t snd_pcm_stream_state(const snd_pcm_substream *substream)
> > +{
> > + snd_BUG_ON(!(sub) || !(sub)->runtime);
> > +
> > + return substream->runtime->status->state;
> > +}
> > ```
> >
> > As we can see, sound driver programmer sometimes checks state of runtime
> > in their code, thus the macro could helps them as well as centre of your
> > change.
>
> This might help if we may need any change in future again.
>
> But the NULL check is superfluous in most places, and the state check
> is done often in a hot path, it's better to rip off. So it's only
> about the read of runtime->state, and then it's doubtful whether it
> brings much value at this moment; that's the reason I didn't introduce
> such a macro for now.
Yes, so I have no strong suggestion to introduce the macro and no
objections to apply the patches as is.
I'm too conservative developer and consider about usage of memory
barrier always when seeing such state check since we should pay enough
attension to instruction reordering and concurrent accesses across
processors for such case. Fortunately we have no reports for such issue
yet. When posting the last message, it was on my mind and it's still.
Regards
Takashi Sakamoto
