For those on lkml, I mistyped the address in the first mail. I just
did a second pass edit on the long debug session prompting this patch
fixed the cc to linux-kernel. So all those not on linux-pci should
be seeing that shortly, or one can look in mmotm.
On Jul 11, 2008, at 11:11 PM, Greg KH wrote:
> On Thu, Jul 10, 2008 at 04:29:37PM -0500, Milton Miller wrote:
>> 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.
>
> But we can try.
>
> As you found out, this flag should have been set in that specific
> driver, right? Why not just fix the driver instead of declaring that
> the flag is not useful at all, despite it actually being used properly
> by a number of drivers already?
Its not one driver, its more like 100 to 150.
Why is giving the driver the data it needs to support a card bad?
The debug session shows that not giving it the data it needs
is worse.
Worse, the kernel thinks it superior to the human and actively ignores
input with out even telling the user it did so.
The only argument I can come up with is if the data is a pointer, then
its likely that that root will not be able to find the correct value or
put it in a script that doesn't get updated on the next kernel install
and causes a crash. Root isn't likely to add driver data unless it
discovers that it is needed, so the existing default of passing 0
should remain.
If you want to argue that being able to tell the driver that it should
treat this card like card X is wrong, you should be arguing that the
whole notion of adding match table entries to the driver is wrong.
When it comes down to it, its the same thing. Yet, we support adding
ids today. The feature was added, and is used successfully with many
drivers. I'm trying to extend the population of drivers where it works
to include those that support more than one similar card but need to
know which one they are dealing with.
You left out:
>> 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).
I did the work to establish the scope of the problem.
So 3 drivers set the flag. 98 + 14 should set it immediately. The 27
that
use the value as a pointer probably can be re-written to use it as an
index. And
the hundreds of other drivers (I don't have the tree here to count)
apparently
don't care, as they most likely never look at that field. (I am
ingoring
conditional copiles that are not enabled by all-yes-config).
> So no, this patch is not good, it should stay as-is.
Obviously I disagree.
Since we added passing the id table entry to get the driver data from
the
table entry, we have close to 150 drivers that use it, instead of old
methods
such has hard coded lists of chips in case statements. Besides making
it easier
to maintain the driver, it actually improves the chances that the
driver can be
used on a new revision or subsystem id of the card.
But we have not been reviewing the setting of the flag with the use in
the driver.
A usage pattern of storing an index or flags has developed, and I claim
that the
flag now hurts more than it helps.
milton
On Jul 12, 2008, at 4:08 PM, Milton Miller wrote:
> You left out:
>>> 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).
>
> I did the work to establish the scope of the problem.
>
For the record or independent investigation, here is the patch and grep
of
output from the kernel log. Hopefully you can read them after this
mailer
mangles them.
milton
[ 30.412634] PCI Driver CyberPro(in cyber2000fb) uses data 3/3 times
for indexes
[ 30.418967] PCI Driver aty128fb(in aty128fb) uses data 37/47 times
for indexes
[ 30.419787] PCI Driver radeonfb(in radeonfb) uses data 98/98 times
for flags
[ 30.421104] PCI Driver sisfb(in sisfb) uses data 11/12 times for
indexes
[ 30.422332] PCI Driver savagefb(in savagefb) uses data 23/23 times
for flags
[ 30.423088] PCI Driver neofb(in neofb) uses data 9/9 times for flags
[ 30.424431] PCI Driver imsttfb(in imsttfb) uses data 1/2 times for
indexes
[ 30.427089] PCI Driver s3fb(in s3fb) uses data 12/12 times for flags
[ 30.428495] PCI Driver sstfb(in sstfb) uses data 1/2 times for
indexes
[ 30.429439] PCI Driver cirrusfb(in cirrusfb) uses data 11/11 times
for indexes
[ 30.431251] PCI Driver gxt4500(in gxt4500) uses data 1/2 times for
indexes
[ 33.082152] PCI Driver epca(in epca) uses data 3/4 times for indexes
[ 37.857569] PCI Driver serial(in 8250_pci) uses data 177/180 times
for indexes
[ 37.868296] PCI Driver jsm(in jsm) uses data 4/5 times for indexes
[ 37.901045] PCI Driver parport_pc(in parport_pc) uses data 52/53
times for indexes
[ 37.906019] PCI Driver parport_serial(in parport_serial) uses data
30/31 times for indexes
[ 38.253736] PCI Driver DAC960(in DAC960) uses data 7/7 times for
pointers
[ 38.983951] PCI Driver e1000e(in e1000e) uses data 29/38 times for
indexes
[ 39.026014] PCI Driver Sundance Technology IPG Triple-Speed
Ethernet(in ipg) uses data 5/6 times for indexes
[ 39.033218] PCI Driver cxgb(in cxgb) uses data 5/7 times for indexes
[ 39.039465] PCI Driver cxgb3(in cxgb3) uses data 9/10 times for
indexes
[ 39.092305] PCI Driver 3c59x(in 3c59x) uses data 37/38 times for
indexes
[ 39.098029] PCI Driver typhoon(in typhoon) uses data 12/13 times for
indexes
[ 39.103976] PCI Driver ne2k-pci(in ne2k_pci) uses data 10/11 times
for indexes
[ 39.122763] PCI Driver e100(in e100) uses data 36/41 times for
indexes
[ 39.129041] PCI Driver tlan(in tlan) uses data 12/13 times for
indexes
[ 39.141155] PCI Driver epic100(in epic100) uses data 2/3 times for
indexes
[ 39.146232] PCI Driver sis190(in sis190) uses data 1/2 times for
indexes
[ 39.151792] PCI Driver sis900(in sis900) uses data 1/2 times for
indexes
[ 39.161699] PCI Driver yellowfin(in yellowfin) uses data 1/2 times
for indexes
[ 39.173144] PCI Driver natsemi(in natsemi) uses data 1/2 times for
indexes
[ 39.184923] PCI Driver fealnx(in fealnx) uses data 2/3 times for
indexes
[ 39.196097] PCI Driver bnx2(in bnx2) uses data 8/9 times for indexes
[ 39.201880] PCI Driver bnx2x(in bnx2x) uses data 2/3 times for
indexes
[ 39.347752] PCI Driver sundance(in sundance) uses data 6/7 times for
indexes
[ 39.372095] PCI Driver forcedeth(in forcedeth) uses data 39/39 times
for flags
[ 39.426772] PCI Driver 8139too(in 8139too) uses data 1/23 times for
indexes
[ 39.455602] PCI Driver r8169(in r8169) uses data 3/10 times for
indexes
[ 39.508688] PCI Driver tmspci(in tmspci) uses data 3/4 times for
indexes
[ 39.520548] PCI Driver fst(in farsync) uses data 7/7 times for
indexes
[ 39.530531] PCI Driver pc300(in pc300) uses data 6/6 times for flags
[ 39.559427] PCI Driver com20020(in com20020_pci) uses data 16/24
times for indexes
[ 39.832122] PCI Driver iwl3945(in iwl3945) uses data 6/6 times for
pointers
[ 39.839834] PCI Driver iwl4965(in iwl4965) uses data 5/5 times for
pointers
[ 39.845806] PCI Driver rt2400pci(in rt2400pci) uses data 1/1 times
for pointers
[ 39.851029] PCI Driver rt2500pci(in rt2500pci) uses data 1/1 times
for pointers
[ 39.856159] PCI Driver rt61pci(in rt61pci) uses data 3/3 times for
pointers
[ 39.877628] PCI Driver ath5k_pci(in ath5k) uses data 17/19 times for
indexes
[ 39.942037] PCI Driver dmfe(in dmfe) uses data 4/4 times for flags
[ 39.949311] PCI Driver winbond-840(in winbond_840) uses data 2/3
times for indexes
[ 39.955770] PCI Driver de2104x(in de2104x) uses data 1/2 times for
indexes
[ 39.961608] PCI Driver tulip(in tulip) uses data 35/35 times for
indexes
[ 39.967483] PCI Driver de4x5(in de4x5) uses data 3/4 times for
indexes
[ 39.974029] PCI Driver uli526x(in uli526x) uses data 2/2 times for
flags
[ 40.101590] PCI Driver via-ircc(in via_ircc) uses data 4/5 times for
indexes
[ 40.150602] PCI Driver sfc(in sfc) uses data 2/2 times for pointers
[ 40.273109] PCI Driver saa7134(in saa7134) uses data 169/173 times
for flags
[ 40.385014] PCI Driver Multimedia eXtension Board(in saa7146) uses
data 1/1 times for pointers
[ 40.392699] PCI Driver hexium HV-PCI6 Orion(in saa7146) uses data
3/3 times for pointers
[ 40.399454] PCI Driver hexium gemini(in saa7146) uses data 2/2 times
for pointers
[ 40.405585] PCI Driver dpc7146 demonstration board(in saa7146) uses
data 1/1 times for pointers
[ 40.694876] PCI Driver budget dvb(in saa7146) uses data 9/9 times
for pointers
[ 40.701401] PCI Driver budget_av(in saa7146) uses data 25/25 times
for pointers
[ 40.707745] PCI Driver budget_ci dvb(in saa7146) uses data 7/7 times
for pointers
[ 40.715095] PCI Driver budget_patch dvb(in saa7146) uses data 1/1
times for pointers
[ 40.722309] PCI Driver dvb(in saa7146) uses data 11/11 times for
pointers
[ 40.757291] PCI Driver bt878(in bt878) uses data 12/12 times for
pointers
[ 40.922318] PCI Driver fore_200e(in fore_200e) uses data 1/1 times
for pointers
[ 40.928212] PCI Driver eni(in eni) uses data 1/2 times for indexes
[ 41.017464] PCI Driver AEC62xx_IDE(in <NULL>) uses data 4/5 times
for indexes
[ 41.023069] PCI Driver ALI15x3_IDE(in <NULL>) uses data 1/2 times
for indexes
[ 41.028564] PCI Driver AMD_IDE(in <NULL>) uses data 22/23 times for
indexes
[ 41.038241] PCI Driver CMD64x_IDE(in <NULL>) uses data 3/4 times for
indexes
[ 41.043407] PCI Driver Cyrix_IDE(in <NULL>) uses data 1/2 times for
indexes
[ 41.057769] PCI Driver HPT366_IDE(in <NULL>) uses data 5/6 times for
indexes
[ 41.084702] PCI Driver Promise_Old_IDE(in <NULL>) uses data 4/5
times for indexes
[ 41.086387] PCI Driver Promise_IDE(in <NULL>) uses data 6/7 times
for indexes
[ 41.086833] PCI Driver PIIX_IDE(in <NULL>) uses data 24/25 times for
indexes
[ 41.087269] PCI Driver Serverworks_IDE(in <NULL>) uses data 4/5
times for indexes
[ 41.087687] PCI Driver SiI_IDE(in <NULL>) uses data 2/3 times for
indexes
[ 41.090972] PCI Driver VIA_IDE(in <NULL>) uses data 2/5 times for
indexes
[ 41.091405] PCI Driver PCI_IDE(in <NULL>) uses data 14/15 times for
indexes
[ 41.296455] PCI Driver aacraid(in aacraid) uses data 63/64 times for
indexes
[ 41.314088] PCI Driver aic94xx(in aic94xx) uses data 9/9 times for
indexes
[ 41.333209] PCI Driver qla1280(in qla1280) uses data 5/6 times for
indexes
[ 41.370864] PCI Driver dmx3191d(in dmx3191d) uses data 1/1 times for
flags
[ 41.473648] PCI Driver ipr sets use_driver_data
[ 41.474349] PCI Driver ipr(in ipr) uses data 11/23 times for indexes
[ 50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for
pointers
[ 50.137434] PCI Driver stex(in stex) uses data 7/11 times for indexes
[ 50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for
indexes
[ 50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for
indexes
[ 50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times for
indexes
[ 50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times
for indexes
[ 50.822788] PCI Driver sata_promise(in sata_promise) uses data 13/17
times for indexes
[ 50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times for
indexes
[ 50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6 times
for indexes
[ 50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times for
indexes
[ 50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times for
indexes
[ 50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times for
indexes
[ 50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times for
indexes
[ 50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times
for indexes
[ 50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5 times
for indexes
[ 50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4
times for flags
[ 50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times
for indexes
[ 50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data 5/7
times for indexes
[ 50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses
data 4/5 times for indexes
[ 50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses
data 4/5 times for indexes
[ 50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13
times for indexes
[ 50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for
pointers
[ 50.137434] PCI Driver stex(in stex) uses data 7/11 times for indexes
[ 50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for
indexes
[ 50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for
indexes
[ 50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times for
indexes
[ 50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times
for indexes
[ 50.822788] PCI Driver sata_promise(in sata_promise) uses data 13/17
times for indexes
[ 50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times for
indexes
[ 50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6 times
for indexes
[ 50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times for
indexes
[ 50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times for
indexes
[ 50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times for
indexes
[ 50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times for
indexes
[ 50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times
for indexes
[ 50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5 times
for indexes
[ 50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4
times for flags
[ 50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times
for indexes
[ 50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data 5/7
times for indexes
[ 50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses
data 4/5 times for indexes
[ 50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses
data 4/5 times for indexes
[ 50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13
times for indexes
[ 52.594478] PCI Driver MTD PCI(in pci) uses data 2/2 times for
pointers
[ 52.784303] PCI Driver yenta_cardbus(in yenta_socket) uses data
47/49 times for pointers
[ 54.779825] PCI Driver ehci_hcd(in ehci_hcd) uses data 1/1 times for
pointers
[ 55.043495] PCI Driver ohci_hcd(in ohci_hcd) uses data 1/1 times for
pointers
[ 55.616852] PCI Driver uhci_hcd(in uhci_hcd) uses data 1/1 times for
pointers
[ 57.372871] PCI Driver i2c_amd756 sets use_driver_data
[ 57.372874] PCI Driver amd756_smbus(in i2c_amd756) uses data 4/5
times for indexes
[ 57.375964] PCI Driver i2c_viapro sets use_driver_data
[ 57.375968] PCI Driver vt596_smbus(in i2c_viapro) uses data 12/12
times for flags
[ 58.609660] PCI Driver c4(in c4) uses data 2/2 times for flags
[ 58.618070] PCI Driver divas(in divas) uses data 11/11 times for
flags
[ 58.660828] PCI Driver hfc4s8s_l1(in hfc4s8s_l1) uses data 4/4 times
for pointers
[ 58.667738] PCI Driver fcpci(in hisax_fcpcipnp) uses data 2/2 times
for pointers
[ 58.725420] PCI Driver sdhci-pci(in sdhci_pci) uses data 7/8 times
for pointers
[ 58.802705] PCI Driver ib_mthca(in ib_mthca) uses data 8/10 times
for indexes
[ 58.908537] PCI Driver trident(in trident) uses data 4/5 times for
indexes
[ 59.167098] PCI Driver ALS300(in snd_als300) uses data 1/2 times for
indexes
[ 59.193937] PCI Driver Bt87x(in snd_bt87x) uses data 10/10 times for
indexes
[ 59.229590] PCI Driver ES1968 (ESS Maestro)(in snd_es1968) uses data
2/3 times for indexes
[ 59.242452] PCI Driver Intel ICH(in snd_intel8x0) uses data 16/23
times for indexes
[ 59.249023] PCI Driver Intel ICH Modem(in snd_intel8x0m) uses data
5/15 times for indexes
[ 59.275975] PCI Driver VIA 82xx Audio(in snd_via82xx) uses data 2/2
times for indexes
[ 59.282351] PCI Driver VIA 82xx Modem(in snd_via82xx_modem) uses
data 1/1 times for indexes
[ 59.293970] PCI Driver au8810(in snd_au8810) uses data 1/1 times for
indexes
[ 59.399694] PCI Driver EMU10K1_Audigy(in snd_emu10k1) uses data 2/3
times for indexes
[ 59.411858] PCI Driver HDA Intel(in snd_hda_intel) uses data 42/51
times for indexes
[ 59.448649] PCI Driver CMI8788(in snd_oxygen) uses data 1/10 times
for indexes
[ 59.454958] PCI Driver AV200(in snd_virtuoso) uses data 2/3 times
for indexes
[ 59.460819] PCI Driver Digigram pcxhr(in snd_pcxhr) uses data 5/6
times for indexes
[ 59.507144] PCI Driver Digigram VX222(in snd_vx222) uses data 1/2
times for indexes
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index a13f534..0aeac3a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -696,6 +696,32 @@ int __pci_register_driver(struct pci_driver *drv,
struct module *owner,
spin_lock_init(&drv->dynids.lock);
INIT_LIST_HEAD(&drv->dynids.list);
+
+ {
+ const struct pci_device_id *ids = drv->id_table;
+ unsigned long max_dat = 0;
+ int count = 0, addr = 0, uses = 0;
+
+ while (ids->vendor || ids->subvendor || ids->class_mask) {
+ count++;
+ if (ids->driver_data)
+ uses++;
+
+ if (ids->driver_data >= PAGE_OFFSET)
+ addr++;
+ else
+ max_dat = max(max_dat, ids->driver_data);
+ ids++;
+ }
+ if (drv->dynids.use_driver_data)
+ pr_info("PCI Driver %s sets use_driver_data\n", mod_name);
+ if (uses)
+ pr_info("PCI Driver %s(in %s) uses data %d/%d times for %s\n",
+ drv->name, mod_name, uses, count,
+ addr ? "pointers" :
+ (max_dat > count) ? "flags" : "indexes");
+ }
+
/* register with core */
error = driver_register(&drv->driver);
if (error)
Greg,
Please respond to this email and explain why the patch
pci: dynids.use_driver_data considered harmful
http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
should not be applied. I am not arguing the correctness of
the removed code, rather its utility and benefit to the linux
community.
As far as I can tell, the code only succeeds in limiting the
usefulness of the pci dynamic id addition mechanism. We chose
not to limit which drivers can have a table entry added, now
let us not limit telling the driver which previous entry is
most similar to our new entry.
If a driver doesn't set this bit, and only 3 out of 419 do,
then the facility can only be used if the driver can function
correctly with the data zero. In some drivers (radeonfb) a
nonzero flag is always set, in some a list of boards or
chipsets is listed in an arbitrary order (e1000e), and in yet
others the field is used as a pointer without checking for NULL
(DAC960, iwl3945).
You sent your request for others to withdraw the patch
from consideration when I resent the patch without seeing your
comments that were less than 12 hours old, but have been silent
for the last 60 hours since I responded to them over the next
several hours. If I do not hear from you with technical
arguments for keeping the code, I will resend the patch for
consideration.
milton
On Jul 12, 2008, at 5:48 PM, Milton Miller wrote:
> On Jul 12, 2008, at 4:08 PM, Milton Miller wrote:
>> You left out:
>>>> 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).
>>
>> I did the work to establish the scope of the problem.
>>
>
> For the record or independent investigation, here is the patch and
> grep of
> output from the kernel log. Hopefully you can read them after this
> mailer
> mangles them.
>
> milton
>
> [ 30.412634] PCI Driver CyberPro(in cyber2000fb) uses data 3/3 times
> for indexes
> [ 30.418967] PCI Driver aty128fb(in aty128fb) uses data 37/47 times
> for indexes
> [ 30.419787] PCI Driver radeonfb(in radeonfb) uses data 98/98 times
> for flags
> [ 30.421104] PCI Driver sisfb(in sisfb) uses data 11/12 times for
> indexes
> [ 30.422332] PCI Driver savagefb(in savagefb) uses data 23/23 times
> for flags
> [ 30.423088] PCI Driver neofb(in neofb) uses data 9/9 times for flags
> [ 30.424431] PCI Driver imsttfb(in imsttfb) uses data 1/2 times for
> indexes
> [ 30.427089] PCI Driver s3fb(in s3fb) uses data 12/12 times for flags
> [ 30.428495] PCI Driver sstfb(in sstfb) uses data 1/2 times for
> indexes
> [ 30.429439] PCI Driver cirrusfb(in cirrusfb) uses data 11/11 times
> for indexes
> [ 30.431251] PCI Driver gxt4500(in gxt4500) uses data 1/2 times for
> indexes
> [ 33.082152] PCI Driver epca(in epca) uses data 3/4 times for indexes
> [ 37.857569] PCI Driver serial(in 8250_pci) uses data 177/180 times
> for indexes
> [ 37.868296] PCI Driver jsm(in jsm) uses data 4/5 times for indexes
> [ 37.901045] PCI Driver parport_pc(in parport_pc) uses data 52/53
> times for indexes
> [ 37.906019] PCI Driver parport_serial(in parport_serial) uses data
> 30/31 times for indexes
> [ 38.253736] PCI Driver DAC960(in DAC960) uses data 7/7 times for
> pointers
> [ 38.983951] PCI Driver e1000e(in e1000e) uses data 29/38 times for
> indexes
> [ 39.026014] PCI Driver Sundance Technology IPG Triple-Speed
> Ethernet(in ipg) uses data 5/6 times for indexes
> [ 39.033218] PCI Driver cxgb(in cxgb) uses data 5/7 times for indexes
> [ 39.039465] PCI Driver cxgb3(in cxgb3) uses data 9/10 times for
> indexes
> [ 39.092305] PCI Driver 3c59x(in 3c59x) uses data 37/38 times for
> indexes
> [ 39.098029] PCI Driver typhoon(in typhoon) uses data 12/13 times
> for indexes
> [ 39.103976] PCI Driver ne2k-pci(in ne2k_pci) uses data 10/11 times
> for indexes
> [ 39.122763] PCI Driver e100(in e100) uses data 36/41 times for
> indexes
> [ 39.129041] PCI Driver tlan(in tlan) uses data 12/13 times for
> indexes
> [ 39.141155] PCI Driver epic100(in epic100) uses data 2/3 times for
> indexes
> [ 39.146232] PCI Driver sis190(in sis190) uses data 1/2 times for
> indexes
> [ 39.151792] PCI Driver sis900(in sis900) uses data 1/2 times for
> indexes
> [ 39.161699] PCI Driver yellowfin(in yellowfin) uses data 1/2 times
> for indexes
> [ 39.173144] PCI Driver natsemi(in natsemi) uses data 1/2 times for
> indexes
> [ 39.184923] PCI Driver fealnx(in fealnx) uses data 2/3 times for
> indexes
> [ 39.196097] PCI Driver bnx2(in bnx2) uses data 8/9 times for indexes
> [ 39.201880] PCI Driver bnx2x(in bnx2x) uses data 2/3 times for
> indexes
> [ 39.347752] PCI Driver sundance(in sundance) uses data 6/7 times
> for indexes
> [ 39.372095] PCI Driver forcedeth(in forcedeth) uses data 39/39
> times for flags
> [ 39.426772] PCI Driver 8139too(in 8139too) uses data 1/23 times for
> indexes
> [ 39.455602] PCI Driver r8169(in r8169) uses data 3/10 times for
> indexes
> [ 39.508688] PCI Driver tmspci(in tmspci) uses data 3/4 times for
> indexes
> [ 39.520548] PCI Driver fst(in farsync) uses data 7/7 times for
> indexes
> [ 39.530531] PCI Driver pc300(in pc300) uses data 6/6 times for flags
> [ 39.559427] PCI Driver com20020(in com20020_pci) uses data 16/24
> times for indexes
> [ 39.832122] PCI Driver iwl3945(in iwl3945) uses data 6/6 times for
> pointers
> [ 39.839834] PCI Driver iwl4965(in iwl4965) uses data 5/5 times for
> pointers
> [ 39.845806] PCI Driver rt2400pci(in rt2400pci) uses data 1/1 times
> for pointers
> [ 39.851029] PCI Driver rt2500pci(in rt2500pci) uses data 1/1 times
> for pointers
> [ 39.856159] PCI Driver rt61pci(in rt61pci) uses data 3/3 times for
> pointers
> [ 39.877628] PCI Driver ath5k_pci(in ath5k) uses data 17/19 times
> for indexes
> [ 39.942037] PCI Driver dmfe(in dmfe) uses data 4/4 times for flags
> [ 39.949311] PCI Driver winbond-840(in winbond_840) uses data 2/3
> times for indexes
> [ 39.955770] PCI Driver de2104x(in de2104x) uses data 1/2 times for
> indexes
> [ 39.961608] PCI Driver tulip(in tulip) uses data 35/35 times for
> indexes
> [ 39.967483] PCI Driver de4x5(in de4x5) uses data 3/4 times for
> indexes
> [ 39.974029] PCI Driver uli526x(in uli526x) uses data 2/2 times for
> flags
> [ 40.101590] PCI Driver via-ircc(in via_ircc) uses data 4/5 times
> for indexes
> [ 40.150602] PCI Driver sfc(in sfc) uses data 2/2 times for pointers
> [ 40.273109] PCI Driver saa7134(in saa7134) uses data 169/173 times
> for flags
> [ 40.385014] PCI Driver Multimedia eXtension Board(in saa7146) uses
> data 1/1 times for pointers
> [ 40.392699] PCI Driver hexium HV-PCI6 Orion(in saa7146) uses data
> 3/3 times for pointers
> [ 40.399454] PCI Driver hexium gemini(in saa7146) uses data 2/2
> times for pointers
> [ 40.405585] PCI Driver dpc7146 demonstration board(in saa7146) uses
> data 1/1 times for pointers
> [ 40.694876] PCI Driver budget dvb(in saa7146) uses data 9/9 times
> for pointers
> [ 40.701401] PCI Driver budget_av(in saa7146) uses data 25/25 times
> for pointers
> [ 40.707745] PCI Driver budget_ci dvb(in saa7146) uses data 7/7
> times for pointers
> [ 40.715095] PCI Driver budget_patch dvb(in saa7146) uses data 1/1
> times for pointers
> [ 40.722309] PCI Driver dvb(in saa7146) uses data 11/11 times for
> pointers
> [ 40.757291] PCI Driver bt878(in bt878) uses data 12/12 times for
> pointers
> [ 40.922318] PCI Driver fore_200e(in fore_200e) uses data 1/1 times
> for pointers
> [ 40.928212] PCI Driver eni(in eni) uses data 1/2 times for indexes
> [ 41.017464] PCI Driver AEC62xx_IDE(in <NULL>) uses data 4/5 times
> for indexes
> [ 41.023069] PCI Driver ALI15x3_IDE(in <NULL>) uses data 1/2 times
> for indexes
> [ 41.028564] PCI Driver AMD_IDE(in <NULL>) uses data 22/23 times for
> indexes
> [ 41.038241] PCI Driver CMD64x_IDE(in <NULL>) uses data 3/4 times
> for indexes
> [ 41.043407] PCI Driver Cyrix_IDE(in <NULL>) uses data 1/2 times for
> indexes
> [ 41.057769] PCI Driver HPT366_IDE(in <NULL>) uses data 5/6 times
> for indexes
> [ 41.084702] PCI Driver Promise_Old_IDE(in <NULL>) uses data 4/5
> times for indexes
> [ 41.086387] PCI Driver Promise_IDE(in <NULL>) uses data 6/7 times
> for indexes
> [ 41.086833] PCI Driver PIIX_IDE(in <NULL>) uses data 24/25 times
> for indexes
> [ 41.087269] PCI Driver Serverworks_IDE(in <NULL>) uses data 4/5
> times for indexes
> [ 41.087687] PCI Driver SiI_IDE(in <NULL>) uses data 2/3 times for
> indexes
> [ 41.090972] PCI Driver VIA_IDE(in <NULL>) uses data 2/5 times for
> indexes
> [ 41.091405] PCI Driver PCI_IDE(in <NULL>) uses data 14/15 times for
> indexes
> [ 41.296455] PCI Driver aacraid(in aacraid) uses data 63/64 times
> for indexes
> [ 41.314088] PCI Driver aic94xx(in aic94xx) uses data 9/9 times for
> indexes
> [ 41.333209] PCI Driver qla1280(in qla1280) uses data 5/6 times for
> indexes
> [ 41.370864] PCI Driver dmx3191d(in dmx3191d) uses data 1/1 times
> for flags
> [ 41.473648] PCI Driver ipr sets use_driver_data
> [ 41.474349] PCI Driver ipr(in ipr) uses data 11/23 times for indexes
> [ 50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for
> pointers
> [ 50.137434] PCI Driver stex(in stex) uses data 7/11 times for
> indexes
> [ 50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for
> indexes
> [ 50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for
> indexes
> [ 50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times
> for indexes
> [ 50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times
> for indexes
> [ 50.822788] PCI Driver sata_promise(in sata_promise) uses data
> 13/17 times for indexes
> [ 50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times
> for indexes
> [ 50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6
> times for indexes
> [ 50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times
> for indexes
> [ 50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times
> for indexes
> [ 50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times
> for indexes
> [ 50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times
> for indexes
> [ 50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times
> for indexes
> [ 50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5
> times for indexes
> [ 50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4
> times for flags
> [ 50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times
> for indexes
> [ 50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data
> 5/7 times for indexes
> [ 50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses
> data 4/5 times for indexes
> [ 50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses
> data 4/5 times for indexes
> [ 50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13
> times for indexes
> [ 50.137032] PCI Driver hptiop(in hptiop) uses data 13/13 times for
> pointers
> [ 50.137434] PCI Driver stex(in stex) uses data 7/11 times for
> indexes
> [ 50.137794] PCI Driver mvsas(in mvsas) uses data 4/5 times for
> indexes
> [ 50.821457] PCI Driver ahci(in ahci) uses data 24/115 times for
> indexes
> [ 50.821921] PCI Driver sata_svw(in sata_svw) uses data 3/7 times
> for indexes
> [ 50.822295] PCI Driver ata_piix(in ata_piix) uses data 46/47 times
> for indexes
> [ 50.822788] PCI Driver sata_promise(in sata_promise) uses data
> 13/17 times for indexes
> [ 50.823838] PCI Driver sata_sil(in sata_sil) uses data 4/7 times
> for indexes
> [ 50.824261] PCI Driver sata_sil24(in sata_sil24) uses data 4/6
> times for indexes
> [ 50.824675] PCI Driver sata_via(in sata_via) uses data 1/7 times
> for indexes
> [ 50.826219] PCI Driver sata_nv(in sata_nv) uses data 11/14 times
> for indexes
> [ 50.826611] PCI Driver sata_uli(in sata_uli) uses data 2/3 times
> for indexes
> [ 50.826979] PCI Driver sata_mv(in sata_mv) uses data 14/16 times
> for indexes
> [ 50.828821] PCI Driver pata_amd(in pata_amd) uses data 19/20 times
> for indexes
> [ 50.829188] PCI Driver pata_artop(in pata_artop) uses data 4/5
> times for indexes
> [ 50.830278] PCI Driver pata_cmd64x(in pata_cmd64x) uses data 3/4
> times for flags
> [ 50.836264] PCI Driver pata_opti(in pata_opti) uses data 1/2 times
> for indexes
> [ 50.838717] PCI Driver pata_pdc2027x(in pata_pdc2027x) uses data
> 5/7 times for indexes
> [ 50.839105] PCI Driver pata_pdc202xx_old(in pata_pdc202xx_old) uses
> data 4/5 times for indexes
> [ 50.840546] PCI Driver pata_serverworks(in pata_serverworks) uses
> data 4/5 times for indexes
> [ 50.843885] PCI Driver ata_generic(in ata_generic) uses data 1/13
> times for indexes
> [ 52.594478] PCI Driver MTD PCI(in pci) uses data 2/2 times for
> pointers
> [ 52.784303] PCI Driver yenta_cardbus(in yenta_socket) uses data
> 47/49 times for pointers
> [ 54.779825] PCI Driver ehci_hcd(in ehci_hcd) uses data 1/1 times
> for pointers
> [ 55.043495] PCI Driver ohci_hcd(in ohci_hcd) uses data 1/1 times
> for pointers
> [ 55.616852] PCI Driver uhci_hcd(in uhci_hcd) uses data 1/1 times
> for pointers
> [ 57.372871] PCI Driver i2c_amd756 sets use_driver_data
> [ 57.372874] PCI Driver amd756_smbus(in i2c_amd756) uses data 4/5
> times for indexes
> [ 57.375964] PCI Driver i2c_viapro sets use_driver_data
> [ 57.375968] PCI Driver vt596_smbus(in i2c_viapro) uses data 12/12
> times for flags
> [ 58.609660] PCI Driver c4(in c4) uses data 2/2 times for flags
> [ 58.618070] PCI Driver divas(in divas) uses data 11/11 times for
> flags
> [ 58.660828] PCI Driver hfc4s8s_l1(in hfc4s8s_l1) uses data 4/4
> times for pointers
> [ 58.667738] PCI Driver fcpci(in hisax_fcpcipnp) uses data 2/2 times
> for pointers
> [ 58.725420] PCI Driver sdhci-pci(in sdhci_pci) uses data 7/8 times
> for pointers
> [ 58.802705] PCI Driver ib_mthca(in ib_mthca) uses data 8/10 times
> for indexes
> [ 58.908537] PCI Driver trident(in trident) uses data 4/5 times for
> indexes
> [ 59.167098] PCI Driver ALS300(in snd_als300) uses data 1/2 times
> for indexes
> [ 59.193937] PCI Driver Bt87x(in snd_bt87x) uses data 10/10 times
> for indexes
> [ 59.229590] PCI Driver ES1968 (ESS Maestro)(in snd_es1968) uses
> data 2/3 times for indexes
> [ 59.242452] PCI Driver Intel ICH(in snd_intel8x0) uses data 16/23
> times for indexes
> [ 59.249023] PCI Driver Intel ICH Modem(in snd_intel8x0m) uses data
> 5/15 times for indexes
> [ 59.275975] PCI Driver VIA 82xx Audio(in snd_via82xx) uses data 2/2
> times for indexes
> [ 59.282351] PCI Driver VIA 82xx Modem(in snd_via82xx_modem) uses
> data 1/1 times for indexes
> [ 59.293970] PCI Driver au8810(in snd_au8810) uses data 1/1 times
> for indexes
> [ 59.399694] PCI Driver EMU10K1_Audigy(in snd_emu10k1) uses data 2/3
> times for indexes
> [ 59.411858] PCI Driver HDA Intel(in snd_hda_intel) uses data 42/51
> times for indexes
> [ 59.448649] PCI Driver CMI8788(in snd_oxygen) uses data 1/10 times
> for indexes
> [ 59.454958] PCI Driver AV200(in snd_virtuoso) uses data 2/3 times
> for indexes
> [ 59.460819] PCI Driver Digigram pcxhr(in snd_pcxhr) uses data 5/6
> times for indexes
> [ 59.507144] PCI Driver Digigram VX222(in snd_vx222) uses data 1/2
> times for indexes
>
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index a13f534..0aeac3a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -696,6 +696,32 @@ int __pci_register_driver(struct pci_driver *drv,
> struct module *owner,
> spin_lock_init(&drv->dynids.lock);
> INIT_LIST_HEAD(&drv->dynids.list);
> +
> + {
> + const struct pci_device_id *ids = drv->id_table;
> + unsigned long max_dat = 0;
> + int count = 0, addr = 0, uses = 0;
> +
> + while (ids->vendor || ids->subvendor || ids->class_mask) {
> + count++;
> + if (ids->driver_data)
> + uses++;
> +
> + if (ids->driver_data >= PAGE_OFFSET)
> + addr++;
> + else
> + max_dat = max(max_dat, ids->driver_data);
> + ids++;
> + }
> + if (drv->dynids.use_driver_data)
> + pr_info("PCI Driver %s sets use_driver_data\n", mod_name);
> + if (uses)
> + pr_info("PCI Driver %s(in %s) uses data %d/%d times for %s\n",
> + drv->name, mod_name, uses, count,
> + addr ? "pointers" :
> + (max_dat > count) ? "flags" : "indexes");
> + }
> +
> /* register with core */
> error = driver_register(&drv->driver);
> if (error)
>
On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
>
> Greg,
>
> Please respond to this email and explain why the patch
>
> pci: dynids.use_driver_data considered harmful
>
> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>
> should not be applied. I am not arguing the correctness of
> the removed code, rather its utility and benefit to the linux
> community.
>
> As far as I can tell, the code only succeeds in limiting the
> usefulness of the pci dynamic id addition mechanism. We chose
> not to limit which drivers can have a table entry added, now
> let us not limit telling the driver which previous entry is
> most similar to our new entry.
>
> If a driver doesn't set this bit, and only 3 out of 419 do,
> then the facility can only be used if the driver can function
> correctly with the data zero. In some drivers (radeonfb) a
> nonzero flag is always set, in some a list of boards or
> chipsets is listed in an arbitrary order (e1000e), and in yet
> others the field is used as a pointer without checking for NULL
> (DAC960, iwl3945).
>
>
>
> You sent your request for others to withdraw the patch
> from consideration when I resent the patch without seeing your
> comments that were less than 12 hours old, but have been silent
> for the last 60 hours since I responded to them over the next
> several hours. If I do not hear from you with technical
> arguments for keeping the code, I will resend the patch for
> consideration.
Sorry, I'm on the road right now and will not get back until Friday.
Then I have the big merges with Linus to get through. I'll try to get
to this by Monday, but my original point still stands, this was
implemented for a reason, saying that not enough drivers use it properly
does not make the need for it to go away. It is required for them, so
perhaps the other 419 drivers also need to have the flag set. That's
pretty trivial to do, right?
thanks,
greg k-h
On Jul 17, 2008, at 2:07 AM, Greg KH wrote:
> On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
>>
>> Greg,
>>
>> Please respond to this email and explain why the patch
>>
>> pci: dynids.use_driver_data considered harmful
>>
>> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>>
>> should not be applied. I am not arguing the correctness of
>> the removed code, rather its utility and benefit to the linux
>> community.
>>
>> As far as I can tell, the code only succeeds in limiting the
>> usefulness of the pci dynamic id addition mechanism.
...
>
> Sorry, I'm on the road right now and will not get back until Friday.
> Then I have the big merges with Linus to get through. I'll try to get
> to this by Monday, but my original point still stands, this was
> implemented for a reason, saying that not enough drivers use it
> properly
> does not make the need for it to go away. It is required for them, so
> perhaps the other 419 drivers also need to have the flag set. That's
> pretty trivial to do, right?
>
Fine, I'll give you a little time. But I want to hear specifics how it
helps drivers. I have shown how it hurts many drivers. My arguement
is that once we set the flag on the drivers, we will end up with it set
on all drivers or the remaining drivers will not care. There were 413+
drivers in linux-next that were compiled by allyesconfig, about 150 used
driver data.
If the purpose is to enforce range checking, then I'll start working on
patches for those 100 drivers to do that. But I don't see why a binary
flag helps any driver. The flag is not "I will accept a additional id
table", its "I will accept the additional driver data that I need to
operate" on my dynamically added id table.
milton
Hi Milton, hi Greg,
On Wed, 16 Jul 2008 05:18:18 -0500, Milton Miller wrote:
> Greg,
>
> Please respond to this email and explain why the patch
>
> pci: dynids.use_driver_data considered harmful
>
> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>
> should not be applied. I am not arguing the correctness of
> the removed code, rather its utility and benefit to the linux
> community.
I _am_ arguing the correctness of the removed code. That code silently
defaults driver_data to 0 when the driver doesn't set
dynids.use_driver_data. This assumes that 0 is always a safe value,
which is not true for all drivers as Milton explains below.
> As far as I can tell, the code only succeeds in limiting the
> usefulness of the pci dynamic id addition mechanism. We chose
> not to limit which drivers can have a table entry added, now
> let us not limit telling the driver which previous entry is
> most similar to our new entry.
>
> If a driver doesn't set this bit, and only 3 out of 419 do,
> then the facility can only be used if the driver can function
> correctly with the data zero. In some drivers (radeonfb) a
> nonzero flag is always set, in some a list of boards or
> chipsets is listed in an arbitrary order (e1000e), and in yet
> others the field is used as a pointer without checking for NULL
> (DAC960, iwl3945).
I have to agree with Milton. Allowing all drivers to use dynamic IDs
first, and then limiting the use of the driver_data field to drivers
setting a specific flag, doesn't make sense. For one thing, just
because the driver sets the flag, doesn't mean that it does the proper
checks on the passed value. For another, as written above, assuming
that 0 is a safe value (that doesn't need to be checked by the driver)
is incorrect.
The correct approach here if you really want to play it safe, would be
to not allow dynamic IDs by default. Drivers would set a flag when they
want to use them (instead of when they want to use driver_data as we
currently do.) Setting the flag would suggest that you have checked
that nothing can go wrong when a dynamic ID is added. As I said above,
it's impossible to actually enforce other than with careful code
review, but if you insist on trying, maybe we can have these drivers
set a validation callback function rather than a flag. In the absence of
validation callback function, dynamic IDs would be forbidden. This has
a cost in terms of driver size though.
That would be a pretty big change, as someone would have to go through
all the PCI drivers and update them. I certainly do not have the time
to do that, but maybe Milton does if we agree on that?
The alternative is to just apply Milton's patch and get away with all
checks. That's perfectly fine with me, as I can't see how it is worse
than what we currently have, but apparently not with Greg (although I
still don't know why.) If Milton's patch isn't going to be applied,
then I would like to suggest applying the following patch of mine as a
measure to prevent what happened to Milton and started this discussion:
* * * * *
Subject: PCI: Don't silently ignore dynids driver data
If driver data is provided when adding a dynamic ID to a PCI driver,
but the driver doesn't actually support that, fail, instead of
silently defaulting to a value of 0.
Likewise, if the driver expects driver_data and the user didn't
provide it, fail, instead of silently defaulting to a value of 0.
Signed-off-by: Jean Delvare <[email protected]>
Cc: Milton Miller <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Jesse Barnes <[email protected]>
---
drivers/pci/pci-driver.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--- linux-2.6.26-rc9.orig/drivers/pci/pci-driver.c 2008-06-23 08:38:32.000000000 +0200
+++ linux-2.6.26-rc9/drivers/pci/pci-driver.c 2008-07-13 09:59:39.000000000 +0200
@@ -54,6 +54,10 @@ store_new_id(struct device_driver *drive
&class, &class_mask, &driver_data);
if (fields < 2)
return -EINVAL;
+ /* Presence of driver_data must match the use_driver_data flag */
+ if ((fields >= 7 && !pdrv->dynids.use_driver_data)
+ || (fields < 7 && pdrv->dynids.use_driver_data))
+ return -EINVAL;
dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
if (!dynid)
@@ -65,8 +69,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);
* * * * *
> You sent your request for others to withdraw the patch
> from consideration when I resent the patch without seeing your
> comments that were less than 12 hours old, but have been silent
> for the last 60 hours since I responded to them over the next
> several hours. If I do not hear from you with technical
> arguments for keeping the code, I will resend the patch for
> consideration.
Milton, please keep in mind that many people are on vacation or
meetings at this time of the year. Delayed replies aren't unusual, and
neither is plain lack of reply. When you come back from vacation and
have over 1300 mails to process, you _have_ to ignore some of the
discussions. Important things will resurface later anyway.
Thanks,
--
Jean Delvare
Hi Greg,
On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
> On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
> >
> > Greg,
> >
> > Please respond to this email and explain why the patch
> >
> > pci: dynids.use_driver_data considered harmful
> >
> > http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> >
> > should not be applied. I am not arguing the correctness of
> > the removed code, rather its utility and benefit to the linux
> > community.
>
> (...) I'll try to get
> to this by Monday, but my original point still stands, this was
> implemented for a reason,
Not a good enough argument, sorry. There have been many cases in the
past where code has been withdrawn after some times because we
realized that we got it wrong in the first place.
So, please explain what the current code is good for. Honestly, my
initial reaction to Milton's proposal was "what an idiot, this flag is
there for an obvious safety reason and we don't want to remove it" but
after reading both his arguments and the code, I found that I have
nothing to backup my claim. If you do, please let us know your
technical reasons.
> saying that not enough drivers use it properly
> does not make the need for it to go away. It is required for them, so
> perhaps the other 419 drivers also need to have the flag set. That's
> pretty trivial to do, right?
If you are suggesting to blindly set the flag to all PCI drivers (or
even just all the ones which make use of the driver_data field -
doesn't make a difference), this simply shows how useless this flag is.
If you don't, then one would have to check the code of all drivers and
add validation code for the driver_data value; but then this no longer
falls into the "trivial" category.
Thanks,
--
Jean Delvare
On Wed, Aug 06, 2008 at 09:31:18AM +0200, Jean Delvare wrote:
> Hi Greg,
>
> On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
> > On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
> > >
> > > Greg,
> > >
> > > Please respond to this email and explain why the patch
> > >
> > > pci: dynids.use_driver_data considered harmful
> > >
> > > http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> > >
> > > should not be applied. I am not arguing the correctness of
> > > the removed code, rather its utility and benefit to the linux
> > > community.
> >
> > (...) I'll try to get
> > to this by Monday, but my original point still stands, this was
> > implemented for a reason,
>
> Not a good enough argument, sorry. There have been many cases in the
> past where code has been withdrawn after some times because we
> realized that we got it wrong in the first place.
Fair enough :)
> So, please explain what the current code is good for. Honestly, my
> initial reaction to Milton's proposal was "what an idiot, this flag is
> there for an obvious safety reason and we don't want to remove it" but
> after reading both his arguments and the code, I found that I have
> nothing to backup my claim. If you do, please let us know your
> technical reasons.
The technical reason was that this flag was needed to let some drivers
work properly with the new_id file, right?
If the flag goes away, they break from what I can tell.
> > saying that not enough drivers use it properly
> > does not make the need for it to go away. It is required for them, so
> > perhaps the other 419 drivers also need to have the flag set. That's
> > pretty trivial to do, right?
>
> If you are suggesting to blindly set the flag to all PCI drivers (or
> even just all the ones which make use of the driver_data field -
> doesn't make a difference), this simply shows how useless this flag is.
> If you don't, then one would have to check the code of all drivers and
> add validation code for the driver_data value; but then this no longer
> falls into the "trivial" category.
It's pretty "trivial" to look to see if the field is set in the pci_id
structure, that should be all that is needed, right?
thanks,
greg k-h
On Aug 14, 2008, at 5:12 PM, Greg KH wrote:
> On Wed, Aug 06, 2008 at 09:31:18AM +0200, Jean Delvare wrote:
>> Hi Greg,
>>
>> On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
>>> On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
>>>>
>>>> Greg,
>>>>
>>>> Please respond to this email and explain why the patch
>>>>
>>>> pci: dynids.use_driver_data considered harmful
>>>>
>>>> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
>>>>
>>>> should not be applied. I am not arguing the correctness of
>>>> the removed code, rather its utility and benefit to the linux
>>>> community.
>>>
>>> (...) I'll try to get
>>> to this by Monday, but my original point still stands, this was
>>> implemented for a reason,
>>
>> Not a good enough argument, sorry. There have been many cases in the
>> past where code has been withdrawn after some times because we
>> realized that we got it wrong in the first place.
>
> Fair enough :)
>
>> So, please explain what the current code is good for. Honestly, my
>> initial reaction to Milton's proposal was "what an idiot, this flag is
>> there for an obvious safety reason and we don't want to remove it" but
>> after reading both his arguments and the code, I found that I have
>> nothing to backup my claim. If you do, please let us know your
>> technical reasons.
>
> The technical reason was that this flag was needed to let some drivers
> work properly with the new_id file, right?
>
> If the flag goes away, they break from what I can tell.
That is not a detailed technical argument, its an assumption.
>
>>> saying that not enough drivers use it
>>> properly
>>> does not make the need for it to go away. It is required for them,
>>> so
>>> perhaps the other 419 drivers also need to have the flag set. That's
>>> pretty trivial to do, right?
>>
>> If you are suggesting to blindly set the flag to all PCI drivers (or
>> even just all the ones which make use of the driver_data field -
>> doesn't make a difference), this simply shows how useless this flag
>> is.
>> If you don't, then one would have to check the code of all drivers and
>> add validation code for the driver_data value; but then this no longer
>> falls into the "trivial" category.
>
> It's pretty "trivial" to look to see if the field is set in the pci_id
> structure, that should be all that is needed, right?
So please identify at least one such driver where the only correct
answer is 0. I even made it easy for you, i identified which drivers
set dynamic_id and how. I identified specific drivers where its the
wrong answer.
So: If you are arguing the code is still needed, please identify at
least one case that it is correct. (Oh, I don't buy that if 0 is safe
but 1 is equally safe, that the existing code is correct).
milton
Hi Greg,
On Thu, 14 Aug 2008 15:12:14 -0700, Greg KH wrote:
> On Wed, Aug 06, 2008 at 09:31:18AM +0200, Jean Delvare wrote:
> > Hi Greg,
> >
> > On Thu, 17 Jul 2008 00:07:59 -0700, Greg KH wrote:
> > > On Wed, Jul 16, 2008 at 05:18:18AM -0500, Milton Miller wrote:
> > > >
> > > > Greg,
> > > >
> > > > Please respond to this email and explain why the patch
> > > >
> > > > pci: dynids.use_driver_data considered harmful
> > > >
> > > > http://www.ussg.iu.edu/hypermail/linux/kernel/0807.1/index.html#2188
> > > >
> > > > should not be applied. I am not arguing the correctness of
> > > > the removed code, rather its utility and benefit to the linux
> > > > community.
> > >
> > > (...) I'll try to get
> > > to this by Monday, but my original point still stands, this was
> > > implemented for a reason,
> >
> > Not a good enough argument, sorry. There have been many cases in the
> > past where code has been withdrawn after some times because we
> > realized that we got it wrong in the first place.
>
> Fair enough :)
>
> > So, please explain what the current code is good for. Honestly, my
> > initial reaction to Milton's proposal was "what an idiot, this flag is
> > there for an obvious safety reason and we don't want to remove it" but
> > after reading both his arguments and the code, I found that I have
> > nothing to backup my claim. If you do, please let us know your
> > technical reasons.
>
> The technical reason was that this flag was needed to let some drivers
> work properly with the new_id file, right?
Hmm, so in fact you don't have a clue? ;)
I wasn't involved in the addition of this flag, but my impression is
that it was added in the hope that it would prevent users from passing
additional data to drivers using the driver_data field but not prepared
(read: not robust enough) to handle possibly wrong data from user-space.
Probably, the idea was that the person adding the
dynids.use_driver_data flag would be responsible for verifying that the
driver had the required checks in place to guarantee that "nothing"
would go wrong even if the data provided by the user wasn't correct.
(This doesn't make much sense IMHO, as there is zero guarantee that the
developer actually did that - but I believe this is the idea behind the
dynids.use_driver_data flag.)
> If the flag goes away, they break from what I can tell.
If the flag goes away, nothing will break per se. In particular, the
drivers which were using the flag won't stop working - you don't prevent
things from happening when removing a safety, on the contrary, you
allow more things to happen. What will change is that all the drivers
which _do_ use the driver_data field but didn't set the
dynids.use_driver_data flag, will now possibly receive their driver_data
from the user.
Which you may say is unsafe, because these drivers may not expect that
(i.e. they probably don't have any check to protect themselves against
bogus driver_data values.) But OTOH most of these drivers can't really
take dynamic IDs at the moment (because the code prevents the user from
passing the required driver_data, silently forcing it to 0) so this
would be a big win in terms of usability. On top of that, considering
that (silently) defaulting to 0 for driver_data is safe, is just plain
wrong. 0 might be an invalid driver_data value for some drivers.
> > > saying that not enough drivers use it properly
> > > does not make the need for it to go away. It is required for them, so
> > > perhaps the other 419 drivers also need to have the flag set. That's
> > > pretty trivial to do, right?
> >
> > If you are suggesting to blindly set the flag to all PCI drivers (or
> > even just all the ones which make use of the driver_data field -
> > doesn't make a difference), this simply shows how useless this flag is.
> > If you don't, then one would have to check the code of all drivers and
> > add validation code for the driver_data value; but then this no longer
> > falls into the "trivial" category.
>
> It's pretty "trivial" to look to see if the field is set in the pci_id
> structure, that should be all that is needed, right?
Well... If you consider that all drivers using the driver_data field
should have the dynids.use_driver_data flag set unconditionally and
without looking at the code, then in fact you agree with Milton and
myself that this flag should be dropped. The whole point of the flag,
if my guess is correct, was to hint the developer that he/she should
double check his/her code before setting the flag. If you set it for
all these drivers, then all it does is preventing users from passing a
driver_data value to drivers which do _not_ use the driver_data... but
doing that has zero effect at the moment, so that wouldn't actually be
any different from dropping dynids.use_driver_data altogether.
So I have to reiterate my support to Milton's idea of dropping
the dynids.use_driver_data flag. It does more harm than good.
Thanks,
--
Jean Delvare
> > It's pretty "trivial" to look to see if the field is set in the pci_id
> > structure, that should be all that is needed, right?
>
> Well... If you consider that all drivers using the driver_data field
> should have the dynids.use_driver_data flag set unconditionally and
> without looking at the code, then in fact you agree with Milton and
> myself that this flag should be dropped. The whole point of the flag,
> if my guess is correct, was to hint the developer that he/she should
> double check his/her code before setting the flag. If you set it for
> all these drivers, then all it does is preventing users from passing a
> driver_data value to drivers which do _not_ use the driver_data... but
> doing that has zero effect at the moment, so that wouldn't actually be
> any different from dropping dynids.use_driver_data altogether.
>
> So I have to reiterate my support to Milton's idea of dropping
> the dynids.use_driver_data flag. It does more harm than good.
Yeah it doesn't seem to be very heavily used at this point. :)
I see around 400 uses of id->driver_data right now, and only 2 of
use_driver_data. If we remove the use_driver_data field, drivers (except for
the 2 using the field) will start to see whatever values userspace passes to
them rather than 0, and at least in some of the few cases I looked at that
could cause breakage due to out of bounds references.
So I think your point about dynamic IDs in general is a good one; this flag
really does look like an "audit was done" bit, but doesn't go as far is it
should.
The patch you posted to forbid dynamic binding unless use_driver_data is iset
is probably the safest thing to do, given that drivers that *don't* set
use_driver_data look like they might misbehave even with a driver_data value
of 0...
What do you think, Greg? Convinced yet? :)
Thanks,
Jesse
Hi Jesse,
On Fri, 15 Aug 2008 10:46:59 -0700, Jesse Barnes wrote:
> So I think your point about dynamic IDs in general is a good one; this flag
> really does look like an "audit was done" bit, but doesn't go as far is it
> should.
>
> The patch you posted to forbid dynamic binding unless use_driver_data is iset
> is probably the safest thing to do, given that drivers that *don't* set
> use_driver_data look like they might misbehave even with a driver_data value
> of 0...
In fact we can do even better than that. We could accept from
user-space only driver_data values which at least one device ID entry in
the driver already uses. That should be fairly easy to implement, and
would offer a level of safety an order of magnitude above what we have
at the moment... And it works both ways: if 0 is not a valid data for
some driver, that would force the user to provide a non-zero (and
valid) data value. And it guarantees that the user can't ask for
something the driver doesn't expect, so drivers don't even need extra
checks. And no need for a use_driver_data flag either.
The only drawback is that it prevents the user from passing a "new"
data value even if it would be valid. But honestly, I don't expect that
case to happen frequently... if ever at all. So I'd say the benefits
totally outweight the drawback.
If the interested people agree with the idea, I'll look into
implementing it.
--
Jean Delvare
On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> In fact we can do even better than that. We could accept from
> user-space only driver_data values which at least one device ID entry in
> the driver already uses. That should be fairly easy to implement, and
> would offer a level of safety an order of magnitude above what we have
> at the moment... And it works both ways: if 0 is not a valid data for
> some driver, that would force the user to provide a non-zero (and
> valid) data value. And it guarantees that the user can't ask for
> something the driver doesn't expect, so drivers don't even need extra
> checks. And no need for a use_driver_data flag either.
Meaning a driver audit of the usage? Yeah that would be great.
> The only drawback is that it prevents the user from passing a "new"
> data value even if it would be valid. But honestly, I don't expect that
> case to happen frequently... if ever at all. So I'd say the benefits
> totally outweight the drawback.
>
> If the interested people agree with the idea, I'll look into
> implementing it.
Well the audit would show if user supplied "new" values are needed; otherwise
the approach sounds good to me.
Thanks
--
Jesse Barnes, Intel Open Source Technology Center
On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > In fact we can do even better than that. We could accept from
> > user-space only driver_data values which at least one device ID entry in
> > the driver already uses. That should be fairly easy to implement, and
> > would offer a level of safety an order of magnitude above what we have
> > at the moment... And it works both ways: if 0 is not a valid data for
> > some driver, that would force the user to provide a non-zero (and
> > valid) data value. And it guarantees that the user can't ask for
> > something the driver doesn't expect, so drivers don't even need extra
> > checks. And no need for a use_driver_data flag either.
>
> Meaning a driver audit of the usage? Yeah that would be great.
>
> > The only drawback is that it prevents the user from passing a "new"
> > data value even if it would be valid. But honestly, I don't expect that
> > case to happen frequently... if ever at all. So I'd say the benefits
> > totally outweight the drawback.
> >
> > If the interested people agree with the idea, I'll look into
> > implementing it.
>
> Well the audit would show if user supplied "new" values are needed; otherwise
> the approach sounds good to me.
That sounds reasonable, and should work properly.
No objection from me.
thanks,
greg k-h
Hi all,
On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
> On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> > On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > > In fact we can do even better than that. We could accept from
> > > user-space only driver_data values which at least one device ID entry in
> > > the driver already uses. That should be fairly easy to implement, and
> > > would offer a level of safety an order of magnitude above what we have
> > > at the moment... And it works both ways: if 0 is not a valid data for
> > > some driver, that would force the user to provide a non-zero (and
> > > valid) data value. And it guarantees that the user can't ask for
> > > something the driver doesn't expect, so drivers don't even need extra
> > > checks. And no need for a use_driver_data flag either.
> >
> > Meaning a driver audit of the usage? Yeah that would be great.
> >
> > > The only drawback is that it prevents the user from passing a "new"
> > > data value even if it would be valid. But honestly, I don't expect that
> > > case to happen frequently... if ever at all. So I'd say the benefits
> > > totally outweight the drawback.
> > >
> > > If the interested people agree with the idea, I'll look into
> > > implementing it.
> >
> > Well the audit would show if user supplied "new" values are needed; otherwise
> > the approach sounds good to me.
>
> That sounds reasonable, and should work properly.
>
> No objection from me.
Ok, here's what it could look like:
* * * * *
From: Jean Delvare <[email protected]>
Subject: PCI: Check dynids driver_data value for validity
Only accept dynids those driver_data value matches one of the driver's
pci_driver_id entry. This prevents the user from accidentally passing
values the drivers do not expect.
Signed-off-by: Jean Delvare <[email protected]>
Cc: Jesse Barnes <[email protected]>
Cc: Milton Miller <[email protected]>
Cc: Greg KH <[email protected]>
---
Documentation/PCI/pci.txt | 4 ++++
drivers/i2c/busses/i2c-amd756.c | 4 ----
drivers/i2c/busses/i2c-viapro.c | 4 ----
drivers/pci/pci-driver.c | 18 ++++++++++++++++--
4 files changed, 20 insertions(+), 10 deletions(-)
--- linux-2.6.27-rc3.orig/Documentation/PCI/pci.txt 2008-08-17 18:24:33.000000000 +0200
+++ linux-2.6.27-rc3/Documentation/PCI/pci.txt 2008-08-17 18:24:38.000000000 +0200
@@ -163,6 +163,10 @@ need pass only as many optional fields a
o class and classmask fields default to 0
o driver_data defaults to 0UL.
+Note that driver_data must match the value used by any of the pci_device_id
+entries defined in the driver. This makes the driver_data field mandatory
+if all the pci_device_id entries have a non-zero driver_data value.
+
Once added, the driver probe routine will be invoked for any unclaimed
PCI devices listed in its (newly updated) pci_ids list.
--- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-amd756.c 2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/i2c/busses/i2c-amd756.c 2008-08-17 19:42:14.000000000 +0200
@@ -332,10 +332,6 @@ static int __devinit amd756_probe(struct
int error;
u8 temp;
- /* driver_data might come from user-space, so check it */
- if (id->driver_data >= ARRAY_SIZE(chipname))
- return -EINVAL;
-
if (amd756_ioport) {
dev_err(&pdev->dev, "Only one device supported "
"(you have a strange motherboard, btw)\n");
--- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-viapro.c 2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/i2c/busses/i2c-viapro.c 2008-08-17 19:42:24.000000000 +0200
@@ -320,10 +320,6 @@ static int __devinit vt596_probe(struct
unsigned char temp;
int error = -ENODEV;
- /* driver_data might come from user-space, so check it */
- if (id->driver_data & 1 || id->driver_data > 0xff)
- return -EINVAL;
-
/* Determine the address of the SMBus areas */
if (force_addr) {
vt596_smba = force_addr & 0xfff0;
--- linux-2.6.27-rc3.orig/drivers/pci/pci-driver.c 2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/pci/pci-driver.c 2008-08-17 19:17:55.000000000 +0200
@@ -43,18 +43,32 @@ store_new_id(struct device_driver *drive
{
struct pci_dynid *dynid;
struct pci_driver *pdrv = to_pci_driver(driver);
+ const struct pci_device_id *ids = pdrv->id_table;
__u32 vendor, device, subvendor=PCI_ANY_ID,
subdevice=PCI_ANY_ID, class=0, class_mask=0;
unsigned long driver_data=0;
int fields=0;
- int retval = 0;
+ int retval;
- fields = sscanf(buf, "%x %x %x %x %x %x %lux",
+ fields = sscanf(buf, "%x %x %x %x %x %x %lx",
&vendor, &device, &subvendor, &subdevice,
&class, &class_mask, &driver_data);
if (fields < 2)
return -EINVAL;
+ /* Only accept driver_data values that match an existing id_table
+ entry */
+ retval = -EINVAL;
+ while (ids->vendor || ids->subvendor || ids->class_mask) {
+ if (driver_data == ids->driver_data) {
+ retval = 0;
+ break;
+ }
+ ids++;
+ }
+ if (retval) /* No match */
+ return retval;
+
dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
if (!dynid)
return -ENOMEM;
* * * * *
The patch above applies on top of Milton's patch removing
dynids.use_driver_data.
Note that I fixed a bug in the code: the driver_data value was scanned
with format "%lux" which isn't valid. It's either "%lu" or "%lx". It
went unnoticed so far because it's the last field. I've made it "%lx"
because that's what our documentation says it should be. The old code
was behaving like "%lu" instead, so that's an interface change. But I
doubt it matters much, given that only 3 drivers were using this field
so far.
As mentioned before, this safety check might be too tight in some
cases (for example if driver_data is a bit field and the new device
needs a flag combination that no supported device uses.) It wouldn't be
difficult to let drivers set a flag saying they will care about the
safety check themselves. I can even implement it now if anybody thinks
that my code is too restrictive.
Thanks,
--
Jean Delvare
On Sun, Aug 17, 2008 at 09:06:59PM +0200, Jean Delvare wrote:
> Hi all,
>
> On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
> > On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> > > On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > > > In fact we can do even better than that. We could accept from
> > > > user-space only driver_data values which at least one device ID entry in
> > > > the driver already uses. That should be fairly easy to implement, and
> > > > would offer a level of safety an order of magnitude above what we have
> > > > at the moment... And it works both ways: if 0 is not a valid data for
> > > > some driver, that would force the user to provide a non-zero (and
> > > > valid) data value. And it guarantees that the user can't ask for
> > > > something the driver doesn't expect, so drivers don't even need extra
> > > > checks. And no need for a use_driver_data flag either.
> > >
> > > Meaning a driver audit of the usage? Yeah that would be great.
> > >
> > > > The only drawback is that it prevents the user from passing a "new"
> > > > data value even if it would be valid. But honestly, I don't expect that
> > > > case to happen frequently... if ever at all. So I'd say the benefits
> > > > totally outweight the drawback.
> > > >
> > > > If the interested people agree with the idea, I'll look into
> > > > implementing it.
> > >
> > > Well the audit would show if user supplied "new" values are needed; otherwise
> > > the approach sounds good to me.
> >
> > That sounds reasonable, and should work properly.
> >
> > No objection from me.
>
> Ok, here's what it could look like:
>
> * * * * *
>
> From: Jean Delvare <[email protected]>
> Subject: PCI: Check dynids driver_data value for validity
>
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Jesse Barnes <[email protected]>
> Cc: Milton Miller <[email protected]>
> Cc: Greg KH <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Looks good, thanks for sticking with it and creating this.
greg k-h
On Sunday, August 17, 2008 12:06 pm Jean Delvare wrote:
> Hi all,
> From: Jean Delvare <[email protected]>
> Subject: PCI: Check dynids driver_data value for validity
>
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Jesse Barnes <[email protected]>
> Cc: Milton Miller <[email protected]>
> Cc: Greg KH <[email protected]>
> ---
> Documentation/PCI/pci.txt | 4 ++++
> drivers/i2c/busses/i2c-amd756.c | 4 ----
> drivers/i2c/busses/i2c-viapro.c | 4 ----
> drivers/pci/pci-driver.c | 18 ++++++++++++++++--
> 4 files changed, 20 insertions(+), 10 deletions(-)
>
> --- linux-2.6.27-rc3.orig/Documentation/PCI/pci.txt 2008-08-17
> 18:24:33.000000000 +0200 +++
> linux-2.6.27-rc3/Documentation/PCI/pci.txt 2008-08-17 18:24:38.000000000
> +0200 @@ -163,6 +163,10 @@ need pass only as many optional fields a
> o class and classmask fields default to 0
> o driver_data defaults to 0UL.
>
> +Note that driver_data must match the value used by any of the
> pci_device_id +entries defined in the driver. This makes the driver_data
> field mandatory +if all the pci_device_id entries have a non-zero
> driver_data value. +
> Once added, the driver probe routine will be invoked for any unclaimed
> PCI devices listed in its (newly updated) pci_ids list.
>
> --- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-amd756.c 2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/i2c/busses/i2c-amd756.c 2008-08-17
> 19:42:14.000000000 +0200 @@ -332,10 +332,6 @@ static int __devinit
> amd756_probe(struct
> int error;
> u8 temp;
>
> - /* driver_data might come from user-space, so check it */
> - if (id->driver_data >= ARRAY_SIZE(chipname))
> - return -EINVAL;
> -
> if (amd756_ioport) {
> dev_err(&pdev->dev, "Only one device supported "
> "(you have a strange motherboard, btw)\n");
> --- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-viapro.c 2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/i2c/busses/i2c-viapro.c 2008-08-17
> 19:42:24.000000000 +0200 @@ -320,10 +320,6 @@ static int __devinit
> vt596_probe(struct
> unsigned char temp;
> int error = -ENODEV;
>
> - /* driver_data might come from user-space, so check it */
> - if (id->driver_data & 1 || id->driver_data > 0xff)
> - return -EINVAL;
> -
> /* Determine the address of the SMBus areas */
> if (force_addr) {
> vt596_smba = force_addr & 0xfff0;
> --- linux-2.6.27-rc3.orig/drivers/pci/pci-driver.c 2008-08-17
> 17:15:57.000000000 +0200 +++
> linux-2.6.27-rc3/drivers/pci/pci-driver.c 2008-08-17 19:17:55.000000000
> +0200 @@ -43,18 +43,32 @@ store_new_id(struct device_driver *drive
> {
> struct pci_dynid *dynid;
> struct pci_driver *pdrv = to_pci_driver(driver);
> + const struct pci_device_id *ids = pdrv->id_table;
> __u32 vendor, device, subvendor=PCI_ANY_ID,
> subdevice=PCI_ANY_ID, class=0, class_mask=0;
> unsigned long driver_data=0;
> int fields=0;
> - int retval = 0;
> + int retval;
>
> - fields = sscanf(buf, "%x %x %x %x %x %x %lux",
> + fields = sscanf(buf, "%x %x %x %x %x %x %lx",
> &vendor, &device, &subvendor, &subdevice,
> &class, &class_mask, &driver_data);
> if (fields < 2)
> return -EINVAL;
>
> + /* Only accept driver_data values that match an existing id_table
> + entry */
> + retval = -EINVAL;
> + while (ids->vendor || ids->subvendor || ids->class_mask) {
> + if (driver_data == ids->driver_data) {
> + retval = 0;
> + break;
> + }
> + ids++;
> + }
> + if (retval) /* No match */
> + return retval;
> +
> dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
> if (!dynid)
> return -ENOMEM;
>
>
> * * * * *
>
> The patch above applies on top of Milton's patch removing
> dynids.use_driver_data.
Looks good; I think we'll want to put this into linux-next along with Milton's
change. I'll push them out after a quick smoke test.
Thanks,
Jesse
On Sunday, August 17, 2008 12:06 pm Jean Delvare wrote:
> Subject: PCI: Check dynids driver_data value for validity
>
> Only accept dynids those driver_data value matches one of the driver's
> pci_driver_id entry. This prevents the user from accidentally passing
> values the drivers do not expect.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Jesse Barnes <[email protected]>
> Cc: Milton Miller <[email protected]>
> Cc: Greg KH <[email protected]>
Applied to linux-next, thanks Jean.
Jesse
On Aug 17, 2008, at 2:06 PM, Jean Delvare wrote:
> On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
>> On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
>>> On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
>>>> In fact we can do even better than that. We could accept from
>>>> user-space only driver_data values which at least one device ID
>>>> entry in
>>>> the driver already uses. That should be fairly easy to implement,
>>>> and
>>>> would offer a level of safety an order of magnitude above what we
>>>> have
>>>> at the moment... And it works both ways: if 0 is not a valid data
>>>> for
>>>> some driver, that would force the user to provide a non-zero (and
>>>> valid) data value. And it guarantees that the user can't ask for
>>>> something the driver doesn't expect, so drivers don't even need
>>>> extra
>>>> checks. And no need for a use_driver_data flag either.
>>>
>>> Meaning a driver audit of the usage? Yeah that would be great.
Thanks Jean for doing this. Sometimes things move quickly after a long
stall. I thought about proposing a similar patch and therefore have to
say Ack.
>>>> The only drawback is that it prevents the user from passing a "new"
>>>> data value even if it would be valid. But honestly, I don't expect
>>>> that
>>>> case to happen frequently... if ever at all. So I'd say the benefits
>>>> totally outweight the drawback.
There are a few drivers that could benefit, mainly ones that I
identified as using flags. For example, the radeon driver uses
different fields of the data to specify crt controller, video output
device, etc. I'm fine with deferring a flag for such drivers until
someone audits a driver and wants the support.
>>>>
>>>> If the interested people agree with the idea, I'll look into
>>>> implementing it.
>>>
>>> Well the audit would show if user supplied "new" values are needed;
>>> otherwise
>>> the approach sounds good to me.
>>
>> That sounds reasonable, and should work properly.
>>
>> No objection from me.
so, if anyone asks,
Concept-Acked-By: Milton Miller <[email protected]>
milton