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: <87r07wufs7.ffs@tglx>
Date: Thu, 31 Oct 2024 22:24:40 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Advait Dhamorikar <advaitdhamorikar@...il.com>, Fabrizio Castro
 <fabrizio.castro.jz@...esas.com>
Cc: linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
 anupnewsmail@...il.com, Advait Dhamorikar <advaitdhamorikar@...il.com>
Subject: Re: [PATCH-next] irqchip/renesas-rzv2h: Fix potentially mismatched
 datatype

On Fri, Nov 01 2024 at 01:06, Advait Dhamorikar wrote:
> This patch updates the type of hw_irq to unsigned long to 

Please do:

git grep 'This patch' Documentation/process/

and read through the matching documentation.

> match irq_hw_number_t.
>
> The variable hw_irq is defined as unsigned int at places,
> However when it is initialized using irqd_to_hwirq(), it returns 
> an irq_hw_number_t, which inturn is a typedef for unsigned long.

We know that, but what is the problem this patch is actually solving?

>  static void rzv2h_icu_eoi(struct irq_data *d)
>  {
>  	struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> -	unsigned int hw_irq = irqd_to_hwirq(d);
> +	unsigned long hw_irq = irqd_to_hwirq(d);
>  	unsigned int tintirq_nr;

It moves the type mismatch and potential truncation a few lines further
down:

	tintirq_nr = hw_irq - ICU_TINT_START;

In fact there is no problem with the existing code because the hardware
interrupt number range for this interrupt chip is guaranteed to be
smaller than UINT_MAX. IOW, a truncation from unsigned long to unsigned
int (on a 64-bit system) does not matter at all.

I'm all for being type safe, but what you are doing is purely cosmetic.

If at all, then the proper change is either

 1) to make the related variables type irq_hw_number_t

    You cannot make assumptions about the type which is behind
    irq_hw_number_t today. The type can change tomorrow, no?

or

 2) Use a proper type cast which documents that the type conversion
    including the potential truncation is intentional and correct.

    This should not be an actual type cast, but a helper inline which
    has the cast and explicitely returns an unsigned int.

I leave it to you to decide which variant is the correct one, but I'm
happy to answer your questions.

Thanks,

        tglx

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ