linux-kernel - Re: possible deadlock in __do_page

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]

Date:   Thu, 24 Jan 2019 08:46:46 -0500
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Todd Kjos <tkjos@...gle.com>,
        syzbot+a76129f18c89f3e2ddd4@...kaller.appspotmail.com,
        ak@...ux.intel.com, Johannes Weiner <hannes@...xchg.org>,
        jack@...e.cz, jrdr.linux@...il.com,
        LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
        mawilcox@...rosoft.com, mgorman@...hsingularity.net,
        syzkaller-bugs@...glegroups.com,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: possible deadlock in __do_page_fault

On Thu, Jan 24, 2019 at 10:52:30AM +0900, Tetsuo Handa wrote:
> Joel Fernandes wrote:
> > > Anyway, I need your checks regarding whether this approach is waiting for
> > > completion at all locations which need to wait for completion.
> > 
> > I think you are waiting in unwanted locations. The only location you need to
> > wait in is ashmem_pin_unpin.
> > 
> > So, to my eyes all that is needed to fix this bug is:
> > 
> > 1. Delete the range from the ashmem_lru_list
> > 2. Release the ashmem_mutex
> > 3. fallocate the range.
> > 4. Do the completion so that any waiting pin/unpin can proceed.
> > 
> > Could you clarify why you feel you need to wait for completion at those other
> > locations?
> 
> Because I don't know how ashmem works.

You sound like you're almost there though.

> > Note that once a range is unpinned, it is open sesame and userspace cannot
> > really expect consistent data from such range till it is pinned again.
> 
> Then, I'm tempted to eliminate shrinker and LRU list (like a draft patch shown
> below). I think this is not equivalent to current code because this shrinks
> upon only range_alloc() time and I don't know whether it is OK to temporarily
> release ashmem_mutex during range_alloc() at "Case #4" of ashmem_pin(), but
> can't we go this direction? 

No, the point of the shrinker is to do a lazy free. We cannot free things
during unpin since it can be pinned again and we need to find that range by
going through the list. We also cannot get rid of any lists. Since if
something is re-pinned, we need to find it and find out if it was purged. We
also need the list for knowing what was unpinned so the shrinker works.

By the way, all this may be going away quite soon (the whole driver) as I
said, so just give it a little bit of time.

I am happy to fix it soon if that's not the case (which I should know soon -
like a couple of weeks) but I'd like to hold off till then.

> By the way, why not to check range_alloc() failure before calling range_shrink() ?

That would be a nice thing to do. Send a patch?

thanks,

 - Joel