[PATCH 1/1] scsi : megaraid_{mm, mbox} : irq data type fix

[PATCH 1/1] scsi : megaraid_{mm, mbox} : irq data type fix

[Seokmann Ju]

This patch fixes incorrect irq data type in the driver which led driver initialization failure in some cases.
The problem was reported by Eric @. Biederman <ebiederm@xmission.com>.

Signed-off-by: Seokmann Ju <seokmann.ju@lsil.com>
---
diff -Naur old/Documentation/scsi/ChangeLog.megaraid new/Documentation/scsi/ChangeLog.megaraid
--- old/Documentation/scsi/ChangeLog.megaraid	2006-08-08 15:52:49.000000000 -0400
+++ new/Documentation/scsi/ChangeLog.megaraid	2006-08-09 08:39:18.000000000 -0400
@@ -1,5 +1,5 @@
 Release Date	: Fri May 19 09:31:45 EST 2006 - Seokmann Ju <sju@lsil.com>
-Current Version : 2.20.4.9 (scsi module), 2.20.2.6 (cmm module)
+Current Version : 2.20.4.9 (scsi module), 2.20.2.7 (cmm module)
 Older Version	: 2.20.4.8 (scsi module), 2.20.2.6 (cmm module)
 
 1.	Fixed a bug in megaraid_init_mbox().
@@ -121,6 +121,23 @@
 	> **************************************************************
 	> ****************
 
+4.	Incorrect data type has been used for 'irq' variable in the driver
+	as pointed out by Eric below.
+
+	> Sent: Monday, August 07, 2006 11:29 AM
+	> Subject: [PATCH] megaraid: Use the proper type to hold the irq number.
+	> 
+	> When testing on a Unisys machine it was discovered that
+	> the megaraid driver would not initialize as it was
+	> requesting irq 162 instead of irq 1442 it was assigned.
+	> The problem was the irq number had been truncated by being
+	> stored in an unsigned char.
+	> 
+	> The ioctl interface appears fundamentally broken as it exports
+	> the irq number to user space in an unsigned char. 
+	> 
+	> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
+
 Release Date	: Mon Apr 11 12:27:22 EST 2006 - Seokmann Ju <sju@lsil.com>
 Current Version : 2.20.4.8 (scsi module), 2.20.2.6 (cmm module)
 Older Version	: 2.20.4.7 (scsi module), 2.20.2.6 (cmm module)
diff -Naur old/drivers/scsi/megaraid/mega_common.h new/drivers/scsi/megaraid/mega_common.h
--- old/drivers/scsi/megaraid/mega_common.h	2006-08-08 15:53:44.000000000 -0400
+++ new/drivers/scsi/megaraid/mega_common.h	2006-08-08 15:56:03.000000000 -0400
@@ -175,7 +175,7 @@
 	uint8_t			max_lun;
 
 	uint32_t		unique_id;
-	uint8_t			irq;
+	unsigned int		irq;
 	uint8_t			ito;
 	caddr_t			ibuf;
 	dma_addr_t		ibuf_dma_h;
diff -Naur old/drivers/scsi/megaraid/megaraid_ioctl.h new/drivers/scsi/megaraid/megaraid_ioctl.h
--- old/drivers/scsi/megaraid/megaraid_ioctl.h	2006-08-08 15:53:44.000000000 -0400
+++ new/drivers/scsi/megaraid/megaraid_ioctl.h	2006-08-08 15:56:41.000000000 -0400
@@ -183,7 +183,7 @@
 	uint8_t		pci_bus;
 	uint8_t		pci_dev_fn;
 	uint8_t		pci_slot;
-	uint8_t		irq;
+	unsigned int	irq;
 
 	uint32_t	unique_id;
 	uint32_t	host_no;
@@ -209,7 +209,7 @@
 typedef struct mcontroller {
 
 	uint64_t	base;
-	uint8_t		irq;
+	unsigned int	irq;
 	uint8_t		numldrv;
 	uint8_t		pcibus;
 	uint16_t	pcidev;
---

Re: [PATCH 1/1] scsi : megaraid_{mm, mbox} : irq data type fix

[James Bottomley]

On Thu, 2006-08-10 at 12:54 -0400, Seokmann Ju wrote:
> This patch fixes incorrect irq data type in the driver which led driver initialization failure in some cases.
> The problem was reported by Eric @. Biederman <ebiederm@xmission.com>.
[...]
> 	uint32_t		unique_id;
> -	uint8_t			irq;
> +	unsigned int		irq;
>  	uint8_t			ito;

This doesn't look right ... you're altering the size of a packed field
within the ioctl body.  This will break the ioctl ABI (and thus all
tools which use it) unless you employ versioning or some other mechanism
to detect the breakage and compensate.  I really don't think you want to
do this.

James

Re: [PATCH 1/1] scsi : megaraid_{mm, mbox} : irq data type fix

[Eric W. Biederman]

Seokmann Ju <sju@lsil.com> writes:

> This patch fixes incorrect irq data type in the driver which led driver
> initialization failure in some cases.
> The problem was reported by Eric @. Biederman <ebiederm@xmission.com>.

We have a race going on, I read your first message then this one pops up.
But I will reply again for wider distribution.

Nacked-by: Eric Biederman.

The problem is that struct mcontroller is exported to user space
and is a packed structure so it has no free bytes.

Therefore you will break user space when you add bytes in the
middle of your structure.  We can't do that.  You either need
to add an extra field at the end (while keeping the structure size the
same) or you need to add a new ioctl that gets this information correct,
or you need to give up entirely on the idea of exporting the irq
number to user space this way.

What you need not to do is break the kernel<->user space ABI.  That
is just confusing and couples the kernel and user space in unpleasant
ways.

I believe struct mraid_hba_info has the same problem but I could not
see where it was exported to user space.

Eric