From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753003AbbC3IyI (ORCPT ); Mon, 30 Mar 2015 04:54:08 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:12384 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752647AbbC3IyF (ORCPT ); Mon, 30 Mar 2015 04:54:05 -0400 X-AuditID: cbfec7f4-b7f126d000001e9a-5f-55190e81a9e9 Message-id: <55190F23.4020009@samsung.com> Date: Mon, 30 Mar 2015 11:53:55 +0300 From: Andrey Ryabinin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-version: 1.0 To: David Rientjes Cc: Andrew Morton , Dave Kleikamp , Christoph Hellwig , Sebastian Ott , Mikulas Patocka , Catalin Marinas , LKML , "linux-mm@kvack.org" , jfs-discussion@lists.sourceforge.net Subject: Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator References: In-reply-to: Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrILMWRmVeSWpSXmKPExsVy+t/xy7qNfJKhBncaNSzmrF/DZvF+WQ+j xcrVR5ksLn5sZbK4vGsOm8W9Nf9ZLSZ2XGWyaFuykcniVNdhRovbd34wO3B5rJm3htFjwaZS j02rOtk8Nn2axO5xYsZvFo8HhzazeOxe8JnJY/fNBjaP9/uusnl83iQXwBXFZZOSmpNZllqk b5fAlfFiw0z2gibJiu9bHzA2MJ4X6mLk5JAQMJGY+6+VDcIWk7hwbz2QzcUhJLCUUWLZlv+s XYwcQE4zk8S8GpAaXgEtidWPDrOC2CwCqhLL7yxmBLHZBPQk/s3azgZSLioQIXH7MidEuaDE j8n3WEBsEQENibYZ/8HGMwu8Z5I4dOA8WEJYIFhi68NrLBB7pzFJNE+Ywg4yiFPAR+LYfk4Q kxlo/v2LWiDlzALyEpvXvGWewCgwC8mKWQhVs5BULWBkXsUomlqaXFCclJ5rqFecmFtcmpeu l5yfu4kREi1fdjAuPmZ1iFGAg1GJh5djpkSoEGtiWXFl7iFGCQ5mJRFeS17JUCHelMTKqtSi /Pii0pzU4kOMTBycUg2MK1fWt13fGdmme8nnyrOJqw+94hFc6XP28/QV2lEhb5KMLvPbR0WY LuvaqVGx8e2d61ckzimIapT+Wip1uOfYxuK7Ry67rJu867PD1ug9P8sauCQDD6mbdpz9IOAe eVN+z4wnSqo7TkctbA6sOPPFQ17uqkHEjJNSDge2Fs8tiOU7/+TX1hsmZ5VYijMSDbWYi4oT AZ5XpzF0AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/27/2015 01:50 AM, David Rientjes wrote: > On Thu, 26 Mar 2015, Andrey Ryabinin wrote: > >>> +static void check_element(mempool_t *pool, void *element) >>> +{ >>> + /* Mempools backed by slab allocator */ >>> + if (pool->free == mempool_free_slab || pool->free == mempool_kfree) >>> + __check_element(pool, element, ksize(element)); >>> + >>> + /* Mempools backed by page allocator */ >>> + if (pool->free == mempool_free_pages) { >>> + int order = (int)(long)pool->pool_data; >>> + void *addr = page_address(element); >>> + >>> + __check_element(pool, addr, 1UL << (PAGE_SHIFT + order)); >>> } >>> } >>> >>> -static void poison_slab_element(mempool_t *pool, void *element) >>> +static void __poison_element(void *element, size_t size) >>> { >>> - if (pool->alloc == mempool_alloc_slab || >>> - pool->alloc == mempool_kmalloc) { >>> - size_t size = ksize(element); >>> - u8 *obj = element; >>> + u8 *obj = element; >>> + >>> + memset(obj, POISON_FREE, size - 1); >>> + obj[size - 1] = POISON_END; >>> +} >>> + >>> +static void poison_element(mempool_t *pool, void *element) >>> +{ >>> + /* Mempools backed by slab allocator */ >>> + if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc) >>> + __poison_element(element, ksize(element)); >>> + >>> + /* Mempools backed by page allocator */ >>> + if (pool->alloc == mempool_alloc_pages) { >>> + int order = (int)(long)pool->pool_data; >>> + void *addr = page_address(element); >>> >>> - memset(obj, POISON_FREE, size - 1); >>> - obj[size - 1] = POISON_END; >>> + __poison_element(addr, 1UL << (PAGE_SHIFT + order)); >> >> I think, it would be better to use kernel_map_pages() here and in >> check_element(). > > Hmm, interesting suggestion. > >> This implies that poison_element()/check_element() has to be moved out of >> CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON ifdef (keeping only slab >> poisoning under this ifdef). > > The mempool poisoning introduced here is really its own poisoning built on > top of whatever the mempool allocator is. Otherwise, it would have called > into the slab subsystem to do the poisoning and include any allocated > space beyond the object size itself. Perhaps, that would be a good thing to do. I mean it makes sense to check redzone for corruption. > Mempool poisoning is agnostic to the > underlying memory just like the chain of elements is, mempools don't even > store size. > > We don't have a need to set PAGE_EXT_DEBUG_POISON on these pages sitting > in the reserved pool, nor do we have a need to do kmap_atomic() since it's > already mapped and must be mapped to be on the reserved pool, which is > handled by mempool_free(). > Well, yes. But this applies only to architectures that don't have ARCH_SUPPORTS_DEBUG_PAGEALLOC. The rest of arches will only benefit from this as kernel_map_pages() potentially could find more bugs.