linux-kernel - Re: [PATCH v2 03/24] media: iris: Fix memory leak by freeing untracked persist buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7c67628-072d-8e31-5f5f-2b322f40dbd4@quicinc.com>
Date: Mon, 18 Aug 2025 08:40:54 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Vikash Garodia
	<quic_vgarodia@...cinc.com>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        "Mauro
 Carvalho Chehab" <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Stefan Schmidt <stefan.schmidt@...aro.org>,
        Vedang Nagar
	<quic_vnagar@...cinc.com>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Renjiang Han <quic_renjiang@...cinc.com>,
        Wangao Wang <quic_wangaow@...cinc.com>
Subject: Re: [PATCH v2 03/24] media: iris: Fix memory leak by freeing
 untracked persist buffer



On 8/16/2025 3:45 PM, Bryan O'Donoghue wrote:
> On 13/08/2025 10:37, Dikshita Agarwal wrote:
>> One internal buffer which is allocated only once per session was not
>> being freed during session close because it was not being tracked as
>> part of internal buffer list which resulted in a memory leak.
>>
>> Add the necessary logic to explicitly free the untracked internal buffer
>> during session close to ensure all allocated memory is released
>> properly.
>>
>> Fixes: 73702f45db81 ("media: iris: allocate, initialize and queue
>> internal buffers")
>> Reviewed-by: Vikash Garodia <quic_vgarodia@...cinc.com>
>> Tested-by: Vikash Garodia <quic_vgarodia@...cinc.com> # X1E80100
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
>> ---
>>   drivers/media/platform/qcom/iris/iris_buffer.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_buffer.c
>> b/drivers/media/platform/qcom/iris/iris_buffer.c
>> index
>> 6425e4919e3b0b849ba801ca9e01921c114144cd..9f664c241149362d44d3a8fa65e2266f9c2e80e0 100644
>> --- a/drivers/media/platform/qcom/iris/iris_buffer.c
>> +++ b/drivers/media/platform/qcom/iris/iris_buffer.c
>> @@ -413,6 +413,16 @@ static int iris_destroy_internal_buffers(struct
>> iris_inst *inst, u32 plane, bool
>>           }
>>       }
>>   +    if (force) {
>> +        buffers = &inst->buffers[BUF_PERSIST];
>> +
>> +        list_for_each_entry_safe(buf, next, &buffers->list, list) {
>> +            ret = iris_destroy_internal_buffer(inst, buf);
>> +            if (ret)
>> +                return ret;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>  
> 
> Why is the logic here not to simply release every index of the enum
> iris_buffer_type ?

All buffers indicated with enum iris_buffer_type are not related to both
encoder and decoder, some are specific for encoder and some are for
decoder. Hence while freeing we read the internal buffer list maintained in
platform data to get the exact buffer list applicable.

> 
> If I'm reading the code right here, len indicates the list of linked lists
> to free, adding BUF_PERSIST appends to the list that may be freed if force
> is true but, then what about the remaining entries BUF_SCRATCH_1 ?
> 

the buffer list here is the list of buffers of a specific kind eg: BUF_PERSIST.

BUF_PERSIST is not part of list maintained in platform data because it has
a different lifecycle than other internal buffers. that's why this specific
routine is needed to free BUF_PERSIST when destroying all buffers.


> Is it valid to leave this routine with force = true but BUF_SCRATCH_1 not
> specifically indexed, if so why ?

Other internal buffers are freed in another routine in the same API which
is not part of this particular code snippet.

Thanks,
Dikshita

> 

> ---
> bod