[PATCH] ipc/sem: do not sleep with a spin lock held

[PATCH] ipc/sem: do not sleep with a spin lock held

[cgel.zte]

From: Minghao Chi <chi.minghao@zte.com.cn>

We can't call kvfree() with a spin lock held, so defer it.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
---
 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

Re: [PATCH] ipc/sem: do not sleep with a spin lock held

[Manfred Spraul]

Hi Minghao,

On 12/22/21 09:10, cgel.zte@gmail.com wrote:
> From: Minghao Chi <chi.minghao@zte.com.cn>
>
> We can't call kvfree() with a spin lock held, so defer it.
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>

Could you add

Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo 
allocation")

Cc: stable@vger.kernel.org

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;

Re: [PATCH] ipc/sem: do not sleep with a spin lock held

[Vasily Averin]

On 22.12.2021 14:45, Manfred Spraul wrote:
> Hi Minghao,
> 
> On 12/22/21 09:10, cgel.zte@gmail.com wrote:
>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>
>> 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 <zealci@zte.com.cn>
>> Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
> 
> Could you add
> 
> Fixes: fc37a3b8b438 ("[PATCH] ipc sem: use kvmalloc for sem_undo allocation")
> 
> Cc: stable@vger.kernel.org
> 
> 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

Re: [PATCH] ipc/sem: do not sleep with a spin lock held

[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, cgel.zte@gmail.com wrote:
>>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>>
>>> 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

Re: [PATCH] ipc/sem: do not sleep with a spin lock held

[Manfred Spraul]

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, cgel.zte@gmail.com wrote:
>>>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>>>
>>>> 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

Re: [PATCH] ipc/sem: do not sleep with a spin lock held

[Vasily Averin]

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, cgel.zte@gmail.com wrote:
>>>>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>>>>
>>>>> 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

Re: [PATCH] ipc/sem: do not sleep with a spin lock held

[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, cgel.zte@gmail.com wrote:
>>>>>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>>>>>
>>>>>> 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
> 

[PATCH v2] ipc/sem: do not sleep with a spin lock held

[cgel.zte]

From: Minghao Chi <chi.minghao@zte.com.cn>

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 <zealci@zte.com.cn>
Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
---
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

[PATCH v2] ipc/sem: do not sleep with a spin lock held

[cgel.zte]

From: Minghao Chi <chi.minghao@zte.com.cn>

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 <zealci@zte.com.cn>
Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
---
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

[PATCH v2] ipc/sem: do not sleep with a spin lock held

[cgel.zte]

From: Minghao Chi <chi.minghao@zte.com.cn>

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 <zealci@zte.com.cn>
Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
---
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

Re: [PATCH v2] ipc/sem: do not sleep with a spin lock held

[Jiri Slaby]

On 23. 12. 21, 4:12, cgel.zte@gmail.com wrote:
> From: Minghao Chi <chi.minghao@zte.com.cn>
> 
> 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 <zealci@zte.com.cn>
> Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
> ---
> 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

Re: [PATCH v2] ipc/sem: do not sleep with a spin lock held

[Manfred Spraul]

Hi Jiri,

On 1/3/22 10:27, Jiri Slaby wrote:
> On 23. 12. 21, 4:12, cgel.zte@gmail.com wrote:
>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>
>> 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

Re: [PATCH v2] ipc/sem: do not sleep with a spin lock held

[Shakeel Butt]

On Mon, Jan 3, 2022 at 9:18 AM Manfred Spraul <manfred@colorfullife.com> wrote:
>
> Hi Jiri,
>
> On 1/3/22 10:27, Jiri Slaby wrote:
> > On 23. 12. 21, 4:12, cgel.zte@gmail.com wrote:
> >> From: Minghao Chi <chi.minghao@zte.com.cn>
> >>
> >> 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
>

Re: [PATCH v2] ipc/sem: do not sleep with a spin lock held

[Shakeel Butt]

On Wed, Dec 22, 2021 at 7:12 PM <cgel.zte@gmail.com> wrote:
>
> From: Minghao Chi <chi.minghao@zte.com.cn>
>
> 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 <zealci@zte.com.cn>
> Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH v2] ipc/sem: do not sleep with a spin lock held

[Manfred Spraul]

On 1/4/22 19:20, Shakeel Butt wrote:
> On Wed, Dec 22, 2021 at 7:12 PM <cgel.zte@gmail.com> wrote:
>> From: Minghao Chi <chi.minghao@zte.com.cn>
>>
>> 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 <zealci@zte.com.cn>
>> Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
> Reviewed-by: Shakeel Butt <shakeelb@google.com>

Reviewed-by: Manfred Spraul <manfred@colorfullife.com>

--

     Manfred