Re: [PATCH 00/11] ALSA: PCM state reference optimization

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Takashi Iwai <tiwai@suse.de>
Cc: Felipe Balbi <balbi@kernel.org>,
	alsa-devel@alsa-project.org,
	Kai Vehmanen <kai.vehmanen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Peter Ujfalusi <peter.ujfalusi@linux.intel.com>,
	Cezary Rojewski <cezary.rojewski@intel.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Liam Girdwood <liam.r.girdwood@linux.intel.com>,
	Mark Brown <broonie@kernel.org>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Bard Liao <yung-chuan.liao@linux.intel.com>
Subject: Re: [PATCH 00/11] ALSA: PCM state reference optimization
Date: Tue, 27 Sep 2022 10:22:07 +0900	[thread overview]
Message-ID: <YzJQP26l3k7yrTrs@workstation> (raw)
In-Reply-To: <20220926135558.26580-1-tiwai@suse.de>

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.

> and the
> second patch flips the PCM status mmap to read-only for hardening,

The patch looks good to me as well as the patch for asihpi driver, and
should be in your tree independent from the others.

> while the remaining patches are for drivers to follow the core
> change.
>
> The conversions are straightforward.  In most places, it's just
> replacing runtime->status->state with runtime->state.

The usage of mentioned macro can control the concerned access. Now
addition of new field, `state` to runtime structure can be done easily.


> Takashi
> 
> ===
> 
> Takashi Iwai (11):
>   ALSA: pcm: Avoid reference to status->state
>   ALSA: pcm: Make mmap status read-only
>   ALSA: aloop: Replace runtime->status->state reference to
>     runtime->state
>   ALSA: firewire: Replace runtime->status->state reference to
>     runtime->state
>   ALSA: hda: Replace runtime->status->state reference to runtime->state
>   ALSA: asihpi: Replace runtime->status->state reference to
>     runtime->state
>   ALSA: usb-audio: Replace runtime->status->state reference to
>     runtime->state
>   ALSA: usx2y: Replace runtime->status->state reference to
>     runtime->state
>   ASoC: intel: Replace runtime->status->state reference to
>     runtime->state
>   ASoC: sh: Replace runtime->status->state reference to runtime->state
>   usb: gadget: Replace runtime->status->state reference to
>     runtime->state
> 
>  drivers/usb/gadget/function/u_uac1_legacy.c |   4 +-
>  include/sound/pcm.h                         |  20 ++-
>  sound/core/oss/pcm_oss.c                    |  42 +++----
>  sound/core/pcm.c                            |   9 +-
>  sound/core/pcm_compat.c                     |   4 +-
>  sound/core/pcm_lib.c                        |  16 +--
>  sound/core/pcm_native.c                     | 128 ++++++++++----------
>  sound/drivers/aloop.c                       |   4 +-
>  sound/firewire/bebob/bebob_pcm.c            |   4 +-
>  sound/firewire/dice/dice-pcm.c              |   4 +-
>  sound/firewire/digi00x/digi00x-pcm.c        |   4 +-
>  sound/firewire/fireface/ff-pcm.c            |   4 +-
>  sound/firewire/fireworks/fireworks_pcm.c    |   4 +-
>  sound/firewire/motu/motu-pcm.c              |   4 +-
>  sound/firewire/oxfw/oxfw-pcm.c              |   8 +-
>  sound/firewire/tascam/tascam-pcm.c          |   4 +-
>  sound/hda/hdmi_chmap.c                      |   2 +-
>  sound/pci/asihpi/asihpi.c                   |   2 +-
>  sound/soc/intel/skylake/skl-pcm.c           |   4 +-
>  sound/soc/sh/rz-ssi.c                       |   2 +-
>  sound/usb/pcm.c                             |   4 +-
>  sound/usb/usx2y/usbusx2yaudio.c             |   3 +-
>  sound/usb/usx2y/usx2yhwdeppcm.c             |   3 +-
>  23 files changed, 150 insertions(+), 133 deletions(-)
> 
> ===
> 
> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: Cezary Rojewski <cezary.rojewski@intel.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> -- 
> 2.35.3

Regards

Takashi Sakamoto
