linux-kernel - Re: [PATCH -mm -V2] mm, swap: Fix race between swapoff and some swap operations

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

Date:   Thu, 14 Dec 2017 21:49:29 -0800
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     "Huang, Ying" <ying.huang@...el.com>
Cc:     Michal Hocko <mhocko@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Hugh Dickins <hughd@...gle.com>,
        Minchan Kim <minchan@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Shaohua Li <shli@...com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        JXrXme Glisse <jglisse@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        David Rientjes <rientjes@...gle.com>,
        Rik van Riel <riel@...hat.com>, Jan Kara <jack@...e.cz>,
        Dave Jiang <dave.jiang@...el.com>,
        Aaron Lu <aaron.lu@...el.com>
Subject: Re: [PATCH -mm -V2] mm, swap: Fix race between swapoff and some swap
 operations

On Fri, Dec 15, 2017 at 09:33:03AM +0800, Huang, Ying wrote:
> Michal Hocko <mhocko@...nel.org> writes:
> 
> > On Thu 14-12-17 21:38:32, Huang, Ying wrote:
> >> From: Huang Ying <ying.huang@...el.com>
> >> 
> >> When the swapin is performed, after getting the swap entry information
> >> from the page table, system will swap in the swap entry, without any
> >> lock held to prevent the swap device from being swapoff.  This may
> >> cause the race like below,
> >> 
> >> CPU 1				CPU 2
> >> -----				-----
> >> 				do_swap_page
> >> 				  swapin_readahead
> >> 				    __read_swap_cache_async
> >> swapoff				      swapcache_prepare
> >>   p->swap_map = NULL		        __swap_duplicate
> >> 					  p->swap_map[?] /* !!! NULL pointer access */
> >> 
> >> Because swap off is usually done when system shutdown only, the race
> >> may not hit many people in practice.  But it is still a race need to
> >> be fixed.
> >> 
> >> To fix the race, get_swap_device() is added to prevent swap device
> >> from being swapoff until put_swap_device() is called.  When
> >> get_swap_device() is called, the caller should have some locks (like
> >> PTL, page lock, or swap_info_struct->lock) held to guarantee the swap
> >> entry is valid, or check the origin of swap entry again to make sure
> >> the swap device hasn't been swapoff already.
> >> 
> >> Because swapoff() is very race code path, to make the normal path runs
> >
> > s@...e@...e@ I suppose
> 
> Oops, thanks for pointing this out!
> 
> >> as fast as possible, SRCU instead of reference count is used to
> >> implement get/put_swap_device().  From get_swap_device() to
> >> put_swap_device(), the reader side of SRCU is locked, so
> >> synchronize_srcu() in swapoff() will wait until put_swap_device() is
> >> called.
> >
> > It is quite unfortunate to pull SRCU as a dependency to the core kernel.
> > Different attempts to do this have failed in the past. This one is
> > slightly different though because I would suspect that those tiny
> > systems do not configure swap. But who knows, maybe they do.
> 
> I remember Paul said there is a tiny implementation of SRCU which can
> fit this requirement.
> 
> Hi, Paul, whether my memory is correct?

Yes, if you build with CONFIG_SMP=n, then you will get Tiny SRCU, which
is quite compact.

							Thanx, Paul

> > Anyway, if you are worried about performance then I would expect some
> > numbers to back that worry. So why don't simply start with simpler
> > ref count based and then optimize it later based on some actual numbers.
> 
> My -V1 is based on ref count.  I think the performance difference should
> be not measurable.  The idea is that swapoff() is so rare, so we should
> accelerate normal path as much as possible, even if this will cause slow
> down in swapoff.  If we cannot use SRCU in the end, we may try RCU,
> preempt off (for stop_machine()), etc.
> 
> > Btw. have you considered pcp refcount framework. I would suspect that
> > this would give you close to SRCU performance.
> 
> No.  I think pcp refcount doesn't fit here.  You should hold a initial
> refcount for pcp refcount, it isn't the case here.
> 
> Best Regards,
> Huang, Ying
>