Message-ID: <20250522181236.00006dda@huawei.com>
Date: Thu, 22 May 2025 18:12:36 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Maud Spierings via B4 Relay
	<devnull+maudspierings.gocontroll.com@...nel.org>
CC: <maudspierings@...ontroll.com>, Jonathan Cameron <jic23@...nel.org>,
	"David Lechner" <dlechner@...libre.com>, Nuno Sá
	<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Christian Heusel
	<christian@...sel.eu>, Linus Walleij <linus.walleij@...aro.org>,
	<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<stable@...r.kernel.org>, <gregkh@...uxfoundation.org>, <dsimic@...jaro.com>
Subject: Re: [PATCH v2] iio: common: st_sensors: Fix use of uninitialize
 device structs

On Thu, 22 May 2025 13:18:55 +0200
Maud Spierings via B4 Relay <devnull+maudspierings.gocontroll.com@...nel.org> wrote:

> From: Maud Spierings <maudspierings@...ontroll.com>
> 
> Throughout the various probe functions &indio_dev->dev is used before it
> is initialized. This caused a kernel panic in st_sensors_power_enable
> when the call to devm_regulator_bulk_get_enable() fails and then calls
> dev_err_probe() with the uninitialized device.
Hi Maud,


Curious.  Given the device_initialize() is in the allocation function
it isn't immediately obvious that something needed might not have been initialized.
Any idea what is being accessed in there that fails? (i.e. any idea if my
shallow detective work found it ;)

https://elixir.bootlin.com/linux/v6.14.7/source/drivers/iio/industrialio-core.c#L1675
in iio_device_alloc() is where device_initialize() is called.

Using the device in the iio_dev structure is almost certainly wrong
but I'm surprised it crashes... So I had a quick dig.

The only path that isn't a simple print that I can spot
is device_set_deferred_reason()  That accesses dev->p (device private
data)  Which is initialized only in device_add().

I wonder if we should harden device_set_deferred_reason()
against p == NULL, perhaps with a message strongly advising against
using it with a device that hasn't been added?

Being in error paths this is the sort of subtle bug that
rarely rears it's head :(

+CC Greg and Dragan for thoughts before anyone spins a patch.

This change is good either way. I'm just on wrong computer to pick it up
right now.

Jonathan




> 
> This seems to only cause a panic with dev_err_probe(), dev_err,
> dev_warn and dev_info don't seem to cause a panic, but are fixed
> as well.
> 
> Signed-off-by: Maud Spierings <maudspierings@...ontroll.com>
> ---
> When I search for general &indio_dev->dev usage, I see quite a lot more
> hits, but I am not sure if there are issues with those too.
> 
> This issue has existed for a long time it seems and therefore it is
> nearly impossible to find a proper fixes tag. I would love to see it at
> least backported to 6.12 as that is where I encountered it, and I
> believe the patch should apply without conflicts.
> 
> The investigation into this issue can be found in this thread [1]
> 
> [1]: https://lore.kernel.org/all/AM7P189MB100986A83D2F28AF3FFAF976E39EA@AM7P189MB1009.EURP189.PROD.OUTLOOK.COM/
> ---
> Changes in v2:
> - Added SoB in commit message
> - Link to v1: https://lore.kernel.org/r/20250522-st_iio_fix-v1-1-d689b35f1612@gocontroll.com
> ---
>  drivers/iio/accel/st_accel_core.c                  | 10 +++----
>  drivers/iio/common/st_sensors/st_sensors_core.c    | 35 +++++++++++-----------
>  drivers/iio/common/st_sensors/st_sensors_trigger.c | 18 +++++------
>  3 files changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index 99cb661fabb2d9cc1943fa8d0a6f3becb71126e6..a7961c610ed203d039bbf298c8883031a578fb0b 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -1353,6 +1353,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev)
>  	union acpi_object *ont;
>  	union acpi_object *elements;
>  	acpi_status status;
> +	struct device *parent = indio_dev->dev.parent;
>  	int ret = -EINVAL;
>  	unsigned int val;
>  	int i, j;
> @@ -1371,7 +1372,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev)
>  	};
>  
>  
> -	adev = ACPI_COMPANION(indio_dev->dev.parent);
> +	adev = ACPI_COMPANION(parent);
>  	if (!adev)
>  		return -ENXIO;
>  
> @@ -1380,8 +1381,7 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev)
>  	if (status == AE_NOT_FOUND) {
>  		return -ENXIO;
>  	} else if (ACPI_FAILURE(status)) {
> -		dev_warn(&indio_dev->dev, "failed to execute _ONT: %d\n",
> -			 status);
> +		dev_warn(parent, "failed to execute _ONT: %d\n", status);
>  		return status;
>  	}
>  
> @@ -1457,12 +1457,12 @@ static int apply_acpi_orientation(struct iio_dev *indio_dev)
>  	}
>  
>  	ret = 0;
> -	dev_info(&indio_dev->dev, "computed mount matrix from ACPI\n");
> +	dev_info(parent, "computed mount matrix from ACPI\n");
>  
>  out:
>  	kfree(buffer.pointer);
>  	if (ret)
> -		dev_dbg(&indio_dev->dev,
> +		dev_dbg(parent,
>  			"failed to apply ACPI orientation data: %d\n", ret);
>  
>  	return ret;
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 8ce1dccfea4f5aaff45d3d40f6542323dd1f0b09..11cbf561b16d41f429745abb516c137cfbb302bb 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -154,7 +154,7 @@ static int st_sensors_set_fullscale(struct iio_dev *indio_dev, unsigned int fs)
>  	return err;
>  
>  st_accel_set_fullscale_error:
> -	dev_err(&indio_dev->dev, "failed to set new fullscale.\n");
> +	dev_err(indio_dev->dev.parent, "failed to set new fullscale.\n");
>  	return err;
>  }
>  
> @@ -231,7 +231,7 @@ int st_sensors_power_enable(struct iio_dev *indio_dev)
>  					     ARRAY_SIZE(regulator_names),
>  					     regulator_names);
>  	if (err)
> -		return dev_err_probe(&indio_dev->dev, err,
> +		return dev_err_probe(parent, err,
>  				     "unable to enable supplies\n");
>  
>  	return 0;
> @@ -241,13 +241,14 @@ EXPORT_SYMBOL_NS(st_sensors_power_enable, "IIO_ST_SENSORS");
>  static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
>  					struct st_sensors_platform_data *pdata)
>  {
> +	struct device *parent = indio_dev->dev.parent;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  
>  	/* Sensor does not support interrupts */
>  	if (!sdata->sensor_settings->drdy_irq.int1.addr &&
>  	    !sdata->sensor_settings->drdy_irq.int2.addr) {
>  		if (pdata->drdy_int_pin)
> -			dev_info(&indio_dev->dev,
> +			dev_info(parent,
>  				 "DRDY on pin INT%d specified, but sensor does not support interrupts\n",
>  				 pdata->drdy_int_pin);
>  		return 0;
> @@ -256,29 +257,27 @@ static int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
>  	switch (pdata->drdy_int_pin) {
>  	case 1:
>  		if (!sdata->sensor_settings->drdy_irq.int1.mask) {
> -			dev_err(&indio_dev->dev,
> -					"DRDY on INT1 not available.\n");
> +			dev_err(parent, "DRDY on INT1 not available.\n");
>  			return -EINVAL;
>  		}
>  		sdata->drdy_int_pin = 1;
>  		break;
>  	case 2:
>  		if (!sdata->sensor_settings->drdy_irq.int2.mask) {
> -			dev_err(&indio_dev->dev,
> -					"DRDY on INT2 not available.\n");
> +			dev_err(parent, "DRDY on INT2 not available.\n");
>  			return -EINVAL;
>  		}
>  		sdata->drdy_int_pin = 2;
>  		break;
>  	default:
> -		dev_err(&indio_dev->dev, "DRDY on pdata not valid.\n");
> +		dev_err(parent, "DRDY on pdata not valid.\n");
>  		return -EINVAL;
>  	}
>  
>  	if (pdata->open_drain) {
>  		if (!sdata->sensor_settings->drdy_irq.int1.addr_od &&
>  		    !sdata->sensor_settings->drdy_irq.int2.addr_od)
> -			dev_err(&indio_dev->dev,
> +			dev_err(parent,
>  				"open drain requested but unsupported.\n");
>  		else
>  			sdata->int_pin_open_drain = true;
> @@ -336,6 +335,7 @@ EXPORT_SYMBOL_NS(st_sensors_dev_name_probe, "IIO_ST_SENSORS");
>  int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  					struct st_sensors_platform_data *pdata)
>  {
> +	struct device *parent = indio_dev->dev.parent;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  	struct st_sensors_platform_data *of_pdata;
>  	int err = 0;
> @@ -343,7 +343,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  	mutex_init(&sdata->odr_lock);
>  
>  	/* If OF/DT pdata exists, it will take precedence of anything else */
> -	of_pdata = st_sensors_dev_probe(indio_dev->dev.parent, pdata);
> +	of_pdata = st_sensors_dev_probe(parent, pdata);
>  	if (IS_ERR(of_pdata))
>  		return PTR_ERR(of_pdata);
>  	if (of_pdata)
> @@ -370,7 +370,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  		if (err < 0)
>  			return err;
>  	} else
> -		dev_info(&indio_dev->dev, "Full-scale not possible\n");
> +		dev_info(parent, "Full-scale not possible\n");
>  
>  	err = st_sensors_set_odr(indio_dev, sdata->odr);
>  	if (err < 0)
> @@ -405,7 +405,7 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  			mask = sdata->sensor_settings->drdy_irq.int2.mask_od;
>  		}
>  
> -		dev_info(&indio_dev->dev,
> +		dev_info(parent,
>  			 "set interrupt line to open drain mode on pin %d\n",
>  			 sdata->drdy_int_pin);
>  		err = st_sensors_write_data_with_mask(indio_dev, addr,
> @@ -593,21 +593,20 @@ EXPORT_SYMBOL_NS(st_sensors_get_settings_index, "IIO_ST_SENSORS");
>  int st_sensors_verify_id(struct iio_dev *indio_dev)
>  {
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
> +	struct device *parent = indio_dev->dev.parent;
>  	int wai, err;
>  
>  	if (sdata->sensor_settings->wai_addr) {
>  		err = regmap_read(sdata->regmap,
>  				  sdata->sensor_settings->wai_addr, &wai);
>  		if (err < 0) {
> -			dev_err(&indio_dev->dev,
> -				"failed to read Who-Am-I register.\n");
> -			return err;
> +			return dev_err_probe(parent, err,
> +					     "failed to read Who-Am-I register.\n");
>  		}
>  
>  		if (sdata->sensor_settings->wai != wai) {
> -			dev_warn(&indio_dev->dev,
> -				"%s: WhoAmI mismatch (0x%x).\n",
> -				indio_dev->name, wai);
> +			dev_warn(parent, "%s: WhoAmI mismatch (0x%x).\n",
> +				 indio_dev->name, wai);
>  		}
>  	}
>  
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 9d4bf822a15dfcdd6c2835f6b9d7698cd3cb0b08..32c3278968089699dff5329e943d92b151b55fdf 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -127,7 +127,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  	sdata->trig = devm_iio_trigger_alloc(parent, "%s-trigger",
>  					     indio_dev->name);
>  	if (sdata->trig == NULL) {
> -		dev_err(&indio_dev->dev, "failed to allocate iio trigger.\n");
> +		dev_err(parent, "failed to allocate iio trigger.\n");
>  		return -ENOMEM;
>  	}
>  
> @@ -143,7 +143,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  	case IRQF_TRIGGER_FALLING:
>  	case IRQF_TRIGGER_LOW:
>  		if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
> -			dev_err(&indio_dev->dev,
> +			dev_err(parent,
>  				"falling/low specified for IRQ but hardware supports only rising/high: will request rising/high\n");
>  			if (irq_trig == IRQF_TRIGGER_FALLING)
>  				irq_trig = IRQF_TRIGGER_RISING;
> @@ -156,21 +156,21 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  				sdata->sensor_settings->drdy_irq.mask_ihl, 1);
>  			if (err < 0)
>  				return err;
> -			dev_info(&indio_dev->dev,
> +			dev_info(parent,
>  				 "interrupts on the falling edge or active low level\n");
>  		}
>  		break;
>  	case IRQF_TRIGGER_RISING:
> -		dev_info(&indio_dev->dev,
> +		dev_info(parent,
>  			 "interrupts on the rising edge\n");
>  		break;
>  	case IRQF_TRIGGER_HIGH:
> -		dev_info(&indio_dev->dev,
> +		dev_info(parent,
>  			 "interrupts active high level\n");
>  		break;
>  	default:
>  		/* This is the most preferred mode, if possible */
> -		dev_err(&indio_dev->dev,
> +		dev_err(parent,
>  			"unsupported IRQ trigger specified (%lx), enforce rising edge\n", irq_trig);
>  		irq_trig = IRQF_TRIGGER_RISING;
>  	}
> @@ -179,7 +179,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  	if (irq_trig == IRQF_TRIGGER_FALLING ||
>  	    irq_trig == IRQF_TRIGGER_RISING) {
>  		if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr) {
> -			dev_err(&indio_dev->dev,
> +			dev_err(parent,
>  				"edge IRQ not supported w/o stat register.\n");
>  			return -EOPNOTSUPP;
>  		}
> @@ -214,13 +214,13 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  					sdata->trig->name,
>  					sdata->trig);
>  	if (err) {
> -		dev_err(&indio_dev->dev, "failed to request trigger IRQ.\n");
> +		dev_err(parent, "failed to request trigger IRQ.\n");
>  		return err;
>  	}
>  
>  	err = devm_iio_trigger_register(parent, sdata->trig);
>  	if (err < 0) {
> -		dev_err(&indio_dev->dev, "failed to register iio trigger.\n");
> +		dev_err(parent, "failed to register iio trigger.\n");
>  		return err;
>  	}
>  	indio_dev->trig = iio_trigger_get(sdata->trig);
> 
> ---
> base-commit: 7bac2c97af4078d7a627500c9bcdd5b033f97718
> change-id: 20250522-st_iio_fix-1c58fdd4d420
> 
> Best regards,

Powered by blists - more mailing lists