2023-06-26 10:56:51

by YE Chengfeng

[permalink] [raw]
Subject: [PATCH v2 1/2] gpiolib: cdev: Fix &lr->wait.lock deadlock issue

linereq_put_event is called from both interrupt context (e.g.,
edge_irq_thread) and process context (process_hw_ts_thread).
Therefore, interrupt should be disabled before acquiring lock
&lr->wait.lock inside linereq_put_event to avoid deadlock when
the lock is held in process context and edge_irq_thread comes.

Similarly, linereq_read_unlocked running in process context
also acquies the same lock. It also need to disable interrupt
otherwise deadlock could happen if the irq edge_irq_thread
comes to execution while the lock is held.

Fix the two potential deadlock issues by spin_lock_bh() and
spin_lock_irq() separately.

Fixes: 73e0341992b6 ("gpiolib: cdev: support edge detection for uAPI v2")
Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0a33971c964c..f768d46bdea7 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -615,13 +615,13 @@ static void linereq_put_event(struct linereq *lr,
{
bool overflow = false;

- spin_lock(&lr->wait.lock);
+ spin_lock_bh(&lr->wait.lock);
if (kfifo_is_full(&lr->events)) {
overflow = true;
kfifo_skip(&lr->events);
}
kfifo_in(&lr->events, le, 1);
- spin_unlock(&lr->wait.lock);
+ spin_unlock_bh(&lr->wait.lock);
if (!overflow)
wake_up_poll(&lr->wait, EPOLLIN);
else
@@ -1514,28 +1514,28 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
return -EINVAL;

do {
- spin_lock(&lr->wait.lock);
+ spin_lock_irq(&lr->wait.lock);
if (kfifo_is_empty(&lr->events)) {
if (bytes_read) {
- spin_unlock(&lr->wait.lock);
+ spin_unlock_irq(&lr->wait.lock);
return bytes_read;
}

if (file->f_flags & O_NONBLOCK) {
- spin_unlock(&lr->wait.lock);
+ spin_unlock_irq(&lr->wait.lock);
return -EAGAIN;
}

- ret = wait_event_interruptible_locked(lr->wait,
+ ret = wait_event_interruptible_locked_irq(lr->wait,
!kfifo_is_empty(&lr->events));
if (ret) {
- spin_unlock(&lr->wait.lock);
+ spin_unlock_irq(&lr->wait.lock);
return ret;
}
}

ret = kfifo_out(&lr->events, &le, 1);
- spin_unlock(&lr->wait.lock);
+ spin_unlock_irq(&lr->wait.lock);
if (ret != 1) {
/*
* This should never happen - we were holding the
--
2.17.1


2023-06-26 11:21:55

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] gpiolib: cdev: Fix &lr->wait.lock deadlock issue

On Mon, Jun 26, 2023 at 10:39:56AM +0000, YE Chengfeng wrote:
> linereq_put_event is called from both interrupt context (e.g.,
> edge_irq_thread) and process context (process_hw_ts_thread).
> Therefore, interrupt should be disabled before acquiring lock
> &lr->wait.lock inside linereq_put_event to avoid deadlock when
> the lock is held in process context and edge_irq_thread comes.
>
> Similarly, linereq_read_unlocked running in process context
> also acquies the same lock. It also need to disable interrupt
> otherwise deadlock could happen if the irq edge_irq_thread
> comes to execution while the lock is held.
>
> Fix the two potential deadlock issues by spin_lock_bh() and
> spin_lock_irq() separately.

Side note: You have two patches in something that seems to be the series,
but lacks the references in email headers. Whenever you send a series,
do not forget to add --thread to `git send-email`.

--
With Best Regards,
Andy Shevchenko



2023-06-26 11:22:56

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] gpiolib: cdev: Fix &lr->wait.lock deadlock issue

On Mon, Jun 26, 2023 at 10:39:56AM +0000, YE Chengfeng wrote:
> linereq_put_event is called from both interrupt context (e.g.,

When we refer to the functions we use func() format.
This applies to the entire commit message and to the other
patch in the series.

> edge_irq_thread) and process context (process_hw_ts_thread).
> Therefore, interrupt should be disabled before acquiring lock
> &lr->wait.lock inside linereq_put_event to avoid deadlock when
> the lock is held in process context and edge_irq_thread comes.
>
> Similarly, linereq_read_unlocked running in process context
> also acquies the same lock. It also need to disable interrupt
> otherwise deadlock could happen if the irq edge_irq_thread
> comes to execution while the lock is held.
>
> Fix the two potential deadlock issues by spin_lock_bh() and
> spin_lock_irq() separately.

...

So, I suggest to fix the commit messages and send a v2 as a real series
(--thread implied).

--
With Best Regards,
Andy Shevchenko