| Message ID | 20220320181542.168147-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: core: WARN in case sample bits do not fit storage bits | expand |
> -----Original Message----- > From: Marek Vasut <marex@denx.de> > Sent: Sunday, March 20, 2022 7:16 PM > To: linux-iio@vger.kernel.org > Cc: Marek Vasut <marex@denx.de>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Daniel Baluta > <daniel.baluta@nxp.com>; Jonathan Cameron > <Jonathan.Cameron@huawei.com> > Subject: [PATCH] iio: core: WARN in case sample bits do not fit storage > bits > > [External] > > Add runtime check to verify whether storagebits are at least as big > as shifted realbits. This should help spot broken drivers which may > set realbits + shift above storagebits. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Daniel Baluta <daniel.baluta@nxp.com> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- Acked-by: Nuno Sá <nuno.sa@analog.com> > drivers/iio/industrialio-buffer.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > buffer.c > index b078eb2f3c9de..bac1e994e7bef 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1629,6 +1629,11 @@ static int > __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > if (channels[i].scan_index < 0) > continue; > > + /* Verify that sample bits fit into storage */ > + WARN_ON(channels[i].scan_type.storagebits < > + channels[i].scan_type.realbits + > + channels[i].scan_type.shift); > + > ret = iio_buffer_add_channel_sysfs(indio_dev, > buffer, > &channels[i]); > if (ret < 0) > -- > 2.35.1
On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: > Add runtime check to verify whether storagebits are at least as big > as shifted realbits. This should help spot broken drivers which may > set realbits + shift above storagebits. Thanks! ... > > + /* Verify that sample bits fit into storage */ > + WARN_ON(channels[i].scan_type.storagebits < > + channels[i].scan_type.realbits + > + channels[i].scan_type.shift); Not sure WARN is a good level (it might be fatal on some setups and we won't that), besides the fact that we may use dev_WARN(). Perhaps dev_warn() would suffice?
On 3/21/22 11:40, Andy Shevchenko wrote: > On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: >> Add runtime check to verify whether storagebits are at least as big >> as shifted realbits. This should help spot broken drivers which may >> set realbits + shift above storagebits. > > Thanks! > > ... > >> >> + /* Verify that sample bits fit into storage */ >> + WARN_ON(channels[i].scan_type.storagebits < >> + channels[i].scan_type.realbits + >> + channels[i].scan_type.shift); > > Not sure WARN is a good level (it might be fatal on some setups and we won't that), > besides the fact that we may use dev_WARN(). Perhaps dev_warn() would suffice? I was actually thinking about BUG(), but that might crash existing systems. I think we want a strong indicator that something wrong is going on which must be fixed and the splat produced by WARN_ON() is a good indicator of that. It also does not crash existing systems, so even if existing users get a warning now, they won't get an unbootable system and can report that warning.
On Mon, Mar 21, 2022 at 03:46:51PM +0100, Marek Vasut wrote: > On 3/21/22 11:40, Andy Shevchenko wrote: > > On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: > > > Add runtime check to verify whether storagebits are at least as big > > > as shifted realbits. This should help spot broken drivers which may > > > set realbits + shift above storagebits. > > > > Thanks! > > > > ... > > > > > + /* Verify that sample bits fit into storage */ > > > + WARN_ON(channels[i].scan_type.storagebits < > > > + channels[i].scan_type.realbits + > > > + channels[i].scan_type.shift); > > > > Not sure WARN is a good level (it might be fatal on some setups and we won't that), > > besides the fact that we may use dev_WARN(). Perhaps dev_warn() would suffice? > > I was actually thinking about BUG(), but that might crash existing systems. > I think we want a strong indicator that something wrong is going on which > must be fixed and the splat produced by WARN_ON() is a good indicator of > that. It also does not crash existing systems, It does crash _some_ of them, unfortunately. > so even if existing users get > a warning now, they won't get an unbootable system and can report that > warning.
On 3/21/22 17:10, Andy Shevchenko wrote: > On Mon, Mar 21, 2022 at 03:46:51PM +0100, Marek Vasut wrote: >> On 3/21/22 11:40, Andy Shevchenko wrote: >>> On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: >>>> Add runtime check to verify whether storagebits are at least as big >>>> as shifted realbits. This should help spot broken drivers which may >>>> set realbits + shift above storagebits. >>> >>> Thanks! >>> >>> ... >>> >>>> + /* Verify that sample bits fit into storage */ >>>> + WARN_ON(channels[i].scan_type.storagebits < >>>> + channels[i].scan_type.realbits + >>>> + channels[i].scan_type.shift); >>> >>> Not sure WARN is a good level (it might be fatal on some setups and we won't that), >>> besides the fact that we may use dev_WARN(). Perhaps dev_warn() would suffice? >> >> I was actually thinking about BUG(), but that might crash existing systems. >> I think we want a strong indicator that something wrong is going on which >> must be fixed and the splat produced by WARN_ON() is a good indicator of >> that. It also does not crash existing systems, > > It does crash _some_ of them, unfortunately. Details please ? WARN_ON() shouldn't cause crash outright, or do I miss something ?
On Mon, 2022-03-21 at 20:46 +0100, Marek Vasut wrote: > On 3/21/22 17:10, Andy Shevchenko wrote: > > On Mon, Mar 21, 2022 at 03:46:51PM +0100, Marek Vasut wrote: > > > On 3/21/22 11:40, Andy Shevchenko wrote: > > > > On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: > > > > > Add runtime check to verify whether storagebits are at least > > > > > as big > > > > > as shifted realbits. This should help spot broken drivers > > > > > which may > > > > > set realbits + shift above storagebits. > > > > > > > > Thanks! > > > > > > > > ... > > > > > > > > > + /* Verify that sample bits fit into > > > > > storage */ > > > > > + WARN_ON(channels[i].scan_type.storage > > > > > bits < > > > > > + channels[i].scan_type.realbit > > > > > s + > > > > > + channels[i].scan_type.shift); > > > > > > > > Not sure WARN is a good level (it might be fatal on some setups > > > > and we won't that), > > > > besides the fact that we may use dev_WARN(). Perhaps dev_warn() > > > > would suffice? > > > > > > I was actually thinking about BUG(), but that might crash > > > existing systems. > > > I think we want a strong indicator that something wrong is going > > > on which > > > must be fixed and the splat produced by WARN_ON() is a good > > > indicator of > > > that. It also does not crash existing systems, > > > > It does crash _some_ of them, unfortunately. > > Details please ? > > WARN_ON() shouldn't cause crash outright, or do I miss something ? Arghh, completely forgot about this... Andy is right, maybe there are other cases (in which case, it would be nice to share :D), but this one is definitely one of them: https://elixir.bootlin.com/linux/latest/source/kernel/panic.c#L579 You can have a cmdline parameter to panic on _WARN() and some systems may have it. That said, the "nice" stack_dump using WARN is way more explicit about saying that something is seriously wrong and must be fixed. dev_warn() is easier to ignore... But surely it is not nice to brick existing systems. Not really sure here... - Nuno Sá
On Tue, Mar 22, 2022 at 08:43:10AM +0100, Nuno Sá wrote: > On Mon, 2022-03-21 at 20:46 +0100, Marek Vasut wrote: > > On 3/21/22 17:10, Andy Shevchenko wrote: > > > On Mon, Mar 21, 2022 at 03:46:51PM +0100, Marek Vasut wrote: > > > > On 3/21/22 11:40, Andy Shevchenko wrote: > > > > > On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: > > > > > > Add runtime check to verify whether storagebits are at least > > > > > > as big > > > > > > as shifted realbits. This should help spot broken drivers > > > > > > which may > > > > > > set realbits + shift above storagebits. > > > > > > > > > > Thanks! > > > > > > > > > > ... > > > > > > > > > > > + /* Verify that sample bits fit into > > > > > > storage */ > > > > > > + WARN_ON(channels[i].scan_type.storage > > > > > > bits < > > > > > > + channels[i].scan_type.realbit > > > > > > s + > > > > > > + channels[i].scan_type.shift); > > > > > > > > > > Not sure WARN is a good level (it might be fatal on some setups > > > > > and we won't that), > > > > > besides the fact that we may use dev_WARN(). Perhaps dev_warn() > > > > > would suffice? > > > > > > > > I was actually thinking about BUG(), but that might crash > > > > existing systems. > > > > I think we want a strong indicator that something wrong is going > > > > on which > > > > must be fixed and the splat produced by WARN_ON() is a good > > > > indicator of > > > > that. It also does not crash existing systems, > > > > > > It does crash _some_ of them, unfortunately. > > > > Details please ? > > > > WARN_ON() shouldn't cause crash outright, or do I miss something ? > > Arghh, completely forgot about this... Andy is right, maybe there are > other cases (in which case, it would be nice to share :D), but this one > is definitely one of them: > > https://elixir.bootlin.com/linux/latest/source/kernel/panic.c#L579 > > You can have a cmdline parameter to panic on _WARN() and some systems > may have it. Yes, I meant panic on warning. And I can't imagine that this driver can be system critical to the extent that we have to crash the system. > That said, the "nice" stack_dump using WARN is way more explicit about > saying that something is seriously wrong and must be fixed. dev_warn() > is easier to ignore... But surely it is not nice to brick existing > systems. > > Not really sure here...
On 3/22/22 10:49, Andy Shevchenko wrote: > On Tue, Mar 22, 2022 at 08:43:10AM +0100, Nuno Sá wrote: >> On Mon, 2022-03-21 at 20:46 +0100, Marek Vasut wrote: >>> On 3/21/22 17:10, Andy Shevchenko wrote: >>>> On Mon, Mar 21, 2022 at 03:46:51PM +0100, Marek Vasut wrote: >>>>> On 3/21/22 11:40, Andy Shevchenko wrote: >>>>>> On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut wrote: >>>>>>> Add runtime check to verify whether storagebits are at least >>>>>>> as big >>>>>>> as shifted realbits. This should help spot broken drivers >>>>>>> which may >>>>>>> set realbits + shift above storagebits. >>>>>> >>>>>> Thanks! >>>>>> >>>>>> ... >>>>>> >>>>>>> + /* Verify that sample bits fit into >>>>>>> storage */ >>>>>>> + WARN_ON(channels[i].scan_type.storage >>>>>>> bits < >>>>>>> + channels[i].scan_type.realbit >>>>>>> s + >>>>>>> + channels[i].scan_type.shift); >>>>>> >>>>>> Not sure WARN is a good level (it might be fatal on some setups >>>>>> and we won't that), >>>>>> besides the fact that we may use dev_WARN(). Perhaps dev_warn() >>>>>> would suffice? >>>>> >>>>> I was actually thinking about BUG(), but that might crash >>>>> existing systems. >>>>> I think we want a strong indicator that something wrong is going >>>>> on which >>>>> must be fixed and the splat produced by WARN_ON() is a good >>>>> indicator of >>>>> that. It also does not crash existing systems, >>>> >>>> It does crash _some_ of them, unfortunately. >>> >>> Details please ? >>> >>> WARN_ON() shouldn't cause crash outright, or do I miss something ? >> >> Arghh, completely forgot about this... Andy is right, maybe there are >> other cases (in which case, it would be nice to share :D), but this one >> is definitely one of them: >> >> https://elixir.bootlin.com/linux/latest/source/kernel/panic.c#L579 >> >> You can have a cmdline parameter to panic on _WARN() and some systems >> may have it. > > Yes, I meant panic on warning. > > And I can't imagine that this driver can be system critical to the extent > that we have to crash the system. Is there something which does trigger a backtrace, but without panic()ing the system ?
On Tue, 2022-03-22 at 10:57 +0100, Marek Vasut wrote: > On 3/22/22 10:49, Andy Shevchenko wrote: > > On Tue, Mar 22, 2022 at 08:43:10AM +0100, Nuno Sá wrote: > > > On Mon, 2022-03-21 at 20:46 +0100, Marek Vasut wrote: > > > > On 3/21/22 17:10, Andy Shevchenko wrote: > > > > > On Mon, Mar 21, 2022 at 03:46:51PM +0100, Marek Vasut wrote: > > > > > > On 3/21/22 11:40, Andy Shevchenko wrote: > > > > > > > On Sun, Mar 20, 2022 at 07:15:42PM +0100, Marek Vasut > > > > > > > wrote: > > > > > > > > Add runtime check to verify whether storagebits are at > > > > > > > > least > > > > > > > > as big > > > > > > > > as shifted realbits. This should help spot broken > > > > > > > > drivers > > > > > > > > which may > > > > > > > > set realbits + shift above storagebits. > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > ... > > > > > > > > > > > > > > > + /* Verify that sample bits fit > > > > > > > > into > > > > > > > > storage */ > > > > > > > > + WARN_ON(channels[i].scan_type.s > > > > > > > > torage > > > > > > > > bits < > > > > > > > > + channels[i].scan_type.r > > > > > > > > ealbit > > > > > > > > s + > > > > > > > > + channels[i].scan_type.s > > > > > > > > hift); > > > > > > > > > > > > > > Not sure WARN is a good level (it might be fatal on some > > > > > > > setups > > > > > > > and we won't that), > > > > > > > besides the fact that we may use dev_WARN(). Perhaps > > > > > > > dev_warn() > > > > > > > would suffice? > > > > > > > > > > > > I was actually thinking about BUG(), but that might crash > > > > > > existing systems. > > > > > > I think we want a strong indicator that something wrong is > > > > > > going > > > > > > on which > > > > > > must be fixed and the splat produced by WARN_ON() is a good > > > > > > indicator of > > > > > > that. It also does not crash existing systems, > > > > > > > > > > It does crash _some_ of them, unfortunately. > > > > > > > > Details please ? > > > > > > > > WARN_ON() shouldn't cause crash outright, or do I miss > > > > something ? > > > > > > Arghh, completely forgot about this... Andy is right, maybe there > > > are > > > other cases (in which case, it would be nice to share :D), but > > > this one > > > is definitely one of them: > > > > > > https://elixir.bootlin.com/linux/latest/source/kernel/panic.c#L579 > > > > > > You can have a cmdline parameter to panic on _WARN() and some > > > systems > > > may have it. > > > > Yes, I meant panic on warning. > > > > And I can't imagine that this driver can be system critical to the > > extent > > that we have to crash the system. > > Is there something which does trigger a backtrace, but without > panic()ing the system ? Nothing I'm aware... Other than directly call 'dump_stack()' - Nuno Sá
On 3/22/22 11:10, Nuno Sá wrote: Hi, [...] >>>> Arghh, completely forgot about this... Andy is right, maybe there >>>> are >>>> other cases (in which case, it would be nice to share :D), but >>>> this one >>>> is definitely one of them: >>>> >>>> https://elixir.bootlin.com/linux/latest/source/kernel/panic.c#L579 >>>> >>>> You can have a cmdline parameter to panic on _WARN() and some >>>> systems >>>> may have it. >>> >>> Yes, I meant panic on warning. >>> >>> And I can't imagine that this driver can be system critical to the >>> extent >>> that we have to crash the system. >> >> Is there something which does trigger a backtrace, but without >> panic()ing the system ? > > Nothing I'm aware... Other than directly call 'dump_stack()' All right, dev_err() it is then. I sent V2.
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index b078eb2f3c9de..bac1e994e7bef 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -1629,6 +1629,11 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, if (channels[i].scan_index < 0) continue; + /* Verify that sample bits fit into storage */ + WARN_ON(channels[i].scan_type.storagebits < + channels[i].scan_type.realbits + + channels[i].scan_type.shift); + ret = iio_buffer_add_channel_sysfs(indio_dev, buffer, &channels[i]); if (ret < 0)
Add runtime check to verify whether storagebits are at least as big as shifted realbits. This should help spot broken drivers which may set realbits + shift above storagebits. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Baluta <daniel.baluta@nxp.com> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- drivers/iio/industrialio-buffer.c | 5 +++++ 1 file changed, 5 insertions(+)