Re: [PATCH] genirq: call cancel_work_sync from irq_set_affinity_notifier

[Sodagudi Prasad]

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@xxxxxxxxxxxxxx>
> > ---
> >  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