mm/util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
One codepath in find_alloc_undo() calls kvfree() while holding a spinlock.
Since vfree() can sleep this is a bug.
Previously, the code path used kfree(), and kfree() is safe to be called
while holding a spinlock.
Minghao proposed to fix this by updating find_alloc_undo().
Alternate proposal to fix this: Instead of changing find_alloc_undo(),
change kvfree() so that the same rules as for kfree() apply:
Having different rules for kfree() and kvfree() just asks for bugs.
Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
Reported-by: Zeal Robot <zealci@zte.com.cn>
Reported-by: Minghao Chi <chi.minghao@zte.com.cn>
Link: https://lore.kernel.org/all/20211222081026.484058-1-chi.minghao@zte.com.cn/
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation")
Cc: stable@vger.kernel.org
Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
mm/util.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/util.c b/mm/util.c
index 741ba32a43ac..7f9181998835 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -610,12 +610,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 interrupt.
*/
void kvfree(const void *addr)
{
if (is_vmalloc_addr(addr))
- vfree(addr);
+ vfree_atomic(addr);
else
kfree(addr);
}
--
2.33.1
Cc'ing more mm folks.
On 2021-12-22 11:48, Manfred Spraul wrote:
> One codepath in find_alloc_undo() calls kvfree() while holding a
> spinlock.
> Since vfree() can sleep this is a bug.
afaict the only other offender is devx_async_cmd_event_destroy_uobj(),
in drivers/infiniband/hw/mlx5/devx.c. I was expecting to find more,
actually.
> Previously, the code path used kfree(), and kfree() is safe to be
> called
> while holding a spinlock.
>
> Minghao proposed to fix this by updating find_alloc_undo().
>
> Alternate proposal to fix this: Instead of changing find_alloc_undo(),
> change kvfree() so that the same rules as for kfree() apply:
> Having different rules for kfree() and kvfree() just asks for bugs.
I agree that it is best to have the same atomic semantics across all
family of calls.
>
> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
I would not expect the added latency to be a big deal unless under
serious memory pressure, for which case things are already fragile to
begin with. Furthermore users of kvfree() are already warned that this
is the slower choice. Feel free to add my:
Acked-by: Davidlohr Bueso <dbueso@suse.de>
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Reported-by: Minghao Chi <chi.minghao@zte.com.cn>
> Link:
> https://lore.kernel.org/all/20211222081026.484058-1-chi.minghao@zte.com.cn/
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
> allocation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
> mm/util.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/util.c b/mm/util.c
> index 741ba32a43ac..7f9181998835 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -610,12 +610,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 interrupt.
> */
> void kvfree(const void *addr)
> {
> if (is_vmalloc_addr(addr))
> - vfree(addr);
> + vfree_atomic(addr);
> else
> kfree(addr);
> }
On 22.12.2021 22:48, Manfred Spraul wrote:
> One codepath in find_alloc_undo() calls kvfree() while holding a spinlock.
> Since vfree() can sleep this is a bug.
>
> Previously, the code path used kfree(), and kfree() is safe to be called
> while holding a spinlock.
>
> Minghao proposed to fix this by updating find_alloc_undo().
>
> Alternate proposal to fix this: Instead of changing find_alloc_undo(),
> change kvfree() so that the same rules as for kfree() apply:
> Having different rules for kfree() and kvfree() just asks for bugs.
>
> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Reported-by: Minghao Chi <chi.minghao@zte.com.cn>
> Link: https://lore.kernel.org/all/20211222081026.484058-1-chi.minghao@zte.com.cn/
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
> mm/util.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/util.c b/mm/util.c
> index 741ba32a43ac..7f9181998835 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -610,12 +610,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 interrupt.
> */
> void kvfree(const void *addr)
> {
> if (is_vmalloc_addr(addr))
> - vfree(addr);
> + vfree_atomic(addr);
> else
> kfree(addr);
> }
I would prefer to release memory ASAP if it's possible.
What do you think about this change?
--- a/mm/util.c
+++ b/mm/util.c
@@ -614,9 +614,12 @@ EXPORT_SYMBOL(kvmalloc_node);
*/
void kvfree(const void *addr)
{
- if (is_vmalloc_addr(addr))
- vfree(addr);
- else
+ if (is_vmalloc_addr(addr)) {
+ if (in_atomic())
+ vfree_atomic();
+ else
+ vfree(addr);
+ } else
kfree(addr);
}
EXPORT_SYMBOL(kvfree);
Hello Vasily,
On 12/23/21 08:21, Vasily Averin wrote:
>
> I would prefer to release memory ASAP if it's possible.
> What do you think about this change?
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -614,9 +614,12 @@ EXPORT_SYMBOL(kvmalloc_node);
> */
> void kvfree(const void *addr)
> {
> - if (is_vmalloc_addr(addr))
> - vfree(addr);
> - else
> + if (is_vmalloc_addr(addr)) {
> + if (in_atomic())
> + vfree_atomic();
> + else
> + vfree(addr);
> + } else
> kfree(addr);
> }
> EXPORT_SYMBOL(kvfree);
>
Unfortunately this cannot work:
> /*
> * Are we running in atomic context? WARNING: this macro cannot
> * always detect atomic context; in particular, it cannot know about
> * held spinlocks in non-preemptible kernels. Thus it should not be
> * used in the general case to determine whether sleeping is possible.
> * Do not use in_atomic() in driver code.
> */
> #define in_atomic() (preempt_count() != 0)
>
--
Manfred
On 23.12.2021 14:52, Manfred Spraul wrote:
> Hello Vasily,
>
> On 12/23/21 08:21, Vasily Averin wrote:
>>
>> I would prefer to release memory ASAP if it's possible.
>> What do you think about this change?
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -614,9 +614,12 @@ EXPORT_SYMBOL(kvmalloc_node);
>> */
>> void kvfree(const void *addr)
>> {
>> - if (is_vmalloc_addr(addr))
>> - vfree(addr);
>> - else
>> + if (is_vmalloc_addr(addr)) {
>> + if (in_atomic())
>> + vfree_atomic();
>> + else
>> + vfree(addr);
>> + } else
>> kfree(addr);
>> }
>> EXPORT_SYMBOL(kvfree);
>>
> Unfortunately this cannot work:
yes, you're right and I do not see any better solution yet.
> One codepath in find_alloc_undo() calls kvfree() while holding a spinlock.
> Since vfree() can sleep this is a bug.
>
> Previously, the code path used kfree(), and kfree() is safe to be called
> while holding a spinlock.
>
> Minghao proposed to fix this by updating find_alloc_undo().
>
> Alternate proposal to fix this: Instead of changing find_alloc_undo(),
> change kvfree() so that the same rules as for kfree() apply:
> Having different rules for kfree() and kvfree() just asks for bugs.
>
> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
>
I guess the issues is with "vmap_purge_lock" mutex? I think it is better
to make the vfree() call as non-blocking one, i.e. the current design is
is suffering from one drawback. It is related to purging the outstanding
lazy areas from caller context. The drain process can be time consuming
and if it is done from high-prio or RT contexts it can hog a CPU. Another
issue is what you have reported that is about calling the schedule() and
holding spinlock. The proposal is to perform a drain in a separate work:
<snip>
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d2a00ad4e1dd..7c5d9b148fa4 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1717,18 +1717,6 @@ static bool __purge_vmap_area_lazy(unsigned long start, unsigned long end)
return true;
}
-/*
- * Kick off a purge of the outstanding lazy areas. Don't bother if somebody
- * is already purging.
- */
-static void try_purge_vmap_area_lazy(void)
-{
- if (mutex_trylock(&vmap_purge_lock)) {
- __purge_vmap_area_lazy(ULONG_MAX, 0);
- mutex_unlock(&vmap_purge_lock);
- }
-}
-
/*
* Kick off a purge of the outstanding lazy areas.
*/
@@ -1740,6 +1728,16 @@ static void purge_vmap_area_lazy(void)
mutex_unlock(&vmap_purge_lock);
}
+static void drain_vmap_area(struct work_struct *work)
+{
+ if (mutex_trylock(&vmap_purge_lock)) {
+ __purge_vmap_area_lazy(ULONG_MAX, 0);
+ mutex_unlock(&vmap_purge_lock);
+ }
+}
+
+static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area);
+
/*
* Free a vmap area, caller ensuring that the area has been unmapped
* and flush_cache_vunmap had been called for the correct range
@@ -1766,7 +1764,7 @@ static void free_vmap_area_noflush(struct vmap_area *va)
/* After this point, we may free va at any time */
if (unlikely(nr_lazy > lazy_max_pages()))
- try_purge_vmap_area_lazy();
+ schedule_work(&drain_vmap_area_work);
}
/*
<snip>
--
Vlad Rezki
On Sat, Dec 25, 2021 at 07:54:12PM +0100, Uladzislau Rezki wrote:
> +static void drain_vmap_area(struct work_struct *work)
> +{
> + if (mutex_trylock(&vmap_purge_lock)) {
> + __purge_vmap_area_lazy(ULONG_MAX, 0);
> + mutex_unlock(&vmap_purge_lock);
> + }
> +}
> +
> +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area);
Presuambly if the worker fails to get the mutex, it should reschedule
itself? And should it even trylock or just always lock?
This kind of ties into something I've been wondering about -- we have
a number of places in the kernel which cache 'freed' vmalloc allocations
in order to speed up future allocations of the same size. Kind of like
slab. Would we be better off trying to cache frequent allocations
inside vmalloc instead of always purging them?
On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote:
> One codepath in find_alloc_undo() calls kvfree() while holding a spinlock.
> Since vfree() can sleep this is a bug.
>
> Previously, the code path used kfree(), and kfree() is safe to be called
> while holding a spinlock.
>
> Minghao proposed to fix this by updating find_alloc_undo().
>
> Alternate proposal to fix this: Instead of changing find_alloc_undo(),
> change kvfree() so that the same rules as for kfree() apply:
> Having different rules for kfree() and kvfree() just asks for bugs.
>
> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
I know we've been around this loop a bunch of times and deferring was
considered. But I forget the conclusion. IIRC, mhocko was involved?
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -610,12 +610,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 interrupt.
> */
> void kvfree(const void *addr)
> {
> if (is_vmalloc_addr(addr))
> - vfree(addr);
> + vfree_atomic(addr);
> else
> kfree(addr);
> }
Hi Andrew,
On 1/27/22 03:53, Andrew Morton wrote:
> On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote:
>
>> One codepath in find_alloc_undo() calls kvfree() while holding a spinlock.
>> Since vfree() can sleep this is a bug.
>>
>> Previously, the code path used kfree(), and kfree() is safe to be called
>> while holding a spinlock.
>>
>> Minghao proposed to fix this by updating find_alloc_undo().
>>
>> Alternate proposal to fix this: Instead of changing find_alloc_undo(),
>> change kvfree() so that the same rules as for kfree() apply:
>> Having different rules for kfree() and kvfree() just asks for bugs.
>>
>> Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
> I know we've been around this loop a bunch of times and deferring was
> considered. But I forget the conclusion. IIRC, mhocko was involved?
I do not remember a mail from mhocko.
Shakeel proposed to use the approach from Chi.
Decision: https://marc.info/?l=linux-kernel&m=164132032717757&w=2
With Reviewed-by:
https://marc.info/?l=linux-kernel&m=164132744522325&w=2
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -610,12 +610,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 interrupt.
>> */
>> void kvfree(const void *addr)
>> {
>> if (is_vmalloc_addr(addr))
>> - vfree(addr);
>> + vfree_atomic(addr);
>> else
>> kfree(addr);
>> }
On Thu 27-01-22 06:59:50, Manfred Spraul wrote:
> Hi Andrew,
>
> On 1/27/22 03:53, Andrew Morton wrote:
> > On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote:
> >
> > > One codepath in find_alloc_undo() calls kvfree() while holding a spinlock.
> > > Since vfree() can sleep this is a bug.
> > >
> > > Previously, the code path used kfree(), and kfree() is safe to be called
> > > while holding a spinlock.
> > >
> > > Minghao proposed to fix this by updating find_alloc_undo().
> > >
> > > Alternate proposal to fix this: Instead of changing find_alloc_undo(),
> > > change kvfree() so that the same rules as for kfree() apply:
> > > Having different rules for kfree() and kvfree() just asks for bugs.
> > >
> > > Disadvantage: Releasing vmalloc'ed memory will be delayed a bit.
> > I know we've been around this loop a bunch of times and deferring was
> > considered. But I forget the conclusion. IIRC, mhocko was involved?
>
> I do not remember a mail from mhocko.
I do not remember either.
>
> Shakeel proposed to use the approach from Chi.
>
> Decision: https://marc.info/?l=linux-kernel&m=164132032717757&w=2
And I would agree with Shakeel and go with the original change to the
ipc code. That is trivial and without any other side effects like this
one. I bet nobody has evaluated what the undconditional deferred freeing
has. At least changelog doesn't really dive into that more than a very
vague statement that this will happen.
> With Reviewed-by:
>
> https://marc.info/?l=linux-kernel&m=164132744522325&w=2
> > > --- a/mm/util.c
> > > +++ b/mm/util.c
> > > @@ -610,12 +610,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 interrupt.
> > > */
> > > void kvfree(const void *addr)
> > > {
> > > if (is_vmalloc_addr(addr))
> > > - vfree(addr);
> > > + vfree_atomic(addr);
> > > else
> > > kfree(addr);
> > > }
>
--
Michal Hocko
SUSE Labs
On Thu, Jan 27, 2022 at 09:25:48AM +0100, Michal Hocko wrote: > On Thu 27-01-22 06:59:50, Manfred Spraul wrote: > > Hi Andrew, > > > > On 1/27/22 03:53, Andrew Morton wrote: > > > On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <manfred@colorfullife.com> wrote: > > > > > > > One codepath in find_alloc_undo() calls kvfree() while holding a spinlock. > > > > Since vfree() can sleep this is a bug. > > > > > > > > Previously, the code path used kfree(), and kfree() is safe to be called > > > > while holding a spinlock. > > > > > > > > Minghao proposed to fix this by updating find_alloc_undo(). > > > > > > > > Alternate proposal to fix this: Instead of changing find_alloc_undo(), > > > > change kvfree() so that the same rules as for kfree() apply: > > > > Having different rules for kfree() and kvfree() just asks for bugs. > > > > > > > > Disadvantage: Releasing vmalloc'ed memory will be delayed a bit. > > > I know we've been around this loop a bunch of times and deferring was > > > considered. But I forget the conclusion. IIRC, mhocko was involved? > > > > I do not remember a mail from mhocko. > > I do not remember either. > > > > > Shakeel proposed to use the approach from Chi. > > > > Decision: https://marc.info/?l=linux-kernel&m=164132032717757&w=2 > > And I would agree with Shakeel and go with the original change to the > ipc code. That is trivial and without any other side effects like this > one. I bet nobody has evaluated what the undconditional deferred freeing > has. At least changelog doesn't really dive into that more than a very > vague statement that this will happen. > Absolutely agree here. Especially that changing the kvfree() will not look stable. After applying the https://www.spinics.net/lists/linux-mm/msg282264.html we will be able to use vfree() from atomic anyway. -- Vlad Rezki
© 2016 - 2026 Red Hat, Inc.