linux-kernel - Re: [RFC PATCH v3 19/37] irqchip: Add irq-kvx-apic-gic driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a3c4327-68f1-4dd6-ba84-2be77625475c@kalrayinc.com>
Date: Fri, 23 Aug 2024 14:37:47 +0200
From: Yann Sionneau <ysionneau@...rayinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org,
 Thomas Gleixner <tglx@...utronix.de>
Cc: Jonathan Borne <jborne@...rayinc.com>, Julian Vetter
 <jvetter@...rayinc.com>, Clement Leger <clement@...ment-leger.fr>, Vincent
 Chardon <vincent.chardon@...ys-design.com>
Subject: Re: [RFC PATCH v3 19/37] irqchip: Add irq-kvx-apic-gic driver

Hello Krzysztof,

On 22/07/2024 14:28, Krzysztof Kozlowski wrote:
> On 22/07/2024 11:41, ysionneau@...rayinc.com wrote:
>> From: Yann Sionneau <ysionneau@...rayinc.com>
>>
> ...
>
>> +
>> +static int __init kvx_init_apic_gic(struct device_node *node,
>> +				    struct device_node *parent)
>> +{
>> +	struct kvx_apic_gic *gic;
>> +	int ret;
>> +	unsigned int irq;
>> +
>> +	if (!parent) {
>> +		pr_err("kvx apic gic does not have parent\n");
> How is this possible? Aren't you controlling the code being executed?

I think this is called from generic code with values from the DT.

If this node does not have any interrupt-parent I guess parent is NULL.

DT is user controlled, not controlled by the driver code.

Also, I can see such tests in several irqchip drivers: irq-imx-gpcv2, irq-sni-exiu, irq-crossbar, irq-meson-gpio, irq-al-fic, irq-tegra, irq-mmp, etc

Isn't it ok?

>> +		return -EINVAL;
>> +	}
>> +
>> +	gic = kzalloc(sizeof(*gic), GFP_KERNEL);
>> +	if (!gic)
>> +		return -ENOMEM;
>> +
>> +	if (of_property_read_u32(node, "kalray,intc-nr-irqs",
>> +						&gic->input_nr_irqs))
> There is no such property. Also, there shouldn't be anyway...
Ok I'll remove this property from the code and just use KVX_GIC_INPUT_IT_COUNT renamed as KVX_GIC_MAX_INPUT_IT_COUNT everywhere with the maximum possible value.
>
>> +		gic->input_nr_irqs = KVX_GIC_INPUT_IT_COUNT;
>> +
>> +	if (WARN_ON(gic->input_nr_irqs > KVX_GIC_INPUT_IT_COUNT)) {
> Why? Please, drop all these WARN_ON from here and other patches. WARN_ON
> is for cases which cannot happen, as it might panic entire system.
Ok, I'll replace with pr_warn().
> Instead, handle the case properly.
>
>> +		ret = -EINVAL;
>> +		goto err_kfree;
>> +	}
>> +
>> +	gic->base = of_io_request_and_map(node, 0, node->name);
>> +	if (!gic->base) {
>> +		ret = -EINVAL;
>> +		goto err_kfree;
>> +	}
>> +
>> +	raw_spin_lock_init(&gic->lock);
>> +	apic_gic_init(gic);