| Message ID | alpine.DEB.2.00.1201170927020.4800@router.home |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mardi 17 janvier 2012 à 09:27 -0600, Christoph Lameter a écrit : > Subject: slub: Do not hold slub_lock when calling sysfs_slab_add() > > sysfs_slab_add() calls various sysfs functions that actually may > end up in userspace doing all sorts of things. > > Release the slub_lock after adding the kmem_cache structure to the list. > At that point the address of the kmem_cache is not known so we are > guaranteed exlusive access to the following modifications to the > kmem_cache structure. > > If the sysfs_slab_add fails then reacquire the slub_lock to > remove the kmem_cache structure from the list. > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > Signed-off-by: Christoph Lameter <cl@linux.com> > > --- > mm/slub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > Index: linux-2.6/mm/slub.c > =================================================================== > --- linux-2.6.orig/mm/slub.c 2012-01-17 03:07:11.140010438 -0600 > +++ linux-2.6/mm/slub.c 2012-01-17 03:26:06.799986908 -0600 > @@ -3929,13 +3929,14 @@ struct kmem_cache *kmem_cache_create(con > if (kmem_cache_open(s, n, > size, align, flags, ctor)) { > list_add(&s->list, &slab_caches); > + up_write(&slub_lock); > if (sysfs_slab_add(s)) { > + down_write(&slub_lock); > list_del(&s->list); > kfree(n); > kfree(s); > goto err; > } > - up_write(&slub_lock); > return s; > } > kfree(n); Thanks ! Acked-by: Eric Dumazet <eric.dumazet@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 17 Jan 2012, Eric Dumazet wrote: > Thanks ! > > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> That may not be the end of it. Slub also calls sysfs from sysfs_add_alias while holding slub_lock. If sysfs allows user space stuff to run then you cannot really hold any locks. How is one supposed to sync adding pointers to sysfs structures in subsystems? Drop all locks and then recheck the memory structures after the sysfs function returns? Awkward. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 17, 2012 at 5:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mardi 17 janvier 2012 à 09:27 -0600, Christoph Lameter a écrit : > >> Subject: slub: Do not hold slub_lock when calling sysfs_slab_add() >> >> sysfs_slab_add() calls various sysfs functions that actually may >> end up in userspace doing all sorts of things. >> >> Release the slub_lock after adding the kmem_cache structure to the list. >> At that point the address of the kmem_cache is not known so we are >> guaranteed exlusive access to the following modifications to the >> kmem_cache structure. >> >> If the sysfs_slab_add fails then reacquire the slub_lock to >> remove the kmem_cache structure from the list. >> >> Reported-by: Sasha Levin <levinsasha928@gmail.com> >> Signed-off-by: Christoph Lameter <cl@linux.com> >> >> --- >> mm/slub.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> Index: linux-2.6/mm/slub.c >> =================================================================== >> --- linux-2.6.orig/mm/slub.c 2012-01-17 03:07:11.140010438 -0600 >> +++ linux-2.6/mm/slub.c 2012-01-17 03:26:06.799986908 -0600 >> @@ -3929,13 +3929,14 @@ struct kmem_cache *kmem_cache_create(con >> if (kmem_cache_open(s, n, >> size, align, flags, ctor)) { >> list_add(&s->list, &slab_caches); >> + up_write(&slub_lock); >> if (sysfs_slab_add(s)) { >> + down_write(&slub_lock); >> list_del(&s->list); >> kfree(n); >> kfree(s); >> goto err; >> } >> - up_write(&slub_lock); >> return s; >> } >> kfree(n); > > Thanks ! > > Acked-by: Eric Dumazet <eric.dumazet@gmail.com> I'm planning to queue this for v3.4 and tagging it for -stable for v3.3. Do we need this for older versions as well? Pekka -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/mm/slub.c =================================================================== --- linux-2.6.orig/mm/slub.c 2012-01-17 03:07:11.140010438 -0600 +++ linux-2.6/mm/slub.c 2012-01-17 03:26:06.799986908 -0600 @@ -3929,13 +3929,14 @@ struct kmem_cache *kmem_cache_create(con if (kmem_cache_open(s, n, size, align, flags, ctor)) { list_add(&s->list, &slab_caches); + up_write(&slub_lock); if (sysfs_slab_add(s)) { + down_write(&slub_lock); list_del(&s->list); kfree(n); kfree(s); goto err; } - up_write(&slub_lock); return s; } kfree(n);