| Message ID | 20180807140605.19156-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adxl345: move null check for i2c id at start of probe | expand |
On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). > > This was found via static checker. > After looking into the code a bit, it's unlikely that there will be a NULL > dereference if the `id` object in that specific code path. > However, it's safe to add a NULL (paranoid) check just to make sure and > remove any uncertainties. I would like to know when would that case happen actually ? Because probe will only be called only when a match occurs either through DT or id matching. Isn't it ?
On Sat, 11 Aug 2018 15:48:33 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: > > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). > > > > This was found via static checker. > > After looking into the code a bit, it's unlikely that there will be a NULL > > dereference if the `id` object in that specific code path. > > However, it's safe to add a NULL (paranoid) check just to make sure and > > remove any uncertainties. > > I would like to know when would that case happen actually ? > > Because probe will only be called only when a match occurs either > through DT or id matching. Isn't it ? > Yes. Alternative would have just not been to check it, but this is fine so applied. I'm not going to rush this through stable though given I don't think it can actually happen. Jonathan
On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote: > On Sat, 11 Aug 2018 15:48:33 +0530 > Himanshu Jha <himanshujha199640@gmail.com> wrote: > > > On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: > > > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). > > > > > > This was found via static checker. > > > After looking into the code a bit, it's unlikely that there will be a NULL > > > dereference if the `id` object in that specific code path. > > > However, it's safe to add a NULL (paranoid) check just to make sure and > > > remove any uncertainties. > > > > I would like to know when would that case happen actually ? > > > > Because probe will only be called only when a match occurs either > > through DT or id matching. Isn't it ? > > > Yes. Alternative would have just not been to check it, but this is fine > so applied. I'm not going to rush this through stable though given > I don't think it can actually happen. Thanks for the confirmation. So, I have another doubt and it seems to be right time to ask. BME680 currently supports both ACPI matching and traditional ID matching. So, it there any priority list to which patch the driver would choose to match the device. ACPI > ID matching ? (In my case this happens) Because this matching tends to decide the `name` attribute of loaded driver. For ACPI: BME0680 (not sure maybe it was I2C0:BME0680) For ID: bme680
On 08/19/2018 07:43 PM, Himanshu Jha wrote: > On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote: >> On Sat, 11 Aug 2018 15:48:33 +0530 >> Himanshu Jha <himanshujha199640@gmail.com> wrote: >> >>> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: >>>> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). >>>> >>>> This was found via static checker. >>>> After looking into the code a bit, it's unlikely that there will be a NULL >>>> dereference if the `id` object in that specific code path. >>>> However, it's safe to add a NULL (paranoid) check just to make sure and >>>> remove any uncertainties. >>> >>> I would like to know when would that case happen actually ? >>> >>> Because probe will only be called only when a match occurs either >>> through DT or id matching. Isn't it ? >>> >> Yes. Alternative would have just not been to check it, but this is fine >> so applied. I'm not going to rush this through stable though given >> I don't think it can actually happen. > > Thanks for the confirmation. > > So, I have another doubt and it seems to be right time to ask. > > BME680 currently supports both ACPI matching and traditional ID > matching. So, it there any priority list to which patch the driver > would choose to match the device. > > ACPI > ID matching ? (In my case this happens) > > Because this matching tends to decide the `name` attribute of loaded > driver. > > For ACPI: BME0680 (not sure maybe it was I2C0:BME0680) > For ID: bme680 Yeah, that's wrong. But pretty much all ACPI drivers have that issue. Maybe we should just deprecate the name attribute.
On Sun, Aug 19, 2018 at 07:59:38PM +0200, Lars-Peter Clausen wrote: > On 08/19/2018 07:43 PM, Himanshu Jha wrote: > > On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote: > >> On Sat, 11 Aug 2018 15:48:33 +0530 > >> Himanshu Jha <himanshujha199640@gmail.com> wrote: > >> > >>> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: > >>>> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). > >>>> > >>>> This was found via static checker. > >>>> After looking into the code a bit, it's unlikely that there will be a NULL > >>>> dereference if the `id` object in that specific code path. > >>>> However, it's safe to add a NULL (paranoid) check just to make sure and > >>>> remove any uncertainties. > >>> > >>> I would like to know when would that case happen actually ? > >>> > >>> Because probe will only be called only when a match occurs either > >>> through DT or id matching. Isn't it ? > >>> > >> Yes. Alternative would have just not been to check it, but this is fine > >> so applied. I'm not going to rush this through stable though given > >> I don't think it can actually happen. > > > > Thanks for the confirmation. > > > > So, I have another doubt and it seems to be right time to ask. > > > > BME680 currently supports both ACPI matching and traditional ID > > matching. So, it there any priority list to which patch the driver > > would choose to match the device. > > > > ACPI > ID matching ? (In my case this happens) > > > > Because this matching tends to decide the `name` attribute of loaded > > driver. > > > > For ACPI: BME0680 (not sure maybe it was I2C0:BME0680) > > For ID: bme680 > > Yeah, that's wrong. But pretty much all ACPI drivers have that issue. > Maybe we should just deprecate the name attribute. libiio is the most affected due to this issue as I can figure out. Particularly, the iio_device_get_name() api: https://analogdevicesinc.github.io/libiio/group__Context.html#gae5807303b638869679ece67270e72e77
On Mon, 20 Aug 2018 00:24:58 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Sun, Aug 19, 2018 at 07:59:38PM +0200, Lars-Peter Clausen wrote: > > On 08/19/2018 07:43 PM, Himanshu Jha wrote: > > > On Sun, Aug 19, 2018 at 06:31:32PM +0100, Jonathan Cameron wrote: > > >> On Sat, 11 Aug 2018 15:48:33 +0530 > > >> Himanshu Jha <himanshujha199640@gmail.com> wrote: > > >> > > >>> On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: > > >>>> Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). > > >>>> > > >>>> This was found via static checker. > > >>>> After looking into the code a bit, it's unlikely that there will be a NULL > > >>>> dereference if the `id` object in that specific code path. > > >>>> However, it's safe to add a NULL (paranoid) check just to make sure and > > >>>> remove any uncertainties. > > >>> > > >>> I would like to know when would that case happen actually ? > > >>> > > >>> Because probe will only be called only when a match occurs either > > >>> through DT or id matching. Isn't it ? > > >>> > > >> Yes. Alternative would have just not been to check it, but this is fine > > >> so applied. I'm not going to rush this through stable though given > > >> I don't think it can actually happen. > > > > > > Thanks for the confirmation. > > > > > > So, I have another doubt and it seems to be right time to ask. > > > > > > BME680 currently supports both ACPI matching and traditional ID > > > matching. So, it there any priority list to which patch the driver > > > would choose to match the device. > > > > > > ACPI > ID matching ? (In my case this happens) > > > > > > Because this matching tends to decide the `name` attribute of loaded > > > driver. > > > > > > For ACPI: BME0680 (not sure maybe it was I2C0:BME0680) > > > For ID: bme680 > > > > Yeah, that's wrong. But pretty much all ACPI drivers have that issue. > > Maybe we should just deprecate the name attribute. > > libiio is the most affected due to this issue as I can figure out. > Particularly, the iio_device_get_name() api: > https://analogdevicesinc.github.io/libiio/group__Context.html#gae5807303b638869679ece67270e72e77 > Yeah, the name thing was one of those ones that got away from us a long time ago and became ABI in far too many drivers. The 'intent' was that it would just provide a convenient "what's this part?" sysfs attribute, so should always return the part number rather than anything to do with any of the bindings. Unfortunately it often doesn't. We could add a new attribute called something like 'part_number' as then it should be more obvious what the intent is an hopefully it'll get used right. Jonathan
On Sat, Aug 11, 2018 at 03:48:33PM +0530, Himanshu Jha wrote: > On Tue, Aug 07, 2018 at 05:06:05PM +0300, Alexandru Ardelean wrote: > > Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). > > > > This was found via static checker. > > After looking into the code a bit, it's unlikely that there will be a NULL > > dereference if the `id` object in that specific code path. > > However, it's safe to add a NULL (paranoid) check just to make sure and > > remove any uncertainties. > > I would like to know when would that case happen actually ? > > Because probe will only be called only when a match occurs either > through DT or id matching. Isn't it ? > Btw, normally Smatch doesn't warn about inconsistent NULL checking if it's clear that the NULL check is not required. In this case, the adxl345 is not in my allmodconfig so I didn't build the cross function DB for it so I didn't have the caller information. And also: drivers/i2c/i2c-core-base.c 391 /* 392 * When there are no more users of probe(), 393 * rename probe_new to probe. 394 */ 395 if (driver->probe_new) 396 status = driver->probe_new(client); 397 else if (driver->probe) 398 status = driver->probe(client, 399 i2c_match_id(driver->id_table, client)); 400 else 401 status = -EINVAL; 402 Smatch isn't smart enough to know that i2c_match_id() doesn't return a NULL. I did look at the code before sending the bug report but it wasn't immediately clear to me either whether it was possible or not. regards, dan carpenter
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c index 785c89de91e7..f22f71315a0c 100644 --- a/drivers/iio/accel/adxl345_i2c.c +++ b/drivers/iio/accel/adxl345_i2c.c @@ -27,6 +27,9 @@ static int adxl345_i2c_probe(struct i2c_client *client, { struct regmap *regmap; + if (!id) + return -ENODEV; + regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config); if (IS_ERR(regmap)) { dev_err(&client->dev, "Error initializing i2c regmap: %ld\n", @@ -35,7 +38,7 @@ static int adxl345_i2c_probe(struct i2c_client *client, } return adxl345_core_probe(&client->dev, regmap, id->driver_data, - id ? id->name : NULL); + id->name); } static int adxl345_i2c_remove(struct i2c_client *client)
Fixes ef89f4b96a2 ("iio: adxl345: Add support for the ADXL375"). This was found via static checker. After looking into the code a bit, it's unlikely that there will be a NULL dereference if the `id` object in that specific code path. However, it's safe to add a NULL (paranoid) check just to make sure and remove any uncertainties. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/accel/adxl345_i2c.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)