| Message ID | 1418077426-8309-7-git-send-email-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Mon, 8 Dec 2014 16:23:43 -0600, Pierre-Louis Bossart wrote: > > Let userspace select audio timestamp config, ignore and zero all > other fields > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > sound/core/pcm_native.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 37a7137..7dcd6bd 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, > struct snd_pcm_runtime *runtime = substream->runtime; > > snd_pcm_stream_lock_irq(substream); > + memset(status, 0, sizeof(*status)); This memset() can be outside the lock. > status->state = runtime->status->state; > status->suspended_state = runtime->status->suspended_state; > if (status->state == SNDRV_PCM_STATE_OPEN) > @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream, > struct snd_pcm_status status; > int res; > > - memset(&status, 0, sizeof(status)); > + if (copy_from_user(&status, _status, sizeof(status))) > + return -EFAULT; > res = snd_pcm_status(substream, &status); You're clearing the data at the beginning of snd_pcm_status(), so it doesn't make sense to do copy_from_user(). In anyway, I prefer passing the extra argument for the parameter user may set instead of initializing the whole struct beforehand. snd_pcm_status() is called only internally, so it's no big problem to change the arguments. Takashi
On 12/10/14, 11:28 AM, Takashi Iwai wrote: > At Mon, 8 Dec 2014 16:23:43 -0600, > Pierre-Louis Bossart wrote: >> >> Let userspace select audio timestamp config, ignore and zero all >> other fields >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> sound/core/pcm_native.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c >> index 37a7137..7dcd6bd 100644 >> --- a/sound/core/pcm_native.c >> +++ b/sound/core/pcm_native.c >> @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, >> struct snd_pcm_runtime *runtime = substream->runtime; >> >> snd_pcm_stream_lock_irq(substream); >> + memset(status, 0, sizeof(*status)); > > This memset() can be outside the lock. > >> status->state = runtime->status->state; >> status->suspended_state = runtime->status->suspended_state; >> if (status->state == SNDRV_PCM_STATE_OPEN) >> @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream, >> struct snd_pcm_status status; >> int res; >> >> - memset(&status, 0, sizeof(status)); >> + if (copy_from_user(&status, _status, sizeof(status))) >> + return -EFAULT; >> res = snd_pcm_status(substream, &status); > > You're clearing the data at the beginning of snd_pcm_status(), so it > doesn't make sense to do copy_from_user(). > > In anyway, I prefer passing the extra argument for the parameter user > may set instead of initializing the whole struct beforehand. > snd_pcm_status() is called only internally, so it's no big problem to > change the arguments. Did you mean something like this, only read the relevant field from the larger structure? if (copy_from_user(&audio_tstamp_data, _status.audio_tstamp_data, sizeof(int))) return -EFAULT; res = snd_pcm_status(substream, &status, audio_tstamp_data);
At Wed, 10 Dec 2014 11:35:42 -0600, Pierre-Louis Bossart wrote: > > On 12/10/14, 11:28 AM, Takashi Iwai wrote: > > At Mon, 8 Dec 2014 16:23:43 -0600, > > Pierre-Louis Bossart wrote: > >> > >> Let userspace select audio timestamp config, ignore and zero all > >> other fields > >> > >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > >> --- > >> sound/core/pcm_native.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > >> index 37a7137..7dcd6bd 100644 > >> --- a/sound/core/pcm_native.c > >> +++ b/sound/core/pcm_native.c > >> @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, > >> struct snd_pcm_runtime *runtime = substream->runtime; > >> > >> snd_pcm_stream_lock_irq(substream); > >> + memset(status, 0, sizeof(*status)); > > > > This memset() can be outside the lock. > > > >> status->state = runtime->status->state; > >> status->suspended_state = runtime->status->suspended_state; > >> if (status->state == SNDRV_PCM_STATE_OPEN) > >> @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream, > >> struct snd_pcm_status status; > >> int res; > >> > >> - memset(&status, 0, sizeof(status)); > >> + if (copy_from_user(&status, _status, sizeof(status))) > >> + return -EFAULT; > >> res = snd_pcm_status(substream, &status); > > > > You're clearing the data at the beginning of snd_pcm_status(), so it > > doesn't make sense to do copy_from_user(). > > > > In anyway, I prefer passing the extra argument for the parameter user > > may set instead of initializing the whole struct beforehand. > > snd_pcm_status() is called only internally, so it's no big problem to > > change the arguments. > > Did you mean something like this, only read the relevant field from the > larger structure? > > if (copy_from_user(&audio_tstamp_data, _status.audio_tstamp_data, > sizeof(int))) > return -EFAULT; > res = snd_pcm_status(substream, &status, audio_tstamp_data); Yes (although get_user() would be more convenient). Takashi
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 37a7137..7dcd6bd 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -706,6 +706,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_stream_lock_irq(substream); + memset(status, 0, sizeof(*status)); status->state = runtime->status->state; status->suspended_state = runtime->status->suspended_state; if (status->state == SNDRV_PCM_STATE_OPEN) @@ -757,7 +758,8 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream, struct snd_pcm_status status; int res; - memset(&status, 0, sizeof(status)); + if (copy_from_user(&status, _status, sizeof(status))) + return -EFAULT; res = snd_pcm_status(substream, &status); if (res < 0) return res;
Let userspace select audio timestamp config, ignore and zero all other fields Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/core/pcm_native.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)