[PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Hangyu Hua]

rpmsg_register_device_override need to call put_device to free vch when
driver_set_override fails.

Fix this by adding a put_device() to the error path.

Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

v2: change the commit log.

 drivers/rpmsg/rpmsg_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 290c1f02da10..5a47cad89fdc 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -618,6 +618,7 @@ int rpmsg_register_device_override(struct rpmsg_device *rpdev,
 					  strlen(driver_override));
 		if (ret) {
 			dev_err(dev, "device_set_override failed: %d\n", ret);
+			put_device(dev);
 			return ret;
 		}
 	}
-- 
2.25.1

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Mathieu Poirier]

On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
> rpmsg_register_device_override need to call put_device to free vch when
> driver_set_override fails.
> 
> Fix this by adding a put_device() to the error path.
> 
> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")

This is funny... Neither Bjorn nor I have reviewed this patch...

> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> 
> v2: change the commit log.
> 
>  drivers/rpmsg/rpmsg_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 290c1f02da10..5a47cad89fdc 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -618,6 +618,7 @@ int rpmsg_register_device_override(struct rpmsg_device *rpdev,
>  					  strlen(driver_override));
>  		if (ret) {
>  			dev_err(dev, "device_set_override failed: %d\n", ret);
> +			put_device(dev);

Applied.

Thanks,
Mathieu

>  			return ret;
>  		}
>  	}
> -- 
> 2.25.1
> 

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Krzysztof Kozlowski]

On 24/06/2022 19:36, Mathieu Poirier wrote:
> On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
>> rpmsg_register_device_override need to call put_device to free vch when
>> driver_set_override fails.
>>
>> Fix this by adding a put_device() to the error path.
>>
>> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
> 
> This is funny... Neither Bjorn nor I have reviewed this patch...

It was a fix for commit in Greg's tree and Greg's pick it up after a
week or something. I am not sure if that's actually funny that Greg has
to pick it up without review :(

Best regards,
Krzysztof

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Mathieu Poirier]

On Fri, 24 Jun 2022 at 11:45, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/06/2022 19:36, Mathieu Poirier wrote:
> > On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
> >> rpmsg_register_device_override need to call put_device to free vch when
> >> driver_set_override fails.
> >>
> >> Fix this by adding a put_device() to the error path.
> >>
> >> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
> >
> > This is funny... Neither Bjorn nor I have reviewed this patch...
>
> It was a fix for commit in Greg's tree and Greg's pick it up after a
> week or something. I am not sure if that's actually funny that Greg has
> to pick it up without review :(
>

The patch was sent out on April 19th and committed 3 days later on
April 22nd.  Is 3 days the new patch review time standard?

> Best regards,
> Krzysztof

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Krzysztof Kozlowski]

On 24/06/2022 20:43, Mathieu Poirier wrote:
> On Fri, 24 Jun 2022 at 11:45, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 24/06/2022 19:36, Mathieu Poirier wrote:
>>> On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
>>>> rpmsg_register_device_override need to call put_device to free vch when
>>>> driver_set_override fails.
>>>>
>>>> Fix this by adding a put_device() to the error path.
>>>>
>>>> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
>>>
>>> This is funny... Neither Bjorn nor I have reviewed this patch...
>>
>> It was a fix for commit in Greg's tree and Greg's pick it up after a
>> week or something. I am not sure if that's actually funny that Greg has
>> to pick it up without review :(
>>
> 
> The patch was sent out on April 19th and committed 3 days later on
> April 22nd.  Is 3 days the new patch review time standard?

Neither 19th, nor 22nd are correct. The patch which you set you never
reviewed, so commit bb17d110cbf2 was sent on 29th of April:
https://lore.kernel.org/all/20220429195946.1061725-1-krzysztof.kozlowski@linaro.org/

And committed on 6 of May, which gives some time for review. Where did
you see the other dates?


Best regards,
Krzysztof

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Krzysztof Kozlowski]

On 25/06/2022 21:40, Krzysztof Kozlowski wrote:
> On 24/06/2022 20:43, Mathieu Poirier wrote:
>> On Fri, 24 Jun 2022 at 11:45, Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 24/06/2022 19:36, Mathieu Poirier wrote:
>>>> On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
>>>>> rpmsg_register_device_override need to call put_device to free vch when
>>>>> driver_set_override fails.
>>>>>
>>>>> Fix this by adding a put_device() to the error path.
>>>>>
>>>>> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
>>>>
>>>> This is funny... Neither Bjorn nor I have reviewed this patch...
>>>
>>> It was a fix for commit in Greg's tree and Greg's pick it up after a
>>> week or something. I am not sure if that's actually funny that Greg has
>>> to pick it up without review :(
>>>
>>
>> The patch was sent out on April 19th and committed 3 days later on
>> April 22nd.  Is 3 days the new patch review time standard?
> 
> Neither 19th, nor 22nd are correct. The patch which you set you never
> reviewed, so commit bb17d110cbf2 was sent on 29th of April:
> https://lore.kernel.org/all/20220429195946.1061725-1-krzysztof.kozlowski@linaro.org/
> 
> And committed on 6 of May, which gives some time for review. Where did
> you see the other dates?


Maybe you refer to much earlier commit, not mentioned here which was on
the list since February and committed indeed around April 22, so it gave
around 2 months of time for review. In the meantime the patch was
changing although since v6 (beginning of April [1]) it was almost
untouched, except one string change.

Therefore this original set, not one being mentioned here bb17d110cbf2,
was quite available. And you were Cc-ed since beginning - the version 2
this year (February, [2]). Two months is usually quite enough for
review, especially for not so big diff.

[1]
https://lore.kernel.org/all/20220403183758.192236-1-krzysztof.kozlowski@linaro.org/

[2]
https://lore.kernel.org/all/20220223191310.347669-1-krzysztof.kozlowski@canonical.com/


Best regards,
Krzysztof

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Mathieu Poirier]

On Sat, Jun 25, 2022 at 09:40:36PM +0200, Krzysztof Kozlowski wrote:
> On 24/06/2022 20:43, Mathieu Poirier wrote:
> > On Fri, 24 Jun 2022 at 11:45, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 24/06/2022 19:36, Mathieu Poirier wrote:
> >>> On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
> >>>> rpmsg_register_device_override need to call put_device to free vch when
> >>>> driver_set_override fails.
> >>>>
> >>>> Fix this by adding a put_device() to the error path.
> >>>>
> >>>> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
> >>>
> >>> This is funny... Neither Bjorn nor I have reviewed this patch...
> >>
> >> It was a fix for commit in Greg's tree and Greg's pick it up after a
> >> week or something. I am not sure if that's actually funny that Greg has
> >> to pick it up without review :(
> >>
> > 
> > The patch was sent out on April 19th and committed 3 days later on
> > April 22nd.  Is 3 days the new patch review time standard?
> 
> Neither 19th, nor 22nd are correct. The patch which you set you never
> reviewed, so commit bb17d110cbf2 was sent on 29th of April:
> https://lore.kernel.org/all/20220429195946.1061725-1-krzysztof.kozlowski@linaro.org/
>

Twitchy fingers... Those dates are for commit 42cd402b8fd4, which is referenced
by bb17d110cbf2.

The end result is the same, that is patches related to the remoteproc/rpmsg
subsystems (or any subsystem) should not be committed before their maintainers
have the opportunity to review them.

> And committed on 6 of May, which gives some time for review. Where did
> you see the other dates?
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2] rpmsg: fix possible refcount leak in rpmsg_register_device_override()

[Krzysztof Kozlowski]

On 28/06/2022 18:31, Mathieu Poirier wrote:
> On Sat, Jun 25, 2022 at 09:40:36PM +0200, Krzysztof Kozlowski wrote:
>> On 24/06/2022 20:43, Mathieu Poirier wrote:
>>> On Fri, 24 Jun 2022 at 11:45, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 24/06/2022 19:36, Mathieu Poirier wrote:
>>>>> On Fri, Jun 24, 2022 at 10:41:20AM +0800, Hangyu Hua wrote:
>>>>>> rpmsg_register_device_override need to call put_device to free vch when
>>>>>> driver_set_override fails.
>>>>>>
>>>>>> Fix this by adding a put_device() to the error path.
>>>>>>
>>>>>> Fixes: bb17d110cbf2 ("rpmsg: Fix calling device_lock() on non-initialized device")
>>>>>
>>>>> This is funny... Neither Bjorn nor I have reviewed this patch...
>>>>
>>>> It was a fix for commit in Greg's tree and Greg's pick it up after a
>>>> week or something. I am not sure if that's actually funny that Greg has
>>>> to pick it up without review :(
>>>>
>>>
>>> The patch was sent out on April 19th and committed 3 days later on
>>> April 22nd.  Is 3 days the new patch review time standard?
>>
>> Neither 19th, nor 22nd are correct. The patch which you set you never
>> reviewed, so commit bb17d110cbf2 was sent on 29th of April:
>> https://lore.kernel.org/all/20220429195946.1061725-1-krzysztof.kozlowski@linaro.org/
>>
> 
> Twitchy fingers... Those dates are for commit 42cd402b8fd4, which is referenced
> by bb17d110cbf2.
> 
> The end result is the same, that is patches related to the remoteproc/rpmsg
> subsystems (or any subsystem) should not be committed before their maintainers
> have the opportunity to review them.

I think two months for your involvement was enough. During this two
months there was a comment from Bjorn, applied and later quite plenty of
time to revise/check/review.

I understand that we are all busy, pretty often working on upstream in
spare time etc. So no one here complained that you did not review this
patch within two months. But I don't understand what shall we do more if
two months are not enough - wait four months? Six months?

Best regards,
Krzysztof