diff for duplicates of <52E4C420.8020800@parallels.com> diff --git a/a/2.txt b/N1/2.txt index 8b13789..b22c6ae 100644 --- a/a/2.txt +++ b/N1/2.txt @@ -1 +1,88 @@ +>From 371649294d90b65a2e86d0873f79bab454285d1a Mon Sep 17 00:00:00 2001 +From: David Rientjes <rientjes@google.com> +Date: Sun, 26 Jan 2014 11:49:39 +0400 +Subject: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path +On kmem_cache_create_memcg() error path we set 'err', but leave 's' (the +new cache ptr) undefined. The latter can be NULL if we could not +allocate the cache, or pointing to a freed area if we failed somewhere +later while trying to initialize it. Initially we checked 'err' +immediately before exiting the function and returned NULL if it was set +ignoring the value of 's': + + out_unlock: + ... + if (err) { + /* report error */ + return NULL; + } + return s; + +Recently this check was, in fact, broken by commit f717eb3abb5e ("slab: +do not panic if we fail to create memcg cache"), which turned it to: + + out_unlock: + ... + if (err && !memcg) { + /* report error */ + return NULL; + } + return s; + +As a result, if we are failing creating a cache for a memcg, we will +skip the check and return 's' that can contain crap. Obviously, commit +f717eb3abb5e intended not to return crap on error allocating a cache for +a memcg, but only to remove the error reporting in this case, so the +check should look like this: + + out_unlock: + ... + if (err) { + if (!memcg) + return NULL; + /* report error */ + return NULL; + } + return s; + +Signed-off-by: David Rientjes <rientjes@google.com> +Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> +Reported-by: Dave Jones <davej@redhat.com> +Cc: Pekka Enberg <penberg@kernel.org> +Cc: Christoph Lameter <cl@linux.com> +--- + mm/slab_common.c | 19 +++++++++++-------- + 1 file changed, 11 insertions(+), 8 deletions(-) + +diff --git a/mm/slab_common.c b/mm/slab_common.c +index 8e40321..1ec3c61 100644 +--- a/mm/slab_common.c ++++ b/mm/slab_common.c +@@ -233,14 +233,17 @@ out_unlock: + mutex_unlock(&slab_mutex); + put_online_cpus(); + +- /* +- * There is no point in flooding logs with warnings or especially +- * crashing the system if we fail to create a cache for a memcg. In +- * this case we will be accounting the memcg allocation to the root +- * cgroup until we succeed to create its own cache, but it isn't that +- * critical. +- */ +- if (err && !memcg) { ++ if (err) { ++ /* ++ * There is no point in flooding logs with warnings or ++ * especially crashing the system if we fail to create a cache ++ * for a memcg. In this case we will be accounting the memcg ++ * allocation to the root cgroup until we succeed to create its ++ * own cache, but it isn't that critical. ++ */ ++ if (!memcg) ++ return NULL; ++ + if (flags & SLAB_PANIC) + panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n", + name, err); +-- +1.7.10.4 diff --git a/a/content_digest b/N1/content_digest index 8376d1e..0c29976 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -5,8 +5,8 @@ "Date\0Sun, 26 Jan 2014 12:15:28 +0400\0" "To\0David Rientjes <rientjes@google.com>" " Andrew Morton <akpm@linux-foundation.org>\0" - "Cc\0linux-kernel@vger.kernel.org" - linux-mm@kvack.org + "Cc\0<linux-kernel@vger.kernel.org>" + <linux-mm@kvack.org> Dave Jones <davej@redhat.com> Pekka Enberg <penberg@kernel.org> " Christoph Lameter <cl@linux.com>\0" @@ -83,5 +83,93 @@ "\01:2\0" "fn\00001-slab-fix-wrong-retval-on-kmem_cache_create_memcg-err.patch\0" "b\0" + ">From 371649294d90b65a2e86d0873f79bab454285d1a Mon Sep 17 00:00:00 2001\n" + "From: David Rientjes <rientjes@google.com>\n" + "Date: Sun, 26 Jan 2014 11:49:39 +0400\n" + "Subject: [PATCH] slab: fix wrong retval on kmem_cache_create_memcg error path\n" + "\n" + "On kmem_cache_create_memcg() error path we set 'err', but leave 's' (the\n" + "new cache ptr) undefined. The latter can be NULL if we could not\n" + "allocate the cache, or pointing to a freed area if we failed somewhere\n" + "later while trying to initialize it. Initially we checked 'err'\n" + "immediately before exiting the function and returned NULL if it was set\n" + "ignoring the value of 's':\n" + "\n" + " out_unlock:\n" + " ...\n" + " if (err) {\n" + " /* report error */\n" + " return NULL;\n" + " }\n" + " return s;\n" + "\n" + "Recently this check was, in fact, broken by commit f717eb3abb5e (\"slab:\n" + "do not panic if we fail to create memcg cache\"), which turned it to:\n" + "\n" + " out_unlock:\n" + " ...\n" + " if (err && !memcg) {\n" + " /* report error */\n" + " return NULL;\n" + " }\n" + " return s;\n" + "\n" + "As a result, if we are failing creating a cache for a memcg, we will\n" + "skip the check and return 's' that can contain crap. Obviously, commit\n" + "f717eb3abb5e intended not to return crap on error allocating a cache for\n" + "a memcg, but only to remove the error reporting in this case, so the\n" + "check should look like this:\n" + "\n" + " out_unlock:\n" + " ...\n" + " if (err) {\n" + " if (!memcg)\n" + " return NULL;\n" + " /* report error */\n" + " return NULL;\n" + " }\n" + " return s;\n" + "\n" + "Signed-off-by: David Rientjes <rientjes@google.com>\n" + "Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>\n" + "Reported-by: Dave Jones <davej@redhat.com>\n" + "Cc: Pekka Enberg <penberg@kernel.org>\n" + "Cc: Christoph Lameter <cl@linux.com>\n" + "---\n" + " mm/slab_common.c | 19 +++++++++++--------\n" + " 1 file changed, 11 insertions(+), 8 deletions(-)\n" + "\n" + "diff --git a/mm/slab_common.c b/mm/slab_common.c\n" + "index 8e40321..1ec3c61 100644\n" + "--- a/mm/slab_common.c\n" + "+++ b/mm/slab_common.c\n" + "@@ -233,14 +233,17 @@ out_unlock:\n" + " \tmutex_unlock(&slab_mutex);\n" + " \tput_online_cpus();\n" + " \n" + "-\t/*\n" + "-\t * There is no point in flooding logs with warnings or especially\n" + "-\t * crashing the system if we fail to create a cache for a memcg. In\n" + "-\t * this case we will be accounting the memcg allocation to the root\n" + "-\t * cgroup until we succeed to create its own cache, but it isn't that\n" + "-\t * critical.\n" + "-\t */\n" + "-\tif (err && !memcg) {\n" + "+\tif (err) {\n" + "+\t\t/*\n" + "+\t\t * There is no point in flooding logs with warnings or\n" + "+\t\t * especially crashing the system if we fail to create a cache\n" + "+\t\t * for a memcg. In this case we will be accounting the memcg\n" + "+\t\t * allocation to the root cgroup until we succeed to create its\n" + "+\t\t * own cache, but it isn't that critical.\n" + "+\t\t */\n" + "+\t\tif (!memcg)\n" + "+\t\t\treturn NULL;\n" + "+\n" + " \t\tif (flags & SLAB_PANIC)\n" + " \t\t\tpanic(\"kmem_cache_create: Failed to create slab '%s'. Error %d\\n\",\n" + " \t\t\t\tname, err);\n" + "-- \n" + 1.7.10.4 -b5b748fdde8c4bf4a0d0847ce1e21756505bacc85bbd437b1d8c3c87f71e1b27 +0f9da16ead7bd53c8cfe9c6098169696ac3d8071d81f209e86d5ef510ea0a3d2
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.