* [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 13:21 ` Quentin Lambert
0 siblings, 0 replies; 12+ messages in thread
From: Quentin Lambert @ 2015-03-18 13:21 UTC (permalink / raw)
To: Lidza Louina
Cc: Mark Hounschell, Greg Kroah-Hartman, driverdev-devel, devel,
linux-kernel, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 13:21 ` Quentin Lambert
0 siblings, 0 replies; 12+ messages in thread
From: Quentin Lambert @ 2015-03-18 13:21 UTC (permalink / raw)
To: Lidza Louina
Cc: Mark Hounschell, Greg Kroah-Hartman, driverdev-devel, devel,
linux-kernel, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
2015-03-18 13:21 ` Quentin Lambert
@ 2015-03-18 13:36 ` Dan Carpenter
-1 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-03-18 13:36 UTC (permalink / raw)
To: Quentin Lambert
Cc: Lidza Louina, devel, Greg Kroah-Hartman, driverdev-devel,
kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 13:36 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-03-18 13:36 UTC (permalink / raw)
To: Quentin Lambert
Cc: Lidza Louina, devel, Greg Kroah-Hartman, driverdev-devel,
kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
2015-03-18 13:36 ` Dan Carpenter
@ 2015-03-18 13:43 ` Quentin Lambert
-1 siblings, 0 replies; 12+ messages in thread
From: Quentin Lambert @ 2015-03-18 13:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lidza Louina, devel, Greg Kroah-Hartman, driverdev-devel,
kernel-janitors, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 13:43 ` Quentin Lambert
0 siblings, 0 replies; 12+ messages in thread
From: Quentin Lambert @ 2015-03-18 13:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lidza Louina, devel, Greg Kroah-Hartman, driverdev-devel,
kernel-janitors, linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
2015-03-18 13:43 ` Quentin Lambert
@ 2015-03-18 13:54 ` Dan Carpenter
-1 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-03-18 13:54 UTC (permalink / raw)
To: Quentin Lambert
Cc: devel, Lidza Louina, driverdev-devel, kernel-janitors,
linux-kernel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 13:54 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-03-18 13:54 UTC (permalink / raw)
To: Quentin Lambert
Cc: Lidza Louina, devel, Greg Kroah-Hartman, driverdev-devel,
kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
2015-03-18 13:54 ` Dan Carpenter
@ 2015-03-18 13:59 ` Quentin Lambert
-1 siblings, 0 replies; 12+ messages in thread
From: Quentin Lambert @ 2015-03-18 13:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Lidza Louina, driverdev-devel, kernel-janitors,
linux-kernel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 13:59 ` Quentin Lambert
0 siblings, 0 replies; 12+ messages in thread
From: Quentin Lambert @ 2015-03-18 13:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lidza Louina, devel, Greg Kroah-Hartman, driverdev-devel,
kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
2015-03-18 13:59 ` Quentin Lambert
@ 2015-03-18 14:03 ` Dan Carpenter
-1 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-03-18 14:03 UTC (permalink / raw)
To: Quentin Lambert
Cc: devel, Lidza Louina, driverdev-devel, kernel-janitors,
linux-kernel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] Staging: dgnc: release the lock before testing for nullity
@ 2015-03-18 14:03 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-03-18 14:03 UTC (permalink / raw)
To: Quentin Lambert
Cc: devel, Lidza Louina, driverdev-devel, kernel-janitors,
linux-kernel, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-03-18 14:03 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-18 13:21 [PATCH v2] Staging: dgnc: release the lock before testing for nullity Quentin Lambert
2015-03-18 13:21 ` Quentin Lambert
2015-03-18 13:36 ` Dan Carpenter
2015-03-18 13:36 ` Dan Carpenter
2015-03-18 13:43 ` Quentin Lambert
2015-03-18 13:43 ` Quentin Lambert
2015-03-18 13:54 ` Dan Carpenter
2015-03-18 13:54 ` Dan Carpenter
2015-03-18 13:59 ` Quentin Lambert
2015-03-18 13:59 ` Quentin Lambert
2015-03-18 14:03 ` Dan Carpenter
2015-03-18 14:03 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.