Messages in this thread | | | Subject | Re: [PATCH 009/190] Revert "media: s5p-mfc: Fix a reference count leak" | From | Krzysztof Kozlowski <> | Date | Fri, 23 Apr 2021 11:29:26 +0200 |
| |
On 23/04/2021 10:41, Julia Lawall wrote: > > > On Fri, 23 Apr 2021, Krzysztof Kozlowski wrote: > >> On 23/04/2021 10:10, Hans Verkuil wrote: >>> On 23/04/2021 10:07, Mauro Carvalho Chehab wrote: >>>> Em Fri, 23 Apr 2021 09:10:32 +0200 >>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu: >>>> >>>>> On Fri, Apr 23, 2021 at 09:04:27AM +0200, Krzysztof Kozlowski wrote: >>>>>> On 21/04/2021 14:58, Greg Kroah-Hartman wrote: >>>>>>> This reverts commit 78741ce98c2e36188e2343434406b0e0bc50b0e7. >>>>>>> >>>>>>> Commits from @umn.edu addresses have been found to be submitted in "bad >>>>>>> faith" to try to test the kernel community's ability to review "known >>>>>>> malicious" changes. The result of these submissions can be found in a >>>>>>> paper published at the 42nd IEEE Symposium on Security and Privacy >>>>>>> entitled, "Open Source Insecurity: Stealthily Introducing >>>>>>> Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University >>>>>>> of Minnesota) and Kangjie Lu (University of Minnesota). >>>>>>> >>>>>>> Because of this, all submissions from this group must be reverted from >>>>>>> the kernel tree and will need to be re-reviewed again to determine if >>>>>>> they actually are a valid fix. Until that work is complete, remove this >>>>>>> change to ensure that no problems are being introduced into the >>>>>>> codebase. >>>>>>> >>>>>>> Cc: Qiushi Wu <wu000273@umn.edu> >>>>>>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>>>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> >>>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>>>> --- >>>>>>> drivers/media/platform/s5p-mfc/s5p_mfc_pm.c | 4 +--- >>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>>> >>>>>> >>>>>> This looks like a good commit but should be done now in a different way >>>>>> - using pm_runtime_resume_and_get(). Therefore I am fine with revert >>>>>> and I can submit later better fix. >>>>> >>>>> Great, thanks for letting me know, I can have someone work on the >>>>> "better fix" at the same time. >>>> >>>> IMO, it is better to keep the fix. I mean, there's no reason to >>>> revert a fix that it is known to be good. >>>> >>>> The "better fix" patch can be produced anytime. A simple coccinelle >>>> ruleset can replace patterns like: >>>> >>>> ret = pm_runtime_get_sync(pm->device); >>>> if (ret < 0) { >>>> pm_runtime_put_noidle(pm->device); >>>> return ret; >>>> } >>>> >>>> and the broken pattern: >>>> >>>> ret = pm_runtime_get_sync(pm->device); >>>> if (ret < 0) >>>> return ret; >>>> >>>> to: >>>> >>>> ret = pm_runtime_resume_and_get(pm->device); >>>> if (ret < 0) >>>> return ret; >>> >>> That's my preference as well. >> >> It won't be that easy because sometimes the error handling is via goto >> (like in other patches here) but anyway I don't mind keeping the >> original commits. > > I tried the following semantic patch: > > @@ > expression ret,e; > @@ > > - ret = pm_runtime_get_sync(e); > + ret = pm_resume_and_get(e); > if (ret < 0) { > ... > ?- pm_runtime_put_noidle(e); > ... > return ret; > } > > It has the following features: > > * The ? means that if pm_runtime_put_noidle is absent, the transformation > will happen anyway. > > * The ... before the return means that the matching will jump over a goto. > > It makes a lot of changes (in a kernel I had handy from March). This is a > complicated API, however, and I don't know if there are any other issues > to take into account, especially in the case where the call to > pm_runtime_put_noidle is not present.
Thanks Julia, looks good.
Minor notice, the drivers could cleanup also with pm_runtime_put(). This would not be the best code, but still would work and should be correct (decrements runtime PM usage counter and tries to suspend the device if it is resumed/active).
Such pattern could be in entire probe like:
device_probe() { if (pm_runtime_get_sync()) return -EINVAL; // PM runtime usage counter inbalance on errors ...
// suspend device pm_runtime_put();
return 0; } I think this should still work fine with your pattern, so overall risk of errors from coccicheck is small.
Best regards, Krzysztof
| |