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  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]
Date:   Mon, 21 Dec 2020 11:50:17 -0800
From:   Shakeel Butt <>
To:     Vitaly Wool <>
Cc:     Minchan Kim <>, Mike Galbraith <>,
        LKML <>,
        linux-mm <>,
        Barry Song <>,
        Sebastian Andrzej Siewior <>,
        NitinGupta <>,
        Sergey Senozhatsky <>,
        Andrew Morton <>
Subject: Re: [PATCH] zsmalloc: do not use bit_spin_lock

On Mon, Dec 21, 2020 at 11:20 AM Vitaly Wool <> wrote:
> On Mon, Dec 21, 2020 at 6:24 PM Minchan Kim <> 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?

> >
> >
> > 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