lkml.org 
[lkml]   [2019]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier
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

\
 
 \ /
  Last update: 2019-03-21 21:32    [W:0.059 / U:11.204 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site