Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1305413 > unrolled thread

[PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker Peter Hurley <peter@hurleysoftware.com> - 2016-01-10 05:50 +0100
    Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste  kworker Ben Hutchings <ben@decadent.org.uk> - 2016-01-11 00:20 +0100
      Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste  kworker Peter Hurley <peter@hurleysoftware.com> - 2016-01-11 01:30 +0100
        Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste  kworker Peter Hurley <peter@hurleysoftware.com> - 2016-01-11 06:50 +0100
          Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste  kworker Ben Hutchings <ben@decadent.org.uk> - 2016-01-11 11:40 +0100

#1305413 — [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

As the function documentation for tty_ldisc_ref_wait() notes, it is
only callable from a tty file_operations routine; otherwise there
is no guarantee the ref won't be NULL.

The key difference with the VT's paste_selection() is that is an ioctl,
where __speakup_paste_selection() is completely asynch kworker, kicked
off from interrupt context.

Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
       tty (ab)usage to match vt")
Cc: <stable@vger.kernel.org>

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/staging/speakup/selection.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index aa5ab6c..86c0b9a 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
 	struct tty_ldisc *ld;
 	DECLARE_WAITQUEUE(wait, current);
 
-	ld = tty_ldisc_ref_wait(tty);
+	ld = tty_ldisc_ref(tty);
+	if (!ld)
+		return;
 	tty_buffer_lock_exclusive(&vc->port);
 
 	add_wait_queue(&vc->paste_wait, &wait);
-- 
2.7.0

[toc] | [next] | [standalone]

#1305735 — Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Multipart message — attachments visible in raw view] — view raw

On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
> As the function documentation for tty_ldisc_ref_wait() notes, it is
> only callable from a tty file_operations routine; otherwise there
> is no guarantee the ref won't be NULL.
> 
> The key difference with the VT's paste_selection() is that is an ioctl,
> where __speakup_paste_selection() is completely asynch kworker, kicked
> off from interrupt context.
> 
> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>        tty (ab)usage to match vt")
> Cc: <stable@vger.kernel.org>
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/staging/speakup/selection.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
> index aa5ab6c..86c0b9a 100644
> --- a/drivers/staging/speakup/selection.c
> +++ b/drivers/staging/speakup/selection.c
> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>  	struct tty_ldisc *ld;
>  	DECLARE_WAITQUEUE(wait, current);
>  
> -	ld = tty_ldisc_ref_wait(tty);
> +	ld = tty_ldisc_ref(tty);
> +	if (!ld)
> +		return;
>  	tty_buffer_lock_exclusive(&vc->port);
>  
>  	add_wait_queue(&vc->paste_wait, &wait);

This leaks a reference to the tty.  Instead of returning directly, I
think you need to add a label and goto the tty_kref_put() at the bottom
of the function.

Ben.

-- 
Ben Hutchings
Power corrupts.  Absolute power is kind of neat.
                           - John Lehman, Secretary of the US Navy 1981-1987

[toc] | [prev] | [next] | [standalone]

#1305752 — Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

On 01/10/2016 03:16 PM, Ben Hutchings wrote:
> On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
>> As the function documentation for tty_ldisc_ref_wait() notes, it is
>> only callable from a tty file_operations routine; otherwise there
>> is no guarantee the ref won't be NULL.
>>
>> The key difference with the VT's paste_selection() is that is an ioctl,
>> where __speakup_paste_selection() is completely asynch kworker, kicked
>> off from interrupt context.
>>
>> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>>        tty (ab)usage to match vt")
>> Cc: <stable@vger.kernel.org>
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  drivers/staging/speakup/selection.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
>> index aa5ab6c..86c0b9a 100644
>> --- a/drivers/staging/speakup/selection.c
>> +++ b/drivers/staging/speakup/selection.c
>> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>>  	struct tty_ldisc *ld;
>>  	DECLARE_WAITQUEUE(wait, current);
>>  
>> -	ld = tty_ldisc_ref_wait(tty);
>> +	ld = tty_ldisc_ref(tty);
>> +	if (!ld)
>> +		return;
>>  	tty_buffer_lock_exclusive(&vc->port);
>>  
>>  	add_wait_queue(&vc->paste_wait, &wait);
> 
> This leaks a reference to the tty.  Instead of returning directly, I
> think you need to add a label and goto the tty_kref_put() at the bottom
> of the function.

Ugh, speakup_paste_selection() is a worse hack than I thought it was.

[toc] | [prev] | [next] | [standalone]

#1305854 — Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

On 01/10/2016 04:25 PM, Peter Hurley wrote:
> On 01/10/2016 03:16 PM, Ben Hutchings wrote:
>> On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
>>> As the function documentation for tty_ldisc_ref_wait() notes, it is
>>> only callable from a tty file_operations routine; otherwise there
>>> is no guarantee the ref won't be NULL.
>>>
>>> The key difference with the VT's paste_selection() is that is an ioctl,
>>> where __speakup_paste_selection() is completely asynch kworker, kicked
>>> off from interrupt context.
>>>
>>> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>>>        tty (ab)usage to match vt")
>>> Cc: <stable@vger.kernel.org>
>>>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>  drivers/staging/speakup/selection.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
>>> index aa5ab6c..86c0b9a 100644
>>> --- a/drivers/staging/speakup/selection.c
>>> +++ b/drivers/staging/speakup/selection.c
>>> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>>>  	struct tty_ldisc *ld;
>>>  	DECLARE_WAITQUEUE(wait, current);
>>>  
>>> -	ld = tty_ldisc_ref_wait(tty);
>>> +	ld = tty_ldisc_ref(tty);
>>> +	if (!ld)
>>> +		return;
>>>  	tty_buffer_lock_exclusive(&vc->port);
>>>  
>>>  	add_wait_queue(&vc->paste_wait, &wait);
>>
>> This leaks a reference to the tty.  Instead of returning directly, I
>> think you need to add a label and goto the tty_kref_put() at the bottom
>> of the function.
> 
> Ugh, speakup_paste_selection() is a worse hack than I thought it was.

What if the kworker has already been scheduled but not run? Leaky reference
anyway.

What guarantees that the kref is gettable to begin with and isn't incrementing
from 0?

This isn't how tty krefs work.

I'll fix the patch to drop the kref but this is broken anyway.

Regards,
Peter Hurley

[toc] | [prev] | [next] | [standalone]

#1306039 — Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Multipart message — attachments visible in raw view] — view raw

On Sun, 2016-01-10 at 21:40 -0800, Peter Hurley wrote:
> On 01/10/2016 04:25 PM, Peter Hurley wrote:
> > On 01/10/2016 03:16 PM, Ben Hutchings wrote:
> > > On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
> > > > As the function documentation for tty_ldisc_ref_wait() notes, it is
> > > > only callable from a tty file_operations routine; otherwise there
> > > > is no guarantee the ref won't be NULL.
> > > > 
> > > > The key difference with the VT's paste_selection() is that is an ioctl,
> > > > where __speakup_paste_selection() is completely asynch kworker, kicked
> > > > off from interrupt context.
> > > > 
> > > > Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
> > > >        tty (ab)usage to match vt")
> > > > Cc: <stable@vger.kernel.org>
> > > > 
> > > > Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > > > ---
> > > >  drivers/staging/speakup/selection.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
> > > > index aa5ab6c..86c0b9a 100644
> > > > --- a/drivers/staging/speakup/selection.c
> > > > +++ b/drivers/staging/speakup/selection.c
> > > > @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
> > > >  	struct tty_ldisc *ld;
> > > >  	DECLARE_WAITQUEUE(wait, current);
> > > >  
> > > > -	ld = tty_ldisc_ref_wait(tty);
> > > > +	ld = tty_ldisc_ref(tty);
> > > > +	if (!ld)
> > > > +		return;
> > > >  	tty_buffer_lock_exclusive(&vc->port);
> > > >  
> > > >  	add_wait_queue(&vc->paste_wait, &wait);
> > > 
> > > This leaks a reference to the tty.  Instead of returning directly, I
> > > think you need to add a label and goto the tty_kref_put() at the bottom
> > > of the function.
> > 
> > Ugh, speakup_paste_selection() is a worse hack than I thought it was.
> 
> What if the kworker has already been scheduled but not run? Leaky reference
> anyway.

I don't think so - the cmpxchg() should ensure that only one paste is
outstanding.

> What guarantees that the kref is gettable to begin with and isn't incrementing
> from 0?

Surely it's a bug in the caller of speakup_paste_selection() if it
doesn't hold a reference?

Ben.

> This isn't how tty krefs work.
> 
> I'll fix the patch to drop the kref but this is broken anyway.
> 
> Regards,
> Peter Hurley
> 
-- 
Ben Hutchings
Q.  Which is the greater problem in the world today, ignorance or apathy?
A.  I don't know and I couldn't care less.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web