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

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

[YE Chengfeng]

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 <cyeaa@connect.ust.hk>
---
 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

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

[andy]

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

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

[andy]

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

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

[andy]

On Mon, Jun 26, 2023 at 11:06:05AM +0000, YE Chengfeng wrote:
> Thanks for the reminder. Would fix the problems and send new patches soon.

Please, avoid top postings!

Also you must follow this https://docs.kernel.org/process/researcher-guidelines.html.

-- 
With Best Regards,
Andy Shevchenko

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

[YE Chengfeng]

Thanks much for the suggestion and guidance. I think the new patch series should be correct now. Note that I use another email to send the patches since I have some problem to setup this one with git send-mail due to 2FA.

Best Regards,
Chengfeng

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

[Chengfeng Ye]

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 <dg573847474@gmail.com>
---
 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