Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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_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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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_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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] Staging: dgnc: release the lock before testing for nullity
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
---
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_lock, flags);
if (ld)
tty_ldisc_deref(ld);
- spin_unlock_irqrestore(>ch_lock, flags);
}
--
2.3.2.223.g7a9409c
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] Staging: dgnc: release the lock before testing for nullity
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.quen...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
