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 <[email protected]>.
Signed-off-by: Seokmann Ju <[email protected]>
---
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 <[email protected]>
-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 <[email protected]>
+
Release Date : Mon Apr 11 12:27:22 EST 2006 - Seokmann Ju <[email protected]>
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;
---
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 <[email protected]>.
[...]
> 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
Seokmann Ju <[email protected]> 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 <[email protected]>.
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