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 <[email protected]>
Reported-by: Minghao Chi <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation")
Cc: [email protected]
Signed-off-by: Manfred Spraul <[email protected]>
---
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 <[email protected]>
>
> Reported-by: Zeal Robot <[email protected]>
> Reported-by: Minghao Chi <[email protected]>
> Link:
> https://lore.kernel.org/all/[email protected]/
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
> allocation")
> Cc: [email protected]
> Signed-off-by: Manfred Spraul <[email protected]>
> ---
> 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 <[email protected]>
> Reported-by: Minghao Chi <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation")
> Cc: [email protected]
> Signed-off-by: Manfred Spraul <[email protected]>
> ---
> 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 Sat, Dec 25, 2021 at 10:58:29PM +0000, Matthew Wilcox wrote:
> 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?
>
mutex_trylock() has no sense here. It should just always get the lock.
Otherwise we can miss the point to purge. Agree with your opinion.
>
> 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?
>
Hm... Some sort of caching would be good. Though it will require some
time to think over all details and design itself. We can cache VAs
instead of purging them until some point or threshold. So basically
we can keep it in our data structures, associate it with some cache,
based on size and reuse it later in the alloc_vmap_area().
All that is related to "vmap_area" caching. Another option is to cache
the "vm_struct". It includes "vmap_area" + pages to drive the mapping.
It is a higher level of caching and i am not sure if an implementation
would be so straightforward.
--
Vlad Rezki
On Sun, Dec 26, 2021 at 06:57:16PM +0100, Uladzislau Rezki wrote:
> On Sat, Dec 25, 2021 at 10:58:29PM +0000, Matthew Wilcox wrote:
> > 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?
> >
> mutex_trylock() has no sense here. It should just always get the lock.
> Otherwise we can miss the point to purge. Agree with your opinion.
>
Below the patch that address Matthew's points:
<snip>
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d2a00ad4e1dd..b82db44fea60 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1717,17 +1717,10 @@ 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);
- }
-}
+static void purge_vmap_area_lazy(void);
+static void drain_vmap_area(struct work_struct *work);
+static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area);
+static atomic_t drain_vmap_area_work_in_progress;
/*
* Kick off a purge of the outstanding lazy areas.
@@ -1740,6 +1733,22 @@ static void purge_vmap_area_lazy(void)
mutex_unlock(&vmap_purge_lock);
}
+static void drain_vmap_area(struct work_struct *work)
+{
+ mutex_lock(&vmap_purge_lock);
+ __purge_vmap_area_lazy(ULONG_MAX, 0);
+ mutex_unlock(&vmap_purge_lock);
+
+ /*
+ * Check if rearming is still required. If not, we are
+ * done and can let a next caller to initiate a new drain.
+ */
+ if (atomic_long_read(&vmap_lazy_nr) > lazy_max_pages())
+ schedule_work(&drain_vmap_area_work);
+ else
+ atomic_set(&drain_vmap_area_work_in_progress, 0);
+}
+
/*
* 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 +1775,8 @@ 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();
+ if (!atomic_xchg(&drain_vmap_area_work_in_progress, 1))
+ schedule_work(&drain_vmap_area_work);
}
/*
<snip>
Manfred, could you please have a look and if you have a time test it?
I mean if it solves your issue. You can take over this patch and resend
it, otherwise i can send it myself later if we all agree with it.
--
Vlad Rezki
Hello Vlad,
On 12/28/21 20:45, Uladzislau Rezki wrote:
> [...]
> Manfred, could you please have a look and if you have a time test it?
> I mean if it solves your issue. You can take over this patch and resend
> it, otherwise i can send it myself later if we all agree with it.
I think we mix tasks: We have a bug in ipc/sem.c, thus we need a
solution suitable for stable.
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
allocation")
Cc: [email protected]
I think for stable, there are only two options:
- change ipc/sem.c, call kvfree() after dropping the spinlock
- change kvfree() to use vfree_atomic().
From my point of view, both approaches are fine.
I.e. I'm waiting for feedback from an mm maintainer.
As soon as it is agreed, I will retest the chosen solution.
Now you propose to redesign vfree(), so that vfree() is safe to be
called while holding spinlocks:
> <snip>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d2a00ad4e1dd..b82db44fea60 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1717,17 +1717,10 @@ 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);
> - }
> -}
> +static void purge_vmap_area_lazy(void);
> +static void drain_vmap_area(struct work_struct *work);
> +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area);
> +static atomic_t drain_vmap_area_work_in_progress;
>
> /*
> * Kick off a purge of the outstanding lazy areas.
> @@ -1740,6 +1733,22 @@ static void purge_vmap_area_lazy(void)
> mutex_unlock(&vmap_purge_lock);
> }
>
> +static void drain_vmap_area(struct work_struct *work)
> +{
> + mutex_lock(&vmap_purge_lock);
> + __purge_vmap_area_lazy(ULONG_MAX, 0);
> + mutex_unlock(&vmap_purge_lock);
> +
> + /*
> + * Check if rearming is still required. If not, we are
> + * done and can let a next caller to initiate a new drain.
> + */
> + if (atomic_long_read(&vmap_lazy_nr) > lazy_max_pages())
> + schedule_work(&drain_vmap_area_work);
> + else
> + atomic_set(&drain_vmap_area_work_in_progress, 0);
> +}
> +
> /*
> * 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 +1775,8 @@ 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();
> + if (!atomic_xchg(&drain_vmap_area_work_in_progress, 1))
> + schedule_work(&drain_vmap_area_work);
> }
>
> /*
> <snip>
I do now know the mm code well enough to understand the side effects of
the change. And doubt that it is suitable for stable, i.e. we need the
simple patch first.
--
Manfred
> Hello Vlad,
>
> On 12/28/21 20:45, Uladzislau Rezki wrote:
> > [...]
> > Manfred, could you please have a look and if you have a time test it?
> > I mean if it solves your issue. You can take over this patch and resend
> > it, otherwise i can send it myself later if we all agree with it.
>
> I think we mix tasks: We have a bug in ipc/sem.c, thus we need a solution
> suitable for stable.
>
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
> allocation")
> Cc: [email protected]
>
> I think for stable, there are only two options:
>
> - change ipc/sem.c, call kvfree() after dropping the spinlock
>
> - change kvfree() to use vfree_atomic().
>
> From my point of view, both approaches are fine.
>
> I.e. I'm waiting for feedback from an mm maintainer.
>
> As soon as it is agreed, I will retest the chosen solution.
>
Here for me it anyway looks like a change and it is hard to judge
if the second solution is stable or not, because it is a new change
and the kvfree() interface is changed internally.
>
> Now you propose to redesign vfree(), so that vfree() is safe to be called
> while holding spinlocks:
>
> > <snip>
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index d2a00ad4e1dd..b82db44fea60 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -1717,17 +1717,10 @@ 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);
> > - }
> > -}
> > +static void purge_vmap_area_lazy(void);
> > +static void drain_vmap_area(struct work_struct *work);
> > +static DECLARE_WORK(drain_vmap_area_work, drain_vmap_area);
> > +static atomic_t drain_vmap_area_work_in_progress;
> > /*
> > * Kick off a purge of the outstanding lazy areas.
> > @@ -1740,6 +1733,22 @@ static void purge_vmap_area_lazy(void)
> > mutex_unlock(&vmap_purge_lock);
> > }
> > +static void drain_vmap_area(struct work_struct *work)
> > +{
> > + mutex_lock(&vmap_purge_lock);
> > + __purge_vmap_area_lazy(ULONG_MAX, 0);
> > + mutex_unlock(&vmap_purge_lock);
> > +
> > + /*
> > + * Check if rearming is still required. If not, we are
> > + * done and can let a next caller to initiate a new drain.
> > + */
> > + if (atomic_long_read(&vmap_lazy_nr) > lazy_max_pages())
> > + schedule_work(&drain_vmap_area_work);
> > + else
> > + atomic_set(&drain_vmap_area_work_in_progress, 0);
> > +}
> > +
> > /*
> > * 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 +1775,8 @@ 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();
> > + if (!atomic_xchg(&drain_vmap_area_work_in_progress, 1))
> > + schedule_work(&drain_vmap_area_work);
> > }
> > /*
> > <snip>
> I do now know the mm code well enough to understand the side effects of the
> change. And doubt that it is suitable for stable, i.e. we need the simple
> patch first.
>
Well, it is as simple as it could be :)
--
Vlad Rezki
On Wed, 22 Dec 2021 20:48:28 +0100 Manfred Spraul <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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