linux-kernel - Re: [PATCH v3] coccinelle: api: semantic patch to use pm_runtime_resume_and

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: <CAJZ5v0ib-RegkeXf7G3mHjp2oRu7cSpCYBH_3BqGfKSe39fh4A@mail.gmail.com>
Date:   Tue, 27 Apr 2021 15:54:56 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Julia Lawall <Julia.Lawall@...ia.fr>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        kernel-janitors@...r.kernel.org,
        Gilles Muller <Gilles.Muller@...ia.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <michal.lkml@...kovi.net>, cocci@...teme.lip6.fr,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Johan Hovold <johan@...nel.org>,
        Zhang Qilong <zhangqilong3@...wei.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH v3] coccinelle: api: semantic patch to use pm_runtime_resume_and_get

On Tue, Apr 27, 2021 at 3:51 PM Julia Lawall <Julia.Lawall@...ia.fr> wrote:
>
> pm_runtime_get_sync keeps a reference count on failure, which can lead
> to leaks.  pm_runtime_resume_and_get drops the reference count in the
> failure case.  This rule very conservatively follows the definition of
> pm_runtime_resume_and_get to address the cases where the reference
> count is unlikely to be needed in the failure case.  Specifically, the
> change is only done when pm_runtime_get_sync is followed immediately
> by an if and when the branch of the if is immediately a call to
> pm_runtime_put_noidle (like in the definition of
> pm_runtime_resume_and_get) or something that is likely a print
> statement followed by a pm_runtime_resume_and_get call.  The patch
> case appears somewhat more complicated, because it also deals with the
> cases where {}s need to be removed.
>
> pm_runtime_resume_and_get was introduced in
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
> deal with usage counter")
>
> Signed-off-by: Julia Lawall <Julia.Lawall@...ia.fr>

This patch looks good to me, so

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

>
> ---
> v3: add the people who signed off on commit dd8088d5a896, expand the log message
> v2: better keyword
>
>  scripts/coccinelle/api/pm_runtime_resume_and_get.cocci |  153 +++++++++++++++++
>  1 file changed, 153 insertions(+)
>
> diff --git a/scripts/coccinelle/api/pm_runtime_resume_and_get.cocci b/scripts/coccinelle/api/pm_runtime_resume_and_get.cocci
> new file mode 100644
> index 000000000000..3387cb606f9b
> --- /dev/null
> +++ b/scripts/coccinelle/api/pm_runtime_resume_and_get.cocci
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Use pm_runtime_resume_and_get.
> +/// pm_runtime_get_sync keeps a reference count on failure,
> +/// which can lead to leaks.  pm_runtime_resume_and_get
> +/// drops the reference count in the failure case.
> +/// This rule addresses the cases where the reference count
> +/// is unlikely to be needed in the failure case.
> +///
> +// Confidence: High
> +// Copyright: (C) 2021 Julia Lawall, Inria
> +// URL: https://coccinelle.gitlabpages.inria.fr/website
> +// Options: --include-headers --no-includes
> +// Keywords: pm_runtime_get_sync
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +@r0 depends on patch && !context && !org && !report@
> +expression ret,e;
> +@@
> +
> +-     ret = pm_runtime_get_sync(e);
> ++     ret = pm_runtime_resume_and_get(e);
> +-     if (ret < 0)
> +-             pm_runtime_put_noidle(e);
> +
> +@r1 depends on patch && !context && !org && !report@
> +expression ret,e;
> +statement S1,S2;
> +@@
> +
> +-     ret = pm_runtime_get_sync(e);
> ++     ret = pm_runtime_resume_and_get(e);
> +      if (ret < 0)
> +-     {
> +-             pm_runtime_put_noidle(e);
> +             S1
> +-     }
> +      else S2
> +
> +@r2 depends on patch && !context && !org && !report@
> +expression ret,e;
> +statement S;
> +@@
> +
> +-     ret = pm_runtime_get_sync(e);
> ++     ret = pm_runtime_resume_and_get(e);
> +      if (ret < 0) {
> +-             pm_runtime_put_noidle(e);
> +             ...
> +      } else S
> +
> +@r3 depends on patch && !context && !org && !report@
> +expression ret,e;
> +identifier f;
> +constant char[] c;
> +statement S;
> +@@
> +
> +-     ret = pm_runtime_get_sync(e);
> ++     ret = pm_runtime_resume_and_get(e);
> +      if (ret < 0)
> +-     {
> +              f(...,c,...);
> +-             pm_runtime_put_noidle(e);
> +-     }
> +      else S
> +
> +@r4 depends on patch && !context && !org && !report@
> +expression ret,e;
> +identifier f;
> +constant char[] c;
> +statement S;
> +@@
> +
> +-     ret = pm_runtime_get_sync(e);
> ++     ret = pm_runtime_resume_and_get(e);
> +      if (ret < 0) {
> +              f(...,c,...);
> +-             pm_runtime_put_noidle(e);
> +             ...
> +      } else S
> +
> +// ----------------------------------------------------------------------------
> +
> +@...context depends on !patch && (context || org || report)@
> +statement S;
> +expression e, ret;
> +position j0, j1;
> +@@
> +
> +*     ret@j0 = pm_runtime_get_sync(e);
> +      if (ret < 0) {
> +*             pm_runtime_put_noidle@j1(e);
> +             ...
> +      } else S
> +
> +@...context depends on !patch && (context || org || report)@
> +identifier f;
> +statement S;
> +constant char []c;
> +expression e, ret;
> +position j0, j1;
> +@@
> +
> +*     ret@j0 = pm_runtime_get_sync(e);
> +      if (ret < 0) {
> +              f(...,c,...);
> +*             pm_runtime_put_noidle@j1(e);
> +             ...
> +      } else S
> +
> +// ----------------------------------------------------------------------------
> +
> +@...ipt:python r2_org depends on org@
> +j0 << r2_context.j0;
> +j1 << r2_context.j1;
> +@@
> +
> +msg = "WARNING: opportunity for pm_runtime_get_sync"
> +coccilib.org.print_todo(j0[0], msg)
> +coccilib.org.print_link(j1[0], "")
> +
> +@...ipt:python r3_org depends on org@
> +j0 << r3_context.j0;
> +j1 << r3_context.j1;
> +@@
> +
> +msg = "WARNING: opportunity for pm_runtime_get_sync"
> +coccilib.org.print_todo(j0[0], msg)
> +coccilib.org.print_link(j1[0], "")
> +
> +// ----------------------------------------------------------------------------
> +
> +@...ipt:python r2_report depends on report@
> +j0 << r2_context.j0;
> +j1 << r2_context.j1;
> +@@
> +
> +msg = "WARNING: opportunity for pm_runtime_get_sync on line %s." % (j0[0].line)
> +coccilib.report.print_report(j0[0], msg)
> +
> +@...ipt:python r3_report depends on report@
> +j0 << r3_context.j0;
> +j1 << r3_context.j1;
> +@@
> +
> +msg = "WARNING: opportunity for pm_runtime_get_sync on %s." % (j0[0].line)
> +coccilib.report.print_report(j0[0], msg)
> +
>