From: Minghao Chi <[email protected]>
We can't call kvfree() with a spin lock held, so defer it.
Reported-by: Zeal Robot <[email protected]>
Signed-off-by: Minghao Chi <[email protected]>
---
ipc/sem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 6693daf4fe11..0dbdb98fdf2d 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1964,6 +1964,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
*/
un = lookup_undo(ulp, semid);
if (un) {
+ spin_unlock(&ulp->lock);
kvfree(new);
goto success;
}
@@ -1976,9 +1977,8 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
ipc_assert_locked_object(&sma->sem_perm);
list_add(&new->list_id, &sma->list_id);
un = new;
-
-success:
spin_unlock(&ulp->lock);
+success:
sem_unlock(sma, -1);
out:
return un;
--
2.25.1
Hi Minghao,
On 12/22/21 09:10, [email protected] wrote:
> From: Minghao Chi <[email protected]>
>
> We can't call kvfree() with a spin lock held, so defer it.
>
> Reported-by: Zeal Robot <[email protected]>
> Signed-off-by: Minghao Chi <[email protected]>
Could you add
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
allocation")
Cc: [email protected]
I will review/test the change in the next few days.
Especially, I would like to check if there are further instances where
the same mistake was made.
> /**
> * kvfree() - Free memory.
> * @addr: Pointer to allocated memory.
> *
> * kvfree 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.
> */
>
As an independent change: Should we add a
might_sleep_if(!in_interrupt());
into kvfree(), to trigger bugs more easily?
> ---
> ipc/sem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 6693daf4fe11..0dbdb98fdf2d 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1964,6 +1964,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
> */
> un = lookup_undo(ulp, semid);
> if (un) {
> + spin_unlock(&ulp->lock);
> kvfree(new);
> goto success;
> }
> @@ -1976,9 +1977,8 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
> ipc_assert_locked_object(&sma->sem_perm);
> list_add(&new->list_id, &sma->list_id);
> un = new;
> -
> -success:
> spin_unlock(&ulp->lock);
> +success:
> sem_unlock(sma, -1);
> out:
> return un;
On 22.12.2021 14:45, Manfred Spraul wrote:
> Hi Minghao,
>
> On 12/22/21 09:10, [email protected] wrote:
>> From: Minghao Chi <[email protected]>
>>
>> We can't call kvfree() with a spin lock held, so defer it.
I'm sorry, but I do not understand why exactly we cannot use kvfree?
Could you explain it in more details?
>> Reported-by: Zeal Robot <[email protected]>
>> Signed-off-by: Minghao Chi <[email protected]>
>
> Could you add
>
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation")
>
> Cc: [email protected]
>
> I will review/test the change in the next few days.
>
> Especially, I would like to check if there are further instances where the same mistake was made.
>
>> /**
>> * kvfree() - Free memory.
>> * @addr: Pointer to allocated memory.
>> *
>> * kvfree 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.
>> */
>>
> As an independent change: Should we add a
>
>
> might_sleep_if(!in_interrupt());
>
> into kvfree(), to trigger bugs more easily?
I think it is good idea in general,
however please do not use "in_interrupt()", it is obsoleted
and in fact means "We're in NMI,IRQ,SoftIRQ context or have BH disabled"
Please use something like in_task()
Thank you, Vasily Averin
On 22.12.2021 18:31, Vasily Averin wrote:
> On 22.12.2021 14:45, Manfred Spraul wrote:
>> Hi Minghao,
>>
>> On 12/22/21 09:10, [email protected] wrote:
>>> From: Minghao Chi <[email protected]>
>>>
>>> We can't call kvfree() with a spin lock held, so defer it.
>
> I'm sorry, but I do not understand why exactly we cannot use kvfree?
> Could you explain it in more details?
Got it,
there is cond_resched() called in __vfree() -> __vunmap()
However I'm still not sure that in_interrupt() is used correctly here.
Thank you,
Vasily Averin
Hi Vasily,
On 12/22/21 16:50, Vasily Averin wrote:
> On 22.12.2021 18:31, Vasily Averin wrote:
>> On 22.12.2021 14:45, Manfred Spraul wrote:
>>> Hi Minghao,
>>>
>>> On 12/22/21 09:10, [email protected] wrote:
>>>> From: Minghao Chi <[email protected]>
>>>>
>>>> We can't call kvfree() with a spin lock held, so defer it.
>> I'm sorry, but I do not understand why exactly we cannot use kvfree?
>> Could you explain it in more details?
> Got it,
> there is cond_resched() called in __vfree() -> __vunmap()
>
> However I'm still not sure that in_interrupt() is used correctly here.
I see three different topics:
- is the current code violating the API? I think yes, thus there is a
bug that needs to be fixed.
- Where is __vunmap() sleeping? Would it be possible to make __vunmap()
safe to be called when owning a spinlock?
- should kvfree() use vfree() [i.e. unsafe when owning a spinlock] or
vfree_atomic [i.e. a bit slower, but safe]
As we did quite many s/kfree/kvfree/ changes, perhaps just switching to
vfree_atomic() is the best solution.
@Andrew: What would you prefer?
In addition, if we do not use vfree_atomic(): Then I would propose to
copy the might_sleep_if() from vfree() into kvfree()
--
Manfred
On 22.12.2021 20:06, Manfred Spraul wrote:
> Hi Vasily,
>
> On 12/22/21 16:50, Vasily Averin wrote:
>> On 22.12.2021 18:31, Vasily Averin wrote:
>>> On 22.12.2021 14:45, Manfred Spraul wrote:
>>>> Hi Minghao,
>>>>
>>>> On 12/22/21 09:10, [email protected] wrote:
>>>>> From: Minghao Chi <[email protected]>
>>>>>
>>>>> We can't call kvfree() with a spin lock held, so defer it.
>>> I'm sorry, but I do not understand why exactly we cannot use kvfree?
>>> Could you explain it in more details?
>> Got it,
>> there is cond_resched() called in __vfree() -> __vunmap()
>>
>> However I'm still not sure that in_interrupt() is used correctly here.
>
> I see three different topics:
>
> - is the current code violating the API? I think yes, thus there is a bug that needs to be fixed.
I'm agree. Found issue is a bug and it should be fixed ASAP,
I'm sorry for a mistake in my patch.
> - Where is __vunmap() sleeping? Would it be possible to make __vunmap() safe to be called when owning a spinlock?
I think it is possible, and we should do it to prevent similar incidents in future.
vfree() should check preempt count to detect this situation (i.e. execution under taken spinlock)
generate WARN_ON and then call __vfree_deferred() to avoid sleep.
> - should kvfree() use vfree() [i.e. unsafe when owning a spinlock] or vfree_atomic [i.e. a bit slower, but safe]
I think it's better to change vfree.
> As we did quite many s/kfree/kvfree/ changes, perhaps just switching to vfree_atomic() is the best solution.
>
> @Andrew: What would you prefer?
>
> In addition, if we do not use vfree_atomic(): Then I would propose to copy the might_sleep_if() from vfree() into kvfree()
I think it does not help, as far as I understand we are in task context, just under taken spinlock.
Thank you,
vasily Averin
On 22.12.2021 20:38, Vasily Averin wrote:
> On 22.12.2021 20:06, Manfred Spraul wrote:
>> Hi Vasily,
>>
>> On 12/22/21 16:50, Vasily Averin wrote:
>>> On 22.12.2021 18:31, Vasily Averin wrote:
>>>> On 22.12.2021 14:45, Manfred Spraul wrote:
>>>>> Hi Minghao,
>>>>>
>>>>> On 12/22/21 09:10, [email protected] wrote:
>>>>>> From: Minghao Chi <[email protected]>
>>>>>>
>>>>>> We can't call kvfree() with a spin lock held, so defer it.
>>>> I'm sorry, but I do not understand why exactly we cannot use kvfree?
>>>> Could you explain it in more details?
>>> Got it,
>>> there is cond_resched() called in __vfree() -> __vunmap()
>>>
>>> However I'm still not sure that in_interrupt() is used correctly here.
>>
>> I see three different topics:
>>
>> - is the current code violating the API? I think yes, thus there is a bug that needs to be fixed.
>
> I'm agree. Found issue is a bug and it should be fixed ASAP,
> I'm sorry for a mistake in my patch.
>
>> - Where is __vunmap() sleeping? Would it be possible to make __vunmap() safe to be called when owning a spinlock?
>
> I think it is possible, and we should do it to prevent similar incidents in future.
> vfree() should check preempt count to detect this situation (i.e. execution under taken spinlock)
> generate WARN_ON and then call __vfree_deferred() to avoid sleep.
>
>> - should kvfree() use vfree() [i.e. unsafe when owning a spinlock] or vfree_atomic [i.e. a bit slower, but safe]
>
> I think it's better to change vfree.
I mean something like this:
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2674,7 +2674,7 @@ void vfree_atomic(const void *addr)
static void __vfree(const void *addr)
{
- if (unlikely(in_interrupt()))
+ if (unlikely(in_atomic())) <<<< VvS: do not sleep in atomic ...
__vfree_deferred(addr);
else
__vunmap(addr, 1);
@@ -2703,7 +2703,7 @@ void vfree(const void *addr)
kmemleak_free(addr);
- might_sleep_if(!in_interrupt());
+ might_sleep_if(in_task()); <<<<< VvS: ... but generate warning if vfree was called
<<<<< in task context with taken spin_lock or spin_lock_bh
if (!addr)
return;
>> As we did quite many s/kfree/kvfree/ changes, perhaps just switching to vfree_atomic() is the best solution.
>>
>> @Andrew: What would you prefer?
>>
>> In addition, if we do not use vfree_atomic(): Then I would propose to copy the might_sleep_if() from vfree() into kvfree()
> I think it does not help, as far as I understand we are in task context, just under taken spinlock.
>
> Thank you,
> vasily Averin
>
From: Minghao Chi <[email protected]>
We can't call kvfree() with a spin lock held, so defer it.
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
allocation")
Reported-by: Zeal Robot <[email protected]>
Signed-off-by: Minghao Chi <[email protected]>
---
changelog since v2:
+ Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
+ allocation")
ipc/sem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 6693daf4fe11..0dbdb98fdf2d 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1964,6 +1964,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
*/
un = lookup_undo(ulp, semid);
if (un) {
+ spin_unlock(&ulp->lock);
kvfree(new);
goto success;
}
@@ -1976,9 +1977,8 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
ipc_assert_locked_object(&sma->sem_perm);
list_add(&new->list_id, &sma->list_id);
un = new;
-
-success:
spin_unlock(&ulp->lock);
+success:
sem_unlock(sma, -1);
out:
return un;
--
2.25.1
From: Minghao Chi <[email protected]>
We can't call kvfree() with a spin lock held, so defer it.
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
allocation")
Reported-by: Zeal Robot <[email protected]>
Signed-off-by: Minghao Chi <[email protected]>
---
changelog since v2:
+ Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
+ allocation")
ipc/sem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 6693daf4fe11..0dbdb98fdf2d 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1964,6 +1964,7 @@ static struct sem_undo *find_alloc_undo(struct
ipc_namespace *ns, int semid)
*/
un = lookup_undo(ulp, semid);
if (un) {
+ spin_unlock(&ulp->lock);
kvfree(new);
goto success;
}
@@ -1976,9 +1977,8 @@ static struct sem_undo *find_alloc_undo(struct
ipc_namespace *ns, int semid)
ipc_assert_locked_object(&sma->sem_perm);
list_add(&new->list_id, &sma->list_id);
un = new;
-
-success:
spin_unlock(&ulp->lock);
+success:
sem_unlock(sma, -1);
out:
return un;
--
2.25.1
From: Minghao Chi <[email protected]>
We can't call kvfree() with a spin lock held, so defer it.
Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
allocation")
Reported-by: Zeal Robot <[email protected]>
Signed-off-by: Minghao Chi <[email protected]>
---
changelog since v2:
+ Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
+ allocation")
ipc/sem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipc/sem.c b/ipc/sem.c
index 6693daf4fe11..0dbdb98fdf2d 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1964,6 +1964,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
*/
un = lookup_undo(ulp, semid);
if (un) {
+ spin_unlock(&ulp->lock);
kvfree(new);
goto success;
}
@@ -1976,9 +1977,8 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
ipc_assert_locked_object(&sma->sem_perm);
list_add(&new->list_id, &sma->list_id);
un = new;
-
-success:
spin_unlock(&ulp->lock);
+success:
sem_unlock(sma, -1);
out:
return un;
--
2.25.1
On 23. 12. 21, 4:12, [email protected] wrote:
> From: Minghao Chi <[email protected]>
>
> We can't call kvfree() with a spin lock held, so defer it.
Sorry, defer what?
There are attempts to fix kvfree instead, not sure which of these
approaches (fix kvfree or its callers) won in the end?
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
> allocation")
>
> Reported-by: Zeal Robot <[email protected]>
> Signed-off-by: Minghao Chi <[email protected]>
> ---
> changelog since v2:
> + Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
> + allocation")
> ipc/sem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 6693daf4fe11..0dbdb98fdf2d 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1964,6 +1964,7 @@ static struct sem_undo *find_alloc_undo(struct ipc_namespace *ns, int semid)
> */
> un = lookup_undo(ulp, semid);
> if (un) {
> + spin_unlock(&ulp->lock);
> kvfree(new);
> goto success;
> }
--
js
suse labs
Hi Jiri,
On 1/3/22 10:27, Jiri Slaby wrote:
> On 23. 12. 21, 4:12, [email protected] wrote:
>> From: Minghao Chi <[email protected]>
>>
>> We can't call kvfree() with a spin lock held, so defer it.
>
> Sorry, defer what?
>
First drop the spinlock, then call kvfree().
> There are attempts to fix kvfree instead, not sure which of these
> approaches (fix kvfree or its callers) won in the end?
>
Exactly. We have three options - but noone volunteered yet to decide:
- change ipc/sem.c [minimal change]
- change kvfree() to use vfree_atomic() [would also fix other changes
that did s/kfree/kvfree/]
- Modify the vma handling so that it becomes safe to call vfree() while
holding a spinlock. [perfect approach, but I'm concerned about side effects]
--
Manfred
On Mon, Jan 3, 2022 at 9:18 AM Manfred Spraul <[email protected]> wrote:
>
> Hi Jiri,
>
> On 1/3/22 10:27, Jiri Slaby wrote:
> > On 23. 12. 21, 4:12, [email protected] wrote:
> >> From: Minghao Chi <[email protected]>
> >>
> >> We can't call kvfree() with a spin lock held, so defer it.
> >
> > Sorry, defer what?
> >
> First drop the spinlock, then call kvfree().
>
>
> > There are attempts to fix kvfree instead, not sure which of these
> > approaches (fix kvfree or its callers) won in the end?
> >
> Exactly. We have three options - but noone volunteered yet to decide:
>
> - change ipc/sem.c [minimal change]
Let's go with the minimal change for now which can easily be
cherry-picked for the stable tree. It seems other approaches need more
work/discussion.
>
> - change kvfree() to use vfree_atomic() [would also fix other changes
> that did s/kfree/kvfree/]
>
> - Modify the vma handling so that it becomes safe to call vfree() while
> holding a spinlock. [perfect approach, but I'm concerned about side effects]
>
>
> --
>
> Manfred
>
On Wed, Dec 22, 2021 at 7:12 PM <[email protected]> wrote:
>
> From: Minghao Chi <[email protected]>
>
> We can't call kvfree() with a spin lock held, so defer it.
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
> allocation")
>
> Reported-by: Zeal Robot <[email protected]>
> Signed-off-by: Minghao Chi <[email protected]>
Reviewed-by: Shakeel Butt <[email protected]>
On 1/4/22 19:20, Shakeel Butt wrote:
> On Wed, Dec 22, 2021 at 7:12 PM <[email protected]> wrote:
>> From: Minghao Chi <[email protected]>
>>
>> We can't call kvfree() with a spin lock held, so defer it.
>> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo
>> allocation")
>>
>> Reported-by: Zeal Robot <[email protected]>
>> Signed-off-by: Minghao Chi <[email protected]>
> Reviewed-by: Shakeel Butt <[email protected]>
Reviewed-by: Manfred Spraul <[email protected]>
--
Manfred