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

Groups > linux.kernel > #1712194 > unrolled thread

Possible race in ucb1400_ts.ko

Back to article view | Back to linux.kernel

Contents

  Possible race in ucb1400_ts.ko Anton Volkov <avolkov@ispras.ru> - 2017-08-15 15:50 +0200
    Re: Possible race in ucb1400_ts.ko Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-08-15 20:20 +0200

#1712194 — Possible race in ucb1400_ts.ko

Hello.

While searching for races in the Linux kernel I've come across 
"drivers/input/touchscreen/ucb1400_ts.ko" module. Here is a question 
that I came up with while analyzing results. Lines are given using the 
info from Linux v4.12.

Consider the following case:

Thread 1:                   Thread 2:
ucb1400_suspend
->ucb1400_ts_start
     ucb->stopped = false
     enable_irq()

ucb1400_resume
->ucb1400_ts_stop           ucb1400_irq
     ucb->stopped = true       while(!ucb->stopped && ...)
     (ucb1400_ts.c: line 230)  (ucb1400_ts.c: line 202)
     disable_irq()

The value of ucb->stopped may be changed in the midst of 'while' loop 
iterations or prevent all of them from happening. Is this feasible from 
your point of view? If so, is it a benign race or is it serious?

Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avolkov@ispras.ru

[toc] | [next] | [standalone]

#1712410

Hi Anton,

On Tue, Aug 15, 2017 at 04:46:25PM +0300, Anton Volkov wrote:
> Hello.
> 
> While searching for races in the Linux kernel I've come across
> "drivers/input/touchscreen/ucb1400_ts.ko" module. Here is a question
> that I came up with while analyzing results. Lines are given using
> the info from Linux v4.12.
> 
> Consider the following case:
> 
> Thread 1:                   Thread 2:
> ucb1400_suspend
> ->ucb1400_ts_start
>     ucb->stopped = false
>     enable_irq()
> 
> ucb1400_resume
> ->ucb1400_ts_stop           ucb1400_irq
>     ucb->stopped = true       while(!ucb->stopped && ...)
>     (ucb1400_ts.c: line 230)  (ucb1400_ts.c: line 202)
>     disable_irq()
> 
> The value of ucb->stopped may be changed in the midst of 'while'
> loop iterations or prevent all of them from happening. Is this
> feasible from your point of view? If so, is it a benign race or is
> it serious?

Well, I guess nobody is using that driver in mainline, or at least not
with platforms that do system suspend. The suspend is supposed to call
ucb1400_ts_stop(), not ucb1400_ts_start(), and resume is messed up as
well.

We need to fix it. Once it is done, then it should look better. The
ucb->stopped can change in the middle of while loop, and that should
cause the interrupt handler to stop running. The "stop" uses
disable_irq() and thus will wait for the interrupt handler to finish
before continuing.

Thanks.

-- 
Dmitry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web