* [PATCH] intel_th: avoid double free in error flow
@ 2019-11-19 17:34 Wen Yang
2019-11-20 13:06 ` Alexander Shishkin
0 siblings, 1 reply; 5+ messages in thread
From: Wen Yang @ 2019-11-19 17:34 UTC (permalink / raw)
To: Alexander Shishkin
Cc: zhiche.yy, xlpang, Wen Yang, Greg Kroah-Hartman, linux-kernel
There is a possible double free issue in intel_th_subdevice_alloc:
651 err = intel_th_device_add_resources(thdev, res, subdev->nres);
652 if (err) {
653 put_device(&thdev->dev);
654 goto fail_put_device; ---> freed
655 }
...
687 fail_put_device:
688 put_device(&thdev->dev); ---> double freed
689
This patch fix it by removing the unnecessary put_device().
Fixes: a753bfcfdb1f ("intel_th: Make the switch allocate its subdevices")
Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
drivers/hwtracing/intel_th/core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
index d5c1821..98d195c 100644
--- a/drivers/hwtracing/intel_th/core.c
+++ b/drivers/hwtracing/intel_th/core.c
@@ -649,10 +649,8 @@ static inline void intel_th_request_hub_module_flush(struct intel_th *th)
}
err = intel_th_device_add_resources(thdev, res, subdev->nres);
- if (err) {
- put_device(&thdev->dev);
+ if (err)
goto fail_put_device;
- }
if (subdev->type == INTEL_TH_OUTPUT) {
if (subdev->mknode)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] intel_th: avoid double free in error flow
2019-11-19 17:34 [PATCH] intel_th: avoid double free in error flow Wen Yang
@ 2019-11-20 13:06 ` Alexander Shishkin
2019-11-20 13:31 ` Wen Yang
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Shishkin @ 2019-11-20 13:06 UTC (permalink / raw)
To: Wen Yang
Cc: zhiche.yy, xlpang, Wen Yang, Greg Kroah-Hartman, linux-kernel,
alexander.shishkin
Wen Yang <wenyang@linux.alibaba.com> writes:
> There is a possible double free issue in intel_th_subdevice_alloc:
>
> 651 err = intel_th_device_add_resources(thdev, res, subdev->nres);
> 652 if (err) {
> 653 put_device(&thdev->dev);
> 654 goto fail_put_device; ---> freed
> 655 }
> ...
> 687 fail_put_device:
> 688 put_device(&thdev->dev); ---> double freed
> 689
>
> This patch fix it by removing the unnecessary put_device().
Unnecessary is a too generous term here.
> Fixes: a753bfcfdb1f ("intel_th: Make the switch allocate its subdevices")
> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org
Cc: stable@ is missing.
> ---
> drivers/hwtracing/intel_th/core.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
> index d5c1821..98d195c 100644
> --- a/drivers/hwtracing/intel_th/core.c
> +++ b/drivers/hwtracing/intel_th/core.c
> @@ -649,10 +649,8 @@ static inline void intel_th_request_hub_module_flush(struct intel_th *th)
> }
>
> err = intel_th_device_add_resources(thdev, res, subdev->nres);
> - if (err) {
> - put_device(&thdev->dev);
> + if (err)
> goto fail_put_device;
> - }
What about the second instance of the same problem a few lines lower?
Thanks,
--
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] intel_th: avoid double free in error flow
2019-11-20 13:06 ` Alexander Shishkin
@ 2019-11-20 13:31 ` Wen Yang
2019-11-20 13:38 ` Alexander Shishkin
0 siblings, 1 reply; 5+ messages in thread
From: Wen Yang @ 2019-11-20 13:31 UTC (permalink / raw)
To: Alexander Shishkin; +Cc: zhiche.yy, xlpang, Greg Kroah-Hartman, linux-kernel
On 2019/11/20 9:06 下午, Alexander Shishkin wrote:
> Wen Yang <wenyang@linux.alibaba.com> writes:
>
>> There is a possible double free issue in intel_th_subdevice_alloc:
>>
>> 651 err = intel_th_device_add_resources(thdev, res, subdev->nres);
>> 652 if (err) {
>> 653 put_device(&thdev->dev);
>> 654 goto fail_put_device; ---> freed
>> 655 }
>> ...
>> 687 fail_put_device:
>> 688 put_device(&thdev->dev); ---> double freed
>> 689
>>
>> This patch fix it by removing the unnecessary put_device().
> Unnecessary is a too generous term here.
>
>> Fixes: a753bfcfdb1f ("intel_th: Make the switch allocate its subdevices")
>> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-kernel@vger.kernel.org
> Cc: stable@ is missing.
>
>> ---
>> drivers/hwtracing/intel_th/core.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
>> index d5c1821..98d195c 100644
>> --- a/drivers/hwtracing/intel_th/core.c
>> +++ b/drivers/hwtracing/intel_th/core.c
>> @@ -649,10 +649,8 @@ static inline void intel_th_request_hub_module_flush(struct intel_th *th)
>> }
>>
>> err = intel_th_device_add_resources(thdev, res, subdev->nres);
>> - if (err) {
>> - put_device(&thdev->dev);
>> + if (err)
>> goto fail_put_device;
>> - }
> What about the second instance of the same problem a few lines lower?
> Thanks,
> --
> Alex
Hi Alex,
Thank you for your comments.
Another example after a few lines lower:
err = device_add(&thdev->dev);
if (err) {
put_device(&thdev->dev);
goto fail_free_res;
}
device_add() has increased the reference count,
so when it returns an error, an additional call to put_device()
is needed here to reduce the reference count.
So the code in this place is correct.
--
Regards,
Wen
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] intel_th: avoid double free in error flow
2019-11-20 13:31 ` Wen Yang
@ 2019-11-20 13:38 ` Alexander Shishkin
2019-11-20 13:46 ` Wen Yang
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Shishkin @ 2019-11-20 13:38 UTC (permalink / raw)
To: Wen Yang
Cc: zhiche.yy, xlpang, Greg Kroah-Hartman, linux-kernel, alexander.shishkin
Wen Yang <wenyang@linux.alibaba.com> writes:
> Another example after a few lines lower:
>
> err = device_add(&thdev->dev);
>
> if (err) {
> put_device(&thdev->dev);
> goto fail_free_res;
>
> }
>
> device_add() has increased the reference count,
>
> so when it returns an error, an additional call to put_device()
>
> is needed here to reduce the reference count.
>
> So the code in this place is correct.
No, device_add() drops its own extra reference in case of error (as it
should), so in "if (err) ..." branch we still only have just one
reference before it goes free.
Regards,
--
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] intel_th: avoid double free in error flow
2019-11-20 13:38 ` Alexander Shishkin
@ 2019-11-20 13:46 ` Wen Yang
0 siblings, 0 replies; 5+ messages in thread
From: Wen Yang @ 2019-11-20 13:46 UTC (permalink / raw)
To: Alexander Shishkin; +Cc: zhiche.yy, xlpang, Greg Kroah-Hartman, linux-kernel
On 2019/11/20 9:38 下午, Alexander Shishkin wrote:
> Wen Yang <wenyang@linux.alibaba.com> writes:
>
>> Another example after a few lines lower:
>>
>> err = device_add(&thdev->dev);
>>
>> if (err) {
>> put_device(&thdev->dev);
>> goto fail_free_res;
>>
>> }
>>
>> device_add() has increased the reference count,
>>
>> so when it returns an error, an additional call to put_device()
>>
>> is needed here to reduce the reference count.
>>
>> So the code in this place is correct.
> No, device_add() drops its own extra reference in case of error (as it
> should), so in "if (err) ..." branch we still only have just one
> reference before it goes free.
>
> Regards,
> --
> Alex
Well, ok, you are right.
We just checked the code and device_add() does release the reference count.
Thank you.
--
Regards,
Wen
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-20 13:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 17:34 [PATCH] intel_th: avoid double free in error flow Wen Yang
2019-11-20 13:06 ` Alexander Shishkin
2019-11-20 13:31 ` Wen Yang
2019-11-20 13:38 ` Alexander Shishkin
2019-11-20 13:46 ` Wen Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).