| Message ID | 20250304060518.1834910-1-sunliming@linux.dev (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: imu: adis: fix uninitialized symbol warning | expand |
On Tue, Mar 04, 2025 at 02:05:18PM +0800, sunliming@linux.dev wrote: > From: sunliming <sunliming@kylinos.cn> > > Fix below kernel warning: > smatch warnings: > drivers/iio/imu/adis.c:319 __adis_check_status() error: uninitialized symbol 'status_16'. > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Signed-off-by: sunliming <sunliming@kylinos.cn> Huh... Someone is using lei to get their email. This patch is fine and it's theoretically the correct thing to do. How the zero-day bot warnings work is the they are first sent to my gmail account and I look them over and either forward them or ignore them. Here is the code: drivers/iio/imu/adis.c 305 int __adis_check_status(struct adis *adis) 306 { 307 unsigned int status; 308 int diag_stat_bits; 309 u16 status_16; 310 int ret; 311 int i; 312 313 if (adis->data->diag_stat_size) { 314 ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status, 315 adis->data->diag_stat_size); 316 } else { 317 ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, 318 &status_16); 319 status = status_16; 320 } 321 if (ret) 322 return ret; 323 So if __adis_read_reg_16() fails, then the next line is an uninitialized read. But then the if (ret) check means that it's fine at run-time. It's a false positive. The other thing to consider it the UBSan will also detect the uninitialized read at runtime and splat. That's still a false positive but it's a headache. But when I was looking at this, I decided that __adis_read_reg_16() was unlikely to fail in real life so I decided to ignore this warning. Initializing the variable to zero doesn't change runtime for sane configs because everyone automatically zeroes stack variables these days. It just silences the Smatch warning. So I'm fine with this patch. (This email is for information only in case you were wondering why the bug report was formatted strangely etc). regards, dan carpenter
On Tue, 4 Mar 2025 09:36:56 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Mar 04, 2025 at 02:05:18PM +0800, sunliming@linux.dev wrote: > > From: sunliming <sunliming@kylinos.cn> > > > > Fix below kernel warning: > > smatch warnings: > > drivers/iio/imu/adis.c:319 __adis_check_status() error: uninitialized symbol 'status_16'. > > > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <error27@gmail.com> > > Signed-off-by: sunliming <sunliming@kylinos.cn> > > Huh... Someone is using lei to get their email. This patch is fine and > it's theoretically the correct thing to do. > > How the zero-day bot warnings work is the they are first sent to my gmail > account and I look them over and either forward them or ignore them. Here > is the code: > > drivers/iio/imu/adis.c > 305 int __adis_check_status(struct adis *adis) > 306 { > 307 unsigned int status; > 308 int diag_stat_bits; > 309 u16 status_16; > 310 int ret; > 311 int i; > 312 > 313 if (adis->data->diag_stat_size) { > 314 ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status, > 315 adis->data->diag_stat_size); > 316 } else { > 317 ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, > 318 &status_16); > 319 status = status_16; > 320 } > 321 if (ret) > 322 return ret; > 323 > > So if __adis_read_reg_16() fails, then the next line is an uninitialized > read. But then the if (ret) check means that it's fine at run-time. > It's a false positive. The other thing to consider it the UBSan will > also detect the uninitialized read at runtime and splat. That's still a > false positive but it's a headache. But when I was looking at this, I > decided that __adis_read_reg_16() was unlikely to fail in real life so I > decided to ignore this warning. > > Initializing the variable to zero doesn't change runtime for sane configs > because everyone automatically zeroes stack variables these days. It > just silences the Smatch warning. So I'm fine with this patch. > > (This email is for information only in case you were wondering why the > bug report was formatted strangely etc). > > regards, > dan carpenter > Thanks! That explanation has me agreeing that this patch seems to make sense as fixing a warning that is reasonable if unlikely to cause problems in practice. I've applied it to the togreg branch of iio.git Jonathan
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c index 1c646c36aeb1..0ea072a4c966 100644 --- a/drivers/iio/imu/adis.c +++ b/drivers/iio/imu/adis.c @@ -306,7 +306,7 @@ int __adis_check_status(struct adis *adis) { unsigned int status; int diag_stat_bits; - u16 status_16; + u16 status_16 = 0; int ret; int i;