mm: check for sleepable context in kvfree
Subsystems (2)
PATCH 1/1 mm: check for sleepable context in kvfree
A lot of callers of kvfree only go down the vfree path under very rare circumstances, and so may never end up hitting the might_sleep_if in it. Ensure that when kvfree is called, that it is operating in a context where it is allowed to sleep.
--- mm/util.c | 2 ++ 1 file changed, 2 insertions(+)
mm/util.c +2 -0
@@ -482,6 +482,8 @@ EXPORT_SYMBOL(kvmalloc_node);*/ void kvfree (const void *addr) { +might_sleep_if (!in_interrupt ()); +if (is_vmalloc_addr (addr))vfree (addr);else
Signature · 1 line
2.21.0
Quoted 5 lines · Jeff Layton
A lot of callers of kvfree only go down the vfree path under very rare circumstances, and so may never end up hitting the
might_sleep_ifin it. Ensure that when kvfree is called, that it is operating in a context where it is allowed to sleep.
Quoted 7 lines
Cc Alexander Viro <[email protected]>Cc Luis Henriques <[email protected]>Signed-off-by Jeff Layton <[email protected]>--- mm/util.c | 2 ++ 1 file changed, 2 insertions(+)
FWIW, I started looking at this after Luis sent me some ceph patches that fixed a few of these problems. I have not done extensive testing with this patch, so maybe consider this an RFC for now.
HCH points out that xfs uses kvfree as a generic "free this no matter what it is" sort of wrapper and expects the callers to work out whether they might be freeing a vmalloc'ed address. If that sort of usage turns out to be prevalent, then we may need another approach to clean this up.
Quoted 13 lines
mm/util.c +2
-0@@ -482,6 +482,8 @@ EXPORT_SYMBOL(kvmalloc_node);*/ void kvfree (const void *addr) { +might_sleep_if (!in_interrupt ()); +if (is_vmalloc_addr (addr))vfree (addr);else
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 14 lines · Jeff Layton
Quoted 5 lines · Jeff Layton
A lot of callers of kvfree only go down the vfree path under very rare circumstances, and so may never end up hitting the
might_sleep_ifin it. Ensure that when kvfree is called, that it is operating in a context where it is allowed to sleep.Quoted 7 lines
Cc Alexander Viro <[email protected]>Cc Luis Henriques <[email protected]>Signed-off-by Jeff Layton <[email protected]>--- mm/util.c | 2 ++ 1 file changed, 2 insertions(+)
Quoted 8 lines
FWIW, I started looking at this after Luis sent me some ceph patches that fixed a few of these problems. I have not done extensive testing with this patch, so maybe consider this an RFC for now.
HCH points out that xfs uses kvfree as a generic "free this no matter what it is" sort of wrapper and expects the callers to work out whether they might be freeing a vmalloc'ed address. If that sort of usage turns out to be prevalent, then we may need another approach to clean this up.
I think it's a bit of a landmine, to be honest. How about we have kvfree() call vfree_atomic() instead?
Quoted 19 lines · Matthew Wilcox
Quoted 18 lines · Jeff Layton
Quoted 5 lines · Jeff Layton
A lot of callers of kvfree only go down the vfree path under very rare circumstances, and so may never end up hitting the
might_sleep_ifin it. Ensure that when kvfree is called, that it is operating in a context where it is allowed to sleep.Quoted 7 lines
Cc Alexander Viro <[email protected]>Cc Luis Henriques <[email protected]>Signed-off-by Jeff Layton <[email protected]>--- mm/util.c | 2 ++ 1 file changed, 2 insertions(+)FWIW, I started looking at this after Luis sent me some ceph patches that fixed a few of these problems. I have not done extensive testing with this patch, so maybe consider this an RFC for now.
Quoted 7 lines
Quoted 4 lines
HCH points out that xfs uses kvfree as a generic "free this no matter what it is" sort of wrapper and expects the callers to work out whether they might be freeing a vmalloc'ed address. If that sort of usage turns out to be prevalent, then we may need another approach to clean this up.
I think it's a bit of a landmine, to be honest. How about we have
kvfree()callvfree_atomic()instead?
Not a bad idea, though it means more overhead for the vfree case.
Since we're spitballing here...could we have kvfree figure out whether it's running in a context where it would need to queue it instead and only do it in that case?
We currently have to figure that out for the might_sleep_if anyway. We could just have it DTRT instead of printk'ing and dumping the stack in that case.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 11 lines · Jeff Layton
Quoted 7 lines · Matthew Wilcox
Quoted 4 lines
HCH points out that xfs uses kvfree as a generic "free this no matter what it is" sort of wrapper and expects the callers to work out whether they might be freeing a vmalloc'ed address. If that sort of usage turns out to be prevalent, then we may need another approach to clean this up.
I think it's a bit of a landmine, to be honest. How about we have
kvfree()callvfree_atomic()instead?Not a bad idea, though it means more overhead for the vfree case.
Quoted 7 lines
Since we're spitballing here...could we have kvfree figure out whether it's running in a context where it would need to queue it instead and only do it in that case?
We currently have to figure that out for the
might_sleep_ifanyway. We could just have it DTRT instead of printk'ing and dumping the stack in that case.
I don't think we have a generic way to determine if we're currently holding a spinlock. ie this can fail:
spin_lock(&my_lock);
kvfree(p);spin_unlock(&my_lock);
If we're preemptible, we can check the preempt count, but !CONFIG_PREEMPT doesn't record the number of spinlocks currently taken.
Quoted 23 lines · Matthew Wilcox
Quoted 11 lines · Jeff Layton
Quoted 7 lines · Matthew Wilcox
Quoted 4 lines
HCH points out that xfs uses kvfree as a generic "free this no matter what it is" sort of wrapper and expects the callers to work out whether they might be freeing a vmalloc'ed address. If that sort of usage turns out to be prevalent, then we may need another approach to clean this up.
I think it's a bit of a landmine, to be honest. How about we have
kvfree()callvfree_atomic()instead?Not a bad idea, though it means more overhead for the vfree case.
Quoted 7 lines
Since we're spitballing here...could we have kvfree figure out whether it's running in a context where it would need to queue it instead and only do it in that case?
We currently have to figure that out for the
might_sleep_ifanyway. We could just have it DTRT instead of printk'ing and dumping the stack in that case.I don't think we have a generic way to determine if we're currently holding a spinlock. ie this can fail:
Quoted 6 lines
spin_lock(&my_lock);
kvfree(p);spin_unlock(&my_lock);If we're preemptible, we can check the preempt count, but !CONFIG_PREEMPT doesn't record the number of spinlocks currently taken.
Ahh right...that makes sense.
Al also suggested on IRC that we could add a kvfree_atomic if that were useful. That might be good for new callers, but we'd probably need a patch like this one to suss out which of the existing kvfree callers would need to switch to using it.
I think you're quite right that this is a landmine. That said, this seems like something we ought to try to clean up.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 28 lines · Jeff Layton
Quoted 18 lines · Matthew Wilcox
Quoted 6 lines · Jeff Layton
Quoted 2 lines · Matthew Wilcox
I think it's a bit of a landmine, to be honest. How about we have
kvfree()callvfree_atomic()instead?Not a bad idea, though it means more overhead for the vfree case.
Quoted 7 lines
Since we're spitballing here...could we have kvfree figure out whether it's running in a context where it would need to queue it instead and only do it in that case?
We currently have to figure that out for the
might_sleep_ifanyway. We could just have it DTRT instead of printk'ing and dumping the stack in that case.I don't think we have a generic way to determine if we're currently holding a spinlock. ie this can fail:
Quoted 6 lines
spin_lock(&my_lock);
kvfree(p);spin_unlock(&my_lock);If we're preemptible, we can check the preempt count, but !CONFIG_PREEMPT doesn't record the number of spinlocks currently taken.
Ahh right...that makes sense.
Quoted 7 lines
Al also suggested on IRC that we could add a
kvfree_atomicif that were useful. That might be good for new callers, but we'd probably need a patch like this one to suss out which of the existing kvfree callers would need to switch to using it.I think you're quite right that this is a landmine. That said, this seems like something we ought to try to clean up.
I'd rather add a kvfree_fast(). So something like this:
mm/util.c +24 -2
@@ -470,6 +470,28 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node) }EXPORT_SYMBOL (kvmalloc_node); +/** +* kvfree_fast() - Free memory. +* @addr: Pointer to allocated memory. +* +* kvfree_fast frees memory allocated by any of vmalloc(), kmalloc() or +* kvmalloc(). It is slightly more efficient to use kfree() or vfree() if +* you are certain that you know which one to use. +* +* Context: Either preemptible task context or not-NMI interrupt. Must not +* hold a spinlock as it can sleep. +*/ +void kvfree_fast (const void *addr) +{ +might_sleep (); + +if (is_vmalloc_addr (addr)) +vfree (addr); +else +kfree (addr); +} +EXPORT_SYMBOL (kvfree_fast); +/** * kvfree() - Free memory. * @addr: Pointer to allocated memory.
@@ -478,12 +500,12 @@ EXPORT_SYMBOL(kvmalloc_node);* It is slightly more efficient to use kfree() or vfree() if you are certain * that you know which one to use. * -* Context: Either preemptible task context or not-NMI interrupt. +* Context: Any context except NMI. */ void kvfree (const void *addr) {if (is_vmalloc_addr (addr)) -vfree (addr); +vfree_atomic (addr);else kfree (addr); }