Date: Sat, 23 Dec 2017 10:24:37 +0100
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Michal Hocko <mhocko@kernel.org>
Cc: alexander.levin@verizon.com,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        Shakeel Butt <shakeelb@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 4.14 108/159] kvm, mm: account kvm related kmem slabs to
 kmemcg
Message-ID: <20171223092437.GI13832@kroah.com>
References: <20171222084623.668990192@linuxfoundation.org>
 <20171222084629.570989756@linuxfoundation.org>
 <20171222093407.GN4831@dhcp22.suse.cz>
 <20171222124122.GA30743@kroah.com>
 <20171222130607.GQ4831@dhcp22.suse.cz>
 <20171222174002.6qiqllf3bhmjangn@sasha-lappy>
 <20171222175616.GS4831@dhcp22.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171222175616.GS4831@dhcp22.suse.cz>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4526
Lines: 90

On Fri, Dec 22, 2017 at 06:56:16PM +0100, Michal Hocko wrote:
> On Fri 22-12-17 17:40:10, Sasha Levin wrote:
> > On Fri, Dec 22, 2017 at 02:06:07PM +0100, Michal Hocko wrote:
> > >On Fri 22-12-17 13:41:22, Greg KH wrote:
> > >> On Fri, Dec 22, 2017 at 10:34:07AM +0100, Michal Hocko wrote:
> > >> > On Fri 22-12-17 09:46:33, Greg KH wrote:
> > >> > > 4.14-stable review patch.  If anyone has any objections, please let me know.
> > >> > >
> > >> > > ------------------
> > >> > >
> > >> > > From: Shakeel Butt <shakeelb@google.com>
> > >> > >
> > >> > >
> > >> > > [ Upstream commit 46bea48ac241fe0b413805952dda74dd0c09ba8b ]
> > >> > >
> > >> > > The kvm slabs can consume a significant amount of system memory
> > >> > > and indeed in our production environment we have observed that
> > >> > > a lot of machines are spending significant amount of memory that
> > >> > > can not be left as system memory overhead. Also the allocations
> > >> > > from these slabs can be triggered directly by user space applications
> > >> > > which has access to kvm and thus a buggy application can leak
> > >> > > such memory. So, these caches should be accounted to kmemcg.
> > >> > >
> > >> > > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > >> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> > > Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
> > >> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >> >
> > >> > The patch is not marked for stable, neither it fixes an existing bug.
> > >> > It is a nice to have thing for sure but I am wondering how this got
> > >> > through stable-filter.
> > >>
> > >> Sasha picked it out, and it seemed like a sane thing to backport.  If
> > >> you think it's not worthy, I'll gladly drop it, but it seemed like such
> > >> a simple bugfix to include.
> > >
> > >It is not that I would have some specific concerns about this particular
> > >patch. It is more of a worry about the overal process. I thought that
> > >_any_ patch backported to the stable tree would require a specific bug
> > >to be fixed or in exceptional cases a performance issue. I have
> > >experienced this pushback myself when trying to push "no real bug report
> > >but better to have this plugged" patches.
> > >
> > >So something has apparently changed in the process, I just haven't
> > >noticed it. I am worried this might lead to more regression in future.
> > >Not that my worry counts all that much as I am not a stable kernel user
> > >though. So this is just my 2c worth of worry.
> > 
> > The way I see it is that stable commits are supposed to fix a bug that
> > a user can hit/exploit, it doesn't have to have an actual user
> > complaining about it.
> > 
> > For this particular commit, the way I read it is that a user can avoid
> > his kmemcg limits (maybe maliciously), which would qualify as an
> > actual bug we want to get fixed.
> 
> How are you going to judge all the possible relations to other
> subsystems? I mean there is a good reason maintainers mark patches for
> stable trees. How do you want to competently decide this for them? Can
> you do that for all subsystems?
> 
> I do not want to underestimate your judgment or misinterpret your
> process here but I _believe_ that picking patches based on the changelog
> without a deep understanding of the subsystem is really risky. We do
> not really have to go a long way to see that. Just look at other patch
> in this very thread [1]. But maybe our our understanding of the stable
> trees are different.

For many subsystems, the maintainers _never_ mark patches for stable.
Others, they catch maybe half of the things they should be applying.

KVM is one such example of the "half" group, they mark patches as
resolving CVE issues at times, yet don't mark them for stable.  So when
I see a patch like this, it triggers the "oh, look, KVM doing the same
thing again", so I take the patch and of course cc: the
developers/maintainers so they can object if they want to.

Over time you get to know what subsystems are like this and what are
not.  MM is one that is really good, I almost never take a mm patch
without being told explicitly to do so.  Others are horrible and never
mark anything, so stuff has to be picked up manually through Sasha's
process or through other ways.

So it's not a perfect system, but it seems to work "good enough", and if
you ever have any questions about any patch, always feel free to ask,
there's usually a story behind almost every one...

thanks,

greg k-h