linux-kernel - Re: [git patch] free

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]

Date:	Tue, 22 Apr 2008 15:25:39 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jeff Garzik <jeff@...zik.org>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>, rmk@....linux.org.uk
Subject: Re: [git patch] free_irq() fixes

On Tue, 22 Apr 2008, Jeff Garzik wrote:
>
> diff --git a/arch/arm/mach-integrator/time.c b/arch/arm/mach-integrator/time.c
> index 5235f64..28726ee 100644
> --- a/arch/arm/mach-integrator/time.c
> +++ b/arch/arm/mach-integrator/time.c
> @@ -137,7 +137,7 @@ static int rtc_probe(struct amba_device *dev, void *id)
>  	return 0;
>  
>   irq_out:
> -	free_irq(dev->irq[0], dev);
> +	free_irq(dev->irq[0], NULL);
>   map_out:
>  	iounmap(rtc_base);
>  	rtc_base = NULL;
> @@ -153,7 +153,7 @@ static int rtc_remove(struct amba_device *dev)
>  
>  	writel(0, rtc_base + RTC_CR);
>  
> -	free_irq(dev->irq[0], dev);
> +	free_irq(dev->irq[0], NULL);
>  	unregister_rtc(&rtc_ops);
>  
>  	iounmap(rtc_base);

Quite frankly, I'd actually prefer to just reinstate "dev" to the irq 
registration instead.

Why? Because that field is how we are able to track multiple interrupt 
registrations that share an IRQ. Now, the "request_irq()" logic has a 
special rule that actually tests that NULL is a valid cookie if the 
IRQF_SHARED bit isn't set, but isn't it a nice thing to double-check 
regardless?

> index 37fe80d..5681c01 100644
> --- a/drivers/char/mwave/tp3780i.c
> +++ b/drivers/char/mwave/tp3780i.c
> @@ -274,7 +274,8 @@ int tp3780I_ReleaseResources(THINKPAD_BD_DATA * pBDData)
>  	release_region(pSettings->usDspBaseIO & (~3), 16);
>  
>  	if (pSettings->bInterruptClaimed) {
> -		free_irq(pSettings->usDspIrq, NULL);
> +		free_irq(pSettings->usDspIrq,
> +			 (void *)(unsigned long) pSettings->usDspIrq);

This, in contrast, *really* sucks as a cookie. In fact, it's useless. If 
there are multiple tp3780i instances on the same irq, they will always 
have the same cookie.

That's why we pass in a "device" pointer or similar - exactly to make sure 
that the cookie is unique for that irq resource!

So it would be much nicer to fix the cookie to be a real device pointer 
(why not "pSettings" itself?) that is stable across the whole series of 
request_irq/free_irq. And that also would tend to get rid of the 
butt-ugly casts.

Willing to fix those up?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/