linux-kernel - Re: [PATCH] zram: fix possible race when checking idle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150807091457.GL1891@swordfish>
Date:	Fri, 7 Aug 2015 18:14:57 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Joonsoo Kim <js1304@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan@...nel.org>,
	Nitin Gupta <ngupta@...are.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	linux-kernel@...r.kernel.org, Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH] zram: fix possible race when checking idle_strm

On (08/07/15 17:03), Joonsoo Kim wrote:
> Currently, when we enter the wait state due to lack of idle stream,
> we check idle_strm list without holding the lock in expanding of
> wait_event define. In this case, some one can see stale value and
> process could fall into wait state without any upcoming wakeup process.

hm... I need to think about it more.

we do wake_up every time we put stream back to the list

zcomp_strm_multi_release():

        spin_lock(&zs->strm_lock);
        if (zs->avail_strm <= zs->max_strm) {
                list_add(&zstrm->list, &zs->idle_strm);
                spin_unlock(&zs->strm_lock);
                wake_up(&zs->strm_wait);
                return;
        }

but I can probably see what you mean... in some very extreme case,
though. I can't even formulate it... eh... we use a multi stream
backend with ->max_strm == 1 and there are two processes, one
just falsely passed the wait_event() `if (condition)' check, the
other one just put stream back to ->idle_strm and called wake_up(),
but the first process hasn't yet executed prepare_to_wait_event()
so it might miss a wakeup. and there should be no other process
doing read or write operation. otherwise, there will be wakeup
eventually.

is this the case you were thinking of?... then yes, this spinlock
may help.

hm... we also can simply forbid downgrading a multi stream backend
to a single stream (in terms of performance it is much slower
than a real single steam anyway). will this do the trick? if we
have more than 1 stream and idle list then there will be more that
one wakeup. and every woken up task will call wakeup once it put
stream.

I don't think I'll be able to check the email until this Sunday,
so here is my

Acked-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>

to this patch if Minchan decides that `forbid downgrading` is
hacky or doesn't work.

very nice finding, Joonsoo!

p.s. hm... we can take a look at 'forbid downgrading a multi
stream backend to a single-streamed'.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/