Messages in this thread | | | Date | Thu, 21 Mar 2019 13:31:33 -0700 | From | Sodagudi Prasad <> | Subject | Re: [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
| |