linux-kernel - RE: [PATCH] lowmemorykiller: prevent multiple instances of low memory killer

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]

Date:	Wed, 24 Apr 2013 10:33:44 +0200
From:	"Dolkow, Snild" <Snild.Dolkow@...ymobile.com>
To:	'David Rientjes' <rientjes@...gle.com>,
	"Anderö, Oskar" 
	<Oskar.Andero@...ymobile.com>
CC:	Dan Carpenter <dan.carpenter@...cle.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	Brian Swetland <swetland@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Lekanovic, Radovan" <Radovan.Lekanovic@...ymobile.com>
Subject: RE: [PATCH] lowmemorykiller: prevent multiple instances of low
 memory killer

> No, it's not.  This is controlled higher in shrink_slab() by this:
>
>	max_pass = do_shrinker_shrink(shrinker, shrink, 0);
>	if (max_pass <= 0)
>		continue;
>

Yes, but the later calls will still not handle other negative values as failures, and there is a chance that more than one thread will get past that first check. 

286		nr_before = do_shrinker_shrink(shrinker, shrink, 0);
287		shrink_ret = do_shrinker_shrink(shrinker, shrink,
288						batch_size);
289		if (shrink_ret == -1)
290			break;
291		if (shrink_ret < nr_before)
292			ret += nr_before - shrink_ret;

If, for example, nr_before happens to be -2 and shrink_ret happens to be -1000 here, we're going to erroneously increase ret by 998.

> and your patch is implemented incorrectly, i.e. it does not return 
> LMK_BUSY if the spinlock is contended which needlessly recalls the 
> shrinker later.

It's worth noting that the LMK has a fastpath for the nr_to_scan=0 case, like the shrinker.h comment recommends. And nr_to_scan=0 is used to query the cache size, so it seems like a good idea to return successfully whenever we can.

> You have a couple of options:
>
>  - return -1 when the spinlock is contended immediately when
>    !sc->nr_to_scan (although it should really be a cmpxchg since a
>    spinlock isn't needed), or

This comes with the risk of nr_before being -1, and shrink_ret being positive. In that case, we will have sent a kill signal, but we're not increasing ret. Not a catastrophe, AFAICT, but not fantastic either.

>  - protect the for_each_process() loop in lowmem_shrink() with an
>    actual spinlock that will detect any previously killed process
>    since it will have the TIF_MEMDIE bit set.

We expect that killing one process will be enough, so spinning seems like a waste of time. If one process wasn't enough, the LMK will trigger again soon.

//Snild
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/