The driver flag dynids.use_driver_data is almost consistently not set, and
causes more problems than it solves. The only use of this flag is to
prevent the system administrator from telling a pci driver additional data
when adding a dynamic match table entry via sysfs.
We do not as a policy protect the kernel from bad acts by root.
If we are worried about drivers doing the wrong thing due the match data, we
should be setting a taint flag, not attempting to cut off the admin.
[1] Searching the next-20080709 tree shows the bit is set by exactly 3 pci
drivers. However, the use of per-match-entry driver data is much more
prevalent: A boot of allyesconfig on a powerpc64 pseries with a debug patch
shows 27 drivers apparently use the field for a pointer, 14 use it for
setting flags, and 98 use it as a table index. (Pointers are defined as
>PAGE_OFFSET, aka in the 64 bit kernel linear mapping. Flags are defined as
the maximum value exceeds the number of entries in the match table. Any
other nonzero value is classified as an index).
Signed-off-by: Milton Miller <miltonm@xxxxxxx>
---
This behavior actually caused me several hours of debug recently.
I was working working with a cxgb3 card on a 2.6.24 driver. The card was
not recognized by the system, because the subsystem device id (owned by the
subsystem vendor, not the silicon manufacturer) did not match the driver
specified value of 1. I inspected the driver expected to handle the card,
found the mismatch, and attempted to add a dynamic id through sysfs to get
the driver to match. I inspected the existing table in the code and noted
that the driver needed to be told "this is card type 2" via the additional
data, and added the appropriate number of zeros to place a 2 in the driver
data column. However, the kernel quickly oopsed and crashed due to a bad
function call address. Great, now the kernel is crashed, what is wrong
with the driver. After coming back and trying another card, I had to start
tracing manually trying to find where the driver went astray. I was able
to quickly determine the bad address was a text string, but that did not
help me find the problem. The call site was in the middle of a large init
function, and it was not obvious even with the listing which function
pointer was corrupted. While I was looking for some data dependent
overwrite, vpd formating or parsing problem, or endian issue, it turned out
not to be the case. I eventually determined the following sequence of
events:
The driver took the driver data and performed a range check. It then
indexed into a table, and (since it was given 0 when I specified 2), it
incorrectly determined the card had two ports instead of one. It then
copied the vpd into an array, and indexed into the array based on a template
of what it thought the vpd would contain (as opposed to parsing the tags in
the vpd with their length fields). The code then searched for descriptions
of the phy on each port. It found port 0, indexed into another table and
filled out its pointers. It then searched for the non-existent port 1,
searching for a non-zero value. Because the card actually only had one
port, it went off the end of the array and found some random number in
memory. It then used that random value as the type to index into its
second array to fill out the function pointers to be used for the second
port. It proceeded on, in the same function after inlining, to iterate over
the ports on the card to initialize each port. It calls the function for
the first port, all goes well. It advances a pointer to point to the next
port, calls the first function, and oopses.
Of course, when debugging, one sees some function pointer is garbage. But
which one? (This was on PowerPC64, so the function pointer is a descriptor
pointer. The pointer itself looks valid, but instead of pointing to a
descriptor with a code address and a constant pool, it points to random
kernel data.
Only after careful analysis of the driver data structure did I find it was
looking for more ports. After all, I had told the driver to use slot two,
and only expected the card to have one port described in adapter info table>
While there is could certainly be more defensive coding in the driver (eg:
parse the vpd instead of relying on knowing the tag layout, only search
until the end of the vpd data, range check the port type before indexing
into an array), the whole session should have been avoided once I looked at
the driver and carefully set the driver data to entry 2, to match the other
users of the silicon.
diff --git a/drivers/i2c/busses/i2c-amd756.c b/drivers/i2c/busses/i2c-amd756.c
index 1ea3925..a3542b0 100644
--- a/drivers/i2c/busses/i2c-amd756.c
+++ b/drivers/i2c/busses/i2c-amd756.c
@@ -412,7 +412,6 @@ static struct pci_driver amd756_driver = {
.id_table = amd756_ids,
.probe = amd756_probe,
.remove = __devexit_p(amd756_remove),
- .dynids.use_driver_data = 1,
};
static int __init amd756_init(void)
diff --git a/drivers/i2c/busses/i2c-viapro.c b/drivers/i2c/busses/i2c-viapro.c
index 862eb35..87a04ea 100644
--- a/drivers/i2c/busses/i2c-viapro.c
+++ b/drivers/i2c/busses/i2c-viapro.c
@@ -468,7 +468,6 @@ static struct pci_driver vt596_driver = {
.name = "vt596_smbus",
.id_table = vt596_ids,
.probe = vt596_probe,
- .dynids.use_driver_data = 1,
};
static int __init i2c_vt596_init(void)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index a13f534..4940a53 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -65,8 +65,7 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
dynid->id.subdevice = subdevice;
dynid->id.class = class;
dynid->id.class_mask = class_mask;
- dynid->id.driver_data = pdrv->dynids.use_driver_data ?
- driver_data : 0UL;
+ dynid->id.driver_data = driver_data;
spin_lock(&pdrv->dynids.lock);
list_add_tail(&dynid->node, &pdrv->dynids.list);
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 999e91e..42ad25a 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -7854,7 +7854,6 @@ static struct pci_driver ipr_driver = {
.remove = ipr_remove,
.shutdown = ipr_shutdown,
.err_handler = &ipr_err_handler,
- .dynids.use_driver_data = 1
};
/**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a6a088e..b217a37 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -345,7 +345,6 @@ struct pci_bus_region {
struct pci_dynids {
spinlock_t lock; /* protects list, index */
struct list_head list; /* for IDs added at runtime */
- unsigned int use_driver_data:1; /* pci_device_id->driver_data is used */
};
/* ---------------------------------------------------------------- */
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
