From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753860AbdDKOUU (ORCPT ); Tue, 11 Apr 2017 10:20:20 -0400 Received: from mx2.suse.de ([195.135.220.15]:50535 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753477AbdDKOUG (ORCPT ); Tue, 11 Apr 2017 10:20:06 -0400 Date: Tue, 11 Apr 2017 16:19:56 +0200 From: Michal Hocko To: Kees Cook Cc: Christoph Lameter , Andrew Morton , Pekka Enberg , David Rientjes , Joonsoo Kim , Linux-MM , LKML Subject: Re: [PATCH] mm: Add additional consistency check Message-ID: <20170411141956.GP6729@dhcp22.suse.cz> References: <20170404113022.GC15490@dhcp22.suse.cz> <20170404151600.GN15132@dhcp22.suse.cz> <20170404194220.GT15132@dhcp22.suse.cz> <20170404201334.GV15132@dhcp22.suse.cz> <20170411134618.GN6729@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue 11-04-17 07:14:01, Kees Cook wrote: > On Tue, Apr 11, 2017 at 6:46 AM, Michal Hocko wrote: > > On Mon 10-04-17 21:58:22, Kees Cook wrote: > >> On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko wrote: > >> > On Tue 04-04-17 14:58:06, Cristopher Lameter wrote: > >> >> On Tue, 4 Apr 2017, Michal Hocko wrote: > >> >> > >> >> > On Tue 04-04-17 14:13:06, Cristopher Lameter wrote: > >> >> > > On Tue, 4 Apr 2017, Michal Hocko wrote: > >> >> > > > >> >> > > > Yes, but we do not have to blow the kernel, right? Why cannot we simply > >> >> > > > leak that memory? > >> >> > > > >> >> > > Because it is a serious bug to attempt to free a non slab object using > >> >> > > slab operations. This is often the result of memory corruption, coding > >> >> > > errs etc. The system needs to stop right there. > >> >> > > >> >> > Why when an alternative is a memory leak? > >> >> > >> >> Because the slab allocators fail also in case you free an object multiple > >> >> times etc etc. Continuation is supported by enabling a special resiliency > >> >> feature via the kernel command line. The alternative is selectable but not > >> >> the default. > >> > > >> > I disagree! We should try to continue as long as we _know_ that the > >> > internal state of the allocator is still consistent and a further > >> > operation will not spread the corruption even more. This is clearly not > >> > the case for an invalid pointer to kfree. > >> > > >> > I can see why checking for an early allocator corruption is not always > >> > feasible and you can only detect after-the-fact but this is not the case > >> > here and putting your system down just because some buggy code is trying > >> > to free something it hasn't allocated is not really useful. I completely > >> > agree with Linus that we overuse BUG way too much and this is just > >> > another example of it. > >> > >> Instead of the proposed BUG here, what's the correct "safe" return value? > > > > I would assume that _you_ as the one who proposes the change would take > > some time to read and understand the code and know this answer. This is > > how we do changes to the kernel: have an objective, understand the code > > and generate the patch. > > > > I am really sad that this particular patch has shown that you didn't > > bother to consider the later part and blindly applied something that you > > haven't thought through properly. Please try harder next time. > > Our objectives are different: I want the kernel to immediately stop > when corruption is detected. Since others are interested in making it > survivable, I was hoping to get a hint about what such an improvement > would look like. I do not think sprinkling BUG_ONs will help that objective. And BUG_ON under IRQ disable is likely not helping an error survivable... > Instead this condescending attitude, can you instead > provide constructive help that will get our users closer to the safe > kernel operation we're all interested in? I would do something like... --- diff --git a/mm/slab.c b/mm/slab.c index bd63450a9b16..87c99a5e9e18 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {} static int slab_max_order = SLAB_MAX_ORDER_LO; static bool slab_max_order_set __initdata; +static inline struct kmem_cache *page_to_cache(struct page *page) +{ + return page->slab_cache; +} + static inline struct kmem_cache *virt_to_cache(const void *obj) { struct page *page = virt_to_head_page(obj); - return page->slab_cache; + return page_to_cache(page); } static inline void *index_to_obj(struct kmem_cache *cache, struct page *page, @@ -3813,14 +3818,18 @@ void kfree(const void *objp) { struct kmem_cache *c; unsigned long flags; + struct page *page; trace_kfree(_RET_IP_, objp); if (unlikely(ZERO_OR_NULL_PTR(objp))) return; + page = virt_to_head_page(obj); + if (CHECK_DATA_CORRUPTION(!PageSlab(page))) + return; local_irq_save(flags); kfree_debugcheck(objp); - c = virt_to_cache(objp); + c = page_to_cache(page); debug_check_no_locks_freed(objp, c->object_size); debug_check_no_obj_freed(objp, c->object_size); -- Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f69.google.com (mail-wm0-f69.google.com [74.125.82.69]) by kanga.kvack.org (Postfix) with ESMTP id 05D956B03AE for ; Tue, 11 Apr 2017 10:20:02 -0400 (EDT) Received: by mail-wm0-f69.google.com with SMTP id t7so157396wmd.20 for ; Tue, 11 Apr 2017 07:20:01 -0700 (PDT) Received: from mx2.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id s25si13124034wrb.187.2017.04.11.07.20.00 for (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 11 Apr 2017 07:20:00 -0700 (PDT) Date: Tue, 11 Apr 2017 16:19:56 +0200 From: Michal Hocko Subject: Re: [PATCH] mm: Add additional consistency check Message-ID: <20170411141956.GP6729@dhcp22.suse.cz> References: <20170404113022.GC15490@dhcp22.suse.cz> <20170404151600.GN15132@dhcp22.suse.cz> <20170404194220.GT15132@dhcp22.suse.cz> <20170404201334.GV15132@dhcp22.suse.cz> <20170411134618.GN6729@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Kees Cook Cc: Christoph Lameter , Andrew Morton , Pekka Enberg , David Rientjes , Joonsoo Kim , Linux-MM , LKML On Tue 11-04-17 07:14:01, Kees Cook wrote: > On Tue, Apr 11, 2017 at 6:46 AM, Michal Hocko wrote: > > On Mon 10-04-17 21:58:22, Kees Cook wrote: > >> On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko wrote: > >> > On Tue 04-04-17 14:58:06, Cristopher Lameter wrote: > >> >> On Tue, 4 Apr 2017, Michal Hocko wrote: > >> >> > >> >> > On Tue 04-04-17 14:13:06, Cristopher Lameter wrote: > >> >> > > On Tue, 4 Apr 2017, Michal Hocko wrote: > >> >> > > > >> >> > > > Yes, but we do not have to blow the kernel, right? Why cannot we simply > >> >> > > > leak that memory? > >> >> > > > >> >> > > Because it is a serious bug to attempt to free a non slab object using > >> >> > > slab operations. This is often the result of memory corruption, coding > >> >> > > errs etc. The system needs to stop right there. > >> >> > > >> >> > Why when an alternative is a memory leak? > >> >> > >> >> Because the slab allocators fail also in case you free an object multiple > >> >> times etc etc. Continuation is supported by enabling a special resiliency > >> >> feature via the kernel command line. The alternative is selectable but not > >> >> the default. > >> > > >> > I disagree! We should try to continue as long as we _know_ that the > >> > internal state of the allocator is still consistent and a further > >> > operation will not spread the corruption even more. This is clearly not > >> > the case for an invalid pointer to kfree. > >> > > >> > I can see why checking for an early allocator corruption is not always > >> > feasible and you can only detect after-the-fact but this is not the case > >> > here and putting your system down just because some buggy code is trying > >> > to free something it hasn't allocated is not really useful. I completely > >> > agree with Linus that we overuse BUG way too much and this is just > >> > another example of it. > >> > >> Instead of the proposed BUG here, what's the correct "safe" return value? > > > > I would assume that _you_ as the one who proposes the change would take > > some time to read and understand the code and know this answer. This is > > how we do changes to the kernel: have an objective, understand the code > > and generate the patch. > > > > I am really sad that this particular patch has shown that you didn't > > bother to consider the later part and blindly applied something that you > > haven't thought through properly. Please try harder next time. > > Our objectives are different: I want the kernel to immediately stop > when corruption is detected. Since others are interested in making it > survivable, I was hoping to get a hint about what such an improvement > would look like. I do not think sprinkling BUG_ONs will help that objective. And BUG_ON under IRQ disable is likely not helping an error survivable... > Instead this condescending attitude, can you instead > provide constructive help that will get our users closer to the safe > kernel operation we're all interested in? I would do something like... --- diff --git a/mm/slab.c b/mm/slab.c index bd63450a9b16..87c99a5e9e18 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {} static int slab_max_order = SLAB_MAX_ORDER_LO; static bool slab_max_order_set __initdata; +static inline struct kmem_cache *page_to_cache(struct page *page) +{ + return page->slab_cache; +} + static inline struct kmem_cache *virt_to_cache(const void *obj) { struct page *page = virt_to_head_page(obj); - return page->slab_cache; + return page_to_cache(page); } static inline void *index_to_obj(struct kmem_cache *cache, struct page *page, @@ -3813,14 +3818,18 @@ void kfree(const void *objp) { struct kmem_cache *c; unsigned long flags; + struct page *page; trace_kfree(_RET_IP_, objp); if (unlikely(ZERO_OR_NULL_PTR(objp))) return; + page = virt_to_head_page(obj); + if (CHECK_DATA_CORRUPTION(!PageSlab(page))) + return; local_irq_save(flags); kfree_debugcheck(objp); - c = virt_to_cache(objp); + c = page_to_cache(page); debug_check_no_locks_freed(objp, c->object_size); debug_check_no_obj_freed(objp, c->object_size); -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org