linux-kernel - Re: [PATCH 2/3] g_NCR5380: Test the IRQ before accepting it

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: <201610310935.58892.linux@rainbow-software.org>
Date:   Mon, 31 Oct 2016 09:35:58 +0100
From:   Ondrej Zary <linux@...nbow-software.org>
To:     Finn Thain <fthain@...egraphics.com.au>
Cc:     Christoph Hellwig <hch@...radead.org>, linux-scsi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] g_NCR5380: Test the IRQ before accepting it

On Monday 31 October 2016, Finn Thain wrote:
> On Sun, 30 Oct 2016, Ondrej Zary wrote:
> > Trigger an IRQ first with a test IRQ handler to find out if it really
> > works. Disable the IRQ if not.
> >
> > This prevents hang when incorrect IRQ was specified by user.
> >
> > Signed-off-by: Ondrej Zary <linux@...nbow-software.org>
> > ---
> >  drivers/scsi/g_NCR5380.c |   32 ++++++++++++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> > index 3790ed5..261e168 100644
> > --- a/drivers/scsi/g_NCR5380.c
> > +++ b/drivers/scsi/g_NCR5380.c
> > @@ -67,6 +67,14 @@
> >  MODULE_ALIAS("g_NCR5380_mmio");
> >  MODULE_LICENSE("GPL");
> >
> > +static bool irq_working;
> > +
> > +static irqreturn_t test_irq(int irq, void *dev_id)
> > +{
> > +	irq_working = true;
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  /*
> >   * Configure I/O address of 53C400A or DTC436 by writing magic numbers
> >   * to ports 0x779 and 0x379.
> > @@ -275,10 +283,30 @@ static int generic_NCR5380_init_one(struct
> > scsi_host_template *tpnt, /* set IRQ for HP C2502 */
> >  		if (board == BOARD_HP_C2502)
> >  			magic_configure(port_idx, instance->irq, magic);
> > -		if (request_irq(instance->irq, generic_NCR5380_intr,
> > -				0, "NCR5380", instance)) {
> > +		/* test if the IRQ is working */
> > +		irq_working = false;
> > +		if (request_irq(instance->irq, test_irq,
> > +				0, "NCR5380-irqtest", NULL)) {
> >  			printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n",
> > instance->host_no, instance->irq); instance->irq = NO_IRQ;
> > +		} else {
> > +			NCR5380_trigger_irq(instance);
> > +			NCR5380_read(RESET_PARITY_INTERRUPT_REG);
> > +			free_irq(instance->irq, NULL);
> > +			if (irq_working) {
> > +				if (request_irq(instance->irq,
> > +						generic_NCR5380_intr, 0,
> > +						"NCR5380", instance)) {
> > +					printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts
> > disabled\n", +					       instance->host_no,
> > +					       instance->irq);
> > +					instance->irq = NO_IRQ;
> > +				}
> > +			} else {
> > +				printk(KERN_WARNING "scsi%d : IRQ%d not working, interrupts
> > disabled\n", +				       instance->host_no, instance->irq);
> > +				instance->irq = NO_IRQ;
> > +			}
> >  		}
> >  	}
>
> If the user omits to specify an irq, you can just default to IRQ_AUTO.
> This might result in NO_IRQ, which gives the same result as this patch.

Looks like a good idea.

> And when the user does specify an IRQ, we should trust them. So this
> compexity doesn't add any value AFAICT. Thanks but no thanks.

This fixes a real problem: specifying wrong IRQ hangs the machine completely. 
It's really easy - if the IRQ is free but configured in BIOS as PCI IRQ (not 
ISA). Everything seems fine except the IRQ will never trigger.

-- 
Ondrej Zary