The patch titled
pci: dynids.use_driver_data considered harmful
has been removed from the -mm tree. Its filename was
pci-dynidsuse_driver_data-considered-harmful.patch
This patch was dropped because it was nacked
The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
------------------------------------------------------
Subject: pci: dynids.use_driver_data considered harmful
From: Milton Miller <miltonm@xxxxxxx>
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).
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.
Signed-off-by: Milton Miller <miltonm@xxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxx>
Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxx>
Cc: Brian King <brking@xxxxxxxxxx>
Cc: Jean Delvare <khali@xxxxxxxxxxxx>
Cc: Tejun Heo <htejun@xxxxxxxxx>
Cc: Michael Ellerman <michael@xxxxxxxxxxxxxx>
Cc: Jeff Garzik <jeff@xxxxxxxxxx>
Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
drivers/i2c/busses/i2c-amd756.c | 1 -
drivers/i2c/busses/i2c-viapro.c | 1 -
drivers/pci/pci-driver.c | 3 +--
drivers/scsi/ipr.c | 1 -
include/linux/pci.h | 1 -
5 files changed, 1 insertion(+), 6 deletions(-)
diff -puN drivers/i2c/busses/i2c-amd756.c~pci-dynidsuse_driver_data-considered-harmful drivers/i2c/busses/i2c-amd756.c
--- a/drivers/i2c/busses/i2c-amd756.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/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 -puN drivers/i2c/busses/i2c-viapro.c~pci-dynidsuse_driver_data-considered-harmful drivers/i2c/busses/i2c-viapro.c
--- a/drivers/i2c/busses/i2c-viapro.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/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 -puN drivers/pci/pci-driver.c~pci-dynidsuse_driver_data-considered-harmful drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/drivers/pci/pci-driver.c
@@ -65,8 +65,7 @@ store_new_id(struct device_driver *drive
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 -puN drivers/scsi/ipr.c~pci-dynidsuse_driver_data-considered-harmful drivers/scsi/ipr.c
--- a/drivers/scsi/ipr.c~pci-dynidsuse_driver_data-considered-harmful
+++ a/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 -puN include/linux/pci.h~pci-dynidsuse_driver_data-considered-harmful include/linux/pci.h
--- a/include/linux/pci.h~pci-dynidsuse_driver_data-considered-harmful
+++ a/include/linux/pci.h
@@ -339,7 +339,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 */
};
/* ---------------------------------------------------------------- */
_
Patches currently in -mm which might be from miltonm@xxxxxxx are
origin.patch
alsa-correct-kcalloc-usage.patch
pci-dynidsuse_driver_data-considered-harmful.patch
kcalloc-remove-runtime-division.patch
--
To unsubscribe from this list: send the line "unsubscribe mm-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
