Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

[Jesse Barnes]

> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/