[PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

From: Jann Horn <jannh@google.com>

[ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]

Having two gigantic arrays that must manually be kept in sync, including
ifdefs, isn't exactly robust.  To make it easier to catch such issues in
the future, add a BUILD_BUG_ON().

Link: http://lkml.kernel.org/r/20181001143138.95119-3-jannh@google.com
Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Roman Gushchin <guro@fb.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Christoph Lameter <clameter@sgi.com>
Cc: Kemi Wang <kemi.wang@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 mm/vmstat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/vmstat.c b/mm/vmstat.c
index 4590aa42b6cd..792374f7088f 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
 	stat_items_size += sizeof(struct vm_event_state);
 #endif
 
+	BUILD_BUG_ON(stat_items_size !=
+		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
 	v = kmalloc(stat_items_size, GFP_KERNEL);
 	m->private = v;
 	if (!v)
-- 
2.17.1

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Andrew Morton]

On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:

> From: Jann Horn <jannh@google.com>
> 
> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
> 
> Having two gigantic arrays that must manually be kept in sync, including
> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
> the future, add a BUILD_BUG_ON().
>
> ...
>
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>  	stat_items_size += sizeof(struct vm_event_state);
>  #endif
>  
> +	BUILD_BUG_ON(stat_items_size !=
> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>  	m->private = v;
>  	if (!v)

I don't think there's any way in which this can make a -stable kernel
more stable!


Generally, I consider -stable in every patch I merge, so for each patch
which doesn't have cc:stable, that tag is missing for a reason.

In other words, your criteria for -stable addition are different from
mine.

And I think your criteria differ from those described in
Documentation/process/stable-kernel-rules.rst.

So... what is your overall thinking on patch selection?

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
>On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
>
>> From: Jann Horn <jannh@google.com>
>>
>> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
>>
>> Having two gigantic arrays that must manually be kept in sync, including
>> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
>> the future, add a BUILD_BUG_ON().
>>
>> ...
>>
>> --- a/mm/vmstat.c
>> +++ b/mm/vmstat.c
>> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>>  	stat_items_size += sizeof(struct vm_event_state);
>>  #endif
>>
>> +	BUILD_BUG_ON(stat_items_size !=
>> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>>  	m->private = v;
>>  	if (!v)
>
>I don't think there's any way in which this can make a -stable kernel
>more stable!
>
>
>Generally, I consider -stable in every patch I merge, so for each patch
>which doesn't have cc:stable, that tag is missing for a reason.
>
>In other words, your criteria for -stable addition are different from
>mine.
>
>And I think your criteria differ from those described in
>Documentation/process/stable-kernel-rules.rst.
>
>So... what is your overall thinking on patch selection?

Indeed, this doesn't fix anything.

My concern is that in the future, we will pull a patch that will cause
the issue described here, and that issue will only be relevant on
stable. It is very hard to debug this, and I suspect that stable kernels
will still pass all their tests with flying colors.

As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
fix outdated vmstat_text") is backported to a kernel that doesn't have
commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").

I also felt safe with this patch since it adds a single BUILD_BUG_ON()
which does nothing during runtime, so the chances it introduces anything
beyond a build regression seemed to be slim to none.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Andrew Morton]

On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:

> On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
> >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
> >
> >> From: Jann Horn <jannh@google.com>
> >>
> >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
> >>
> >> Having two gigantic arrays that must manually be kept in sync, including
> >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
> >> the future, add a BUILD_BUG_ON().
> >>
> >> ...
> >>
> >> --- a/mm/vmstat.c
> >> +++ b/mm/vmstat.c
> >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
> >>  	stat_items_size += sizeof(struct vm_event_state);
> >>  #endif
> >>
> >> +	BUILD_BUG_ON(stat_items_size !=
> >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
> >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
> >>  	m->private = v;
> >>  	if (!v)
> >
> >I don't think there's any way in which this can make a -stable kernel
> >more stable!
> >
> >
> >Generally, I consider -stable in every patch I merge, so for each patch
> >which doesn't have cc:stable, that tag is missing for a reason.
> >
> >In other words, your criteria for -stable addition are different from
> >mine.
> >
> >And I think your criteria differ from those described in
> >Documentation/process/stable-kernel-rules.rst.
> >
> >So... what is your overall thinking on patch selection?
> 
> Indeed, this doesn't fix anything.
> 
> My concern is that in the future, we will pull a patch that will cause
> the issue described here, and that issue will only be relevant on
> stable. It is very hard to debug this, and I suspect that stable kernels
> will still pass all their tests with flying colors.
> 
> As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
> fix outdated vmstat_text") is backported to a kernel that doesn't have
> commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
> 
> I also felt safe with this patch since it adds a single BUILD_BUG_ON()
> which does nothing during runtime, so the chances it introduces anything
> beyond a build regression seemed to be slim to none.

Well OK.  But my question was general and covers basically every
autosel patch which originated in -mm.

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Thu, Nov 15, 2018 at 02:47:19PM -0800, Andrew Morton wrote:
>On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:
>
>> On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
>> >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
>> >
>> >> From: Jann Horn <jannh@google.com>
>> >>
>> >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
>> >>
>> >> Having two gigantic arrays that must manually be kept in sync, including
>> >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
>> >> the future, add a BUILD_BUG_ON().
>> >>
>> >> ...
>> >>
>> >> --- a/mm/vmstat.c
>> >> +++ b/mm/vmstat.c
>> >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>> >>  	stat_items_size += sizeof(struct vm_event_state);
>> >>  #endif
>> >>
>> >> +	BUILD_BUG_ON(stat_items_size !=
>> >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>> >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>> >>  	m->private = v;
>> >>  	if (!v)
>> >
>> >I don't think there's any way in which this can make a -stable kernel
>> >more stable!
>> >
>> >
>> >Generally, I consider -stable in every patch I merge, so for each patch
>> >which doesn't have cc:stable, that tag is missing for a reason.
>> >
>> >In other words, your criteria for -stable addition are different from
>> >mine.
>> >
>> >And I think your criteria differ from those described in
>> >Documentation/process/stable-kernel-rules.rst.
>> >
>> >So... what is your overall thinking on patch selection?
>>
>> Indeed, this doesn't fix anything.
>>
>> My concern is that in the future, we will pull a patch that will cause
>> the issue described here, and that issue will only be relevant on
>> stable. It is very hard to debug this, and I suspect that stable kernels
>> will still pass all their tests with flying colors.
>>
>> As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
>> fix outdated vmstat_text") is backported to a kernel that doesn't have
>> commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
>>
>> I also felt safe with this patch since it adds a single BUILD_BUG_ON()
>> which does nothing during runtime, so the chances it introduces anything
>> beyond a build regression seemed to be slim to none.
>
>Well OK.  But my question was general and covers basically every
>autosel patch which originated in -mm.

Sure. I picked 3 patches that show up on top when I google for AUTOSEL
in linux-mm, maybe they'll be a good example to help me understand why
they were not selected.

This one fixes a case where too few struct pages are allocated when
using mirrorred memory:

	https://marc.info/?l=linux-mm&m=154211933211147&w=2

Race condition with memory hotplug due to missing locks:

	https://marc.info/?l=linux-mm&m=154211934011188&w=2

Raising an OOM event that causes issues in userspace when no OOM has
actually occured:

	https://marc.info/?l=linux-mm&m=154211939811582&w=2


I think that all 3 cases represent a "real" bug users can hit, and I
honestly don't know why they were not tagged for stable.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Thu 15-11-18 18:01:18, Sasha Levin wrote:
> On Thu, Nov 15, 2018 at 02:47:19PM -0800, Andrew Morton wrote:
> > On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:
> > 
> > > On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
> > > >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
> > > >
> > > >> From: Jann Horn <jannh@google.com>
> > > >>
> > > >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
> > > >>
> > > >> Having two gigantic arrays that must manually be kept in sync, including
> > > >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
> > > >> the future, add a BUILD_BUG_ON().
> > > >>
> > > >> ...
> > > >>
> > > >> --- a/mm/vmstat.c
> > > >> +++ b/mm/vmstat.c
> > > >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
> > > >>  	stat_items_size += sizeof(struct vm_event_state);
> > > >>  #endif
> > > >>
> > > >> +	BUILD_BUG_ON(stat_items_size !=
> > > >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
> > > >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
> > > >>  	m->private = v;
> > > >>  	if (!v)
> > > >
> > > >I don't think there's any way in which this can make a -stable kernel
> > > >more stable!
> > > >
> > > >
> > > >Generally, I consider -stable in every patch I merge, so for each patch
> > > >which doesn't have cc:stable, that tag is missing for a reason.
> > > >
> > > >In other words, your criteria for -stable addition are different from
> > > >mine.
> > > >
> > > >And I think your criteria differ from those described in
> > > >Documentation/process/stable-kernel-rules.rst.
> > > >
> > > >So... what is your overall thinking on patch selection?
> > > 
> > > Indeed, this doesn't fix anything.
> > > 
> > > My concern is that in the future, we will pull a patch that will cause
> > > the issue described here, and that issue will only be relevant on
> > > stable. It is very hard to debug this, and I suspect that stable kernels
> > > will still pass all their tests with flying colors.
> > > 
> > > As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
> > > fix outdated vmstat_text") is backported to a kernel that doesn't have
> > > commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
> > > 
> > > I also felt safe with this patch since it adds a single BUILD_BUG_ON()
> > > which does nothing during runtime, so the chances it introduces anything
> > > beyond a build regression seemed to be slim to none.
> > 
> > Well OK.  But my question was general and covers basically every
> > autosel patch which originated in -mm.
> 
> Sure. I picked 3 patches that show up on top when I google for AUTOSEL
> in linux-mm, maybe they'll be a good example to help me understand why
> they were not selected.
> 
> This one fixes a case where too few struct pages are allocated when
> using mirrorred memory:
> 
> 	https://marc.info/?l=linux-mm&m=154211933211147&w=2

Let me quote "I found this bug by reading the code." I do not think
anybody has ever seen this in practice.

> Race condition with memory hotplug due to missing locks:
> 
> 	https://marc.info/?l=linux-mm&m=154211934011188&w=2

Memory hotplug locking is dubious at best and this patch doesn't really
fix it. It fixes a theoretical problem. I am not aware anybody would be
hitting in practice. We need to rework the locking quite extensively.

> Raising an OOM event that causes issues in userspace when no OOM has
> actually occured:
> 
> 	https://marc.info/?l=linux-mm&m=154211939811582&w=2

The patch makes sense I just do not think this is a stable material. The
semantic of the event was and still is suboptimal.

> I think that all 3 cases represent a "real" bug users can hit, and I
> honestly don't know why they were not tagged for stable.

It would be much better to ask in the respective email thread rather
than spamming mailing with AUTOSEL patches which rarely get any
attention.

We have been through this discussion several times already and I thought
we have agreed that those subsystems which are seriously considering stable
are opted out from the AUTOSEL automagic. Has anything changed in that
regards.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
>On Thu 15-11-18 18:01:18, Sasha Levin wrote:
>> On Thu, Nov 15, 2018 at 02:47:19PM -0800, Andrew Morton wrote:
>> > On Thu, 15 Nov 2018 17:37:18 -0500 Sasha Levin <sashal@kernel.org> wrote:
>> >
>> > > On Thu, Nov 15, 2018 at 02:08:10PM -0800, Andrew Morton wrote:
>> > > >On Tue, 13 Nov 2018 00:52:51 -0500 Sasha Levin <sashal@kernel.org> wrote:
>> > > >
>> > > >> From: Jann Horn <jannh@google.com>
>> > > >>
>> > > >> [ Upstream commit f0ecf25a093fc0589f0a6bc4c1ea068bbb67d220 ]
>> > > >>
>> > > >> Having two gigantic arrays that must manually be kept in sync, including
>> > > >> ifdefs, isn't exactly robust.  To make it easier to catch such issues in
>> > > >> the future, add a BUILD_BUG_ON().
>> > > >>
>> > > >> ...
>> > > >>
>> > > >> --- a/mm/vmstat.c
>> > > >> +++ b/mm/vmstat.c
>> > > >> @@ -1189,6 +1189,8 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
>> > > >>  	stat_items_size += sizeof(struct vm_event_state);
>> > > >>  #endif
>> > > >>
>> > > >> +	BUILD_BUG_ON(stat_items_size !=
>> > > >> +		     ARRAY_SIZE(vmstat_text) * sizeof(unsigned long));
>> > > >>  	v = kmalloc(stat_items_size, GFP_KERNEL);
>> > > >>  	m->private = v;
>> > > >>  	if (!v)
>> > > >
>> > > >I don't think there's any way in which this can make a -stable kernel
>> > > >more stable!
>> > > >
>> > > >
>> > > >Generally, I consider -stable in every patch I merge, so for each patch
>> > > >which doesn't have cc:stable, that tag is missing for a reason.
>> > > >
>> > > >In other words, your criteria for -stable addition are different from
>> > > >mine.
>> > > >
>> > > >And I think your criteria differ from those described in
>> > > >Documentation/process/stable-kernel-rules.rst.
>> > > >
>> > > >So... what is your overall thinking on patch selection?
>> > >
>> > > Indeed, this doesn't fix anything.
>> > >
>> > > My concern is that in the future, we will pull a patch that will cause
>> > > the issue described here, and that issue will only be relevant on
>> > > stable. It is very hard to debug this, and I suspect that stable kernels
>> > > will still pass all their tests with flying colors.
>> > >
>> > > As an example, consider the case where commit 28e2c4bb99aa ("mm/vmstat.c:
>> > > fix outdated vmstat_text") is backported to a kernel that doesn't have
>> > > commit 7a9cdebdcc17 ("mm: get rid of vmacache_flush_all() entirely").
>> > >
>> > > I also felt safe with this patch since it adds a single BUILD_BUG_ON()
>> > > which does nothing during runtime, so the chances it introduces anything
>> > > beyond a build regression seemed to be slim to none.
>> >
>> > Well OK.  But my question was general and covers basically every
>> > autosel patch which originated in -mm.
>>
>> Sure. I picked 3 patches that show up on top when I google for AUTOSEL
>> in linux-mm, maybe they'll be a good example to help me understand why
>> they were not selected.
>>
>> This one fixes a case where too few struct pages are allocated when
>> using mirrorred memory:
>>
>> 	https://marc.info/?l=linux-mm&m=154211933211147&w=2
>
>Let me quote "I found this bug by reading the code." I do not think
>anybody has ever seen this in practice.
>
>> Race condition with memory hotplug due to missing locks:
>>
>> 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
>
>Memory hotplug locking is dubious at best and this patch doesn't really
>fix it. It fixes a theoretical problem. I am not aware anybody would be
>hitting in practice. We need to rework the locking quite extensively.

The word "theoretical" used in the stable rules file does not mean
that we need to have actual reports of users hitting bugs before we
start backporting the relevant patch, it simply suggests that there
needs to be a reasonable explanation of how this issue can be hit.

For this memory hotplug patch in particular, I use the hv_balloon driver
at this very moment (running a linux guest on windows, with "dynamic
memory" enabled). Should I wait for it to crash before I can fix it?

Is the upstream code perfect? No, but that doesn't mean that it's not
working at all, and if there are users they expect to see fixes going in
and not just sitting idly waiting for a big rewrite that will come in a
few years.

Memory hotplug fixes are not something you think should go to stable?
Andrew sent a few of them to stable, so that can't be the case.

>> Raising an OOM event that causes issues in userspace when no OOM has
>> actually occured:
>>
>> 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
>
>The patch makes sense I just do not think this is a stable material. The
>semantic of the event was and still is suboptimal.

I really fail to understand your reasoning about -stable here. This
patch is something people actually hit in the field, spent time on
triaging and analysing it, and submitting a fix which looks reasonably
straightforward.

That fix was acked by quite a few folks (including yourself) and merged
in. And as far as we can tell, it actually fixed the problem.

Why is it not stable material?

My understanding is that you're concerned with the patch itself being
"suboptimal", but in that case - why did you ack it?

>> I think that all 3 cases represent a "real" bug users can hit, and I
>> honestly don't know why they were not tagged for stable.
>
>It would be much better to ask in the respective email thread rather
>than spamming mailing with AUTOSEL patches which rarely get any
>attention.

I actually tried it, but the comments I got is that it gets in the way
and people preferred something they can filter.

>We have been through this discussion several times already and I thought
>we have agreed that those subsystems which are seriously considering stable
>are opted out from the AUTOSEL automagic. Has anything changed in that
>regards.

I checked in with Andrew to get his input on this, he suggested that
these patches should be sent to linux-mm and he'll give it a close look.

Ultimately this is the subsystem's decision, yes, but I was under the
impression that this decision wasn't made yet.

I guess that I'm really failing to understand why patches like the third
one here (the OOM one) are being kept out.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Fri 16-11-18 13:19:04, Sasha Levin wrote:
> On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
[...]
> > > Race condition with memory hotplug due to missing locks:
> > > 
> > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
> > 
> > Memory hotplug locking is dubious at best and this patch doesn't really
> > fix it. It fixes a theoretical problem. I am not aware anybody would be
> > hitting in practice. We need to rework the locking quite extensively.
> 
> The word "theoretical" used in the stable rules file does not mean
> that we need to have actual reports of users hitting bugs before we
> start backporting the relevant patch, it simply suggests that there
> needs to be a reasonable explanation of how this issue can be hit.
> 
> For this memory hotplug patch in particular, I use the hv_balloon driver
> at this very moment (running a linux guest on windows, with "dynamic
> memory" enabled). Should I wait for it to crash before I can fix it?
> 
> Is the upstream code perfect? No, but that doesn't mean that it's not
> working at all, and if there are users they expect to see fixes going in
> and not just sitting idly waiting for a big rewrite that will come in a
> few years.
> 
> Memory hotplug fixes are not something you think should go to stable?
> Andrew sent a few of them to stable, so that can't be the case.

I am not arguing about hotplug fixes in general. I was arguing that this
particular one is a theoretical one and hotplug locking is quite subtle.
E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
So in general unless the issue is really triggered easily I am rather
conservative.

> > > Raising an OOM event that causes issues in userspace when no OOM has
> > > actually occured:
> > > 
> > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
> > 
> > The patch makes sense I just do not think this is a stable material. The
> > semantic of the event was and still is suboptimal.
> 
> I really fail to understand your reasoning about -stable here. This
> patch is something people actually hit in the field, spent time on
> triaging and analysing it, and submitting a fix which looks reasonably
> straightforward.
> 
> That fix was acked by quite a few folks (including yourself) and merged
> in. And as far as we can tell, it actually fixed the problem.
> 
> Why is it not stable material?

Because the semantic of the OOM event is quite tricky itself. We have
discussed this patch and concluded that the updated one is more
sensible. But it is not yet clear whether this is actually what other
users expect as well. That to me does sound quite risky for a stable
kernel.

> My understanding is that you're concerned with the patch itself being
> "suboptimal", but in that case - why did you ack it?
> 
> > > I think that all 3 cases represent a "real" bug users can hit, and I
> > > honestly don't know why they were not tagged for stable.
> > 
> > It would be much better to ask in the respective email thread rather
> > than spamming mailing with AUTOSEL patches which rarely get any
> > attention.
> 
> I actually tried it, but the comments I got is that it gets in the way
> and people preferred something they can filter.

which means that AUTOSEL just goes to /dev/null...

> > We have been through this discussion several times already and I thought
> > we have agreed that those subsystems which are seriously considering stable
> > are opted out from the AUTOSEL automagic. Has anything changed in that
> > regards.
> 
> I checked in with Andrew to get his input on this, he suggested that
> these patches should be sent to linux-mm and he'll give it a close look.

If Andrew is happy to get AUTOSEL patches then I will not object of
course but let's not merge these patches without and expclicit OK.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Sasha Levin]

On Fri, Nov 16, 2018 at 07:44:57PM +0100, Michal Hocko wrote:
>On Fri 16-11-18 13:19:04, Sasha Levin wrote:
>> On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
>[...]
>> > > Race condition with memory hotplug due to missing locks:
>> > >
>> > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
>> >
>> > Memory hotplug locking is dubious at best and this patch doesn't really
>> > fix it. It fixes a theoretical problem. I am not aware anybody would be
>> > hitting in practice. We need to rework the locking quite extensively.
>>
>> The word "theoretical" used in the stable rules file does not mean
>> that we need to have actual reports of users hitting bugs before we
>> start backporting the relevant patch, it simply suggests that there
>> needs to be a reasonable explanation of how this issue can be hit.
>>
>> For this memory hotplug patch in particular, I use the hv_balloon driver
>> at this very moment (running a linux guest on windows, with "dynamic
>> memory" enabled). Should I wait for it to crash before I can fix it?
>>
>> Is the upstream code perfect? No, but that doesn't mean that it's not
>> working at all, and if there are users they expect to see fixes going in
>> and not just sitting idly waiting for a big rewrite that will come in a
>> few years.
>>
>> Memory hotplug fixes are not something you think should go to stable?
>> Andrew sent a few of them to stable, so that can't be the case.
>
>I am not arguing about hotplug fixes in general. I was arguing that this
>particular one is a theoretical one and hotplug locking is quite subtle.
>E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
>http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
>So in general unless the issue is really triggered easily I am rather
>conservative.

We have millions of machines running linux, everything is triggered
"easily" at that scale.

>> > > Raising an OOM event that causes issues in userspace when no OOM has
>> > > actually occured:
>> > >
>> > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
>> >
>> > The patch makes sense I just do not think this is a stable material. The
>> > semantic of the event was and still is suboptimal.
>>
>> I really fail to understand your reasoning about -stable here. This
>> patch is something people actually hit in the field, spent time on
>> triaging and analysing it, and submitting a fix which looks reasonably
>> straightforward.
>>
>> That fix was acked by quite a few folks (including yourself) and merged
>> in. And as far as we can tell, it actually fixed the problem.
>>
>> Why is it not stable material?
>
>Because the semantic of the OOM event is quite tricky itself. We have
>discussed this patch and concluded that the updated one is more
>sensible. But it is not yet clear whether this is actually what other
>users expect as well. That to me does sound quite risky for a stable
>kernel.

So there's another patch following this one that fixes it? Sure - can I
take both?

Users expect to not have their containers die randomly, if you're saying
that you're still working on a fix for that then that is a different
story than saying "we fixed it, but it should not go to stable".

And let's also draw a line there, users will not wait for the OOM event
logic to be perfect before they can expect their workloads to run
without issues.

>> My understanding is that you're concerned with the patch itself being
>> "suboptimal", but in that case - why did you ack it?
>>
>> > > I think that all 3 cases represent a "real" bug users can hit, and I
>> > > honestly don't know why they were not tagged for stable.
>> >
>> > It would be much better to ask in the respective email thread rather
>> > than spamming mailing with AUTOSEL patches which rarely get any
>> > attention.
>>
>> I actually tried it, but the comments I got is that it gets in the way
>> and people preferred something they can filter.
>
>which means that AUTOSEL just goes to /dev/null...

Or just not get mixed with the process? for some people it's easier to
see AUTOSEL mails with the way it works now rather than if they suddenly
show up as a continuation of a weeks old thread.

>> > We have been through this discussion several times already and I thought
>> > we have agreed that those subsystems which are seriously considering stable
>> > are opted out from the AUTOSEL automagic. Has anything changed in that
>> > regards.
>>
>> I checked in with Andrew to get his input on this, he suggested that
>> these patches should be sent to linux-mm and he'll give it a close look.
>
>If Andrew is happy to get AUTOSEL patches then I will not object of
>course but let's not merge these patches without and expclicit OK.

This is fair. I think that the process has caused some unnecessary
friction: we all want the same result but just disagree on the means :)

I won't merge any mm/ AUTOSEL patches until this gets clearer.

--
Thanks,
Sasha

Re: [PATCH AUTOSEL 3.18 8/9] mm/vmstat.c: assert that vmstat_text is in sync with stat_items_size

[Michal Hocko]

On Fri 16-11-18 14:19:10, Sasha Levin wrote:
> On Fri, Nov 16, 2018 at 07:44:57PM +0100, Michal Hocko wrote:
> > On Fri 16-11-18 13:19:04, Sasha Levin wrote:
> > > On Fri, Nov 16, 2018 at 09:55:25AM +0100, Michal Hocko wrote:
> > [...]
> > > > > Race condition with memory hotplug due to missing locks:
> > > > >
> > > > > 	https://marc.info/?l=linux-mm&m=154211934011188&w=2
> > > >
> > > > Memory hotplug locking is dubious at best and this patch doesn't really
> > > > fix it. It fixes a theoretical problem. I am not aware anybody would be
> > > > hitting in practice. We need to rework the locking quite extensively.
> > > 
> > > The word "theoretical" used in the stable rules file does not mean
> > > that we need to have actual reports of users hitting bugs before we
> > > start backporting the relevant patch, it simply suggests that there
> > > needs to be a reasonable explanation of how this issue can be hit.
> > > 
> > > For this memory hotplug patch in particular, I use the hv_balloon driver
> > > at this very moment (running a linux guest on windows, with "dynamic
> > > memory" enabled). Should I wait for it to crash before I can fix it?
> > > 
> > > Is the upstream code perfect? No, but that doesn't mean that it's not
> > > working at all, and if there are users they expect to see fixes going in
> > > and not just sitting idly waiting for a big rewrite that will come in a
> > > few years.
> > > 
> > > Memory hotplug fixes are not something you think should go to stable?
> > > Andrew sent a few of them to stable, so that can't be the case.
> > 
> > I am not arguing about hotplug fixes in general. I was arguing that this
> > particular one is a theoretical one and hotplug locking is quite subtle.
> > E.g. 381eab4a6ee mm/memory_hotplug: fix online/offline_pages called w.o. mem_hotplug_lock
> > http://lkml.kernel.org/r/20181114070909.GB2653@MiWiFi-R3L-srv
> > So in general unless the issue is really triggered easily I am rather
> > conservative.
> 
> We have millions of machines running linux, everything is triggered
> "easily" at that scale.

yet a zero report...

> > > > > Raising an OOM event that causes issues in userspace when no OOM has
> > > > > actually occured:
> > > > >
> > > > > 	https://marc.info/?l=linux-mm&m=154211939811582&w=2
> > > >
> > > > The patch makes sense I just do not think this is a stable material. The
> > > > semantic of the event was and still is suboptimal.
> > > 
> > > I really fail to understand your reasoning about -stable here. This
> > > patch is something people actually hit in the field, spent time on
> > > triaging and analysing it, and submitting a fix which looks reasonably
> > > straightforward.
> > > 
> > > That fix was acked by quite a few folks (including yourself) and merged
> > > in. And as far as we can tell, it actually fixed the problem.
> > > 
> > > Why is it not stable material?
> > 
> > Because the semantic of the OOM event is quite tricky itself. We have
> > discussed this patch and concluded that the updated one is more
> > sensible. But it is not yet clear whether this is actually what other
> > users expect as well. That to me does sound quite risky for a stable
> > kernel.
> 
> So there's another patch following this one that fixes it? Sure - can I
> take both?

No. There is no known bug. I am arguing that such a change needs some
time to settle. I am quite skeptical that this will actually trigger
any bug.

I will not _object_ if this was merged if somebody explicitly asks for
it. I am saying that I am not convinced it is a stable material.

So I guess our views on what is stable material differ. As I have said
several times already, I think the volume of patches flowing to the
stable tree is really high. To the point that taking stable trees for
our SLES kernels become problematic. I have heard the similar from
others. More is not always better. But let's not repeat this discussion
again. If Andrew doesn't mind then keep sending AUTOSEL emails but
please let's not apply those patches automatically.

Thanks!
-- 
Michal Hocko
SUSE Labs

[PATCH AUTOSEL 3.18 9/9] mm: don't warn about large allocations for slab

[Sasha Levin]

From: Dmitry Vyukov <dvyukov@google.com>

[ Upstream commit 61448479a9f2c954cde0cfe778cb6bec5d0a748d ]

Slub does not call kmalloc_slab() for sizes > KMALLOC_MAX_CACHE_SIZE,
instead it falls back to kmalloc_large().

For slab KMALLOC_MAX_CACHE_SIZE == KMALLOC_MAX_SIZE and it calls
kmalloc_slab() for all allocations relying on NULL return value for
over-sized allocations.

This inconsistency leads to unwanted warnings from kmalloc_slab() for
over-sized allocations for slab.  Returning NULL for failed allocations is
the expected behavior.

Make slub and slab code consistent by checking size >
KMALLOC_MAX_CACHE_SIZE in slab before calling kmalloc_slab().

While we are here also fix the check in kmalloc_slab().  We should check
against KMALLOC_MAX_CACHE_SIZE rather than KMALLOC_MAX_SIZE.  It all kinda
worked because for slab the constants are the same, and slub always checks
the size against KMALLOC_MAX_CACHE_SIZE before kmalloc_slab().  But if we
get there with size > KMALLOC_MAX_CACHE_SIZE anyhow bad things will
happen.  For example, in case of a newly introduced bug in slub code.

Also move the check in kmalloc_slab() from function entry to the size >
192 case.  This partially compensates for the additional check in slab
code and makes slub code a bit faster (at least theoretically).

Also drop __GFP_NOWARN in the warning check.  This warning means a bug in
slab code itself, user-passed flags have nothing to do with it.

Nothing of this affects slob.

Link: http://lkml.kernel.org/r/20180927171502.226522-1-dvyukov@gmail.com
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: syzbot+87829a10073277282ad1@syzkaller.appspotmail.com
Reported-by: syzbot+ef4e8fc3a06e9019bb40@syzkaller.appspotmail.com
Reported-by: syzbot+6e438f4036df52cbb863@syzkaller.appspotmail.com
Reported-by: syzbot+8574471d8734457d98aa@syzkaller.appspotmail.com
Reported-by: syzbot+af1504df0807a083dbd9@syzkaller.appspotmail.com
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 mm/slab.c        |  4 ++++
 mm/slab_common.c | 12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index b7f9f6456a61..0b8ff2152f60 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3465,6 +3465,8 @@ __do_kmalloc_node(size_t size, gfp_t flags, int node, unsigned long caller)
 {
 	struct kmem_cache *cachep;
 
+	if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
+		return NULL;
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
@@ -3497,6 +3499,8 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 	struct kmem_cache *cachep;
 	void *ret;
 
+	if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
+		return NULL;
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index dcdab81bd240..d8489833d423 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -653,18 +653,18 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
 {
 	int index;
 
-	if (unlikely(size > KMALLOC_MAX_SIZE)) {
-		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
-		return NULL;
-	}
-
 	if (size <= 192) {
 		if (!size)
 			return ZERO_SIZE_PTR;
 
 		index = size_index[size_index_elem(size)];
-	} else
+	} else {
+		if (unlikely(size > KMALLOC_MAX_CACHE_SIZE)) {
+			WARN_ON(1);
+			return NULL;
+		}
 		index = fls(size - 1);
+	}
 
 #ifdef CONFIG_ZONE_DMA
 	if (unlikely((flags & GFP_DMA)))
-- 
2.17.1