* Re: [RFC 3/3] mm, slub: stop freeing kmem_cache_node structures on node offline
@ 2021-01-07 0:44 kernel test robot
0 siblings, 0 replies; 3+ messages in thread
From: kernel test robot @ 2021-01-07 0:44 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 4026 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210106174029.12654-4-vbabka@suse.cz>
References: <20210106174029.12654-4-vbabka@suse.cz>
TO: Vlastimil Babka <vbabka@suse.cz>
Hi Vlastimil,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master hnaz-linux-mm/master v5.11-rc2 next-20210104]
[cannot apply to mmotm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vlastimil-Babka/mm-slab-slub-remove-cpu-and-memory-hotplug-locks/20210107-014224
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 09162bc32c880a791c6c0668ce0745cf7958f576
:::::: branch date: 7 hours ago
:::::: commit date: 7 hours ago
compiler: c6x-elf-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
cppcheck possible warnings: (new ones prefixed by >>, may not real problems)
>> mm/slub.c:4223:26: warning: Unused variable: n [unusedVariable]
struct kmem_cache_node *n;
^
>> mm/slub.c:4224:21: warning: Unused variable: s [unusedVariable]
struct kmem_cache *s;
^
mm/slub.c:5581:4: warning: Either the condition '!name' is redundant or there is pointer arithmetic with NULL pointer. [nullPointerArithmeticRedundantCheck]
*p++ = ':';
^
mm/slub.c:5579:9: note: Assuming that condition '!name' is not redundant
BUG_ON(!name);
^
mm/slub.c:5577:12: note: Assignment 'p=name', assigned value is 0
char *p = name;
^
mm/slub.c:5581:4: note: Null pointer addition
*p++ = ':';
^
vim +4223 mm/slub.c
b9049e234401e1 Yasunori Goto 2007-10-21 4220
b9049e234401e1 Yasunori Goto 2007-10-21 4221 static void slab_mem_offline_callback(void *arg)
b9049e234401e1 Yasunori Goto 2007-10-21 4222 {
b9049e234401e1 Yasunori Goto 2007-10-21 @4223 struct kmem_cache_node *n;
b9049e234401e1 Yasunori Goto 2007-10-21 @4224 struct kmem_cache *s;
b9049e234401e1 Yasunori Goto 2007-10-21 4225 struct memory_notify *marg = arg;
b9049e234401e1 Yasunori Goto 2007-10-21 4226 int offline_node;
b9049e234401e1 Yasunori Goto 2007-10-21 4227
b9d5ab2562ecee Lai Jiangshan 2012-12-11 4228 offline_node = marg->status_change_nid_normal;
b9049e234401e1 Yasunori Goto 2007-10-21 4229
b9049e234401e1 Yasunori Goto 2007-10-21 4230 /*
b9049e234401e1 Yasunori Goto 2007-10-21 4231 * If the node still has available memory. we need kmem_cache_node
b9049e234401e1 Yasunori Goto 2007-10-21 4232 * for it yet.
b9049e234401e1 Yasunori Goto 2007-10-21 4233 */
b9049e234401e1 Yasunori Goto 2007-10-21 4234 if (offline_node < 0)
b9049e234401e1 Yasunori Goto 2007-10-21 4235 return;
b9049e234401e1 Yasunori Goto 2007-10-21 4236
18004c5d4084d9 Christoph Lameter 2012-07-06 4237 mutex_lock(&slab_mutex);
27153f04e4a5f9 Vlastimil Babka 2021-01-06 4238 node_clear(offline_node, slab_nodes);
b9049e234401e1 Yasunori Goto 2007-10-21 4239 /*
9d5e878e997461 Vlastimil Babka 2021-01-06 4240 * We no longer free kmem_cache_node structures here, as it would be
9d5e878e997461 Vlastimil Babka 2021-01-06 4241 * racy with all get_node() users, and infeasible to protect them with
9d5e878e997461 Vlastimil Babka 2021-01-06 4242 * slab_mutex.
b9049e234401e1 Yasunori Goto 2007-10-21 4243 */
18004c5d4084d9 Christoph Lameter 2012-07-06 4244 mutex_unlock(&slab_mutex);
b9049e234401e1 Yasunori Goto 2007-10-21 4245 }
b9049e234401e1 Yasunori Goto 2007-10-21 4246
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC 3/3] mm, slub: stop freeing kmem_cache_node structures on node offline
2021-01-06 17:40 ` [RFC 3/3] mm, slub: stop freeing kmem_cache_node structures on node offline Vlastimil Babka
@ 2021-01-07 0:49 ` kernel test robot
0 siblings, 0 replies; 3+ messages in thread
From: kernel test robot @ 2021-01-07 0:49 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3790 bytes --]
Hi Vlastimil,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master hnaz-linux-mm/master v5.11-rc2 next-20210104]
[cannot apply to mmotm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vlastimil-Babka/mm-slab-slub-remove-cpu-and-memory-hotplug-locks/20210107-014224
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 09162bc32c880a791c6c0668ce0745cf7958f576
compiler: c6x-elf-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <rong.a.chen@intel.com>
cppcheck possible warnings: (new ones prefixed by >>, may not real problems)
>> mm/slub.c:4223:26: warning: Unused variable: n [unusedVariable]
struct kmem_cache_node *n;
^
>> mm/slub.c:4224:21: warning: Unused variable: s [unusedVariable]
struct kmem_cache *s;
^
mm/slub.c:5581:4: warning: Either the condition '!name' is redundant or there is pointer arithmetic with NULL pointer. [nullPointerArithmeticRedundantCheck]
*p++ = ':';
^
mm/slub.c:5579:9: note: Assuming that condition '!name' is not redundant
BUG_ON(!name);
^
mm/slub.c:5577:12: note: Assignment 'p=name', assigned value is 0
char *p = name;
^
mm/slub.c:5581:4: note: Null pointer addition
*p++ = ':';
^
vim +4223 mm/slub.c
b9049e234401e1 Yasunori Goto 2007-10-21 4220
b9049e234401e1 Yasunori Goto 2007-10-21 4221 static void slab_mem_offline_callback(void *arg)
b9049e234401e1 Yasunori Goto 2007-10-21 4222 {
b9049e234401e1 Yasunori Goto 2007-10-21 @4223 struct kmem_cache_node *n;
b9049e234401e1 Yasunori Goto 2007-10-21 @4224 struct kmem_cache *s;
b9049e234401e1 Yasunori Goto 2007-10-21 4225 struct memory_notify *marg = arg;
b9049e234401e1 Yasunori Goto 2007-10-21 4226 int offline_node;
b9049e234401e1 Yasunori Goto 2007-10-21 4227
b9d5ab2562ecee Lai Jiangshan 2012-12-11 4228 offline_node = marg->status_change_nid_normal;
b9049e234401e1 Yasunori Goto 2007-10-21 4229
b9049e234401e1 Yasunori Goto 2007-10-21 4230 /*
b9049e234401e1 Yasunori Goto 2007-10-21 4231 * If the node still has available memory. we need kmem_cache_node
b9049e234401e1 Yasunori Goto 2007-10-21 4232 * for it yet.
b9049e234401e1 Yasunori Goto 2007-10-21 4233 */
b9049e234401e1 Yasunori Goto 2007-10-21 4234 if (offline_node < 0)
b9049e234401e1 Yasunori Goto 2007-10-21 4235 return;
b9049e234401e1 Yasunori Goto 2007-10-21 4236
18004c5d4084d9 Christoph Lameter 2012-07-06 4237 mutex_lock(&slab_mutex);
27153f04e4a5f9 Vlastimil Babka 2021-01-06 4238 node_clear(offline_node, slab_nodes);
b9049e234401e1 Yasunori Goto 2007-10-21 4239 /*
9d5e878e997461 Vlastimil Babka 2021-01-06 4240 * We no longer free kmem_cache_node structures here, as it would be
9d5e878e997461 Vlastimil Babka 2021-01-06 4241 * racy with all get_node() users, and infeasible to protect them with
9d5e878e997461 Vlastimil Babka 2021-01-06 4242 * slab_mutex.
b9049e234401e1 Yasunori Goto 2007-10-21 4243 */
18004c5d4084d9 Christoph Lameter 2012-07-06 4244 mutex_unlock(&slab_mutex);
b9049e234401e1 Yasunori Goto 2007-10-21 4245 }
b9049e234401e1 Yasunori Goto 2007-10-21 4246
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 3+ messages in thread
* [RFC 3/3] mm, slub: stop freeing kmem_cache_node structures on node offline
2021-01-06 17:40 [RFC 0/3] mm, slab, slub: remove cpu and memory hotplug locks Vlastimil Babka
@ 2021-01-06 17:40 ` Vlastimil Babka
2021-01-07 0:49 ` kernel test robot
0 siblings, 1 reply; 3+ messages in thread
From: Vlastimil Babka @ 2021-01-06 17:40 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Vladimir Davydov, Qian Cai, David Hildenbrand,
Michal Hocko, Vlastimil Babka
Commit e4f8e513c3d3 ("mm/slub: fix a deadlock in show_slab_objects()") has
fixed a problematic locking order by removing the memory hotplug lock
get/put_online_mems() from show_slab_objects(). During the discussion, it was
argued [1] that this is OK, because existing slabs on the node would prevent
a hotremove to proceed.
That's true, but per-node kmem_cache_node structures are not necessarily
allocated on the same node and may exist even without actual slab pages
on the same node. Any path that uses get_node() directly or via
for_each_kmem_cache_node() (such as show_slab_objects()) can race with
freeing of kmem_cache_node even with the !NULL check, resulting in
use-after-free.
To that end, commit e4f8e513c3d3 argues in a comment that:
* We don't really need mem_hotplug_lock (to hold off
* slab_mem_going_offline_callback) here because slab's memory hot
* unplug code doesn't destroy the kmem_cache->node[] data.
While it's true that slab_mem_going_offline_callback() doesn't free
the kmem_cache_node, the later callback slab_mem_offline_callback() actually
does, so the race and use-after-free exists. Not just for show_slab_objects()
after commit e4f8e513c3d3, but also many other places that are not under
slab_mutex. And adding slab_mutex locking or other synchronization to SLUB
paths such as get_any_partial() would be bad for performance and error-prone.
The easiest solution is therefore to make the abovementioned comment true and
stop freeing the kmem_cache_node structures, accepting some wasted memory in
the full memory node removal scenario. Analogically we also don't free
hotremoved pgdat as mentioned in [1], nor the similar per-node structures in
SLAB. Importantly this approach will not block the hotremove, as generally such
nodes should be movable in order to succeed hotremove in the first place, and
thus the GFP_KERNEL allocated kmem_cache_node will come from elsewhere.
[1] https://lore.kernel.org/linux-mm/20190924151147.GB23050@dhcp22.suse.cz/
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/slub.c | 26 +++++++++++---------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index 2e2edd5c9cfc..d7c4f08dcf39 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4268,21 +4268,11 @@ static void slab_mem_offline_callback(void *arg)
mutex_lock(&slab_mutex);
node_clear(offline_node, slab_nodes);
- list_for_each_entry(s, &slab_caches, list) {
- n = get_node(s, offline_node);
- if (n) {
- /*
- * if n->nr_slabs > 0, slabs still exist on the node
- * that is going down. We were unable to free them,
- * and offline_pages() function shouldn't call this
- * callback. So, we must fail.
- */
- BUG_ON(slabs_node(s, offline_node));
-
- s->node[offline_node] = NULL;
- kmem_cache_free(kmem_cache_node, n);
- }
- }
+ /*
+ * We no longer free kmem_cache_node structures here, as it would be
+ * racy with all get_node() users, and infeasible to protect them with
+ * slab_mutex.
+ */
mutex_unlock(&slab_mutex);
}
@@ -4308,6 +4298,12 @@ static int slab_mem_going_online_callback(void *arg)
*/
mutex_lock(&slab_mutex);
list_for_each_entry(s, &slab_caches, list) {
+ /*
+ * The structure may already exist if the node was previously
+ * onlined and offlined.
+ */
+ if (get_node(s, nid))
+ continue;
/*
* XXX: kmem_cache_alloc_node will fallback to other nodes
* since memory is not yet available from the node that
--
2.29.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-07 0:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-07 0:44 [RFC 3/3] mm, slub: stop freeing kmem_cache_node structures on node offline kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-01-06 17:40 [RFC 0/3] mm, slab, slub: remove cpu and memory hotplug locks Vlastimil Babka
2021-01-06 17:40 ` [RFC 3/3] mm, slub: stop freeing kmem_cache_node structures on node offline Vlastimil Babka
2021-01-07 0:49 ` kernel test robot
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.