linux-kernel - Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

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, 15 Feb 2011 09:25:00 -0800
From:	Greg KH <gregkh@...e.de>
To:	KY Srinivasan <kys@...rosoft.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"virtualization@...ts.osdl.org" <virtualization@...ts.osdl.org>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	Hank Janssen <hjanssen@...rosoft.com>
Subject: Re: [PATCH ]:Staging: hv: Allocate the vmbus irq dynamically

On Tue, Feb 15, 2011 at 04:53:41PM +0000, KY Srinivasan wrote:
> > > @@ -518,19 +534,23 @@ static int vmbus_bus_init(void)
> > >  	}
> > >
> > >  	/* Get the interrupt resource */
> > > -	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > > -			  driver->name, NULL);
> > > -
> > > -	if (ret != 0) {
> > > -		DPRINT_ERR(VMBUS_DRV, "ERROR - Unable to request IRQ %d",
> > > -			   vmbus_irq);
> > > -
> > > +get_irq_again:
> > > +	vmbus_irq = vmbus_get_irq();
> > > +	if ((vmbus_irq < 0) || (prev_irq == vmbus_irq)) {
> > > +		printk(KERN_WARNING "VMBUS_DRV: Failed to allocate IRQ\n");
> > >  		bus_unregister(&vmbus_drv_ctx->bus);
> > > -
> > >  		ret = -1;
> > 
> > Please provide a "real" error code.
> 
> Will do.
> > 
> > >  		goto cleanup;
> > >  	}
> > > -	vector = VMBUS_IRQ_VECTOR;
> > > +	prev_irq = vmbus_irq;
> > > +
> > > +	ret = request_irq(vmbus_irq, vmbus_isr, IRQF_SAMPLE_RANDOM,
> > > +			  driver->name, NULL);
> > > +
> > > +	if (ret != 0)
> > > +		goto get_irq_again;
> > 
> > You just set up the potential for an endless loop.  You almost _never_
> > want to goto backwards in a function.  Please don't do this.
> 
> The loop is not endless. We need to take care of two conditions here:
> 1) From the point we selected an irq line to the point where we call
> request_irq(), it is possible that someone else could have requested
> the same line and so we need to close this window.
> 2) request_irq() may fail for reasons other than the fact that we
> may have lost the line that we thought we got.
> 
> So, while we loopback, we check to see if the irq line we got is the same 
> that we got before (refer to the check soon after the call
> to vmbus_get_irq()). This check would ensure that we would not loop
> endlessly.

That's very subtle, and easy to overlook.  So please don't do it.  Make
it obvious that you will not constantly loop, and again, don't call goto
backwards.  goto is for error handling only, unless you are writing
scheduler code, and you aren't doing that here.

Please rework your logic before resending this.

thanks,

greg k-h
--
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/