[PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[PATCH] gpio: gpiolib-cdev: Fix potential &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_irqsave.

Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
---
 drivers/gpio/gpiolib-cdev.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 0a33971c964c..714631fde9a8 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -614,14 +614,15 @@ static void linereq_put_event(struct linereq *lr,
 			      struct gpio_v2_line_event *le)
 {
 	bool overflow = false;
+	unsigned long flags;
 
-	spin_lock(&lr->wait.lock);
+	spin_lock_irqsave(&lr->wait.lock, flags);
 	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_irqrestore(&lr->wait.lock, flags);
 	if (!overflow)
 		wake_up_poll(&lr->wait, EPOLLIN);
 	else
@@ -1505,6 +1506,7 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
 	struct linereq *lr = file->private_data;
 	struct gpio_v2_line_event le;
 	ssize_t bytes_read = 0;
+	unsigned long flags;
 	int ret;
 
 	if (!lr->gdev->chip)
@@ -1514,28 +1516,28 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
 		return -EINVAL;
 
 	do {
-		spin_lock(&lr->wait.lock);
+		spin_lock_irqsave(&lr->wait.lock, flags);
 		if (kfifo_is_empty(&lr->events)) {
 			if (bytes_read) {
-				spin_unlock(&lr->wait.lock);
+				spin_unlock_irqrestore(&lr->wait.lock, flags);
 				return bytes_read;
 			}
 
 			if (file->f_flags & O_NONBLOCK) {
-				spin_unlock(&lr->wait.lock);
+				spin_unlock_irqrestore(&lr->wait.lock, flags);
 				return -EAGAIN;
 			}
 
 			ret = wait_event_interruptible_locked(lr->wait,
 					!kfifo_is_empty(&lr->events));
 			if (ret) {
-				spin_unlock(&lr->wait.lock);
+				spin_unlock_irqrestore(&lr->wait.lock, flags);
 				return ret;
 			}
 		}
 
 		ret = kfifo_out(&lr->events, &le, 1);
-		spin_unlock(&lr->wait.lock);
+		spin_unlock_irqrestore(&lr->wait.lock, flags);
 		if (ret != 1) {
 			/*
 			 * This should never happen - we were holding the
-- 
2.17.1

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[Andy Shevchenko]

On Sun, Jun 25, 2023 at 5:45 PM YE Chengfeng <cyeaa@connect.ust.hk> 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

acquires

> 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_irqsave.

Sounds legit to me, but
1) do you have any warning/oops/etc to show the real case?
2) shouldn't we annotate with respective lockdep asserts this code?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[Kent Gibson]

On Sun, Jun 25, 2023 at 02:45:12PM +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.
> 

So, in both cases, a process context holding the lock is interrupted, on
the same CPU, and the edge_irq_thread() deadlocks on that lock, as the
interrupted process holds the lock and cannot proceed.
That makes sense to me, but it would be good for Bart to confirm as he
knows a lot more about the kfifo locking than I do.

Note that the same problem also exists in lineevent_read_unlocked() - the
uAPI v1 equivalent of linereq_read_unlocked().

> Fix the two potential deadlock issues by spin_lock_irqsave.
> 

spin_lock_bh() should be sufficient, given that edge_irq_thread() is run
in a softirq?  That is faster and would allow the hard irq handlers to
still run, and timestamp the event, but inhibit the edge_irq_thread()
from being called on that CPU until the lock is released.
(hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it is
never called from hard irq context, so there is a good chance I'm missing
something here??)
More on spin_lock choice below.

This should have a Fixes tag.
For v2, it has been there since it was added, so:

73e0341992b6 ("gpiolib: cdev: support edge detection for uAPI v2")

And it also applies to lineevent_read_unlocked() from uAPI v1, so there
should be a separate fix for that, or at least a separate tag.

I looks to me that it was first introduced in uAPI v1 here:

dea9c80ee672 ("gpiolib: rework the locking mechanism for lineevent kfifo")

> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
> ---
>  drivers/gpio/gpiolib-cdev.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 0a33971c964c..714631fde9a8 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -614,14 +614,15 @@ static void linereq_put_event(struct linereq *lr,
>  			      struct gpio_v2_line_event *le)
>  {
>  	bool overflow = false;
> +	unsigned long flags;
>  
> -	spin_lock(&lr->wait.lock);
> +	spin_lock_irqsave(&lr->wait.lock, flags);

linereq_put_event() is never called from hard irq context, so
spin_lock_irq() or spin_lock_bh() should suffice?

>  	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_irqrestore(&lr->wait.lock, flags);
>  	if (!overflow)
>  		wake_up_poll(&lr->wait, EPOLLIN);
>  	else
> @@ -1505,6 +1506,7 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
>  	struct linereq *lr = file->private_data;
>  	struct gpio_v2_line_event le;
>  	ssize_t bytes_read = 0;
> +	unsigned long flags;
>  	int ret;
>  
>  	if (!lr->gdev->chip)
> @@ -1514,28 +1516,28 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
>  		return -EINVAL;
>  
>  	do {
> -		spin_lock(&lr->wait.lock);
> +		spin_lock_irqsave(&lr->wait.lock, flags);

linereq_read_unlocked() is only ever called in process context, so this
could be spin_lock_irq() or even spin_lock_bh()?

>  		if (kfifo_is_empty(&lr->events)) {
>  			if (bytes_read) {
> -				spin_unlock(&lr->wait.lock);
> +				spin_unlock_irqrestore(&lr->wait.lock, flags);
>  				return bytes_read;
>  			}
>  
>  			if (file->f_flags & O_NONBLOCK) {
> -				spin_unlock(&lr->wait.lock);
> +				spin_unlock_irqrestore(&lr->wait.lock, flags);
>  				return -EAGAIN;
>  			}
>  
>  			ret = wait_event_interruptible_locked(lr->wait,
>  					!kfifo_is_empty(&lr->events));

wait_event_interruptible_locked() works with locks that are
spin_lock()/spin_unlock(), so this will leave irqs disabled while
waiting for a new event??

And while there is a wait_event_interruptible_locked_irq(), there is
no wait_event_interruptible_locked_bh() form that I can see, so using
spin_lock_bh() would require some extra work.

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

Anyway, good catch.

Cheers,
Kent.

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[YE Chengfeng]

> 1) do you have any warning/oops/etc to show the real case?
> 2) shouldn't we annotate with respective lockdep asserts this code?
The bugs were detected by an experimental static code analyzer that I am 
implementing. I don't have input to trigger it, so I manually review the 
report and then send the ones I believe to be true to you. Perhaps next 
time I should mention this while sending the patch.

> Note that the same problem also exists in lineevent_read_unlocked() - the
> uAPI v1 equivalent of linereq_read_unlocked().
Thanks for the reminder, I make a new patch for this.

> This should have a Fixes tag.
> For v2, it has been there since it was added, so:
> 
> 73e0341992b6 ("gpiolib: cdev: support edge detection for uAPI v2")
> 
> And it also applies to lineevent_read_unlocked() from uAPI v1, so there
> should be a separate fix for that, or at least a separate tag.
> 
> I looks to me that it was first introduced in uAPI v1 here:
> 
> dea9c80ee672 ("gpiolib: rework the locking mechanism for lineevent kfifo”)
No problem, add these fixes tag in the v2 patches.

> linereq_put_event() is never called from hard irq context, so
> spin_lock_irq() or spin_lock_bh() should suffice?
Change to spin_lock_bh() in the v2 patch.

> wait_event_interruptible_locked() works with locks that are
> spin_lock()/spin_unlock(), so this will leave irqs disabled while
> waiting for a new event??
> And while there is a wait_event_interruptible_locked_irq(), there is
> no wait_event_interruptible_locked_bh() form that I can see, so using
> spin_lock_bh() would require some extra work.
I am willing to help but not sure how to use spin_lock_bh() for reasons as you
mentioned, so the v2 patches change to wait_event_interruptible_locked_irq() 
and spin_lock_irq().

Thanks all for the help to improve the fixes.

Best Regards,
Chengfeng

> 2023年6月26日 下午3:23，Kent Gibson <warthog618@gmail.com> 写道：
> 
> On Sun, Jun 25, 2023 at 02:45:12PM +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.
>> 
> 
> So, in both cases, a process context holding the lock is interrupted, on
> the same CPU, and the edge_irq_thread() deadlocks on that lock, as the
> interrupted process holds the lock and cannot proceed.
> That makes sense to me, but it would be good for Bart to confirm as he
> knows a lot more about the kfifo locking than I do.
> 
> Note that the same problem also exists in lineevent_read_unlocked() - the
> uAPI v1 equivalent of linereq_read_unlocked().
> 
>> Fix the two potential deadlock issues by spin_lock_irqsave.
>> 
> 
> spin_lock_bh() should be sufficient, given that edge_irq_thread() is run
> in a softirq?  That is faster and would allow the hard irq handlers to
> still run, and timestamp the event, but inhibit the edge_irq_thread()
> from being called on that CPU until the lock is released.
> (hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it is
> never called from hard irq context, so there is a good chance I'm missing
> something here??)
> More on spin_lock choice below.
> 
> This should have a Fixes tag.
> For v2, it has been there since it was added, so:
> 
> 73e0341992b6 ("gpiolib: cdev: support edge detection for uAPI v2")
> 
> And it also applies to lineevent_read_unlocked() from uAPI v1, so there
> should be a separate fix for that, or at least a separate tag.
> 
> I looks to me that it was first introduced in uAPI v1 here:
> 
> dea9c80ee672 ("gpiolib: rework the locking mechanism for lineevent kfifo")
> 
>> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
>> ---
>> drivers/gpio/gpiolib-cdev.c | 16 +++++++++-------
>> 1 file changed, 9 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
>> index 0a33971c964c..714631fde9a8 100644
>> --- a/drivers/gpio/gpiolib-cdev.c
>> +++ b/drivers/gpio/gpiolib-cdev.c
>> @@ -614,14 +614,15 @@ static void linereq_put_event(struct linereq *lr,
>> 			      struct gpio_v2_line_event *le)
>> {
>> 	bool overflow = false;
>> +	unsigned long flags;
>> 
>> -	spin_lock(&lr->wait.lock);
>> +	spin_lock_irqsave(&lr->wait.lock, flags);
> 
> linereq_put_event() is never called from hard irq context, so
> spin_lock_irq() or spin_lock_bh() should suffice?
> 
>> 	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_irqrestore(&lr->wait.lock, flags);
>> 	if (!overflow)
>> 		wake_up_poll(&lr->wait, EPOLLIN);
>> 	else
>> @@ -1505,6 +1506,7 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
>> 	struct linereq *lr = file->private_data;
>> 	struct gpio_v2_line_event le;
>> 	ssize_t bytes_read = 0;
>> +	unsigned long flags;
>> 	int ret;
>> 
>> 	if (!lr->gdev->chip)
>> @@ -1514,28 +1516,28 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
>> 		return -EINVAL;
>> 
>> 	do {
>> -		spin_lock(&lr->wait.lock);
>> +		spin_lock_irqsave(&lr->wait.lock, flags);
> 
> linereq_read_unlocked() is only ever called in process context, so this
> could be spin_lock_irq() or even spin_lock_bh()?
> 
>> 		if (kfifo_is_empty(&lr->events)) {
>> 			if (bytes_read) {
>> -				spin_unlock(&lr->wait.lock);
>> +				spin_unlock_irqrestore(&lr->wait.lock, flags);
>> 				return bytes_read;
>> 			}
>> 
>> 			if (file->f_flags & O_NONBLOCK) {
>> -				spin_unlock(&lr->wait.lock);
>> +				spin_unlock_irqrestore(&lr->wait.lock, flags);
>> 				return -EAGAIN;
>> 			}
>> 
>> 			ret = wait_event_interruptible_locked(lr->wait,
>> 					!kfifo_is_empty(&lr->events));
> 
> wait_event_interruptible_locked() works with locks that are
> spin_lock()/spin_unlock(), so this will leave irqs disabled while
> waiting for a new event??
> 
> And while there is a wait_event_interruptible_locked_irq(), there is
> no wait_event_interruptible_locked_bh() form that I can see, so using
> spin_lock_bh() would require some extra work.
> 
>> 			if (ret) {
>> -				spin_unlock(&lr->wait.lock);
>> +				spin_unlock_irqrestore(&lr->wait.lock, flags);
>> 				return ret;
>> 			}
>> 		}
>> 
>> 		ret = kfifo_out(&lr->events, &le, 1);
>> -		spin_unlock(&lr->wait.lock);
>> +		spin_unlock_irqrestore(&lr->wait.lock, flags);
>> 		if (ret != 1) {
>> 			/*
>> 			 * This should never happen - we were holding the
>> -- 
>> 2.17.1
> 
> Anyway, good catch.
> 
> Cheers,
> Kent.

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[andy]

On Mon, Jun 26, 2023 at 10:38:53AM +0000, YE Chengfeng wrote:
> > 1) do you have any warning/oops/etc to show the real case?
> > 2) shouldn't we annotate with respective lockdep asserts this code?
> The bugs were detected by an experimental static code analyzer that I am 
> implementing. I don't have input to trigger it, so I manually review the 
> report and then send the ones I believe to be true to you. Perhaps next 
> time I should mention this while sending the patch.

This you have to mention in the submission.
https://docs.kernel.org/process/researcher-guidelines.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[Bartosz Golaszewski]

On Mon, Jun 26, 2023 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sun, Jun 25, 2023 at 02:45:12PM +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.
> >
>
> So, in both cases, a process context holding the lock is interrupted, on
> the same CPU, and the edge_irq_thread() deadlocks on that lock, as the
> interrupted process holds the lock and cannot proceed.
> That makes sense to me, but it would be good for Bart to confirm as he
> knows a lot more about the kfifo locking than I do.
>

Yeah, I'm not sure this is correct. edge_irq_thread() runs in process
context, so the whole premise of the patch seems to be flawed. What
tool reported this? Can this be a false positive? Have you seen this
happen in real life?

> Note that the same problem also exists in lineevent_read_unlocked() - the
> uAPI v1 equivalent of linereq_read_unlocked().
>
> > Fix the two potential deadlock issues by spin_lock_irqsave.
> >
>
> spin_lock_bh() should be sufficient, given that edge_irq_thread() is run
> in a softirq?  That is faster and would allow the hard irq handlers to
> still run, and timestamp the event, but inhibit the edge_irq_thread()
> from being called on that CPU until the lock is released.
> (hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it is
> never called from hard irq context, so there is a good chance I'm missing
> something here??)
> More on spin_lock choice below.

Again: this is incorrect - edge_irq_thread() doesn't execute in
softirq context which can be verified by calling in_softirq() from it.

>
> This should have a Fixes tag.
> For v2, it has been there since it was added, so:
>
> 73e0341992b6 ("gpiolib: cdev: support edge detection for uAPI v2")
>
> And it also applies to lineevent_read_unlocked() from uAPI v1, so there
> should be a separate fix for that, or at least a separate tag.
>
> I looks to me that it was first introduced in uAPI v1 here:
>
> dea9c80ee672 ("gpiolib: rework the locking mechanism for lineevent kfifo")
>
> > Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index 0a33971c964c..714631fde9a8 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -614,14 +614,15 @@ static void linereq_put_event(struct linereq *lr,
> >                             struct gpio_v2_line_event *le)
> >  {
> >       bool overflow = false;
> > +     unsigned long flags;
> >
> > -     spin_lock(&lr->wait.lock);
> > +     spin_lock_irqsave(&lr->wait.lock, flags);
>
> linereq_put_event() is never called from hard irq context, so
> spin_lock_irq() or spin_lock_bh() should suffice?
>

AFAICT it is only ever called from process context and so spin_lock()
is correct here.

Bart

> >       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_irqrestore(&lr->wait.lock, flags);
> >       if (!overflow)
> >               wake_up_poll(&lr->wait, EPOLLIN);
> >       else
> > @@ -1505,6 +1506,7 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
> >       struct linereq *lr = file->private_data;
> >       struct gpio_v2_line_event le;
> >       ssize_t bytes_read = 0;
> > +     unsigned long flags;
> >       int ret;
> >
> >       if (!lr->gdev->chip)
> > @@ -1514,28 +1516,28 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
> >               return -EINVAL;
> >
> >       do {
> > -             spin_lock(&lr->wait.lock);
> > +             spin_lock_irqsave(&lr->wait.lock, flags);
>
> linereq_read_unlocked() is only ever called in process context, so this
> could be spin_lock_irq() or even spin_lock_bh()?
>
> >               if (kfifo_is_empty(&lr->events)) {
> >                       if (bytes_read) {
> > -                             spin_unlock(&lr->wait.lock);
> > +                             spin_unlock_irqrestore(&lr->wait.lock, flags);
> >                               return bytes_read;
> >                       }
> >
> >                       if (file->f_flags & O_NONBLOCK) {
> > -                             spin_unlock(&lr->wait.lock);
> > +                             spin_unlock_irqrestore(&lr->wait.lock, flags);
> >                               return -EAGAIN;
> >                       }
> >
> >                       ret = wait_event_interruptible_locked(lr->wait,
> >                                       !kfifo_is_empty(&lr->events));
>
> wait_event_interruptible_locked() works with locks that are
> spin_lock()/spin_unlock(), so this will leave irqs disabled while
> waiting for a new event??
>
> And while there is a wait_event_interruptible_locked_irq(), there is
> no wait_event_interruptible_locked_bh() form that I can see, so using
> spin_lock_bh() would require some extra work.
>
> >                       if (ret) {
> > -                             spin_unlock(&lr->wait.lock);
> > +                             spin_unlock_irqrestore(&lr->wait.lock, flags);
> >                               return ret;
> >                       }
> >               }
> >
> >               ret = kfifo_out(&lr->events, &le, 1);
> > -             spin_unlock(&lr->wait.lock);
> > +             spin_unlock_irqrestore(&lr->wait.lock, flags);
> >               if (ret != 1) {
> >                       /*
> >                        * This should never happen - we were holding the
> > --
> > 2.17.1
>
> Anyway, good catch.
>
> Cheers,
> Kent.

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[YE Chengfeng]

> Yeah, I'm not sure this is correct. edge_irq_thread() runs in process
> 
> context, so the whole premise of the patch seems to be flawed. What
> tool reported this? Can this be a false positive? Have you seen this
> 
> happen in real life?

It seems like it’s my mistake, I misidentify thread_irq as runing inside
irq context. Then it’s truth that the patch is unnecessary.

The static analysis tool is built by me, I noticed lockdep occasionally reports
such similar deadlocks in other place thus intent to build a static tool to locate
such bugs. It has detected other true bugs these days but it’s a pity this
one is really a false positive.

Indeed thanks so much for your time and sorry for bothering you with the false report.

Thanks again,
Chengfeng

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[Kent Gibson]

On Mon, Jun 26, 2023 at 05:50:47PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 26, 2023 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> >
> > spin_lock_bh() should be sufficient, given that edge_irq_thread() is run
> > in a softirq?  That is faster and would allow the hard irq handlers to
> > still run, and timestamp the event, but inhibit the edge_irq_thread()
> > from being called on that CPU until the lock is released.
> > (hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it is
> > never called from hard irq context, so there is a good chance I'm missing
> > something here??)
> > More on spin_lock choice below.
> 
> Again: this is incorrect - edge_irq_thread() doesn't execute in
> softirq context which can be verified by calling in_softirq() from it.
> 

Ok, that matches what I had initially thought.  Wading through the kernel
doc got me thinking the secondary handler was run as a softirq.
But it is a threaded irq used here, so the thread handler runs in a
kernel thread, as does the debounce_work_func() and hte thread handler
process_hw_ts_thread().
That's a relief.

While we are on the subject of spin_locks, why does
gpio_desc_to_lineinfo() use spin_lock_irqsave()?
I assume the _irq is necessary as the desc could be updated at interrupt
level, but AFAICT gpio_desc_to_lineinfo() is only ever called from process
context, so why not just spin_lock_irq()?

Cheers,
Kent.

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[Bartosz Golaszewski]

On Tue, Jun 27, 2023 at 3:43 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Jun 26, 2023 at 05:50:47PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Jun 26, 2023 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > >
> > > spin_lock_bh() should be sufficient, given that edge_irq_thread() is run
> > > in a softirq?  That is faster and would allow the hard irq handlers to
> > > still run, and timestamp the event, but inhibit the edge_irq_thread()
> > > from being called on that CPU until the lock is released.
> > > (hmmm, gpio_desc_to_lineinfo() also uses spin_lock_irqsave() but it is
> > > never called from hard irq context, so there is a good chance I'm missing
> > > something here??)
> > > More on spin_lock choice below.
> >
> > Again: this is incorrect - edge_irq_thread() doesn't execute in
> > softirq context which can be verified by calling in_softirq() from it.
> >
>
> Ok, that matches what I had initially thought.  Wading through the kernel
> doc got me thinking the secondary handler was run as a softirq.
> But it is a threaded irq used here, so the thread handler runs in a
> kernel thread, as does the debounce_work_func() and hte thread handler
> process_hw_ts_thread().
> That's a relief.
>
> While we are on the subject of spin_locks, why does
> gpio_desc_to_lineinfo() use spin_lock_irqsave()?
> I assume the _irq is necessary as the desc could be updated at interrupt
> level, but AFAICT gpio_desc_to_lineinfo() is only ever called from process
> context, so why not just spin_lock_irq()?
>
> Cheers,
> Kent.

Didn't we use an atomic notifier before for some reason? Then it got
changed to blocking but the lock stayed like this? It does look like
spin_lock_irq() would be fine here. On the other hand - if something
isn't broken... :)

Bart

Re: [PATCH] gpio: gpiolib-cdev: Fix potential &lr->wait.lock deadlock issue

[Kent Gibson]

On Tue, Jun 27, 2023 at 01:47:16PM +0200, Bartosz Golaszewski wrote:
> On Tue, Jun 27, 2023 at 3:43 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Mon, Jun 26, 2023 at 05:50:47PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, Jun 26, 2023 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > >
> >
> > While we are on the subject of spin_locks, why does
> > gpio_desc_to_lineinfo() use spin_lock_irqsave()?
> > I assume the _irq is necessary as the desc could be updated at interrupt
> > level, but AFAICT gpio_desc_to_lineinfo() is only ever called from process
> > context, so why not just spin_lock_irq()?
> >
> > Cheers,
> > Kent.
> 
> Didn't we use an atomic notifier before for some reason? Then it got
> changed to blocking but the lock stayed like this? It does look like
> spin_lock_irq() would be fine here. On the other hand - if something
> isn't broken... :)
> 

Yeah, it was atomic before blocking, but that doesn't explain the need
for the save - interrupts are always enabled in that function.
Not a big difference either way, and irqsave is always safe, so no
problem with leaving it as is - I just thought it odd when I noticed it,
while spin locks and context were front of mind.

Cheers,
Kent.