linux-kernel - [RFC PATCH 0/2] mm/vmstat: Reduce zone lock hold time when reading /proc/pagetypeinfo

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]

Message-Id: <20191023102737.32274-1-mhocko@kernel.org>
Date:   Wed, 23 Oct 2019 12:27:35 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...e.de>, Waiman Long <longman@...hat.com>
Cc:     Johannes Weiner <hannes@...xchg.org>, Roman Gushchin <guro@...com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
        Jann Horn <jannh@...gle.com>, Song Liu <songliubraving@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rafael Aquini <aquini@...hat.com>, <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: [RFC PATCH 0/2] mm/vmstat: Reduce zone lock hold time when reading /proc/pagetypeinfo

On Wed 23-10-19 10:56:08, Mel Gorman wrote:
> On Wed, Oct 23, 2019 at 11:04:22AM +0200, Michal Hocko wrote:
> > So can we go with this to address the security aspect of this and have
> > something trivial to backport.
> > 
> 
> Yes.

Ok, patch 1 in reply to this email.

> > > > > There is a free_area structure associated with each page order. There
> > > > > is also a nr_free count within the free_area for all the different
> > > > > migration types combined. Tracking the number of free list entries
> > > > > for each migration type will probably add some overhead to the fast
> > > > > paths like moving pages from one migration type to another which may
> > > > > not be desirable.
> > > > 
> > > > Have you tried to measure that overhead?
> > > >  
> > > 
> > > I would prefer this option not be taken. It would increase the cost of
> > > watermark calculations which is a relatively fast path.
> > 
> > Is the change for the wmark check going to require more than
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index c0b2e0306720..5d95313ba4a5 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3448,9 +3448,6 @@ bool __zone_watermark_ok(struct zone *z, unsigned int order, unsigned long mark,
> >  		struct free_area *area = &z->free_area[o];
> >  		int mt;
> >  
> > -		if (!area->nr_free)
> > -			continue;
> > -
> >  		for (mt = 0; mt < MIGRATE_PCPTYPES; mt++) {
> >  			if (!free_area_empty(area, mt))
> >  				return true;
> > 
> > Is this really going to be visible in practice? Sure we are going to do
> > more checks but most orders tend to have at least some memory in a
> > reasonably balanced system and we can hardly expect an optimal
> > allocation path on those that are not.
> >  
> 
> You also have to iterate over them all later in the same function.  The the
> free counts are per migrate type then they would have to be iterated over
> every time.
> 
> Similarly, there would be multiple places where all the counters would
> have to be iterated -- find_suitable_fallback, show_free_areas,
> fast_isolate_freepages, fill_contig_page_info, zone_init_free_lists etc.
> 
> It'd be a small cost but given that it's aimed at fixing a problem with
> reading pagetypeinfo, is it really worth it? I don't think so.

Fair enough.

[...]
> > As pointed out in other email. The problem with this patch is that it
> > hasn't really removed the iteration over the whole free_list which is
> > the primary problem. So I think that we should either consider this a
> > non-issue and make it "admin knows this is potentially expensive" or do
> > something like Andrew was suggesting if we do not want to change the
> > nr_free accounting.
> > 
> 
> Again, the cost is when reading a proc file. From what Andrew said,
> the lock is necessary to safely walk the list but if anything. I would
> be ok with limiting the length of the walk but honestly, I would also
> be ok with simply deleting the proc file. The utility for debugging a
> problem with it is limited now (it was more important when fragmentation
> avoidance was first introduced) and there is little an admin can do with
> the information. I can't remember the last time I asked for the contents
> of the file when trying to debug a problem. There is a possibility that
> someone will complain but I'm not aware of any utility that reads the
> information and does something useful with it. In the unlikely event
> something breaks, the file can be re-added with a limited walk.

I went with a bound to the pages iteratred over in the free_list. See
patch 2.

-- 
Michal Hocko
SUSE Labs