[PATCH 5.10] media: ti: cal: revert "media: ti: cal: fix possible memory leak in cal_ctx_create()"

[PATCH 5.10] media: ti: cal: revert "media: ti: cal: fix possible memory leak in cal_ctx_create()"

[Gaosheng Cui]

This reverts commit c7a218cbf67fffcd99b76ae3b5e9c2e8bef17c8c.

The memory of ctx is allocated by devm_kzalloc in cal_ctx_create,
it should not be freed by kfree when cal_ctx_v4l2_init() fails,
otherwise kfree() will cause double free, so revert this patch.

Fixes: c7a218cbf67f ("media: ti: cal: fix possible memory leak in cal_ctx_create()")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
 drivers/media/platform/ti-vpe/cal.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 93121c90d76a..2eef245c31a1 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -624,10 +624,8 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
 	ctx->cport = inst;
 
 	ret = cal_ctx_v4l2_init(ctx);
-	if (ret) {
-		kfree(ctx);
+	if (ret)
 		return NULL;
-	}
 
 	return ctx;
 }
-- 
2.25.1

Re: [PATCH 5.10] media: ti: cal: revert "media: ti: cal: fix possible memory leak in cal_ctx_create()"

[Greg KH]

On Mon, Apr 03, 2023 at 08:17:04PM +0800, Gaosheng Cui wrote:
> This reverts commit c7a218cbf67fffcd99b76ae3b5e9c2e8bef17c8c.
> 
> The memory of ctx is allocated by devm_kzalloc in cal_ctx_create,
> it should not be freed by kfree when cal_ctx_v4l2_init() fails,
> otherwise kfree() will cause double free, so revert this patch.
> 
> Fixes: c7a218cbf67f ("media: ti: cal: fix possible memory leak in cal_ctx_create()")
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>  drivers/media/platform/ti-vpe/cal.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 93121c90d76a..2eef245c31a1 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -624,10 +624,8 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
>  	ctx->cport = inst;
>  
>  	ret = cal_ctx_v4l2_init(ctx);
> -	if (ret) {
> -		kfree(ctx);
> +	if (ret)
>  		return NULL;
> -	}
>  
>  	return ctx;
>  }
> -- 
> 2.25.1
> 

Why is this not needed to be reverted in Linus's tree first?

thanks,

greg k-h

Re: [PATCH 5.10] media: ti: cal: revert "media: ti: cal: fix possible memory leak in cal_ctx_create()"

[Greg KH]

On Mon, Apr 03, 2023 at 09:11:43PM +0800, cuigaosheng wrote:
> On 2023/4/3 20:53, Greg KH wrote:
> > On Mon, Apr 03, 2023 at 08:17:04PM +0800, Gaosheng Cui wrote:
> > > This reverts commit c7a218cbf67fffcd99b76ae3b5e9c2e8bef17c8c.
> > > 
> > > The memory of ctx is allocated by devm_kzalloc in cal_ctx_create,
> > > it should not be freed by kfree when cal_ctx_v4l2_init() fails,
> > > otherwise kfree() will cause double free, so revert this patch.
> > > 
> > > Fixes: c7a218cbf67f ("media: ti: cal: fix possible memory leak in cal_ctx_create()")
> > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> > > ---
> > >   drivers/media/platform/ti-vpe/cal.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > > index 93121c90d76a..2eef245c31a1 100644
> > > --- a/drivers/media/platform/ti-vpe/cal.c
> > > +++ b/drivers/media/platform/ti-vpe/cal.c
> > > @@ -624,10 +624,8 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
> > >   	ctx->cport = inst;
> > >   	ret = cal_ctx_v4l2_init(ctx);
> > > -	if (ret) {
> > > -		kfree(ctx);
> > > +	if (ret)
> > >   		return NULL;
> > > -	}
> > >   	return ctx;
> > >   }
> > > -- 
> > > 2.25.1
> > > 
> > Why is this not needed to be reverted in Linus's tree first?
> > 
> > thanks,
> > 
> > greg k-h
> > .
> 
> Thanks for taking time to review this patch.
> 
> The memory of ctx is allocated by kzalloc since commit 9e67f24e4d90
> ("media: ti-vpe: cal: fix ctx uninitialization"), so the fixes tag
> of patch c7a218cbf67fis not entirely accurate, mainline should merge this
> patch, but it should not be merged into 5.10, my apologies  for
> notdiscovering this bug earlier. Gaosheng.
> 

Great, can you please put all of this information in the changelog
explaining why this is only needed for this one branch and resend it?

thanks,

greg k-h

Re: [PATCH 5.10] media: ti: cal: revert "media: ti: cal: fix possible memory leak in cal_ctx_create()"

[cuigaosheng]

On 2023/4/3 21:35, Greg KH wrote:
> On Mon, Apr 03, 2023 at 09:11:43PM +0800, cuigaosheng wrote:
>> On 2023/4/3 20:53, Greg KH wrote:
>>> On Mon, Apr 03, 2023 at 08:17:04PM +0800, Gaosheng Cui wrote:
>>>> This reverts commit c7a218cbf67fffcd99b76ae3b5e9c2e8bef17c8c.
>>>>
>>>> The memory of ctx is allocated by devm_kzalloc in cal_ctx_create,
>>>> it should not be freed by kfree when cal_ctx_v4l2_init() fails,
>>>> otherwise kfree() will cause double free, so revert this patch.
>>>>
>>>> Fixes: c7a218cbf67f ("media: ti: cal: fix possible memory leak in cal_ctx_create()")
>>>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
>>>> ---
>>>>    drivers/media/platform/ti-vpe/cal.c | 4 +---
>>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
>>>> index 93121c90d76a..2eef245c31a1 100644
>>>> --- a/drivers/media/platform/ti-vpe/cal.c
>>>> +++ b/drivers/media/platform/ti-vpe/cal.c
>>>> @@ -624,10 +624,8 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
>>>>    	ctx->cport = inst;
>>>>    	ret = cal_ctx_v4l2_init(ctx);
>>>> -	if (ret) {
>>>> -		kfree(ctx);
>>>> +	if (ret)
>>>>    		return NULL;
>>>> -	}
>>>>    	return ctx;
>>>>    }
>>>> -- 
>>>> 2.25.1
>>>>
>>> Why is this not needed to be reverted in Linus's tree first?
>>>
>>> thanks,
>>>
>>> greg k-h
>>> .
>> Thanks for taking time to review this patch.
>>
>> The memory of ctx is allocated by kzalloc since commit 9e67f24e4d90
>> ("media: ti-vpe: cal: fix ctx uninitialization"), so the fixes tag
>> of patch c7a218cbf67fis not entirely accurate, mainline should merge this
>> patch, but it should not be merged into 5.10, my apologies  for
>> notdiscovering this bug earlier. Gaosheng.
>>
> Great, can you please put all of this information in the changelog
> explaining why this is only needed for this one branch and resend it?
>
> thanks,
>
> greg k-h
> .

I have updated the commit message and resend it, thanks!

Gaosheng.