[PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

[Lars-Peter Clausen]

On 12/17/20 12:06 PM, Takashi Iwai wrote:
> On Thu, 17 Dec 2020 11:59:23 +0100,
> Lars-Peter Clausen wrote:
>> On 12/17/20 10:55 AM, Takashi Iwai wrote:
>>> On Thu, 17 Dec 2020 10:43:45 +0100,
>>> Lars-Peter Clausen wrote:
>>>> On 12/17/20 5:15 PM, Robin Gong wrote:
>>>>> Since mmap for userspace is based on page alignment, add page alignment
>>>>> for iram alloc from pool, otherwise, some good data located in the same
>>>>> page of dmab->area maybe touched wrongly by userspace like pulseaudio.
>>>>>
>>>> I wonder, do we also have to align size to be a multiple of PAGE_SIZE
>>>> to avoid leaking unrelated data?
>>> Hm, a good question.  Basically the PCM buffer size itself shouldn't
>>> be influenced by that (i.e. no hw-constraint or such is needed), but
>>> the padding should be cleared indeed.  I somehow left those to the
>>> allocator side, but maybe it's safer to clear the whole buffer in
>>> sound/core/memalloc.c commonly.
>> What I meant was that most of the APIs that we use to allocate memory
>> work on a PAGE_SIZE granularity. I.e. if you request a buffer that
>> where the size is not a multiple of PAGE_SIZE internally they will
>> still allocate a buffer that is a multiple of PAGE_SIZE and mark the
>> unused bytes as reserved.
>>
>> But I believe that is not the case gen_pool_dma_alloc(). It will
>> happily allocate those extra bytes to some other allocation request.
>>
>> That we need to zero out the reserved bytes even for those other APIs
>> is a very good additional point!
>>
>> I looked at this a few years ago and I'm pretty sure that we cleared
>> out the allocated area, but I can't find that anymore in the current
>> code. Which is not so great I guess.
> IIRC, we used GFP_ZERO in the past for the normal page allocations,
> but this was dropped as it's no longer supported or so.
>
> Also, we clear out the PCM buffer in hw_params call, but this is for
> the requested size, not the actual allocated size, hence the padding
> bytes will remain uncleared.
Ah! That memset() in hw_params is new.
>
> So I believe it's safer to add an extra memset() like my test patch.

Yea, we definitely want that.

Do we care about leaking audio samples from a previous application. I.e. 
application 'A' allocates a buffer plays back some data and then closes 
the device again. Application 'B' then opens the same audio devices 
allocates a slightly smaller buffer, so that it still uses the same 
number of pages. The buffer from the previous allocation get reused, but 
the remainder of the last page wont get cleared in hw_params().