Re: [PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

[Sodagudi Prasad <psodagud@codeaurora.org>]

On 2019-03-21 09:19, Thomas Gleixner wrote:
> Prasad,
> 
> On Wed, 20 Mar 2019, Prasad Sodagudi wrote:
> 
>> Subject: [PATCH] genirq: call cancel_work_sync from 
>> irq_set_affinity_notifier
> 
> Please do not decribe WHAT the code change is. Give a consice 
> explanation
> WHY this change is done. The above is like '[PATCH] foo: Increment bar 
> by 5'.
> 
>   [PATCH] genirq: Prevent UAF and work list corruption
> 
>> When ever notification of IRQ affinity changes, call
>> cancel_work_sync from irq_set_affinity_notifier to cancel
>> all pending works to avoid work list corruption.
> 
> Again, you describe first WHAT you are doing instead of telling WHY.
> 
>   When irq_set_affinity_notifier() replaces the notifier, then the
>   reference count on the old notifier is dropped which causes it to be
>   freed. But nothing ensures that the old notifier is not longer queued 
> in
>   the work list. If it is queued this results in a use after free and
>   possibly in work list corruption.
> 
>   Ensure that the work is canceled before the reference is dropped.
> 
> See?

Hi Tglx,

Thanks for suggesting commit text and modifications.

> 
> This gives precise context first and then describes the cure.
> 
> Also it is completely irrelevant whether this is achieved by calling
> cancel_work_sync() or by something else. What matters is that it's
> canceled. Changelogs describe context and concepts not implementation
> details. The implementation details are in the patch itself.
> 
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> ---
>>  kernel/irq/manage.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 9ec34a2..da8b2ee 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -356,6 +356,9 @@ static void irq_affinity_notify(struct work_struct 
>> *work)
>>  	desc->affinity_notify = notify;
>>  	raw_spin_unlock_irqrestore(&desc->lock, flags);
>> 
>> +	if (!notify && old_notify)
>> +		cancel_work_sync(&old_notify->work);
> 
> That '!notify' doesn't make any sense.

Yes. I will remove this in the next patch set.  Thanks for reviewing.

-thanks, Prasad
> 
> Thanks,
> 
> 	tglx

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project