From: Zhang Qiang <[email protected]>
The callback function "rcu_free_wq" could be called after memory
was released for "rescuer" already, Thus delete a misplaced call
of the function "kfree".
Fixes: 6ba94429c8e7 ("workqueue: Reorder sysfs code")
Signed-off-by: Zhang Qiang <[email protected]>
---
v1->v2->v3:
Only commit information modification.
kernel/workqueue.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 891ccad5f271..a2451cdcd503 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3491,7 +3491,6 @@ static void rcu_free_wq(struct rcu_head *rcu)
else
free_workqueue_attrs(wq->unbound_attrs);
- kfree(wq->rescuer);
kfree(wq);
}
--
2.24.1
> The callback function "rcu_free_wq" could be called after memory
> was released for "rescuer" already, Thus delete a misplaced call
> of the function "kfree".
I got into the mood to follow your interpretation of the
software situation for a moment.
I have taken another look also at the implementation of the function “destroy_workqueue”.
* The function call “kfree(rescuer)” can be performed there in an if branch
after the statement “wq->rescuer = NULL” was executed.
* This data processing is independent from a possible call of the
function “call_rcu(&wq->rcu, rcu_free_wq)” in another if branch.
Thus it seems that a null pointer is intentionally passed by a data structure
member to this callback function on demand.
The corresponding call of the function “kfree” can tolerate this special case.
Now I find that the proposed change can be inappropriate.
I'm sorry for undesirable confusion because of this patch review.
Regards,
Markus
> The callback function "rcu_free_wq" could be called after memory
> was released for "rescuer" already, Thus delete a misplaced call
> of the function "kfree".
I got into the mood to follow your interpretation of the
software situation for a moment.
I have taken another look also at the implementation of the function “destroy_workqueue”.
* The function call “kfree(rescuer)” can be performed there in an if branch
after the statement “wq->rescuer = NULL” was executed.
* This data processing is independent from a possible call of the
function “call_rcu(&wq->rcu, rcu_free_wq)” in another if branch.
Thus it seems that a null pointer is intentionally passed by a data structure
member to this callback function on demand.
The corresponding call of the function “kfree” can tolerate this special case.
Now I find that the proposed change can be inappropriate.
I'm sorry for undesirable confusion because of this patch review.
Regards,
Markus
On Mon, May 25, 2020 at 5:22 PM <[email protected]> wrote:
>
> From: Zhang Qiang <[email protected]>
>
> The callback function "rcu_free_wq" could be called after memory
> was released for "rescuer" already, Thus delete a misplaced call
> of the function "kfree".
Hello
wq->rescuer is guaranteed to be NULL in rcu_free_wq()
since def98c84b6cd
("workqueue: Fix spurious sanity check failures in destroy_workqueue()")
And the resucer is already free in destroy_workqueue()
since 8efe1223d73c
("workqueue: Fix missing kfree(rescuer) in destroy_workqueue()")
The patch is a cleanup to remove a "kfree(NULL);".
But the changelog is misleading.
>
> Fixes: 6ba94429c8e7 ("workqueue: Reorder sysfs code")
It is totally unrelated.
> Signed-off-by: Zhang Qiang <[email protected]>
> ---
> v1->v2->v3:
> Only commit information modification.
> kernel/workqueue.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 891ccad5f271..a2451cdcd503 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3491,7 +3491,6 @@ static void rcu_free_wq(struct rcu_head *rcu)
> else
> free_workqueue_attrs(wq->unbound_attrs);
>
> - kfree(wq->rescuer);
> kfree(wq);
> }
>
> --
> 2.24.1
>
> wq->rescuer is guaranteed to be NULL in rcu_free_wq()
I was unsure about this data processing detail.
> The patch is a cleanup to remove a "kfree(NULL);".
I would prefer also an improved commit message according to
the understanding of the software situation in this direction.
Regards,
Markus
Thank you reply
There is something wrong with my description. is it feasible to describe as follows:
The resucer is already free in "destroy_workqueue" and
"wq->rescuer = NULL" was executed, but in "rcu_free_wq"
it's release again (equivalent to kfree(NULL)), this is
unnecessary, so should remove.
On 5/26/20 4:56 PM, Lai Jiangshan wrote:
> On Mon, May 25, 2020 at 5:22 PM <[email protected]> wrote:
>>
>> From: Zhang Qiang <[email protected]>
>>
>> The callback function "rcu_free_wq" could be called after memory
>> was released for "rescuer" already, Thus delete a misplaced call
>> of the function "kfree".
>
> Hello
>
> wq->rescuer is guaranteed to be NULL in rcu_free_wq()
> since def98c84b6cd
> ("workqueue: Fix spurious sanity check failures in destroy_workqueue()")
>
> And the resucer is already free in destroy_workqueue()
> since 8efe1223d73c
> ("workqueue: Fix missing kfree(rescuer) in destroy_workqueue()")
>
> The patch is a cleanup to remove a "kfree(NULL);".
> But the changelog is misleading.
>
>>
>> Fixes: 6ba94429c8e7 ("workqueue: Reorder sysfs code")
>
> It is totally unrelated.
>
>> Signed-off-by: Zhang Qiang <[email protected]>
>> ---
>> v1->v2->v3:
>> Only commit information modification.
>> kernel/workqueue.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 891ccad5f271..a2451cdcd503 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -3491,7 +3491,6 @@ static void rcu_free_wq(struct rcu_head *rcu)
>> else
>> free_workqueue_attrs(wq->unbound_attrs);
>>
>> - kfree(wq->rescuer);
>> kfree(wq);
>> }
>>
>> --
>> 2.24.1
>>