Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1620899 > unrolled thread

Re: [PATCH] mm: Add additional consistency check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-11 07:00 +0200
    Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-11 15:50 +0200
      Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-11 16:20 +0200
        Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-11 16:30 +0200
          Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-11 18:10 +0200
          Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-11 18:20 +0200
            Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-11 18:30 +0200
            Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-11 18:30 +0200
              Re: [PATCH] mm: Add additional consistency check Kees Cook <keescook@chromium.org> - 2017-04-11 18:40 +0200
          Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-11 18:20 +0200
            Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-11 18:50 +0200
              Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-11 20:10 +0200
                Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-11 20:40 +0200
                  Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-11 20:50 +0200
                    Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-11 21:00 +0200
                      Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-11 21:00 +0200
                        Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-11 21:40 +0200
                          Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-17 17:30 +0200
                            Re: [PATCH] mm: Add additional consistency check Michal Hocko <mhocko@kernel.org> - 2017-04-18 08:50 +0200
                              Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-18 15:40 +0200
                              Re: [PATCH] mm: Add additional consistency check Christoph Lameter <cl@linux.com> - 2017-04-18 15:40 +0200

Page 1 of 2  [1] 2  Next page →

#1620899 — Re: [PATCH] mm: Add additional consistency check

On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko <mhocko@kernel.org> 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?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1621282

On Mon 10-04-17 21:58:22, Kees Cook wrote:
> On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko <mhocko@kernel.org> 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.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1621337

On Tue, Apr 11, 2017 at 6:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Mon 10-04-17 21:58:22, Kees Cook wrote:
>> On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko <mhocko@kernel.org> 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. 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?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1621353

On Tue 11-04-17 07:14:01, Kees Cook wrote:
> On Tue, Apr 11, 2017 at 6:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Mon 10-04-17 21:58:22, Kees Cook wrote:
> >> On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko <mhocko@kernel.org> 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

[toc] | [prev] | [next] | [standalone]

#1621439

On Tue, Apr 11, 2017 at 7:19 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 11-04-17 07:14:01, Kees Cook wrote:
>> On Tue, Apr 11, 2017 at 6:46 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Mon 10-04-17 21:58:22, Kees Cook wrote:
>> >> On Tue, Apr 4, 2017 at 1:13 PM, Michal Hocko <mhocko@kernel.org> 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...

Yes, agreed. Handling it cleanly is always better.

>> 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);

Awesome! Thank you very much! I'll play with this.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1621445

On Tue, Apr 11, 2017 at 9:16 AM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
>
>>  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)))
>
> There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
> Consistency checks are configuraable in the slab allocator.
>
> Mentioned that before and got this lecture about data consistency checks.

It seems that enabling the debug checks comes with a non-trivial
performance impact. I'd like to see consistency checks by default so
we can handle intentional heap corruption attacks better. This check
isn't expensive...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1621458

On Tue, 11 Apr 2017, Kees Cook wrote:

> It seems that enabling the debug checks comes with a non-trivial
> performance impact. I'd like to see consistency checks by default so
> we can handle intentional heap corruption attacks better. This check
> isn't expensive...

Note also that these checks can be enabled and disabled at runtime for
each slab cache.

[toc] | [prev] | [next] | [standalone]

#1621459

On Tue, 11 Apr 2017, Kees Cook wrote:

> It seems that enabling the debug checks comes with a non-trivial
> performance impact. I'd like to see consistency checks by default so
> we can handle intentional heap corruption attacks better. This check
> isn't expensive...

Its in a very hot code and frequently used code path.

[toc] | [prev] | [next] | [standalone]

#1621461

On Tue, Apr 11, 2017 at 9:23 AM, Christoph Lameter <cl@linux.com> wrote:
> On Tue, 11 Apr 2017, Kees Cook wrote:
>
>> It seems that enabling the debug checks comes with a non-trivial
>> performance impact. I'd like to see consistency checks by default so
>> we can handle intentional heap corruption attacks better. This check
>> isn't expensive...
>
> Its in a very hot code and frequently used code path.

Yeah, absolutely. All the more reason to make sure the kernel can't be
attacked through it. :) As with the automotive industry analogy[1]
from Konstantin, we need to make sure Linux not only run fast and
efficiently, but also fails gracefully by default.

> Note also that these checks can be enabled and disabled at runtime for
> each slab cache.

Correct, but my understanding is that enabling them through the debug
system ends up being much more expensive than this smaller check. The
debug code is fairly comprehensive, but it's not been designed for
efficient attack detection, etc.

-Kees

[1] http://kernsec.org/files/lss2015/giant-bags-of-mostly-water.pdf

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1621446

On Tue, 11 Apr 2017, Michal Hocko wrote:

>  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)))

There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
Consistency checks are configuraable in the slab allocator.

Mentioned that before and got this lecture about data consistency checks.

[toc] | [prev] | [next] | [standalone]

#1621466

On Tue 11-04-17 11:16:42, Cristopher Lameter wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
> 
> >  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)))
> 
> There is a flag SLAB_DEBUG_OBJECTS that is available for this check.

Which is way too late, at least for the kfree path. page->slab_cache
on anything else than PageSlab is just a garbage. And my understanding
of the patch objective is to stop those from happening.

> Consistency checks are configuraable in the slab allocator.

and they have to be compiled in (at least for SLAB) AFAIR.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1621521

On Tue, 11 Apr 2017, Michal Hocko wrote:

> >
> > There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
>
> Which is way too late, at least for the kfree path. page->slab_cache
> on anything else than PageSlab is just a garbage. And my understanding
> of the patch objective is to stop those from happening.

We are looking here at SLAB. SLUB code can legitimately have a compound
page there because large allocations fallback to the page allocator.

Garbage would be attempting to free a page that has !PageSLAB set but also
is no compound page. That condition is already checked in kfree() with a
BUG_ON() and that BUG_ON has been there for a long time. Certainly we can
make SLAB consistent if there is no check there already. Slab just
attempts a free on that object which will fail too.

So we are already handling that condition. Why change things? Add a BUG_ON
if you want to make SLAB consistent.

[toc] | [prev] | [next] | [standalone]

#1621540

On Tue 11-04-17 13:03:01, Cristopher Lameter wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
> 
> > >
> > > There is a flag SLAB_DEBUG_OBJECTS that is available for this check.
> >
> > Which is way too late, at least for the kfree path. page->slab_cache
> > on anything else than PageSlab is just a garbage. And my understanding
> > of the patch objective is to stop those from happening.
> 
> We are looking here at SLAB. SLUB code can legitimately have a compound
> page there because large allocations fallback to the page allocator.
> 
> Garbage would be attempting to free a page that has !PageSLAB set but also
> is no compound page. That condition is already checked in kfree() with a
> BUG_ON() and that BUG_ON has been there for a long time.

Are you talking about SLAB or SLUB here?  The only
BUG_ON(PageSlab(page)) in SLAB I can see is in kmem_freepages and that
is way too late because we already rely on cachep which is not
trustworthy. Or am I missing some other place you have in mind?

> Certainly we can
> make SLAB consistent if there is no check there already. Slab just
> attempts a free on that object which will fail too.
> 
> So we are already handling that condition. Why change things? Add a BUG_ON
> if you want to make SLAB consistent.

I hate to repeat myself but let me do it for the last time in this
thread. BUG_ON for something that is recoverable is completely
inappropriate. And I consider kfree with a bogus pointer something that
we can easily recover from. There are other cases where the internal
state of the allocator is compromised to the point where continuing is
not possible and BUGing there is acceptable but kfree(garbage) is not
that case. 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1621549

On Tue, 11 Apr 2017, Michal Hocko wrote:

> > So we are already handling that condition. Why change things? Add a BUG_ON
> > if you want to make SLAB consistent.
>
> I hate to repeat myself but let me do it for the last time in this
> thread. BUG_ON for something that is recoverable is completely
> inappropriate. And I consider kfree with a bogus pointer something that
> we can easily recover from. There are other cases where the internal
> state of the allocator is compromised to the point where continuing is
> not possible and BUGing there is acceptable but kfree(garbage) is not
> that case.

kfree(garbage) by the core kernel has so far been taken as a sign of
severe memory corruption and the kernels have been oopsing when this
occurred. This has been that way for a decade or so. kfree() is used by
the allocators and various other core kernel components. If the metadata
of the core kernel is compromised then it is safest to stop right there.

If you want to change things then someone has to do some work. What you
are saying is not the way things are implemented. Sorry.

Making both allocators consistent is ok with me and is a improvement of
the code.

[toc] | [prev] | [next] | [standalone]

#1621554

On Tue 11-04-17 13:44:02, Cristopher Lameter wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
> 
> > > So we are already handling that condition. Why change things? Add a BUG_ON
> > > if you want to make SLAB consistent.
> >
> > I hate to repeat myself but let me do it for the last time in this
> > thread. BUG_ON for something that is recoverable is completely
> > inappropriate. And I consider kfree with a bogus pointer something that
> > we can easily recover from. There are other cases where the internal
> > state of the allocator is compromised to the point where continuing is
> > not possible and BUGing there is acceptable but kfree(garbage) is not
> > that case.
> 
> kfree(garbage) by the core kernel has so far been taken as a sign of
> severe memory corruption and the kernels have been oopsing when this
> occurred. This has been that way for a decade or so.

which doesn't make it a valid decision. We just overuse BUG*

> kfree() is used by
> the allocators and various other core kernel components. If the metadata
> of the core kernel is compromised then it is safest to stop right there.
> 
> If you want to change things then someone has to do some work. What you
> are saying is not the way things are implemented. Sorry.

I didn't say anything like that. Hence the proposed patch which still
needs some more thinking and evaluation.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1621558

On Tue, 11 Apr 2017, Michal Hocko wrote:

> I didn't say anything like that. Hence the proposed patch which still
> needs some more thinking and evaluation.

This patch does not even affect kfree(). Could you start another
discussion thread where you discuss your suggestions for the changes in
the allocators and how we could go about this?

[toc] | [prev] | [next] | [standalone]

#1621581

On Tue 11-04-17 13:59:44, Cristopher Lameter wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
> 
> > I didn't say anything like that. Hence the proposed patch which still
> > needs some more thinking and evaluation.
> 
> This patch does not even affect kfree().

Ehm? Are we even talking about the same thing? The whole discussion was
to catch invalid pointers to _kfree_ and why BUG* is not the best way to
handle that. 

> Could you start another
> discussion thread where you discuss your suggestions for the changes in
> the allocators and how we could go about this?

I presume Kees will pursue
http://lkml.kernel.org/r/20170411141956.GP6729@dhcp22.suse.cz or
something along those lines.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1624703

On Tue, 11 Apr 2017, Michal Hocko wrote:

> On Tue 11-04-17 13:59:44, Cristopher Lameter wrote:
> > On Tue, 11 Apr 2017, Michal Hocko wrote:
> >
> > > I didn't say anything like that. Hence the proposed patch which still
> > > needs some more thinking and evaluation.
> >
> > This patch does not even affect kfree().
>
> Ehm? Are we even talking about the same thing? The whole discussion was
> to catch invalid pointers to _kfree_ and why BUG* is not the best way to
> handle that.

The patch does not do that. See my review. Invalid points to kfree are
already caught with a bug on. See kfree in mm/slub.c

[toc] | [prev] | [next] | [standalone]

#1625073

On Mon 17-04-17 10:22:29, Cristopher Lameter wrote:
> On Tue, 11 Apr 2017, Michal Hocko wrote:
> 
> > On Tue 11-04-17 13:59:44, Cristopher Lameter wrote:
> > > On Tue, 11 Apr 2017, Michal Hocko wrote:
> > >
> > > > I didn't say anything like that. Hence the proposed patch which still
> > > > needs some more thinking and evaluation.
> > >
> > > This patch does not even affect kfree().
> >
> > Ehm? Are we even talking about the same thing? The whole discussion was
> > to catch invalid pointers to _kfree_ and why BUG* is not the best way to
> > handle that.
> 
> The patch does not do that. See my review. Invalid points to kfree are
> already caught with a bug on. See kfree in mm/slub.c

Are you even reading those emails? First of all we are talking about
slab here. Secondly I've already pointed out that the BUG_ON(!PageSlab)
in kmem_freepages is already too late because we do operate on a
potential garbage from invalid page...
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1625326

On Tue, 18 Apr 2017, Michal Hocko wrote:

> Are you even reading those emails? First of all we are talking about
> slab here. Secondly I've already pointed out that the BUG_ON(!PageSlab)
> in kmem_freepages is already too late because we do operate on a
> potential garbage from invalid page...

Before I forget:

1. The patch affects both slab and slub since it patches mm/slab.h and is
called by both allocators.

2. The check in the patch we are discussing here when calling
kmem_cache_free() will be executing before kmem_freepages() is called
in slab.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web