* [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change()
@ 2022-02-09 3:50 Jia-Ju Bai
2022-02-09 14:30 ` Heikki Krogerus
0 siblings, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2022-02-09 3:50 UTC (permalink / raw)
To: heikki.krogerus, Greg KH, kyletso, jackp, andy.shevchenko,
unixbhaskar, subbaram, mrana
Cc: linux-usb, linux-kernel
Hello,
My static analysis tool reports a possible deadlock in the ucsi driver
in Linux 5.16:
ucsi_pr_swap()
mutex_lock(&con->lock); --> Line 962 (Lock A)
wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
ucsi_handle_connector_change()
mutex_lock(&con->lock); --> Line 763 (Lock A)
complete(&con->complete); --> Line 782 (Wake X)
complete(&con->complete); --> Line 807 (Wake X)
When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock
A". If ucsi_handle_connector_change() is executed at this time, "Wake X"
cannot be performed to wake up "Wait X" in
ucsi_handle_connector_change(), because "Lock A" has been already held
by ucsi_handle_connector_change(), causing a possible deadlock.
I find that "Wait X" is performed with a timeout, to relieve the
possible deadlock; but I think this timeout can cause inefficient execution.
I am not quite sure whether this possible problem is real.
Any feedback would be appreciated, thanks :)
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change()
2022-02-09 3:50 [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change() Jia-Ju Bai
@ 2022-02-09 14:30 ` Heikki Krogerus
2022-02-10 2:24 ` Jia-Ju Bai
2022-03-25 20:39 ` Jack Pham
0 siblings, 2 replies; 6+ messages in thread
From: Heikki Krogerus @ 2022-02-09 14:30 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: Greg KH, kyletso, jackp, andy.shevchenko, unixbhaskar, subbaram,
mrana, linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]
On Wed, Feb 09, 2022 at 11:50:57AM +0800, Jia-Ju Bai wrote:
> Hello,
>
> My static analysis tool reports a possible deadlock in the ucsi driver in
> Linux 5.16:
>
> ucsi_pr_swap()
> mutex_lock(&con->lock); --> Line 962 (Lock A)
> wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
>
> ucsi_handle_connector_change()
> mutex_lock(&con->lock); --> Line 763 (Lock A)
> complete(&con->complete); --> Line 782 (Wake X)
> complete(&con->complete); --> Line 807 (Wake X)
>
> When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock A".
> If ucsi_handle_connector_change() is executed at this time, "Wake X" cannot
> be performed to wake up "Wait X" in ucsi_handle_connector_change(), because
> "Lock A" has been already held by ucsi_handle_connector_change(), causing a
> possible deadlock.
> I find that "Wait X" is performed with a timeout, to relieve the possible
> deadlock; but I think this timeout can cause inefficient execution.
>
> I am not quite sure whether this possible problem is real.
> Any feedback would be appreciated, thanks :)
This is probable a regression from commit ad74b8649bea ("usb: typec:
ucsi: Preliminary support for alternate modes"). Can you test does
this patch fix the issue (attached)?
thanks,
--
heikki
[-- Attachment #2: 0001-usb-typec-ucsi-Test-fix.patch --]
[-- Type: text/plain, Size: 1963 bytes --]
From 2ad06425a3df7be656f8a5b3c202aab45554fd17 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 9 Feb 2022 17:27:19 +0300
Subject: [PATCH] usb: typec: ucsi: Test fix
Interim.
Not-Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
drivers/usb/typec/ucsi/ucsi.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index f0c2fa19f3e0f..225104beda8be 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -956,14 +956,18 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
if (ret < 0)
goto out_unlock;
+ mutex_unlock(&con->lock);
+
if (!wait_for_completion_timeout(&con->complete,
msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS)))
- ret = -ETIMEDOUT;
+ return -ETIMEDOUT;
+
+ return 0;
out_unlock:
mutex_unlock(&con->lock);
- return ret < 0 ? ret : 0;
+ return ret;
}
static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
@@ -992,11 +996,13 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
if (ret < 0)
goto out_unlock;
+ mutex_unlock(&con->lock);
+
if (!wait_for_completion_timeout(&con->complete,
- msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) {
- ret = -ETIMEDOUT;
- goto out_unlock;
- }
+ msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS)))
+ return -ETIMEDOUT;
+
+ mutex_lock(&con->lock);
/* Something has gone wrong while swapping the role */
if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) !=
@@ -1372,6 +1378,7 @@ void ucsi_unregister(struct ucsi *ucsi)
ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
for (i = 0; i < ucsi->cap.num_connectors; i++) {
+ complete(&ucsi->connector[i].complete);
cancel_work_sync(&ucsi->connector[i].work);
ucsi_unregister_partner(&ucsi->connector[i]);
ucsi_unregister_altmodes(&ucsi->connector[i],
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change()
2022-02-09 14:30 ` Heikki Krogerus
@ 2022-02-10 2:24 ` Jia-Ju Bai
2022-02-10 14:30 ` Heikki Krogerus
2022-03-25 20:39 ` Jack Pham
1 sibling, 1 reply; 6+ messages in thread
From: Jia-Ju Bai @ 2022-02-10 2:24 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Greg KH, kyletso, jackp, andy.shevchenko, unixbhaskar, subbaram,
mrana, linux-usb, linux-kernel
On 2022/2/9 22:30, Heikki Krogerus wrote:
> On Wed, Feb 09, 2022 at 11:50:57AM +0800, Jia-Ju Bai wrote:
>> Hello,
>>
>> My static analysis tool reports a possible deadlock in the ucsi driver in
>> Linux 5.16:
>>
>> ucsi_pr_swap()
>> mutex_lock(&con->lock); --> Line 962 (Lock A)
>> wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
>>
>> ucsi_handle_connector_change()
>> mutex_lock(&con->lock); --> Line 763 (Lock A)
>> complete(&con->complete); --> Line 782 (Wake X)
>> complete(&con->complete); --> Line 807 (Wake X)
>>
>> When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock A".
>> If ucsi_handle_connector_change() is executed at this time, "Wake X" cannot
>> be performed to wake up "Wait X" in ucsi_handle_connector_change(), because
>> "Lock A" has been already held by ucsi_handle_connector_change(), causing a
>> possible deadlock.
>> I find that "Wait X" is performed with a timeout, to relieve the possible
>> deadlock; but I think this timeout can cause inefficient execution.
>>
>> I am not quite sure whether this possible problem is real.
>> Any feedback would be appreciated, thanks :)
> This is probable a regression from commit ad74b8649bea ("usb: typec:
> ucsi: Preliminary support for alternate modes"). Can you test does
> this patch fix the issue (attached)?
Hi Heikki,
Thanks for the reply and patch.
After the patch is used, my tool does not report this deadlock in the
ucsi driver.
Thus, I think this patch should be okay to fix the deadlock :)
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change()
2022-02-10 2:24 ` Jia-Ju Bai
@ 2022-02-10 14:30 ` Heikki Krogerus
0 siblings, 0 replies; 6+ messages in thread
From: Heikki Krogerus @ 2022-02-10 14:30 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: Greg KH, kyletso, jackp, andy.shevchenko, unixbhaskar, subbaram,
mrana, linux-usb, linux-kernel
On Thu, Feb 10, 2022 at 10:24:42AM +0800, Jia-Ju Bai wrote:
>
>
> On 2022/2/9 22:30, Heikki Krogerus wrote:
> > On Wed, Feb 09, 2022 at 11:50:57AM +0800, Jia-Ju Bai wrote:
> > > Hello,
> > >
> > > My static analysis tool reports a possible deadlock in the ucsi driver in
> > > Linux 5.16:
> > >
> > > ucsi_pr_swap()
> > > mutex_lock(&con->lock); --> Line 962 (Lock A)
> > > wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
> > >
> > > ucsi_handle_connector_change()
> > > mutex_lock(&con->lock); --> Line 763 (Lock A)
> > > complete(&con->complete); --> Line 782 (Wake X)
> > > complete(&con->complete); --> Line 807 (Wake X)
> > >
> > > When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock A".
> > > If ucsi_handle_connector_change() is executed at this time, "Wake X" cannot
> > > be performed to wake up "Wait X" in ucsi_handle_connector_change(), because
> > > "Lock A" has been already held by ucsi_handle_connector_change(), causing a
> > > possible deadlock.
> > > I find that "Wait X" is performed with a timeout, to relieve the possible
> > > deadlock; but I think this timeout can cause inefficient execution.
> > >
> > > I am not quite sure whether this possible problem is real.
> > > Any feedback would be appreciated, thanks :)
> > This is probable a regression from commit ad74b8649bea ("usb: typec:
> > ucsi: Preliminary support for alternate modes"). Can you test does
> > this patch fix the issue (attached)?
>
> Hi Heikki,
>
> Thanks for the reply and patch.
> After the patch is used, my tool does not report this deadlock in the ucsi
> driver.
> Thus, I think this patch should be okay to fix the deadlock :)
Thanks for testing it. I'll tag you as the reporter and tester in the
patch.
Br,
--
heikki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change()
2022-02-09 14:30 ` Heikki Krogerus
2022-02-10 2:24 ` Jia-Ju Bai
@ 2022-03-25 20:39 ` Jack Pham
2022-03-28 8:28 ` Heikki Krogerus
1 sibling, 1 reply; 6+ messages in thread
From: Jack Pham @ 2022-03-25 20:39 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Jia-Ju Bai, Greg KH, kyletso, andy.shevchenko, unixbhaskar,
subbaram, mrana, linux-usb, linux-kernel
Hi Heikki,
On Wed, Feb 09, 2022 at 04:30:31PM +0200, Heikki Krogerus wrote:
> On Wed, Feb 09, 2022 at 11:50:57AM +0800, Jia-Ju Bai wrote:
> > Hello,
> >
> > My static analysis tool reports a possible deadlock in the ucsi driver in
> > Linux 5.16:
> >
> > ucsi_pr_swap()
> > mutex_lock(&con->lock); --> Line 962 (Lock A)
> > wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
> >
> > ucsi_handle_connector_change()
> > mutex_lock(&con->lock); --> Line 763 (Lock A)
> > complete(&con->complete); --> Line 782 (Wake X)
> > complete(&con->complete); --> Line 807 (Wake X)
> >
> > When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock A".
> > If ucsi_handle_connector_change() is executed at this time, "Wake X" cannot
> > be performed to wake up "Wait X" in ucsi_handle_connector_change(), because
> > "Lock A" has been already held by ucsi_handle_connector_change(), causing a
> > possible deadlock.
> > I find that "Wait X" is performed with a timeout, to relieve the possible
> > deadlock; but I think this timeout can cause inefficient execution.
> >
> > I am not quite sure whether this possible problem is real.
> > Any feedback would be appreciated, thanks :)
>
> This is probable a regression from commit ad74b8649bea ("usb: typec:
> ucsi: Preliminary support for alternate modes"). Can you test does
> this patch fix the issue (attached)?
We encountered a slightly different twist to this bug. Instead of
deadlocking, we see that the dr_swap() / pr_swap() operations actually
jump out of the wait_for_completion_timeout() immediately, even before
any partner change occurs. This is because the con->complete may
already have its done flag set to true from the first time
ucsi_handle_connector_change() runs, and is never reset after that.
In addition to the unlocking below, I think we need to also add
reinit_completion() calls at the start of ucsi_{pr,dr}_swap().
Thanks,
Jack
> From 2ad06425a3df7be656f8a5b3c202aab45554fd17 Mon Sep 17 00:00:00 2001
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Date: Wed, 9 Feb 2022 17:27:19 +0300
> Subject: [PATCH] usb: typec: ucsi: Test fix
>
> Interim.
>
> Not-Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/ucsi/ucsi.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index f0c2fa19f3e0f..225104beda8be 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -956,14 +956,18 @@ static int ucsi_dr_swap(struct typec_port *port, enum typec_data_role role)
> if (ret < 0)
> goto out_unlock;
>
> + mutex_unlock(&con->lock);
> +
> if (!wait_for_completion_timeout(&con->complete,
> msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS)))
> - ret = -ETIMEDOUT;
> + return -ETIMEDOUT;
> +
> + return 0;
>
> out_unlock:
> mutex_unlock(&con->lock);
>
> - return ret < 0 ? ret : 0;
> + return ret;
> }
>
> static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
> @@ -992,11 +996,13 @@ static int ucsi_pr_swap(struct typec_port *port, enum typec_role role)
> if (ret < 0)
> goto out_unlock;
>
> + mutex_unlock(&con->lock);
> +
> if (!wait_for_completion_timeout(&con->complete,
> - msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS))) {
> - ret = -ETIMEDOUT;
> - goto out_unlock;
> - }
> + msecs_to_jiffies(UCSI_SWAP_TIMEOUT_MS)))
> + return -ETIMEDOUT;
> +
> + mutex_lock(&con->lock);
>
> /* Something has gone wrong while swapping the role */
> if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) !=
> @@ -1372,6 +1378,7 @@ void ucsi_unregister(struct ucsi *ucsi)
> ucsi->ops->async_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
>
> for (i = 0; i < ucsi->cap.num_connectors; i++) {
> + complete(&ucsi->connector[i].complete);
> cancel_work_sync(&ucsi->connector[i].work);
> ucsi_unregister_partner(&ucsi->connector[i]);
> ucsi_unregister_altmodes(&ucsi->connector[i],
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change()
2022-03-25 20:39 ` Jack Pham
@ 2022-03-28 8:28 ` Heikki Krogerus
0 siblings, 0 replies; 6+ messages in thread
From: Heikki Krogerus @ 2022-03-28 8:28 UTC (permalink / raw)
To: Jack Pham
Cc: Jia-Ju Bai, Greg KH, kyletso, andy.shevchenko, unixbhaskar,
subbaram, mrana, linux-usb, linux-kernel
On Fri, Mar 25, 2022 at 01:39:59PM -0700, Jack Pham wrote:
> Hi Heikki,
>
> On Wed, Feb 09, 2022 at 04:30:31PM +0200, Heikki Krogerus wrote:
> > On Wed, Feb 09, 2022 at 11:50:57AM +0800, Jia-Ju Bai wrote:
> > > Hello,
> > >
> > > My static analysis tool reports a possible deadlock in the ucsi driver in
> > > Linux 5.16:
> > >
> > > ucsi_pr_swap()
> > > mutex_lock(&con->lock); --> Line 962 (Lock A)
> > > wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
> > >
> > > ucsi_handle_connector_change()
> > > mutex_lock(&con->lock); --> Line 763 (Lock A)
> > > complete(&con->complete); --> Line 782 (Wake X)
> > > complete(&con->complete); --> Line 807 (Wake X)
> > >
> > > When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock A".
> > > If ucsi_handle_connector_change() is executed at this time, "Wake X" cannot
> > > be performed to wake up "Wait X" in ucsi_handle_connector_change(), because
> > > "Lock A" has been already held by ucsi_handle_connector_change(), causing a
> > > possible deadlock.
> > > I find that "Wait X" is performed with a timeout, to relieve the possible
> > > deadlock; but I think this timeout can cause inefficient execution.
> > >
> > > I am not quite sure whether this possible problem is real.
> > > Any feedback would be appreciated, thanks :)
> >
> > This is probable a regression from commit ad74b8649bea ("usb: typec:
> > ucsi: Preliminary support for alternate modes"). Can you test does
> > this patch fix the issue (attached)?
>
> We encountered a slightly different twist to this bug. Instead of
> deadlocking, we see that the dr_swap() / pr_swap() operations actually
> jump out of the wait_for_completion_timeout() immediately, even before
> any partner change occurs. This is because the con->complete may
> already have its done flag set to true from the first time
> ucsi_handle_connector_change() runs, and is never reset after that.
>
> In addition to the unlocking below, I think we need to also add
> reinit_completion() calls at the start of ucsi_{pr,dr}_swap().
OK. I'll add that to the patch.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-28 8:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 3:50 [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and ucsi_handle_connector_change() Jia-Ju Bai
2022-02-09 14:30 ` Heikki Krogerus
2022-02-10 2:24 ` Jia-Ju Bai
2022-02-10 14:30 ` Heikki Krogerus
2022-03-25 20:39 ` Jack Pham
2022-03-28 8:28 ` Heikki Krogerus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.