linux-kernel - Re: [PATCH 4/4] media: rkisp1: Fix IRQ disable race issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c7117713-bc98-459c-a475-2ec838215cd9@ideasonboard.com>
Date:   Wed, 6 Dec 2023 12:08:00 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Dafna Hirschfeld <dafna@...tmail.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Paul Elder <paul.elder@...asonboard.com>,
        Alexander Stein <alexander.stein@...tq-group.com>,
        kieran.bingham@...asonboard.com, umang.jain@...asonboard.com,
        aford173@...il.com, linux-media@...r.kernel.org,
        linux-rockchip@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] media: rkisp1: Fix IRQ disable race issue

On 05/12/2023 14:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Dec 05, 2023 at 10:09:35AM +0200, Tomi Valkeinen wrote:
>> In rkisp1_isp_stop() and rkisp1_csi_disable() the driver masks the
>> interrupts and then apparently assumes that the interrupt handler won't
>> be running, and proceeds in the stop procedure. This is not the case, as
>> the interrupt handler can already be running, which would lead to the
>> ISP being disabled while the interrupt handler handling a captured
>> frame.
>>
>> It is not clear to me if this problem causes a real issue, but shutting
>> down the ISP while an interrupt handler is running sounds rather bad.
> 
> Agreed.
> 
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
>>   drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c | 14 +++++++++++++-
>>   drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c | 20 +++++++++++++++++---
>>   2 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>> index f6b54654b435..f0cef766fc0c 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-csi.c
>> @@ -125,8 +125,20 @@ static void rkisp1_csi_disable(struct rkisp1_csi *csi)
>>   	struct rkisp1_device *rkisp1 = csi->rkisp1;
>>   	u32 val;
>>   
>> -	/* Mask and clear interrupts. */
>> +	/* Mask MIPI interrupts. */
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_IMSC, 0);
>> +
>> +	/* Flush posted writes */
>> +	rkisp1_read(rkisp1, RKISP1_CIF_MIPI_IMSC);
>> +
>> +	/*
>> +	 * Wait until the IRQ handler has ended. The IRQ handler may get called
>> +	 * even after this, but it will return immediately as the MIPI
>> +	 * interrupts have been masked.
>> +	 */
> 
> This comment will need to be updated if patch 3/4 gets replaced by a
> patch that drops IRQF_SHARED.

I don't think it needs an update, as the irq handling is divided into 
multiple parts. The handler here may get called due to a MI or ISP 
interrupt, and vice versa.

  Tomi

> 
>> +	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MIPI]);
>> +
>> +	/* Clear MIPI interrupt status */
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MIPI_ICR, ~0);
>>   
>>   	val = rkisp1_read(rkisp1, RKISP1_CIF_MIPI_CTRL);
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>> index d6b8786661ad..a6dd497c884c 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-isp.c
>> @@ -364,11 +364,25 @@ static void rkisp1_isp_stop(struct rkisp1_isp *isp)
>>   	 * ISP(mi) stop in mi frame end -> Stop ISP(mipi) ->
>>   	 * Stop ISP(isp) ->wait for ISP isp off
>>   	 */
>> -	/* stop and clear MI and ISP interrupts */
>> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
>> -	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
>>   
>> +	/* Mask MI and ISP interrupts */
>> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_IMSC, 0);
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MI_IMSC, 0);
>> +
>> +	/* Flush posted writes */
>> +	rkisp1_read(rkisp1, RKISP1_CIF_MI_IMSC);
>> +
>> +	/*
>> +	 * Wait until the IRQ handler has ended. The IRQ handler may get called
>> +	 * even after this, but it will return immediately as the MI and ISP
>> +	 * interrupts have been masked.
>> +	 */
> 
> Same here.
> 
>> +	synchronize_irq(rkisp1->irqs[RKISP1_IRQ_ISP]);
>> +	if (rkisp1->irqs[RKISP1_IRQ_ISP] != rkisp1->irqs[RKISP1_IRQ_MI])
>> +		synchronize_irq(rkisp1->irqs[RKISP1_IRQ_MI]);
> 
> It would be nice if we could avoid the double synchronize_irq() for
> platforms where RKISP1_IRQ_MIPI and RKISP1_IRQ_ISP are identical, but I
> understand that would be difficult.
> 
>> +
>> +	/* Clear MI and ISP interrupt status */
>> +	rkisp1_write(rkisp1, RKISP1_CIF_ISP_ICR, ~0);
>>   	rkisp1_write(rkisp1, RKISP1_CIF_MI_ICR, ~0);
>>   
>>   	/* stop ISP */
>