* [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
@ 2017-05-20 10:52 ` Thomas Gleixner
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2017-05-20 10:52 UTC (permalink / raw)
To: LKML
Cc: linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
memcg_propagate_slab_attrs() abuses the sysfs attribute file functions to
propagate settings from the root kmem_cache to a newly created
kmem_cache. It does that with:
attr->show(root, buf);
attr->store(new, buf, strlen(bug);
Aside of being a lazy and absurd hackery this is broken because it does not
check the return value of the show() function.
Some of the show() functions return 0 w/o touching the buffer. That means in
such a case the store function is called with the stale content of the
previous show(). That causes nonsense like invoking kmem_cache_shrink() on
a newly created kmem_cache. In the worst case it would cause handing in an
uninitialized buffer.
This should be rewritten proper by adding a propagate() callback to those
slub_attributes which must be propagated and avoid that insane conversion
to and from ASCII, but that's too large for a hot fix.
Check at least the return value of the show() function, so calling store()
with stale content is prevented.
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
mm/slub.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5512,6 +5512,7 @@ static void memcg_propagate_slab_attrs(s
char mbuf[64];
char *buf;
struct slab_attribute *attr = to_slab_attr(slab_attrs[i]);
+ ssize_t len;
if (!attr || !attr->store || !attr->show)
continue;
@@ -5536,8 +5537,9 @@ static void memcg_propagate_slab_attrs(s
buf = buffer;
}
- attr->show(root_cache, buf);
- attr->store(s, buf, strlen(buf));
+ len = attr->show(root_cache, buf);
+ if (len > 0)
+ attr->store(s, buf, len);
}
if (buffer)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
@ 2017-05-20 10:52 ` Thomas Gleixner
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2017-05-20 10:52 UTC (permalink / raw)
To: LKML
Cc: linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
memcg_propagate_slab_attrs() abuses the sysfs attribute file functions to
propagate settings from the root kmem_cache to a newly created
kmem_cache. It does that with:
attr->show(root, buf);
attr->store(new, buf, strlen(bug);
Aside of being a lazy and absurd hackery this is broken because it does not
check the return value of the show() function.
Some of the show() functions return 0 w/o touching the buffer. That means in
such a case the store function is called with the stale content of the
previous show(). That causes nonsense like invoking kmem_cache_shrink() on
a newly created kmem_cache. In the worst case it would cause handing in an
uninitialized buffer.
This should be rewritten proper by adding a propagate() callback to those
slub_attributes which must be propagated and avoid that insane conversion
to and from ASCII, but that's too large for a hot fix.
Check at least the return value of the show() function, so calling store()
with stale content is prevented.
Reported-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
mm/slub.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5512,6 +5512,7 @@ static void memcg_propagate_slab_attrs(s
char mbuf[64];
char *buf;
struct slab_attribute *attr = to_slab_attr(slab_attrs[i]);
+ ssize_t len;
if (!attr || !attr->store || !attr->show)
continue;
@@ -5536,8 +5537,9 @@ static void memcg_propagate_slab_attrs(s
buf = buffer;
}
- attr->show(root_cache, buf);
- attr->store(s, buf, strlen(buf));
+ len = attr->show(root_cache, buf);
+ if (len > 0)
+ attr->store(s, buf, len);
}
if (buffer)
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
2017-05-20 10:52 ` Thomas Gleixner
@ 2017-05-20 13:16 ` Christoph Hellwig
-1 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2017-05-20 13:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
On Sat, May 20, 2017 at 12:52:03PM +0200, Thomas Gleixner wrote:
> This should be rewritten proper by adding a propagate() callback to those
> slub_attributes which must be propagated and avoid that insane conversion
> to and from ASCII
Exactly..
>, but that's too large for a hot fix.
What made this such a hot fix? Looks like this crap has been in
for quite a while.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
@ 2017-05-20 13:16 ` Christoph Hellwig
0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2017-05-20 13:16 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
On Sat, May 20, 2017 at 12:52:03PM +0200, Thomas Gleixner wrote:
> This should be rewritten proper by adding a propagate() callback to those
> slub_attributes which must be propagated and avoid that insane conversion
> to and from ASCII
Exactly..
>, but that's too large for a hot fix.
What made this such a hot fix? Looks like this crap has been in
for quite a while.
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
2017-05-20 13:16 ` Christoph Hellwig
@ 2017-05-20 14:25 ` Steven Rostedt
-1 siblings, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2017-05-20 14:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Thomas Gleixner, LKML, linux-mm, Johannes Weiner, Michal Hocko,
Christoph Lameter, Andrew Morton, Peter Zijlstra
On Sat, 20 May 2017 06:16:45 -0700
Christoph Hellwig <hch@infradead.org> wrote:
> On Sat, May 20, 2017 at 12:52:03PM +0200, Thomas Gleixner wrote:
> > This should be rewritten proper by adding a propagate() callback to those
> > slub_attributes which must be propagated and avoid that insane conversion
> > to and from ASCII
>
> Exactly..
>
> >, but that's too large for a hot fix.
>
> What made this such a hot fix? Looks like this crap has been in
> for quite a while.
It can cause a deadlock with get_online_cpus() that has been uncovered
by recent cpu hotplug and lockdep changes that Thomas and Peter have
been doing.
[ 102.567308] Possible unsafe locking scenario:
[ 102.567308]
[ 102.574846] CPU0 CPU1
[ 102.580148] ---- ----
[ 102.585421] lock(cpu_hotplug.lock);
[ 102.589808] lock(slab_mutex);
[ 102.596166] lock(cpu_hotplug.lock);
[ 102.603028] lock(slab_mutex);
[ 102.606846]
[ 102.606846] *** DEADLOCK ***
-- Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
@ 2017-05-20 14:25 ` Steven Rostedt
0 siblings, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2017-05-20 14:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Thomas Gleixner, LKML, linux-mm, Johannes Weiner, Michal Hocko,
Christoph Lameter, Andrew Morton, Peter Zijlstra
On Sat, 20 May 2017 06:16:45 -0700
Christoph Hellwig <hch@infradead.org> wrote:
> On Sat, May 20, 2017 at 12:52:03PM +0200, Thomas Gleixner wrote:
> > This should be rewritten proper by adding a propagate() callback to those
> > slub_attributes which must be propagated and avoid that insane conversion
> > to and from ASCII
>
> Exactly..
>
> >, but that's too large for a hot fix.
>
> What made this such a hot fix? Looks like this crap has been in
> for quite a while.
It can cause a deadlock with get_online_cpus() that has been uncovered
by recent cpu hotplug and lockdep changes that Thomas and Peter have
been doing.
[ 102.567308] Possible unsafe locking scenario:
[ 102.567308]
[ 102.574846] CPU0 CPU1
[ 102.580148] ---- ----
[ 102.585421] lock(cpu_hotplug.lock);
[ 102.589808] lock(slab_mutex);
[ 102.596166] lock(cpu_hotplug.lock);
[ 102.603028] lock(slab_mutex);
[ 102.606846]
[ 102.606846] *** DEADLOCK ***
-- Steve
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
2017-05-20 13:16 ` Christoph Hellwig
@ 2017-05-21 20:01 ` Thomas Gleixner
-1 siblings, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2017-05-21 20:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: LKML, linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
On Sat, 20 May 2017, Christoph Hellwig wrote:
> On Sat, May 20, 2017 at 12:52:03PM +0200, Thomas Gleixner wrote:
> > This should be rewritten proper by adding a propagate() callback to those
> > slub_attributes which must be propagated and avoid that insane conversion
> > to and from ASCII
>
> Exactly..
>
> >, but that's too large for a hot fix.
>
> What made this such a hot fix? Looks like this crap has been in
> for quite a while.
Well, having something in tree which uses stale or uninitialized buffers
does justify a hot fix which can be easily backported to stable.
Thanks,
tglx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
@ 2017-05-21 20:01 ` Thomas Gleixner
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2017-05-21 20:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: LKML, linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
On Sat, 20 May 2017, Christoph Hellwig wrote:
> On Sat, May 20, 2017 at 12:52:03PM +0200, Thomas Gleixner wrote:
> > This should be rewritten proper by adding a propagate() callback to those
> > slub_attributes which must be propagated and avoid that insane conversion
> > to and from ASCII
>
> Exactly..
>
> >, but that's too large for a hot fix.
>
> What made this such a hot fix? Looks like this crap has been in
> for quite a while.
Well, having something in tree which uses stale or uninitialized buffers
does justify a hot fix which can be easily backported to stable.
Thanks,
tglx
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
2017-05-20 10:52 ` Thomas Gleixner
@ 2017-05-22 1:25 ` David Rientjes
-1 siblings, 0 replies; 10+ messages in thread
From: David Rientjes @ 2017-05-22 1:25 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
On Sat, 20 May 2017, Thomas Gleixner wrote:
> memcg_propagate_slab_attrs() abuses the sysfs attribute file functions to
> propagate settings from the root kmem_cache to a newly created
> kmem_cache. It does that with:
>
> attr->show(root, buf);
> attr->store(new, buf, strlen(bug);
>
> Aside of being a lazy and absurd hackery this is broken because it does not
> check the return value of the show() function.
>
> Some of the show() functions return 0 w/o touching the buffer. That means in
> such a case the store function is called with the stale content of the
> previous show(). That causes nonsense like invoking kmem_cache_shrink() on
> a newly created kmem_cache. In the worst case it would cause handing in an
> uninitialized buffer.
>
> This should be rewritten proper by adding a propagate() callback to those
> slub_attributes which must be propagated and avoid that insane conversion
> to and from ASCII, but that's too large for a hot fix.
>
> Check at least the return value of the show() function, so calling store()
> with stale content is prevented.
>
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
Acked-by: David Rientjes <rientjes@google.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes
@ 2017-05-22 1:25 ` David Rientjes
0 siblings, 0 replies; 10+ messages in thread
From: David Rientjes @ 2017-05-22 1:25 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, linux-mm, Johannes Weiner, Michal Hocko, Christoph Lameter,
Andrew Morton, Steven Rostedt, Peter Zijlstra
On Sat, 20 May 2017, Thomas Gleixner wrote:
> memcg_propagate_slab_attrs() abuses the sysfs attribute file functions to
> propagate settings from the root kmem_cache to a newly created
> kmem_cache. It does that with:
>
> attr->show(root, buf);
> attr->store(new, buf, strlen(bug);
>
> Aside of being a lazy and absurd hackery this is broken because it does not
> check the return value of the show() function.
>
> Some of the show() functions return 0 w/o touching the buffer. That means in
> such a case the store function is called with the stale content of the
> previous show(). That causes nonsense like invoking kmem_cache_shrink() on
> a newly created kmem_cache. In the worst case it would cause handing in an
> uninitialized buffer.
>
> This should be rewritten proper by adding a propagate() callback to those
> slub_attributes which must be propagated and avoid that insane conversion
> to and from ASCII, but that's too large for a hot fix.
>
> Check at least the return value of the show() function, so calling store()
> with stale content is prevented.
>
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
Acked-by: David Rientjes <rientjes@google.com>
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-22 1:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-20 10:52 [PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes Thomas Gleixner
2017-05-20 10:52 ` Thomas Gleixner
2017-05-20 13:16 ` Christoph Hellwig
2017-05-20 13:16 ` Christoph Hellwig
2017-05-20 14:25 ` Steven Rostedt
2017-05-20 14:25 ` Steven Rostedt
2017-05-21 20:01 ` Thomas Gleixner
2017-05-21 20:01 ` Thomas Gleixner
2017-05-22 1:25 ` David Rientjes
2017-05-22 1:25 ` David Rientjes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.