| Message ID | alpine.DEB.2.21.1911111553020.15366@www.lameter.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [FIX] slub: Remove kmalloc under list_lock from list_slab_objects() V2 | expand |
On Mon, 11 Nov 2019 15:55:05 +0000 (UTC) Christopher Lameter <cl@linux.com> wrote: > Regardless of the issue with memcgs allowing allocations from its > kmalloc array during shutdown: This patch cleans things up and properly > allocates the bitmap outside of the list_lock. > > > [FIX] slub: Remove kmalloc under list_lock from list_slab_objects() V2 > > V1->V2 : Properly handle CONFIG_SLUB_DEBUG. Handle bitmap free correctly. > > list_slab_objects() is called when a slab is destroyed and there are objects still left > to list the objects in the syslog. This is a pretty rare event. > > And there it seems we take the list_lock and call kmalloc while holding that lock. > > Perform the allocation in free_partial() before the list_lock is taken. No response here? It looks a lot simpler than the originally proposed patch? > --- linux.orig/mm/slub.c 2019-10-15 13:54:57.032655296 +0000 > +++ linux/mm/slub.c 2019-11-11 15:52:11.616397853 +0000 > @@ -3690,14 +3690,15 @@ error: > } > > static void list_slab_objects(struct kmem_cache *s, struct page *page, > - const char *text) > + const char *text, unsigned long *map) > { > #ifdef CONFIG_SLUB_DEBUG > void *addr = page_address(page); > void *p; > - unsigned long *map = bitmap_zalloc(page->objects, GFP_ATOMIC); > + > if (!map) > return; > + > slab_err(s, page, text, s->name); > slab_lock(page); > > @@ -3710,7 +3711,6 @@ static void list_slab_objects(struct kme > } > } > slab_unlock(page); > - bitmap_free(map); > #endif > } > > @@ -3723,6 +3723,11 @@ static void free_partial(struct kmem_cac > { > LIST_HEAD(discard); > struct page *page, *h; > + unsigned long *map = NULL; > + > +#ifdef CONFIG_SLUB_DEBUG > + map = bitmap_alloc(oo_objects(s->max), GFP_KERNEL); > +#endif > > BUG_ON(irqs_disabled()); > spin_lock_irq(&n->list_lock); > @@ -3732,11 +3737,16 @@ static void free_partial(struct kmem_cac > list_add(&page->slab_list, &discard); > } else { > list_slab_objects(s, page, > - "Objects remaining in %s on __kmem_cache_shutdown()"); > + "Objects remaining in %s on __kmem_cache_shutdown()", > + map); > } > } > spin_unlock_irq(&n->list_lock); > > +#ifdef CONFIG_SLUB_DEBUG > + bitmap_free(map); > +#endif > + > list_for_each_entry_safe(page, h, &discard, slab_list) > discard_slab(s, page); > }
On Sat, 30 Nov 2019, Andrew Morton wrote: > > Perform the allocation in free_partial() before the list_lock is taken. > > No response here? It looks a lot simpler than the originally proposed > patch? Yup. I prefer this one but its my own patch so I cannot Ack this.
On Mon, Dec 02, 2019 at 03:12:20PM +0000, Christopher Lameter wrote: > On Sat, 30 Nov 2019, Andrew Morton wrote: > > > > Perform the allocation in free_partial() before the list_lock is taken. > > > > No response here? It looks a lot simpler than the originally proposed > > patch? > > Yup. I prefer this one but its my own patch so I cannot Ack this. Hi, there is a pending question from Tetsuo-san. I'd be happy to ack once it's address.
On 12/7/19 11:03 PM, Yu Zhao wrote: > On Mon, Dec 02, 2019 at 03:12:20PM +0000, Christopher Lameter wrote: >> On Sat, 30 Nov 2019, Andrew Morton wrote: >> >>>> Perform the allocation in free_partial() before the list_lock is taken. >>> >>> No response here? It looks a lot simpler than the originally proposed >>> patch? >> >> Yup. I prefer this one but its my own patch so I cannot Ack this. > > Hi, there is a pending question from Tetsuo-san. I'd be happy to ack > once it's address. Tetsuo's mails don't reach linux-mm for a while and he has given up trying to do something about it. It's hard to discuss anything outside the direct CC group then. I don't know what's the pending question, for example.
On Sun, 12 Jan 2020, Tetsuo Handa wrote: > On 2020/01/10 23:11, Vlastimil Babka wrote: > Hmm, this one? Even non-ML destinations are sometimes rejected (e.g. > 554 5.7.1 Service unavailable; Client host [202.181.97.72] blocked using b.barracudacentral.org; http://www.barracudanetworks.com/reputation/?pr=1&ip=202.181.97.72 > ). Anyway, I just worried whether it is really safe to do memory allocation > which might involve memory reclaim. You MM guys know better... We are talking about a call to destroy the kmem_cache. This is not done under any lock. The lock was taken inside that function before the call to list_slab_objects. That can be avoided.
Index: linux/mm/slub.c =================================================================== --- linux.orig/mm/slub.c 2019-10-15 13:54:57.032655296 +0000 +++ linux/mm/slub.c 2019-11-11 15:52:11.616397853 +0000 @@ -3690,14 +3690,15 @@ error: } static void list_slab_objects(struct kmem_cache *s, struct page *page, - const char *text) + const char *text, unsigned long *map) { #ifdef CONFIG_SLUB_DEBUG void *addr = page_address(page); void *p; - unsigned long *map = bitmap_zalloc(page->objects, GFP_ATOMIC); + if (!map) return; + slab_err(s, page, text, s->name); slab_lock(page); @@ -3710,7 +3711,6 @@ static void list_slab_objects(struct kme } } slab_unlock(page); - bitmap_free(map); #endif } @@ -3723,6 +3723,11 @@ static void free_partial(struct kmem_cac { LIST_HEAD(discard); struct page *page, *h; + unsigned long *map = NULL; + +#ifdef CONFIG_SLUB_DEBUG + map = bitmap_alloc(oo_objects(s->max), GFP_KERNEL); +#endif BUG_ON(irqs_disabled()); spin_lock_irq(&n->list_lock); @@ -3732,11 +3737,16 @@ static void free_partial(struct kmem_cac list_add(&page->slab_list, &discard); } else { list_slab_objects(s, page, - "Objects remaining in %s on __kmem_cache_shutdown()"); + "Objects remaining in %s on __kmem_cache_shutdown()", + map); } } spin_unlock_irq(&n->list_lock); +#ifdef CONFIG_SLUB_DEBUG + bitmap_free(map); +#endif + list_for_each_entry_safe(page, h, &discard, slab_list) discard_slab(s, page); }
Regardless of the issue with memcgs allowing allocations from its kmalloc array during shutdown: This patch cleans things up and properly allocates the bitmap outside of the list_lock. [FIX] slub: Remove kmalloc under list_lock from list_slab_objects() V2 V1->V2 : Properly handle CONFIG_SLUB_DEBUG. Handle bitmap free correctly. list_slab_objects() is called when a slab is destroyed and there are objects still left to list the objects in the syslog. This is a pretty rare event. And there it seems we take the list_lock and call kmalloc while holding that lock. Perform the allocation in free_partial() before the list_lock is taken. Fixes: bbd7d57bfe852d9788bae5fb171c7edb4021d8ac ("slub: Potential stack overflow") Signed-off-by: Christoph Lameter