Staging: dgnc: release the lock before testing for nullity
Subsystems (2)
PATCH v2 1/1 Staging: dgnc: release the lock before testing for nullity
The refactoring intrduced inc84a083b995b ("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.
---
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(-)
drivers/staging/dgnc/dgnc_tty.c +1 -1
@@ -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); }
Signature · 1 line
2.3.2.223.g7a9409c
Quoted 6 lines · 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.
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
Quoted 5 lines · Dan Carpenter
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.
Quoted 5 lines · Quentin Lambert
Quoted 2 lines · Dan Carpenter
This changelog still doesn't make sense so I took a look at the code.
Quoted 7 lines
Quoted 3 lines
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
Quoted 5 lines · Dan Carpenter
Quoted 4 lines · Quentin Lambert
Quoted 2 lines · Dan Carpenter
This changelog still doesn't make sense so I took a look at the code.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
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?
Quoted 4 lines
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.
Quoted 8 lines
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.
Quoted 1 line
This kind of change is a bit dangerous though so it requires testing.
Ok, should I act on that? What do you advice?
Quoted 2 lines
regards,
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