netdev - Re: delay via-rhine irq initialisation.

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:	Sat, 22 Dec 2007 22:36:44 -0500
From:	Jeff Garzik <jeff@...zik.org>
To:	Dave Jones <davej@...hat.com>
CC:	netdev@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: delay via-rhine irq initialisation.

Dave Jones wrote:
> With CONFIG_DEBUG_SHIRQ set, via-rhine complains during init.
> (See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=377721 for a report).
> 
> Does this diff look right?
> (I don't have a via-rhine handy to test with)
> 
> We may be able to get away with moving the request_irq to just after the
> alloc_tbufs(), but I feel if a real interrupt occured, this diff would
> stand more chance of doing the right thing.
> 
> Comments?
> 
> 	Dave
> 
> Delay irq registration until after we've allocated ring buffers,
> otherwise DEBUG_SHIRQ will complain.
> 
> Signed-off-by: Dave Jones <davej@...hat.com>
> 
> diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
> index 07263cd..37b3efb 100644
> --- a/drivers/net/via-rhine.c
> +++ b/drivers/net/via-rhine.c
> @@ -1151,24 +1151,28 @@ static int rhine_open(struct net_device *dev)
>  	void __iomem *ioaddr = rp->base;
>  	int rc;
>  
> -	rc = request_irq(rp->pdev->irq, &rhine_interrupt, IRQF_SHARED, dev->name,
> -			dev);
> -	if (rc)
> -		return rc;
> -
>  	if (debug > 1)
>  		printk(KERN_DEBUG "%s: rhine_open() irq %d.\n",
>  		       dev->name, rp->pdev->irq);
>  
>  	rc = alloc_ring(dev);
> -	if (rc) {
> -		free_irq(rp->pdev->irq, dev);
> +	if (rc)
>  		return rc;
> -	}
> +
>  	alloc_rbufs(dev);
>  	alloc_tbufs(dev);
>  	rhine_chip_reset(dev);
>  	init_registers(dev);
> +
> +	rc = request_irq(rp->pdev->irq, &rhine_interrupt, IRQF_SHARED, dev->name,
> +			dev);
> +	if (rc) {
> +		free_rbufs(dev);
> +		free_tbufs(dev);
> +		free_ring(dev);
> +		return rc;
> +	}
> +

Absolutely we want to do this.  DEBUG_SHIRQ is only one of many reasons 
-- quite simply, you are fixing an order-of-init bug that leads to races 
and other badness.

I would request that the error cleanup be done in the standard style for 
net drivers (indeed, most drivers):

	rc = request_irq(...);
	if (rc)
		goto err_out;

	...

	return 0;

     err_out:
	...
	return rc;

Also I would change the subject to something like "fix order of init 
bugs" or "fix races" or whatnot.

We must assume, when writing drivers, that an interrupt will be 
delivered _immediately_ once request_irq() is called -- possibly even 
before request_irq() has returned its return code.

Regards,

	Jeff


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html