linux-kernel - Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic

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: <20220721164807.GA14440@quicinc.com>
Date:   Thu, 21 Jul 2022 09:48:07 -0700
From:   Guru Das Srinagesh <quic_gurus@...cinc.com>
To:     Rajendra Nayak <quic_rjendra@...cinc.com>
CC:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        "David Heidelberg" <david@...t.cz>,
        Robert Marko <robimarko@...il.com>,
        Elliot Berman <quic_eberman@...cinc.com>
Subject: Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic

On Jul 01 2022 16:51, Rajendra Nayak wrote:
> >>+
> >>+            if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
> >>+                wait_for_completion(wq);
> >>+                fill_wq_resume_args(smc, smc_call_ctx);
> >>+                wq = NULL;
> >>+                continue;
> >>+            } else {
> >>+                fill_wq_wake_ack_args(smc, smc_call_ctx);
> >>+                continue;
> >>+            }
> >>+        } else if ((long)res->a0 < 0) {
> >>+            /* Error, simply return to caller */
> >>+            break;
> 
> if my understanding above is correct, shouldn't we do a
> >>+            if (wq)
> >>+                scm_waitq_flag_handler(wq, flags);
> in the error case also?

Yes, you're right, since both error or success means that a request is
complete. We should act the same way for error as for success. Thanks for
catching this.

> Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?

Because that is not part of the protocol - calling scm_waitq_flag_handler(wq, flags)
would result in a completion being freed, meaning a sleeping call would be
woken up, which is not what we want. When a WAITQ_WAKE is received, the
action to be taken is to immediately respond with a wq_wake_ack() and nothing
else.