Re: [PATCH] zram: fix possible race when checking idle_strm

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

Hello Joonsoo,

On (08/10/15 09:32), Joonsoo Kim wrote:
> > on the other hand... it's actually
> > 
> > 	wait_event() is
> > 
> > 	if (condition)
> > 		break;
> > 	prepare_to_wait_event(&wq, &__wait, state)
> > 	if (condition)
> > 		break;
> > 	schedule();
> > 
> > if first condition check was false and we missed a wakeup call between
> > first condition and prepare_to_wait_event(), then second condition
> > check should do the trick I think (or you expect that second condition
> > check may be wrongly pre-fetched or something).
> 
...
> I expected that second condition can be false if compiler reuse result
> of first check for optimization. I guess that there is no prevention
> for this kind of optimization.

hm... so we have outer and inner checks (out of loop and inside of loop).
can compiler decide that outer and inner checks are equivalent here?

  #define wait_event(wq, condition)                                       \
  do {                                                                    \
          might_sleep();                                                  \
          if (condition)                                                  \
                  break;                                                  \
      ....
                for (;;) {                                                \
                  long __int = prepare_to_wait_event(&wq, &__wait, state);\
                                                                          \
                  if (condition)                                          \
                          break;                                          \
                                                                          \
                  if (___wait_is_interruptible(state) && __int) {         \
                          __ret = __int;                                  \
                          if (exclusive) {                                \
                                  abort_exclusive_wait(&wq, &__wait,      \
                                                       state, NULL);      \
                                  goto __out;                             \
                          }                                               \
                          break;                                          \
                  }                                                       \
                                                                          \
                  cmd;                                                    \
                }                                                         \
      ....
  } while (0)


I probably don't have enough knowledge about compilers; but I think it
must keep two checks. But I may be wrong.

just out of curiosity, a quick grep

wait_event(zatm_vcc->tx_wait, !skb_peek(&zatm_vcc->tx_queue))
wait_event(pmu->recv.wait, (pmu->recv.process == 0))
wait_event(ep->com.waitq, ep->com.rpl_done)
wait_event(cs->waitqueue, !cs->waiting)
wait_event(resync_wait, (mddev->sync_thread == NULL &&...
wait_event(mddev->sb_wait, mddev->flags == 0 ||...

and so on.

	-ss

> So, following is the problem sequence I thought.
> T1 means thread 1, T2 means another thread, 2.
> 
> <T1-1> check if idle_strm is empty or not with holding the lock
> <T1-2> It is  so do spin_unlock and run wait_event macro
> <T1-3> check if idle_strm is empty or not
> <T1-4> It is still empty
> 
> <T2-1> do strm release
> <T2-2> call wake_up
> 
> <T1-5> add T1 to wait queue
> <T1-6> check if idle_strm is empty or not
> <T1-7> compiler reuse <T1-4>'s result or CPU just fetch cached
> result so T1 starts waiting
> 
> In this case, T1 can be sleep permanently. To prevent compiler
> optimization or fetching cached value, we need a lock here.

	-ss