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
| ||
|
Date: Mon, 21 Dec 2020 11:50:17 -0800 From: Shakeel Butt <shakeelb@...gle.com> To: Vitaly Wool <vitaly.wool@...sulko.com> Cc: Minchan Kim <minchan@...nel.org>, Mike Galbraith <efault@....de>, LKML <linux-kernel@...r.kernel.org>, linux-mm <linux-mm@...ck.org>, Barry Song <song.bao.hua@...ilicon.com>, Sebastian Andrzej Siewior <bigeasy@...utronix.de>, NitinGupta <ngupta@...are.org>, Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>, Andrew Morton <akpm@...ux-foundation.org> Subject: Re: [PATCH] zsmalloc: do not use bit_spin_lock On Mon, Dec 21, 2020 at 11:20 AM Vitaly Wool <vitaly.wool@...sulko.com> wrote: > > On Mon, Dec 21, 2020 at 6:24 PM Minchan Kim <minchan@...nel.org> wrote: > > > > On Sun, Dec 20, 2020 at 02:22:28AM +0200, Vitaly Wool wrote: > > > zsmalloc takes bit spinlock in its _map() callback and releases it > > > only in unmap() which is unsafe and leads to zswap complaining > > > about scheduling in atomic context. > > > > > > To fix that and to improve RT properties of zsmalloc, remove that > > > bit spinlock completely and use a bit flag instead. > > > > I don't want to use such open code for the lock. > > > > I see from Mike's patch, recent zswap change introduced the lockdep > > splat bug and you want to improve zsmalloc to fix the zswap bug and > > introduce this patch with allowing preemption enabling. > > This understanding is upside down. The code in zswap you are referring > to is not buggy. You may claim that it is suboptimal but there is > nothing wrong in taking a mutex. > Is this suboptimal for all or just the hardware accelerators? Sorry, I am not very familiar with the crypto API. If I select lzo or lz4 as a zswap compressor will the [de]compression be async or sync? > > https://lore.kernel.org/linux-mm/fae85e4440a8ef6f13192476bd33a4826416fc58.camel@gmx.de/ > > > > zs_[un/map]_object is designed to be used in fast path(i.e., > > zs_map_object/4K page copy/zs_unmap_object) so the spinlock is > > perfectly fine for API point of view. However, zswap introduced > > using the API with mutex_lock/crypto_wait_req where allowing > > preemption, which was wrong. > > Taking a spinlock in one callback and releasing it in another is > unsafe and error prone. What if unmap was called on completion of a > DMA-like transfer from another context, like a threaded IRQ handler? > In that case this spinlock might never be released. > > Anyway I can come up with a zswap patch explicitly stating that > zsmalloc is not fully compliant with zswap / zpool API The documentation of zpool_map_handle() clearly states "This may hold locks, disable interrupts, and/or preemption, ...", so how come zsmalloc is not fully compliant? > to avoid > confusion for the time being. Would that be ok with you? > > Best regards, > Vitaly >
Powered by blists - more mailing lists