Re: [PATCH 12/24] media/video: fix dangling pointers
On Sunday 21 March 2010 15:14:17 Mark Brown wrote: > On Sun, Mar 21, 2010 at 02:46:55PM +0100, Jean Delvare wrote: > > On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote: > > > > I feel I am missing something here. Why does clientdata have to be set to > > > NULL when we are tearing down the device anyway? > > > We're not tearing down the device, that's the point. We are only > > unbinding it from its driver. The device itself survives the operation. > > That's the subsystem point of view, not the driver point of view. As > far as the driver is concerned the device appears when probe() is called > and vanishes after remove() has completed, any management the subsystem > does in between is up to it. Right. And from the point of view of the driver I see no reason why I would have to zero the client data pointer when I know nobody will ever access it again. If there is a good reason, then that is (again, from the PoV of the driver) completely unexpected and it is guaranteed that driver writers will continue to make that mistake in the future. > > 1* It is good practice to have memory freed not too far from where it > > was allocated, otherwise there is always a risk of unmatched pairs. In > > this regard, it seems preferable to let each i2c driver kfree the > > device memory it kalloc'd, be it in probe() or remove(). > > I agree with this. There are also some use cases where the device data > is actually static (eg, a generic description of the device or a > reference to some other shared resource rather than per device allocated > data). Definitely. Freeing should be done in the i2c drivers. > > 2* References to allocated memory should be dropped before that memory > > is freed. This means that we want to call i2c_set_clientdata(c, NULL) > > before kfree(d). As a corollary, we can't do the former in i2c-core and > > the later in device drivers. > > This is where the mismatch between the subsystem view of the device > lifetime and the driver view of the device lifetime comes into play. > For the driver once the device is unregistered the device no longer > exists - if the driver tries to work with the device it's buggy. This > means that to the driver returning from the remove() function is > dropping the reference to the data and there's no reason for the driver > to take any other action. > > The device may hang around after the remove() has happened, but if the > device driver knows or cares about it then it's doing something wrong. > Similarly on probe() we can't assme anything about the pointer since > even if we saw the device before we can't guarantee that some other > driver didn't do so as well. The situation is similar to that with > kfree() - we don't memset() data we're freeing with that, even though it > might contain pointers to other things. Indeed. The client data is for the client. Once the client is gone (unbound) the client data can safely be set back to NULL. > > So we are in the difficult situation where we can't do both in i2c-core > > because that violates point 1 above, we can't do half in i2c-core and > > half in device drivers because this violates point 2 above, so we fall > > back to doing both in device drivers, which doesn't violate any point > > but duplicates the code all around. > > Personally I'd much rather just not bother setting the driver data in > the removal path, it seems unneeded. I had assumed that the subsystem > code cared for some reason when I saw the patch series. Anyway, should this really be necessary, then for the media drivers this should be done in v4l2_device_unregister_subdev() and not in every driver. But this just feels like an i2c core thing to me. After remove() is called the core should just set the client data to NULL. If there are drivers that rely on the current behavior, then those drivers should be reviewed first as to the reason why they need it. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
i2c probing
Hi,
Do you know why there is 2 methods of probing i2c [1] and [2], with different
quirks for eeprom.
Why can't they be merged together ?
Thanks
Matthieu
PS : please keep me in CC
[1]
i2c_detect_address
/* Make sure the address is valid */
if (addr < 0x03 || addr > 0x77) {
dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n",
addr);
return -EINVAL;
}
/* Skip if already in use */
if (i2c_check_addr(adapter, addr))
return 0;
/* Make sure there is something at this address */
if (i2c_smbus_xfer(adapter, addr, 0, 0, 0, I2C_SMBUS_QUICK, NULL) < 0)
return 0;
/* Prevent 24RF08 corruption */
if ((addr & ~0x0f) == 0x50)
i2c_smbus_xfer(adapter, addr, 0, 0, 0, I2C_SMBUS_QUICK, NULL);
[2]
i2c_new_probed_device
if (addr_list[i] < 0x03 || addr_list[i] > 0x77) {
dev_warn(&adap->dev, "Invalid 7-bit address "
"0x%02x\n", addr_list[i]);
continue;
}
/* Check address availability */
if (i2c_check_addr(adap, addr_list[i])) {
dev_dbg(&adap->dev, "Address 0x%02x already in "
"use, not probing\n", addr_list[i]);
continue;
}
/* Test address responsiveness
The default probe method is a quick write, but it is known
to corrupt the 24RF08 EEPROMs due to a state machine bug,
and could also irreversibly write-protect some EEPROMs, so
for address ranges 0x30-0x37 and 0x50-0x5f, we use a byte
read instead. Also, some bus drivers don't implement
quick write, so we fallback to a byte read it that case
too. */
if ((addr_list[i] & ~0x07) == 0x30
|| (addr_list[i] & ~0x0f) == 0x50
|| !i2c_check_functionality(adap, I2C_FUNC_SMBUS_QUICK)) {
union i2c_smbus_data data;
if (i2c_smbus_xfer(adap, addr_list[i], 0,
I2C_SMBUS_READ, 0,
I2C_SMBUS_BYTE, &data) >= 0)
break;
} else {
if (i2c_smbus_xfer(adap, addr_list[i], 0,
I2C_SMBUS_WRITE, 0,
I2C_SMBUS_QUICK, NULL) >= 0)
break;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/24] media/video: fix dangling pointers
On Sun, Mar 21, 2010 at 02:46:55PM +0100, Jean Delvare wrote: > On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote: > > I feel I am missing something here. Why does clientdata have to be set to > > NULL when we are tearing down the device anyway? > We're not tearing down the device, that's the point. We are only > unbinding it from its driver. The device itself survives the operation. That's the subsystem point of view, not the driver point of view. As far as the driver is concerned the device appears when probe() is called and vanishes after remove() has completed, any management the subsystem does in between is up to it. > 1* It is good practice to have memory freed not too far from where it > was allocated, otherwise there is always a risk of unmatched pairs. In > this regard, it seems preferable to let each i2c driver kfree the > device memory it kalloc'd, be it in probe() or remove(). I agree with this. There are also some use cases where the device data is actually static (eg, a generic description of the device or a reference to some other shared resource rather than per device allocated data). > 2* References to allocated memory should be dropped before that memory > is freed. This means that we want to call i2c_set_clientdata(c, NULL) > before kfree(d). As a corollary, we can't do the former in i2c-core and > the later in device drivers. This is where the mismatch between the subsystem view of the device lifetime and the driver view of the device lifetime comes into play. For the driver once the device is unregistered the device no longer exists - if the driver tries to work with the device it's buggy. This means that to the driver returning from the remove() function is dropping the reference to the data and there's no reason for the driver to take any other action. The device may hang around after the remove() has happened, but if the device driver knows or cares about it then it's doing something wrong. Similarly on probe() we can't assme anything about the pointer since even if we saw the device before we can't guarantee that some other driver didn't do so as well. The situation is similar to that with kfree() - we don't memset() data we're freeing with that, even though it might contain pointers to other things. > So we are in the difficult situation where we can't do both in i2c-core > because that violates point 1 above, we can't do half in i2c-core and > half in device drivers because this violates point 2 above, so we fall > back to doing both in device drivers, which doesn't violate any point > but duplicates the code all around. Personally I'd much rather just not bother setting the driver data in the removal path, it seems unneeded. I had assumed that the subsystem code cared for some reason when I saw the patch series. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
i2c interface bugs in dvb drivers
Hi,
some dvb driver that export a i2c interface contains some mistakes because they
mainly used by driver (frontend, tuner) wrote for them.
But the i2c interface is exposed to everybody.
One mistake is expect msg[i].addr with 8 bits address instead of 7 bits
address. This make them use eeprom address at 0xa0 instead of 0x50. Also they
shift tuner address (qt1010 tuner is likely to be at address 0x62, but some put
it a 0xc4 (af9015, af9005, dtv5100)).
Other mistakes is in xfer callback. Often the controller support a limited i2c
support (n bytes write then m bytes read). The driver try to convert the linux
i2c msg to this pattern, but they often miss cases :
- msg[i].len can be null
- msg write are not always followed by msg read
And this can be dangerous if these interfaces are exported to userspace via
i2c-dev :
- some scanning program avoid eeprom by filtering 0x5x range, but now it is at
0xax range (well that should happen because scan limit should be 0x77)
- some read only command can be interpreted as write command.
What should be done ?
Fix the drivers.
Have a mode where i2c interface are not exported to everybody.
Don't care.
First why does the i2c stack doesn't check that the address is on 7 bits (like
the attached patch) ?
Also I believe a program for testing i2c interface corner case should catch
most of these bugs :
- null msg[i].len
- different transactions on a device :
- one write/read transaction
- one write transaction then one read transaction
[...]
Does a such program exist ?
Matthieu
PS : please keep me in CC
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 3202a86..91e63ea 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1150,6 +1150,17 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
}
#endif
+ for (ret = 0; ret < num; ret++) {
+ if (msgs[ret].flags & I2C_M_TEN) {
+/* XXX what"s I2C_M_TEN range */
+if (msgs[ret].addr < 0x03 || msgs[ret].addr > 0x377)
+ return -EINVAL;
+ }
+ else {
+if (msgs[ret].addr < 0x03 || msgs[ret].addr > 0x77)
+ return -EINVAL;
+ }
+ }
if (in_atomic() || irqs_disabled()) {
ret = rt_mutex_trylock(&adap->bus_lock);
Re: [PATCH 12/24] media/video: fix dangling pointers
Hi Hans, On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote: > On Saturday 20 March 2010 15:12:53 Wolfram Sang wrote: > > Fix I2C-drivers which missed setting clientdata to NULL before freeing the > > structure it points to. Also fix drivers which do this _after_ the structure > > was freed already. > > I feel I am missing something here. Why does clientdata have to be set to > NULL when we are tearing down the device anyway? We're not tearing down the device, that's the point. We are only unbinding it from its driver. The device itself survives the operation. (This is different from the legacy i2c binding model where the device itself would indeed be removed at that point, and that would be the reason why so many i2c drivers have it wrong now.) > And if there is a good reason for doing this, then it should be done in > v4l2_device_unregister_subdev or even in the i2c core, not in each drivers. Mark Brown (Cc'd) suggested this already, but I have mixed feelings about this. My reasoning is: 1* It is good practice to have memory freed not too far from where it was allocated, otherwise there is always a risk of unmatched pairs. In this regard, it seems preferable to let each i2c driver kfree the device memory it kalloc'd, be it in probe() or remove(). 2* References to allocated memory should be dropped before that memory is freed. This means that we want to call i2c_set_clientdata(c, NULL) before kfree(d). As a corollary, we can't do the former in i2c-core and the later in device drivers. So we are in the difficult situation where we can't do both in i2c-core because that violates point 1 above, we can't do half in i2c-core and half in device drivers because this violates point 2 above, so we fall back to doing both in device drivers, which doesn't violate any point but duplicates the code all around. Now, if we decide to handle this at a deeper driver model layer (i2c-core instead of device drivers) then I'm not sure why we would stop there... Wouldn't it be the driver core's job to clear the reference and free the allocated memory? I get the feeling that this would be a job for managed resources as some drivers already do for I/O ports and IRQs. Managed resources don't care about symmetry of allocation and freeing, by design (so it can violate point 1 above.) Aha! Isn't it exactly what devm_kzalloc() is all about? At this point, I guess that each subsystem maintainer can decide to either apply Wolfram's patch, or switch the drivers to managed resources. Very nice that we don't have to make this a subsystem-wide decision, so every actor can do the changes if/when they see fit. > And why does coccinelle apparently find this in e.g. cs5345.c but not in > saa7115.c, which has exactly the same construct? For that matter, I think > almost all v4l i2c drivers do this. That I can't say, sorry. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/24] media/video: fix dangling pointers
Hello Hans, > > Fix I2C-drivers which missed setting clientdata to NULL before freeing the > > structure it points to. Also fix drivers which do this _after_ the structure > > was freed already. > > I feel I am missing something here. Why does clientdata have to be set to > NULL when we are tearing down the device anyway? > > And if there is a good reason for doing this, then it should be done in > v4l2_device_unregister_subdev or even in the i2c core, not in each drivers. Discussion to take this into the i2c-layer has already started. Regarding V4L, I noticed there is a v4l2_i2c_subdev_init() but no v4l2_i2c_subdev_exit(), so I grepped what drivers are doing. There are some which set clientdata to NULL, so I thought this was accepted in general. > And why does coccinelle apparently find this in e.g. cs5345.c but not in > saa7115.c, which has exactly the same construct? For that matter, I think It was the to_state()-call inside kfree() which prevented the match. I would need to extend my patch for V4L, it seems. Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak
On Sat, 2010-03-20 at 15:41 +, Russell King wrote: > > Thanks. > > Acked-by: Russell King > > David, are you going to pick this up? http://git.infradead.org/mtd-2.6.git/commitdiff/395b2288 Thanks. -- David WoodhouseOpen Source Technology Centre david.woodho...@intel.com Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
