2018-03-22 11:17:32

by Aniruddha Banerjee

[permalink] [raw]
Subject: [RFC PATCH] irqchip: arm-gic: take gic_lock when updating irq type

The kernel documentation states that the irq-chip driver should
handle the locking of the irq-chip registers. In the irq-gic,
the accesses to the irqchip are seemingly not protected and multiple
writes to SPIs from different irq descriptors do RMW requests without
taking the irq-chip lock. When multiple irqs call the request_irq at
the same time, there can be a simultaneous write at the gic
distributor, leading to a race. Acquire the irq_controller lock when the
irq_type is updated.

This patch is only for GICv2; however, I have noticed a similar
implementation in GICv3. This patch is sent as an RFC in case I am
missing anything.

Signed-off-by: Aniruddha Banerjee <[email protected]>
---
drivers/irqchip/irq-gic.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4c797b43614d..61380f5a2254 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -67,6 +67,8 @@ static void gic_check_cpu_features(void)
#define gic_check_cpu_features() do { } while(0)
#endif

+static DEFINE_RAW_SPINLOCK(irq_controller_lock);
+
union gic_base {
void __iomem *common_base;
void __percpu * __iomem *percpu_base;
@@ -529,6 +531,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
{
void __iomem *base = gic_dist_base(d);
unsigned int gicirq = gic_irq(d);
+ int ret;

/* Interrupt configuration for SGIs can't be changed */
if (gicirq < 16)
@@ -539,7 +542,11 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
type != IRQ_TYPE_EDGE_RISING)
return -EINVAL;

- return gic_configure_irq(gicirq, type, base, NULL);
+ raw_spin_lock(&irq_controller_lock);
+ ret = gic_configure_irq(gicirq, type, base, NULL);
+ raw_spin_unlock(&irq_controller_lock);
+
+ return ret;
}

static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
--
2.15.1


2018-03-22 12:21:16

by Marc Zyngier

[permalink] [raw]
Subject: Re: [RFC PATCH] irqchip: arm-gic: take gic_lock when updating irq type

On 22/03/18 10:28, Aniruddha Banerjee wrote:

[a PGP-encrypted email]

Given that you're cc-ing various mailing lists, please resend this email
unencrypted.

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2018-03-26 13:20:24

by Daniel Thompson

[permalink] [raw]
Subject: Re: [RFC PATCH] irqchip: arm-gic: take gic_lock when updating irq type

On Thu, Mar 22, 2018 at 12:19:15PM +0000, Marc Zyngier wrote:
> On 22/03/18 10:28, Aniruddha Banerjee wrote:
> [a PGP-encrypted email]
>
> Given that you're cc-ing various mailing lists, please resend this email
> unencrypted.

I am very puzzled about what is going on here. I could read the
original mail fine and I was not in To: or Cc:. It also seems to
have made it through to the archives OK.

https://www.mail-archive.com/[email protected]/msg1645375.html
https://www.spinics.net/lists/linux-tegra/msg32829.html


Daniel.

2018-03-26 14:28:06

by Marc Zyngier

[permalink] [raw]
Subject: Re: [RFC PATCH] irqchip: arm-gic: take gic_lock when updating irq type

On 26/03/18 14:19, Daniel Thompson wrote:
> On Thu, Mar 22, 2018 at 12:19:15PM +0000, Marc Zyngier wrote:
>> On 22/03/18 10:28, Aniruddha Banerjee wrote:
>> [a PGP-encrypted email]
>>
>> Given that you're cc-ing various mailing lists, please resend this email
>> unencrypted.
>
> I am very puzzled about what is going on here. I could read the
> original mail fine and I was not in To: or Cc:. It also seems to
> have made it through to the archives OK.
>
> https://www.mail-archive.com/[email protected]/msg1645375.html
> https://www.spinics.net/lists/linux-tegra/msg32829.html

It looks like any email I getting from nvidia is encrypted. The message
on the list is in clear text, but not the one I received, hence my
(admittedly premature) rant.

M.
--
Jazz is not dead. It just smells funny...