linux-kernel - Re: [patch] staging: ion: use two separate locks for heaps and clients in ion

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] [day] [month] [year] [list]

Message-ID: <20161009135842.GD30914@kroah.com>
Date:   Sun, 9 Oct 2016 15:58:43 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     YiPing Xu <xuyiping@...ilicon.com>
Cc:     Laura Abbott <labbott@...hat.com>, sumit.semwal@...aro.org,
        arve@...roid.com, riandrews@...roid.com,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        puck.chen@...ilicon.com, lili53@...wei.com,
        suzhuangluan@...ilicon.com
Subject: Re: [patch] staging: ion: use two separate locks for heaps and
 clients in ion_device

On Fri, Oct 07, 2016 at 04:27:49PM +0800, YiPing Xu wrote:
> 
> 
> On 2016/10/5 2:02, Laura Abbott wrote:
> > On 09/30/2016 01:18 AM, Xu YiPing wrote:
> > > ion_alloc may get into slow path to get free page,
> > > the call stack:
> > > 
> > > __alloc_pages_slowpath
> > > ion_page_pool_alloc_pages
> > > alloc_buffer_page
> > > ion_system_heap_allocate
> > > ion_buffer_create  <-- hold ion_device->lock
> > > ion_alloc
> > > 
> > > after that, kernel invokes low-memory killer to kill some apps in
> > > order to free memory in android system. However, sometimes, the
> > > killing is blocked,
> > > the app's call stack:
> > > 
> > > rwsem_down_write_failed
> > > down_write
> > > ion_client_destroy
> > > ion_release
> > > fput
> > > do_signal
> > > 
> > > the killing is blocked because ion_device->lock is held by ion_alloc.
> > > 
> > > ion_alloc hold the lock for accessing the heaps list,
> > > ion_destroy_client hold the lock for accessing the clients list.
> > > 
> > > so, we can use two separate locks for heaps and clients, to avoid the
> > > unnecessary race.
> > > 
> > 
> > I've reviewed this and it looks okay at first pass but I don't want it
> > applied just yet. Ion locking is a bit of a mess and has been added
> 
> yes, and now "debugfs_mutex" and "ion_root_client" is redundant, after
> commit 49d200deaa680501f19a247b1fffb29301e51d2b and
> 9fd4dcece43a53e5a9e65a973df5693702ee6401.

Ok, now dropping this patch from my queue.

thanks,

greg k-h