Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 21 Mar 2019 13:31:33 -0700
From:   Sodagudi Prasad <psodagud@codeaurora.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Marc Zyngier <marc.zyngier@arm.com>
Subject: Re: [PATCH] genirq: call cancel_work_sync from
 irq_set_affinity_notifier
In-Reply-To: <alpine.DEB.2.21.1903211651030.1784@nanos.tec.linutronix.de>
References: <1553119211-29761-1-git-send-email-psodagud@codeaurora.org>
 <alpine.DEB.2.21.1903211651030.1784@nanos.tec.linutronix.de>
Message-ID: <fd8a18f33a3f385575048969482fd22f@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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