| Message ID | 20250303132124.52811-2-s.seva@4sigma.it (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: st_lsm6dsx: fix possible lockup during FIFO read | expand |
> Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions > from falling in an infinite loop in case pattern_len is equal to zero and > the device FIFO is not empty. > > Signed-off-by: Silvano Seva <s.seva@4sigma.it> > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > index 0a7cd8c1aa33..7f343614f8a5 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > @@ -395,12 +395,17 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) * > ST_LSM6DSX_CHAN_SIZE; > fifo_len = (fifo_len / pattern_len) * pattern_len; > + if (!fifo_len) > + return 0; I do not think this check is necessary since if fifo_len is 0 we will not run the for loop, right? > > acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]); > gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]); > if (hw->iio_devs[ST_LSM6DSX_ID_EXT0]) > ext_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_EXT0]); > > + if (!pattern_len) > + pattern_len = ST_LSM6DSX_SAMPLE_SIZE; same here, I do not think pattern_len can be 0 since hw->sip must be greater than 0 in order to enable the FIFO. Moreover, this check should be some lines above since we have already divided fifo_len by pattern_len here. > + > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > err = st_lsm6dsx_read_block(hw, ST_LSM6DSX_REG_FIFO_OUTL_ADDR, > hw->buff, pattern_len, > @@ -623,6 +628,9 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw) > if (!fifo_len) > return 0; > > + if (!pattern_len) > + pattern_len = ST_LSM6DSX_TAGGED_SAMPLE_SIZE; for the reason above, this is not necessary. Regards, Lorenzo > + > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > err = st_lsm6dsx_read_block(hw, > ST_LSM6DSX_REG_FIFO_OUT_TAG_ADDR, > -- > 2.48.1 >
On Thu, Mar 6, 2025 at 4:48 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions > > from falling in an infinite loop in case pattern_len is equal to zero and > > the device FIFO is not empty. > > > > Signed-off-by: Silvano Seva <s.seva@4sigma.it> > > --- > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > index 0a7cd8c1aa33..7f343614f8a5 100644 > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > @@ -395,12 +395,17 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) * > > ST_LSM6DSX_CHAN_SIZE; > > fifo_len = (fifo_len / pattern_len) * pattern_len; > > + if (!fifo_len) > > + return 0; > > I do not think this check is necessary since if fifo_len is 0 we will not run > the for loop, right? This check is present in the st_lsm6dsx_read_tagged_fifo() function, i added it here for consistency. I agree with you that is not strictly necessary. > > > > > acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]); > > gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]); > > if (hw->iio_devs[ST_LSM6DSX_ID_EXT0]) > > ext_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_EXT0]); > > > > + if (!pattern_len) > > + pattern_len = ST_LSM6DSX_SAMPLE_SIZE; > > same here, I do not think pattern_len can be 0 since hw->sip must be greater > than 0 in order to enable the FIFO. Moreover, this check should be some lines > above since we have already divided fifo_len by pattern_len here. > There is a situation causing the subsequent for loop to never terminate, hanging the kernel boot process: given a system which doesn't have an hardware reset line allowing the kernel to re-initialize the IMU hardware, in case of an hot reboot the driver probe() function attempts to flush the FIFO, which may contain some data, while the hw->sip is zero. The complete execution path is the following: - call of st_lsm6dsx_probe(); - allocation of the st_lsm6dsx_hw structure via the devm_kzalloc, zero-initializing the sip field; - call of st_lsm6dsx_init_device(); - call of st_lsm6dsx_reset_device(); - call of st_lsm6dsx_flush_fifo(); - call of st_lsm6dsx_read_fifo/st_lsm6dsx_read_tagged_fifo via the fifo_ops function pointer. An alternative solution to solve this situation is initializing the hw->sip field to a sane default value in either the probe() or init_device() function. > > + > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > err = st_lsm6dsx_read_block(hw, ST_LSM6DSX_REG_FIFO_OUTL_ADDR, > > hw->buff, pattern_len, > > @@ -623,6 +628,9 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw) > > if (!fifo_len) > > return 0; > > > > + if (!pattern_len) > > + pattern_len = ST_LSM6DSX_TAGGED_SAMPLE_SIZE; > > for the reason above, this is not necessary. > > Regards, > Lorenzo > > > + > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > err = st_lsm6dsx_read_block(hw, > > ST_LSM6DSX_REG_FIFO_OUT_TAG_ADDR, > > -- > > 2.48.1 > >
> On Thu, Mar 6, 2025 at 4:48 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions > > > from falling in an infinite loop in case pattern_len is equal to zero and > > > the device FIFO is not empty. > > > > > > Signed-off-by: Silvano Seva <s.seva@4sigma.it> > > > --- > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > index 0a7cd8c1aa33..7f343614f8a5 100644 > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > @@ -395,12 +395,17 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) * > > > ST_LSM6DSX_CHAN_SIZE; > > > fifo_len = (fifo_len / pattern_len) * pattern_len; > > > + if (!fifo_len) > > > + return 0; > > > > I do not think this check is necessary since if fifo_len is 0 we will not run > > the for loop, right? > > This check is present in the st_lsm6dsx_read_tagged_fifo() function, i > added it here for consistency. I agree with you that is not strictly > necessary. > > > > > > > > > acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]); > > > gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]); > > > if (hw->iio_devs[ST_LSM6DSX_ID_EXT0]) > > > ext_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_EXT0]); > > > > > > + if (!pattern_len) > > > + pattern_len = ST_LSM6DSX_SAMPLE_SIZE; > > > > same here, I do not think pattern_len can be 0 since hw->sip must be greater > > than 0 in order to enable the FIFO. Moreover, this check should be some lines > > above since we have already divided fifo_len by pattern_len here. > > > > There is a situation causing the subsequent for loop to never > terminate, hanging the kernel boot process: given a system which > doesn't have an hardware reset line allowing the kernel to > re-initialize the IMU hardware, in case of an hot reboot the driver > probe() function attempts to flush the FIFO, which may contain some > data, while the hw->sip is zero. > The complete execution path is the following: > - call of st_lsm6dsx_probe(); > - allocation of the st_lsm6dsx_hw structure via the devm_kzalloc, > zero-initializing the sip field; > - call of st_lsm6dsx_init_device(); > - call of st_lsm6dsx_reset_device(); > - call of st_lsm6dsx_flush_fifo(); > - call of st_lsm6dsx_read_fifo/st_lsm6dsx_read_tagged_fifo via the > fifo_ops function pointer. ack, I can see the issue now, thx for pointing this out. I should we should set pattern_len to ST_LSM6DSX_SAMPLE_SIZE or ST_LSM6DSX_TAGGED_SAMPLE_SIZE if it is 0. Can you please move the check before updating fifo_len in st_lsm6dsx_read_fifo()? Regards, Lorenzo > > An alternative solution to solve this situation is initializing the > hw->sip field to a sane default value in either the probe() or > init_device() function. > > > > + > > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > > err = st_lsm6dsx_read_block(hw, ST_LSM6DSX_REG_FIFO_OUTL_ADDR, > > > hw->buff, pattern_len, > > > @@ -623,6 +628,9 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw) > > > if (!fifo_len) > > > return 0; > > > > > > + if (!pattern_len) > > > + pattern_len = ST_LSM6DSX_TAGGED_SAMPLE_SIZE; > > > > for the reason above, this is not necessary. > > > > Regards, > > Lorenzo > > > > > + > > > for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { > > > err = st_lsm6dsx_read_block(hw, > > > ST_LSM6DSX_REG_FIFO_OUT_TAG_ADDR, > > > -- > > > 2.48.1 > > >
Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions
from falling in an infinite loop in case pattern_len is equal to zero and
the device FIFO is not empty.
Signed-off-by: Silvano Seva <s.seva@4sigma.it>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 0a7cd8c1aa33..39a7e15ae9ae 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -392,9 +392,14 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
if (fifo_status & cpu_to_le16(ST_LSM6DSX_FIFO_EMPTY_MASK))
return 0;
+ if (!pattern_len)
+ pattern_len = ST_LSM6DSX_SAMPLE_SIZE;
+
fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) *
ST_LSM6DSX_CHAN_SIZE;
fifo_len = (fifo_len / pattern_len) * pattern_len;
+ if (!fifo_len)
+ return 0;
acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]);
@@ -623,6 +628,9 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw)
if (!fifo_len)
return 0;
+ if (!pattern_len)
+ pattern_len = ST_LSM6DSX_TAGGED_SAMPLE_SIZE;
+
for (read_len = 0; read_len < fifo_len; read_len += pattern_len) {
err = st_lsm6dsx_read_block(hw,
ST_LSM6DSX_REG_FIFO_OUT_TAG_ADDR,
On Fri, Mar 7, 2025 at 3:40 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > On Thu, Mar 6, 2025 at 4:48 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > > > Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions > > > > from falling in an infinite loop in case pattern_len is equal to zero and > > > > the device FIFO is not empty. > > > > > > > > Signed-off-by: Silvano Seva <s.seva@4sigma.it> > > > > --- > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > index 0a7cd8c1aa33..7f343614f8a5 100644 > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c > > > > @@ -395,12 +395,17 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) > > > > fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) * > > > > ST_LSM6DSX_CHAN_SIZE; > > > > fifo_len = (fifo_len / pattern_len) * pattern_len; > > > > + if (!fifo_len) > > > > + return 0; > > > > > > I do not think this check is necessary since if fifo_len is 0 we will not run > > > the for loop, right? > > > > This check is present in the st_lsm6dsx_read_tagged_fifo() function, i > > added it here for consistency. I agree with you that is not strictly > > necessary. > > > > > > > > > > > > > acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]); > > > > gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]); > > > > if (hw->iio_devs[ST_LSM6DSX_ID_EXT0]) > > > > ext_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_EXT0]); > > > > > > > > + if (!pattern_len) > > > > + pattern_len = ST_LSM6DSX_SAMPLE_SIZE; > > > > > > same here, I do not think pattern_len can be 0 since hw->sip must be greater > > > than 0 in order to enable the FIFO. Moreover, this check should be some lines > > > above since we have already divided fifo_len by pattern_len here. > > > > > > > There is a situation causing the subsequent for loop to never > > terminate, hanging the kernel boot process: given a system which > > doesn't have an hardware reset line allowing the kernel to > > re-initialize the IMU hardware, in case of an hot reboot the driver > > probe() function attempts to flush the FIFO, which may contain some > > data, while the hw->sip is zero. > > The complete execution path is the following: > > - call of st_lsm6dsx_probe(); > > - allocation of the st_lsm6dsx_hw structure via the devm_kzalloc, > > zero-initializing the sip field; > > - call of st_lsm6dsx_init_device(); > > - call of st_lsm6dsx_reset_device(); > > - call of st_lsm6dsx_flush_fifo(); > > - call of st_lsm6dsx_read_fifo/st_lsm6dsx_read_tagged_fifo via the > > fifo_ops function pointer. > > ack, I can see the issue now, thx for pointing this out. I should we should set > pattern_len to ST_LSM6DSX_SAMPLE_SIZE or ST_LSM6DSX_TAGGED_SAMPLE_SIZE if it is > 0. Can you please move the check before updating fifo_len in > st_lsm6dsx_read_fifo()? > > Regards, > Lorenzo > Changes done, I sent the updated patch in a separate reply email: I hope that doing so didn't cause any trouble. Thank you, Silvano.
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c index 0a7cd8c1aa33..7f343614f8a5 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c @@ -395,12 +395,17 @@ int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw) fifo_len = (le16_to_cpu(fifo_status) & fifo_diff_mask) * ST_LSM6DSX_CHAN_SIZE; fifo_len = (fifo_len / pattern_len) * pattern_len; + if (!fifo_len) + return 0; acc_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]); gyro_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_GYRO]); if (hw->iio_devs[ST_LSM6DSX_ID_EXT0]) ext_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_EXT0]); + if (!pattern_len) + pattern_len = ST_LSM6DSX_SAMPLE_SIZE; + for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { err = st_lsm6dsx_read_block(hw, ST_LSM6DSX_REG_FIFO_OUTL_ADDR, hw->buff, pattern_len, @@ -623,6 +628,9 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw) if (!fifo_len) return 0; + if (!pattern_len) + pattern_len = ST_LSM6DSX_TAGGED_SAMPLE_SIZE; + for (read_len = 0; read_len < fifo_len; read_len += pattern_len) { err = st_lsm6dsx_read_block(hw, ST_LSM6DSX_REG_FIFO_OUT_TAG_ADDR,
Prevent st_lsm6dsx_read_fifo and st_lsm6dsx_read_tagged_fifo functions from falling in an infinite loop in case pattern_len is equal to zero and the device FIFO is not empty. Signed-off-by: Silvano Seva <s.seva@4sigma.it> --- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 8 ++++++++ 1 file changed, 8 insertions(+)