[PATCH v2] Staging: dgnc: release the lock before testing for nullity

[PATCH v2] Staging: dgnc: release the lock before testing for nullity

[Quentin Lambert]

The refactoring intrduced in
c84a083b995b ("Staging: dgnc: Use goto for spinlock release before return")
inverts the order in which the lock is released and ld is tested for nullity.

This patch restores the execution flow.

Fixes: c84a083b995b ("Staging: dgnc: Use goto for spinlock release before return")
Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
---
Changes since v1:
 - the commit message details the point of the patch
 - remove a blank line between the Fixes line and the signed-off line.

 drivers/staging/dgnc/dgnc_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 8445f84ddaa3..f1c4d07a0aaa 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -656,9 +656,9 @@ void dgnc_input(struct channel_t *ch)
 	return;
 
 exit_unlock:
+	spin_unlock_irqrestore(&ch->ch_lock, flags);
 	if (ld)
 		tty_ldisc_deref(ld);
-	spin_unlock_irqrestore(&ch->ch_lock, flags);
 }
 
 
-- 
2.3.2.223.g7a9409c

Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity

[Dan Carpenter]

On Wed, Mar 18, 2015 at 02:21:08PM +0100, Quentin Lambert wrote:
> The refactoring intrduced in
> c84a083b995b ("Staging: dgnc: Use goto for spinlock release before return")
> inverts the order in which the lock is released and ld is tested for nullity.
> 
> This patch restores the execution flow.
> 

This changelog still doesn't make sense so I took a look at the code.

tty_ldisc_deref() is an unlock function.  So this is a lock ordering
bug.  What makes you think the original ordering was correct?  Who
reported this bug?  What are the effects of this bug?

Please answer the questions and we can work together on a proper fix if
needed.

regards,
dan carpenter

Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity

[Quentin Lambert]

On 18/03/2015 14:36, Dan Carpenter wrote:
> This changelog still doesn't make sense so I took a look at the code.
>
> tty_ldisc_deref() is an unlock function.  So this is a lock ordering
> bug.  What makes you think the original ordering was correct?  Who
> reported this bug?  What are the effects of this bug?
I was the one who introduced the ordering change in the first place.
I am just trying to fix it because although nobody complained I am not
sure of the impact and restoring the previous control flow seems to be the
right thing to do.

Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity

[Dan Carpenter]

On Wed, Mar 18, 2015 at 02:43:01PM +0100, Quentin Lambert wrote:
> 
> 
> On 18/03/2015 14:36, Dan Carpenter wrote:
> >This changelog still doesn't make sense so I took a look at the code.
> >
> >tty_ldisc_deref() is an unlock function.  So this is a lock ordering
> >bug.  What makes you think the original ordering was correct?  Who
> >reported this bug?  What are the effects of this bug?
> I was the one who introduced the ordering change in the first place.
> I am just trying to fix it because although nobody complained I am not
> sure of the impact and restoring the previous control flow seems to be the
> right thing to do.

Your changelog should tell me this stuff.

The original code is wrong.  We take "spin_lock_irqsave(&ch->ch_lock,
flags);" before we do "ld = tty_ldisc_ref(tp);" so we should deref
before we unlock.

It's normally:

lock_outer();
lock_inner();
unlock_inner();
unlock_outer();

On the success path we unlock first then deref and that is a mistake.

This kind of change is a bit dangerous though so it requires testing.

regards,
dan carpenter

Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity

[Quentin Lambert]

On 18/03/2015 14:54, Dan Carpenter wrote:
> On Wed, Mar 18, 2015 at 02:43:01PM +0100, Quentin Lambert wrote:
>>
>> On 18/03/2015 14:36, Dan Carpenter wrote:
>>> This changelog still doesn't make sense so I took a look at the code.
>>>
>>> tty_ldisc_deref() is an unlock function.  So this is a lock ordering
>>> bug.  What makes you think the original ordering was correct?  Who
>>> reported this bug?  What are the effects of this bug?
>> I was the one who introduced the ordering change in the first place.
>> I am just trying to fix it because although nobody complained I am not
>> sure of the impact and restoring the previous control flow seems to be the
>> right thing to do.
> Your changelog should tell me this stuff.
Should I send a third version then?
> The original code is wrong.  We take "spin_lock_irqsave(&ch->ch_lock,
> flags);" before we do "ld = tty_ldisc_ref(tp);" so we should deref
> before we unlock.
>
> It's normally:
>
> lock_outer();
> lock_inner();
> unlock_inner();
> unlock_outer();
>
> On the success path we unlock first then deref and that is a mistake.
I didn't know that thank you.
> This kind of change is a bit dangerous though so it requires testing.
Ok, should I act on that? What do you advice?
> regards,
> dan carpenter

Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity

[Dan Carpenter]

I don't know anyone with this hardware who can test a patch.  If you
find someone then you should send a patch to reorder the other original
unlock and deref.

Otherwise, I would just leave it alone for now.

regards,
dan carpenter