| Message ID | 1560843846-4631-1-git-send-email-bgoswami@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: compress: avoid integer overflow for long duration offload playback | expand |
On 6/18/19 9:44 AM, bgoswami@codeaurora.org wrote: > From: Banajit Goswami <bgoswami@codeaurora.org> > > Currently a 32 bit variable is used for storing number of bytes > copied to DSP, which can overflow when playback continues for a long > duration. > Change data type for this variable to __u64 to prevent overflow. This clearly looks like a bug, the number of bytes transferred is stored as u64 in the runtime. I have no memories of this being intentional. That said, it seems odd to me that you have an overflow on the number of bytes but not on the number of PCM frames. Shouldn't both the pcm_frames and pcm_io_frames fields also be changed to u64 while we are at it? And the second issue is that this may impact apps. This is a ABI change, isn't it? > > Signed-off-by: Dhananjay Kumar <dhakumar@codeaurora.org> > Signed-off-by: Banajit Goswami <bgoswami@codeaurora.org> > --- > include/uapi/sound/compress_offload.h | 2 +- > sound/core/compress_offload.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h > index 56d9567..db5edf3 100644 > --- a/include/uapi/sound/compress_offload.h > +++ b/include/uapi/sound/compress_offload.h > @@ -67,7 +67,7 @@ struct snd_compr_params { > */ > struct snd_compr_tstamp { > __u32 byte_offset; > - __u32 copied_total; > + __u64 copied_total; > __u32 pcm_frames; > __u32 pcm_io_frames; > __u32 sampling_rate; > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > index a1a6fd7..2d0a709 100644 > --- a/sound/core/compress_offload.c > +++ b/sound/core/compress_offload.c > @@ -184,7 +184,7 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, > if (!stream->ops->pointer) > return -ENOTSUPP; > stream->ops->pointer(stream, tstamp); > - pr_debug("dsp consumed till %d total %d bytes\n", > + pr_debug("dsp consumed till %d total %llu bytes\n", > tstamp->byte_offset, tstamp->copied_total); > if (stream->direction == SND_COMPRESS_PLAYBACK) > stream->runtime->total_bytes_transferred = tstamp->copied_total; >
On Tue, 18 Jun 2019 10:02:59 +0200, Pierre-Louis Bossart wrote: > > On 6/18/19 9:44 AM, bgoswami@codeaurora.org wrote: > > From: Banajit Goswami <bgoswami@codeaurora.org> > > > > Currently a 32 bit variable is used for storing number of bytes > > copied to DSP, which can overflow when playback continues for a long > > duration. > > Change data type for this variable to __u64 to prevent overflow. > > This clearly looks like a bug, the number of bytes transferred is > stored as u64 in the runtime. I have no memories of this being > intentional. > > That said, it seems odd to me that you have an overflow on the number > of bytes but not on the number of PCM frames. Shouldn't both the > pcm_frames and pcm_io_frames fields also be changed to u64 while we > are at it? > > And the second issue is that this may impact apps. This is a ABI > change, isn't it? Yes, it breaks ABI, and we can't take this as is. If we need 64bit values, a new ioctl must be provided while keeping the old one still working. thanks, Takashi > > > > Signed-off-by: Dhananjay Kumar <dhakumar@codeaurora.org> > > Signed-off-by: Banajit Goswami <bgoswami@codeaurora.org> > > --- > > include/uapi/sound/compress_offload.h | 2 +- > > sound/core/compress_offload.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h > > index 56d9567..db5edf3 100644 > > --- a/include/uapi/sound/compress_offload.h > > +++ b/include/uapi/sound/compress_offload.h > > @@ -67,7 +67,7 @@ struct snd_compr_params { > > */ > > struct snd_compr_tstamp { > > __u32 byte_offset; > > - __u32 copied_total; > > + __u64 copied_total; > > __u32 pcm_frames; > > __u32 pcm_io_frames; > > __u32 sampling_rate; > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > > index a1a6fd7..2d0a709 100644 > > --- a/sound/core/compress_offload.c > > +++ b/sound/core/compress_offload.c > > @@ -184,7 +184,7 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, > > if (!stream->ops->pointer) > > return -ENOTSUPP; > > stream->ops->pointer(stream, tstamp); > > - pr_debug("dsp consumed till %d total %d bytes\n", > > + pr_debug("dsp consumed till %d total %llu bytes\n", > > tstamp->byte_offset, tstamp->copied_total); > > if (stream->direction == SND_COMPRESS_PLAYBACK) > > stream->runtime->total_bytes_transferred = tstamp->copied_total; > > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index 56d9567..db5edf3 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -67,7 +67,7 @@ struct snd_compr_params { */ struct snd_compr_tstamp { __u32 byte_offset; - __u32 copied_total; + __u64 copied_total; __u32 pcm_frames; __u32 pcm_io_frames; __u32 sampling_rate; diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index a1a6fd7..2d0a709 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -184,7 +184,7 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream, if (!stream->ops->pointer) return -ENOTSUPP; stream->ops->pointer(stream, tstamp); - pr_debug("dsp consumed till %d total %d bytes\n", + pr_debug("dsp consumed till %d total %llu bytes\n", tstamp->byte_offset, tstamp->copied_total); if (stream->direction == SND_COMPRESS_PLAYBACK) stream->runtime->total_bytes_transferred = tstamp->copied_total;